stake root #723

gpsanant · 2024-09-02T23:11:14Z

The StakeRootCompendium

core

MAX_TOTAL_CHARGE make this a storage var and settable
fix reversion of getOperatorSetLeaves for >=150 operators
keep strategies around after rejoining
Not blocking initial review or merging
- actual proof verification.
- operatorSetRoot proofs
- blacklisting protocol
- submission consumption library

misc

the setters and externally callable functions need natspec
better line comments where helpful
a better name for
- MIN_PROOFS_DURATION
- chargePerProof
- rootConfirmer

notes for the initial review

We mainly want feedback on

large architectural suggestions
disagreements on behavior in certain cases
suggestions for a blacklisting protocol
- currently im thinking the pauser can blacklist on the ops can reverse a blacklist, but im not sure...

src/contracts/core/AVSDirectory.sol

src/contracts/core/StakeRootCompendium.sol

src/contracts/core/StakeRootCompendiumStorage.sol

src/contracts/core/StakeRootCompendium.sol

src/contracts/core/AVSDirectory.sol

src/contracts/core/DelegationManagerStorage.sol

src/contracts/pods/EigenPodManager.sol

src/contracts/core/StakeRootCompendium.sol

src/contracts/core/StakeRootCompendiumStorage.sol

src/contracts/core/StakeRootCompendium.sol

src/contracts/libraries/Merkle.sol

src/contracts/core/StakeRootCompendium.sol

src/contracts/interfaces/IStakeRootCompendium.sol

0xClandestine · 2024-09-18T13:56:59Z

src/contracts/core/AllocationManager.sol

+     *
+     * @return the list of total magnitudes for each strategy and the list of allocated magnitudes for each strategy
+     */
+    function getTotalAndAllocatedMagnitudes(


Fn is a bit messy, would suggest changing to:

function getTotalAndAllocatedMagnitudes( address operator, OperatorSet calldata operatorSet, IStrategy[] calldata strategies ) external view returns (uint64[] memory totalMagnitude, uint64[] memory allocatedMagnitude) { totalMagnitude = new uint64[](strategies.length); allocatedMagnitude = new uint64[](strategies.length); for (uint256 i = 0; i < strategies.length; ++i) { (totalMagnitude[i], allocatedMagnitude[i]) = _getTotalAndAllocatedMagnitude(operator, operatorSet, strategies[i]); } }

0xClandestine · 2024-09-18T13:57:57Z

src/contracts/core/DelegationManager.sol

@@ -873,6 +873,18 @@ contract DelegationManager is
        return SlashingLib.descaleShares(operatorScaledShares[operator][strategy], totalMagnitude);
    }

+    /// @notice Given array of strategies, returns array of scaled shares for the operator
+    function getOperatorScaledShares(


Same as above:

function getOperatorScaledShares( address operator, IStrategy[] memory strategies ) external view returns (uint256[] memory scaledShares) { scaledShares = new uint256[](strategies.length); for (uint256 i = 0; i < strategies.length; ++i) { scaledShares[i] = operatorScaledShares[operator][strategies[i]]; } }

0xClandestine · 2024-09-18T13:58:54Z

src/contracts/interfaces/IStakeRootCompendium.sol

+        uint96 cumulativeChargePerStrategyLastPaid;
+    }
+
+    struct StakeRootSubmission {


0xClandestine · 2024-09-18T13:59:37Z

src/contracts/libraries/Merkle.sol

@@ -53,7 +51,7 @@ library Merkle {
        bytes32 leaf,
        uint256 index
    ) internal pure returns (bytes32) {
-        require(proof.length % 32 == 0, ProofLengthNotMultipleOf32());
+        require(proof.length % 32 == 0, "Merkle.processInclusionProofKeccak: proof length should be a multiple of 32");


Was this intentional?

no, will patch

0xClandestine · 2024-09-18T13:59:57Z

src/contracts/libraries/Merkle.sol

@@ -109,7 +107,10 @@ library Merkle {
        bytes32 leaf,
        uint256 index
    ) internal view returns (bytes32) {
-        require(proof.length != 0 && proof.length % 32 == 0, ProofLengthNotMultipleOf32());


Same as above.

0xClandestine · 2024-09-18T14:03:27Z

src/contracts/libraries/Merkle.sol

+     *  @return The computed Merkle root of the tree.
+     *  @dev This pads to the next power of 2. very inefficient! just for POC
+     */
+    function merkleizeKeccak256(bytes32[] memory leaves) internal pure returns (bytes32) {


Bitshifts are a bit cheaper than multiplying or dividing by 2 at the cost of readability.

100 >> 1 └ Decimal: 50 100 << 1 └ Decimal: 200

0xClandestine · 2024-09-18T14:05:34Z

src/contracts/core/StakeRootCompendiumStorage.sol

+    /// @notice the id of the program being verified when roots are posted
+    bytes32 public immutable imageId;
+
+    /// @notice the interval in seconds at which proofs can be posted


Reorder variables such that they pack into less slots.

0xClandestine · 2024-09-18T14:18:12Z

Variable naming could use some work, like depositInfos.

0xClandestine · 2024-09-18T14:30:08Z

custom errors missing

src/contracts/core/StakeRootCompendium.sol

+            "StakeRootCompendium.getStakeRoot: operatorSetsInStakeTree vs. operatorSetRoots mismatch"
+        );
+        for (uint256 i = 0; i < operatorSetsInStakeTree.length; i++) {
+            require(
+                operatorSets[i].avs == operatorSetsInStakeTree[i].avs,
+                "StakeRootCompendium.getStakeRoot: operatorSets vs. operatorSetsInStakeTree avs mismatch"
+            );
+            require(
+                operatorSets[i].operatorSetId == operatorSetsInStakeTree[i].operatorSetId,
+                "StakeRootCompendium.getStakeRoot: operatorSets vs. operatorSetsInStakeTree operatorSetId mismatch"
+            );
+        }
+        return Merkle.merkleizeKeccak256(operatorSetRoots);
+    }
+
+    /// @inheritdoc IStakeRootCompendium
+    function getOperatorSetLeaves(
+        uint256 operatorSetIndex,
+        uint256 startOperatorIndex,
+        uint256 numOperators
+    ) external view returns (OperatorSet memory, address[] memory, OperatorLeaf[] memory) {
+        require(
+            operatorSetIndex < operatorSets.length,
+            "StakeRootCompendium.getOperatorSetLeaves: operator set index out of bounds"
+        );
+        OperatorSet memory operatorSet = operatorSets[operatorSetIndex];
+        address[] memory operators = avsDirectory.getOperatorsInOperatorSet(operatorSet, startOperatorIndex, numOperators);
+        (IStrategy[] memory strategies, uint256[] memory multipliers) = _getStrategiesAndMultipliers(operatorSet);
+
+        OperatorLeaf[] memory operatorLeaves = new OperatorLeaf[](operators.length);
+        for (uint256 i = 0; i < operatorLeaves.length; i++) {
+            // calculate the weighted sum of the operator's shares for the strategies given the multipliers
+            (uint256 delegatedStake, uint256 slashableStake) = _getStakes(operatorSet, strategies, multipliers, operators[i]);
+
+            operatorLeaves[i] = OperatorLeaf({
+                delegatedStake: delegatedStake,
+                slashableStake: slashableStake,
+                extraData: operatorExtraDatas[operatorSet.avs][operatorSet.operatorSetId][operators[i]]
+            });
+        }
+        return (operatorSet, operators, operatorLeaves);
+    }
+
+    /// @inheritdoc IStakeRootCompendium
+    function getOperatorSetRoot(
+        OperatorSet calldata operatorSet,
+        OperatorLeaf[] calldata operatorLeaves
+    ) external view returns (bytes32) {
+        require(
+            avsDirectory.isOperatorSet(operatorSet.avs, operatorSet.operatorSetId),
+            "StakeRootCompendium.getOperatorSetRoot: operator set does not exist"
+        );
+        require(
+            operatorLeaves.length == avsDirectory.getNumOperatorsInOperatorSet(operatorSet),
+            "AVSSyncTree.getOperatorSetRoot: operator set size mismatch"
+        );
+
+        uint256 totalDelegatedStake;
+        uint256 totalSlashableStake;
+        bytes32 prevExtraData;
+        bytes32[] memory operatorLeavesHashes = new bytes32[](operatorLeaves.length);
+        for (uint256 i = 0; i < operatorLeaves.length; i++) {
+            require(uint256(prevExtraData) < uint256(operatorLeaves[i].extraData), "StakeRootCompendium.getOperatorSetRoot: extraData not sorted");
+            prevExtraData = operatorLeaves[i].extraData;
+
+            operatorLeavesHashes[i] = keccak256(
+                abi.encodePacked(
+                    operatorLeaves[i].delegatedStake,
+                    operatorLeaves[i].slashableStake,
+                    operatorLeaves[i].extraData
+                )
+            );
+
+            totalDelegatedStake += operatorLeaves[i].delegatedStake;
+            totalSlashableStake += operatorLeaves[i].slashableStake;
+        }
+
+        bytes32 operatorTreeRoot = Merkle.merkleizeKeccak256(operatorLeavesHashes);
+        return keccak256(
+            abi.encodePacked(
+                operatorTreeRoot,
+                keccak256(
+                    abi.encodePacked(
+                        totalDelegatedStake, 
+                        totalSlashableStake, 
+                        operatorSetExtraDatas[operatorSet.avs][operatorSet.operatorSetId]
+                    )
+                )
+            )
+        );
+    }
+
+    function _safeTransferETH(address to, uint256 amount) internal {
+        (bool success,) = to.call{value: amount}("");
+        require(success, EthTransferFailed());
+    }
+
+    // in case of charge problems
+    receive() external payable {}
+}


src/contracts/core/StakeRootCompendium.sol

+    function deposit(
+        OperatorSet calldata operatorSet
+    ) external payable {
+        DepositInfo storage depositInfo = depositInfos[operatorSet.avs][operatorSet.operatorSetId];
+        if (!_isInStakeTree(operatorSet)) {
+            (, uint256 cumulativeChargePerOperatorSet, uint256 cumulativeChargePerStrategy) =
+                _calculateCumulativeCharges();
+
+            depositInfo.lastDemandIncreaseTimestamp = uint32(block.timestamp);
+            depositInfo.cumulativeChargePerOperatorSetLastPaid = uint96(cumulativeChargePerOperatorSet);
+            depositInfo.cumulativeChargePerStrategyLastPaid = uint96(cumulativeChargePerStrategy);
+
+            _updateTotalStrategies(
+                0, operatorSetToStrategyAndMultipliers[operatorSet.avs][operatorSet.operatorSetId].length()
+            );
+
+            operatorSetToIndex[operatorSet.avs][operatorSet.operatorSetId].push(
+                uint32(block.timestamp), uint224(operatorSets.length)
+            );
+
+            operatorSets.push(operatorSet);
+        }
+
+        depositInfo.balance += uint96(msg.value);
+        // note that they've shown increased demand for proofs
+        depositInfo.lastDemandIncreaseTimestamp = uint32(block.timestamp);
+        // make sure they have enough to pay for MIN_PROOFS_PREPAID
+        require(
+            depositInfo.balance
+                >= minDepositBalance(
+                    operatorSetToStrategyAndMultipliers[operatorSet.avs][operatorSet.operatorSetId].length()
+                ),
+            InsufficientDepositBalance()
+        );
+    }


src/contracts/core/StakeRootCompendiumStorage.sol

+    uint256 public immutable MIN_PREPAID_PROOFS;
+
+    /// @notice the total number of strategies among all operator sets (with duplicates)
+    uint256 public totalStrategies;


src/contracts/core/StakeRootCompendiumStorage.sol

+    /// @dev Contains info about cumulative charges.
+    CumulativeChargeParams public cumulativeChargeParams;
+    /// @notice deposit balance to be deducted for operatorSets
+    mapping(address => mapping(uint32 => DepositInfo)) public depositInfos;


ypatil12

Regarding testnet griefing, we may also be able to relax the safety assumption over liveness and have AVSs use the operatorSetRoot.... though the custom ERC-20 is a better idea IMO

Will comment on blacklisting in a separate review

ypatil12 · 2024-09-20T04:51:53Z

src/contracts/core/StakeRootCompendium.sol

+    function _isInStakeTree(
+        OperatorSet memory operatorSet
+    ) internal view returns (bool) {
+        (bool exists,, uint224 index) = operatorSetToIndex[operatorSet.avs][operatorSet.operatorSetId].latestSnapshot();


We're encoding operatorSets into bytes32 everywhere (AM, AVSD), let's do that here. We should have a library with internal encode/decode function that the above two contract, and this, should inherit from. A bit cleaner on the storage side as well :)

ypatil12 · 2024-09-20T04:57:26Z

src/contracts/core/StakeRootCompendiumStorage.sol

+        // however the new impl does not have access to the immutable variables of the last impl so we can't reference the old verifier and imageId
+        verifier = _verifier;
+        imageId = _imageId;
+    }


Storage gap??

ypatil12 · 2024-09-20T05:05:02Z

src/contracts/core/StakeRootCompendium.sol

+                    operatorSetToStrategyAndMultipliers[operatorSet.avs][operatorSet.operatorSetId].length()
+                ),
+            InsufficientDepositBalance()
+        );


If we want to react to demand increases, we need to run metrics on this, and thus need events that tell us:

How much is being deposited

Timestamp at which deposit, or demand increase is occurring

you're in charge of events brother

cc @bowenli86

Let's plan for an initial event audit next week cc @shotaronowhere

ypatil12 · 2024-09-20T05:05:49Z

src/contracts/core/StakeRootCompendium.sol

+            require(strategiesAndMultipliers.remove(address(strategies[i])), NonexistentStrategy());
+        }
+
+        _updateTotalStrategies(numStrategiesBefore, numStrategiesBefore - strategies.length);


Demand has reduced, we'd probably want an event here too

Why not keep track of lastDemandDecreaseTimestamp? Sounds relevant

We only care about demand increases in storage since withdrawability is pushed further out in the future by increases in demand so offchain prover sotware doesn't scale to meet "demand" and then get rugged

ypatil12 · 2024-09-20T05:22:11Z

src/contracts/core/StakeRootCompendium.sol

+    /// CHARGE MANAGEMENT
+
+    /// @inheritdoc IStakeRootCompendium
+    function removeOperatorSetsFromStakeTree(


Nit: it's not clear that this is a keeper method that is "penalizing" AVSs. First read sounds like it enables AVSs to remove their own operator sets from the tree (even if they're not in the red)

it's permissionless!

It's intended for people to remove AVSs from the stakeTree that won't have the ability to pay soon

Agree, but naming should be better

ohhhh, i misinterpreted your statement. yes, this could be better. maybe pruneLowBalanceOperatorSets

ypatil12 · 2024-09-20T06:01:05Z

src/contracts/core/StakeRootCompendium.sol

+        operatorSets.pop();
+
+        // update the index of the operator sets
+        // note when there is only one operator set left, the index will not be updated as the operator set will be removed in the next step


Or even if the operatorSet passed in to call data is in fact the last operatorSet in the opeartorSets array (ie. equal to substituteOperatorSet. There doesn't necessarily have to be one operator set left.

Why can't we just simplify this logic to if operatorSet == substituteOperatorSet then you only need to push REMOVED_INDEXand pop... else do previous in addition to handling substitute

didn't even think about this case. agree it should be handled gracefully

ypatil12 · 2024-09-20T06:10:35Z

src/contracts/core/StakeRootCompendiumStorage.sol

+    /// @dev Contains cumulative charges for operator sets, strategies, and max total charge.
+    ChargeParams public chargeParams;
+    /// @dev Contains info about cumulative charges.
+    CumulativeChargeParams public cumulativeChargeParams;


The proofIntervalSeconds in CumulativeChargeParams should be an immutable parameter?

nvm didn't see setProofIntervalSeconds earlier. However, I don't necessarily see why it needs to be packed into cumulativeChargeparams if it is mutated in a separate function

i think this was a gas efficiency/storage packing choice by @shotaronowhere and @0xClandestine

ypatil12 · 2024-09-20T06:19:56Z

src/contracts/core/StakeRootCompendium.sol

+        DepositInfo storage depositInfo = depositInfos[operatorSet.avs][operatorSet.operatorSetId];
+
+        // subtract new total charge from last paid total charge
+        uint256 pendingCharge = cumulativeChargePerOperatorSet - depositInfo.cumulativeChargePerOperatorSetLastPaid


Nit: this logic is repeated here and in getDepositBalance

ypatil12 · 2024-09-20T06:31:58Z

src/contracts/core/StakeRootCompendium.sol

+    ) public view returns (bool) {
+        // they must have paid for all of their prepaid proofs before withdrawing after a demand increase
+        return block.timestamp
+            > depositInfos[operatorSet.avs][operatorSet.operatorSetId].lastDemandIncreaseTimestamp


What happens if demand decreases (ie. remove strategies/multipliers)? Shouldn't the AVS be on the hook for the smaller set of strategies/multipliers instead of all of them when initially set?

Nope, the withdrawability requirement is primarily to impose a positive cost for increasing your demand for compute.

otherwise you could make provers scale up a bunch offchain and then withdraw. you could also sandwitch a calculationTimestamp with and increase and decrease and withdraw right after

ypatil12 · 2024-09-20T06:34:02Z

src/contracts/core/StakeRootCompendium.sol

+
+        // debt any pending charge
+        uint256 balance = _updateDepositInfo(operatorSet);
+        if (balance < MIN_BALANCE_THRESHOLD + amount) {


IMO we should have this function specifically prevent you from withdrawing amount if you go less than MIN_BALANCE_THRESHOLD - high potential for footguns by AVSs. Would be better to have a separate, voluntary, removeFromStakeTreeAndWithdraw func

great point. agree

* chore: forge fmt src/contracts * nit: organize imports * fix: bump pragma -> ^0.8.27 * refactor(optimization): significantly reduce sloads/sstores * refactor: custom errors * refactor(optimization): significantly reduce sloads/sstores missed some stuff * refactor: rename compendium -> manager * refactor: more storage optmizations * feat: safe eth transfer helper * refactor: manager -> compendium * refactor: variable renaming * refactor: rename compendium -> manager * feat: add `proofIntervalSeconds` getter * feat: fixed accounting bug and refactoring (#767) * refactor: review reconciliations * refactor: review reconciliations * fix: rename colluding param * fix: types and naming * fix: revert stakeroot calculation changes * fix: revert stakeRoot calc views --------- Co-authored-by: shotaro <[email protected]>

github-advanced-security bot found potential problems Sep 5, 2024

View reviewed changes

gpsanant force-pushed the stake-root branch from d4d7f4e to 7e408b5 Compare September 6, 2024 06:42

github-advanced-security bot found potential problems Sep 7, 2024

View reviewed changes

src/contracts/core/StakeRootCompendium.sol Fixed Show fixed Hide fixed

src/contracts/core/StakeRootCompendium.sol Fixed Show fixed Hide fixed

src/contracts/core/StakeRootCompendium.sol Fixed Show fixed Hide fixed

8sunyuan force-pushed the slashing-magnitudes branch 4 times, most recently from 33c32ac to 835a62f Compare September 12, 2024 22:01

github-advanced-security bot found potential problems Sep 13, 2024

View reviewed changes

github-advanced-security bot found potential problems Sep 16, 2024

View reviewed changes

gpsanant force-pushed the stake-root branch from 7023144 to 92252da Compare September 16, 2024 20:29

github-advanced-security bot found potential problems Sep 16, 2024

View reviewed changes

0xClandestine force-pushed the slashing-magnitudes branch from 0e122f9 to b871886 Compare September 17, 2024 16:26

gpsanant force-pushed the stake-root branch from 4187c46 to 39b9383 Compare September 18, 2024 00:32

gpsanant commented Sep 18, 2024

View reviewed changes

src/contracts/libraries/Merkle.sol Show resolved Hide resolved

github-advanced-security bot found potential problems Sep 18, 2024

View reviewed changes

0xClandestine self-requested a review September 18, 2024 13:54

0xClandestine reviewed Sep 18, 2024

View reviewed changes

github-advanced-security bot found potential problems Sep 19, 2024

View reviewed changes

ypatil12 reviewed Sep 20, 2024

View reviewed changes

gpsanant force-pushed the stake-root branch from 988c440 to 6d7510e Compare September 20, 2024 07:50

gpsanant and others added 7 commits September 20, 2024 18:14

init

f29dc22

add strats on rejoin

a54693e

sho's fixes

bdf3cd5

misc (#768)

a307372

change interface

cdf22da

simplify return values in getOperatorSetLeaves

0be4168

gpsanant force-pushed the stake-root branch from 6d7510e to 0be4168 Compare September 21, 2024 01:14

fix compile errors

c84ad89

stake root #723

Are you sure you want to change the base?

stake root #723

Conversation

gpsanant commented Sep 2, 2024 • edited Loading

core

misc

notes for the initial review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xClandestine commented Sep 18, 2024

0xClandestine commented Sep 18, 2024

ypatil12 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpsanant Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpsanant commented Sep 2, 2024 •

edited

Loading

ypatil12 left a comment •

edited

Loading

gpsanant Sep 20, 2024 •

edited

Loading