Stake Root Simplifications [WIP] #741

shotaronowhere · 2024-09-11T18:22:50Z

This PR represents WIP simplification and extension of the stake root contracts. It is not in a finalized state as items such as test coverage, safe type casting, natspec comments, incentive compatibility for removals, modifications needed to charging, and agreement of the simplification approach are needed.

Simplifications

Removing checkpoint history data structures. Some of these data structures were used to track balance updates and introduced both logical and computational complexity as binary search was used in some cases. There's still a checkpoint history used for the total charge, but I think we can get rid of that with another approach (storage proof or synchronous charging).
The async nature of charging is another point of complexity related to the blacklist window. We should discuss what the goal is here for blacklisting in general, but in the context of charging, it looks like an unneccesary complication that impacts the happy path due to an unhappy path concern. As far as I see, the original implementation delays payment of the proof submitter in case some point of the zk-proving stack is compromised, and you don't want to compensate someone for producing a proof which does not effectively compute the stake root. On the otherhand, proof charging is rate limited and should this event happen, the contract is upgradable anyways so we could effectively pause the contract. If we are designing a contract that will be immutable in the future, then that changes the analysis. I don't think the the complexity of async charging is not worth the rare edge case it protects against.
Removal from the stake tree leaves strategies dirty

AVS' can withdraw the balances at any time and remove operator sets from the tree.

src/contracts/core/StakeRootCompendium.sol

src/contracts/core/StakeRootCompendiumStorage.sol

gpsanant · 2024-09-11T18:50:48Z

src/contracts/core/StakeRootCompendium.sol

+        _addStrategiesAndMultipliers(IAVSDirectory.OperatorSet({avs: msg.sender, operatorSetId: operatorSetId}), strategiesAndMultipliers);
+    }
+
+    // natspec


gpsanant · 2024-09-11T18:51:19Z

src/contracts/core/StakeRootCompendium.sol

+
+    // natspec
+    /// @notice Initializes an operator set with a given set of strategies and multipliers
+    /// @dev This a convinience (gas saving) function for avs'


we use multiline comments stylistically

/** * ... */

gpsanant · 2024-09-11T18:52:23Z

src/contracts/core/StakeRootCompendium.sol

+    /// @dev This a convinience (gas saving) function for avs'
+    /// @param operatorSetId The ID of the operator set to initialize
+    /// @param strategiesAndMultipliers The strategies and multipliers to initialize the operator set with
+    function initOperatorSetInStakeTree(


hm.

Can we just make addStrategiesAndMultipliers payable and deposit if msg.value > 0?

gpsanant · 2024-09-11T18:53:30Z

src/contracts/core/StakeRootCompendium.sol

@@ -26,170 +25,139 @@ contract StakeRootCompendium is StakeRootCompendiumStorage {

    /// @inheritdoc IStakeRootCompendium
    function depositForOperatorSet(IAVSDirectory.OperatorSet calldata operatorSet) external payable {


just deposit to be consistent with new internal funcs

gpsanant · 2024-09-11T18:56:15Z

src/contracts/core/StakeRootCompendium.sol

-                uint256 multipliedDelegatedShares = delegatedShares[i] * multipliers[i] / 1 ether;
-                delegatedStake += multipliedDelegatedShares * totalMagnitudes[i];
-                slashableStake += multipliedDelegatedShares * allocatedMagnitudes[i];
+                delegatedStake += delegatedShares[i] * totalMagnitudes[i] / 1 ether * multipliers[i];


i think the previous saves more gas in the zkVM

not sure though, we should benchmark

ahh okay this was inadvertent

gpsanant · 2024-09-11T19:07:43Z

src/contracts/core/StakeRootCompendium.sol

-        }
+        IAVSDirectory.OperatorSet memory substituteOperatorSet = operatorSets[operatorSets.length - 1];
+        // note: operatorSetIndex is 1 indexed so operatorSetOneBasedIndex - 1 is strictly non-negative
+        operatorSets[operatorSetOneBasedIndex - 1] = substituteOperatorSet;


i definitely prefer 0 based indexing haha

gpsanant · 2024-09-11T19:09:02Z

src/contracts/core/StakeRootCompendium.sol

-        return latestSnapshottedCumulativeCharge + uint224(currentCharge * (latestCalculationTimestamp - latestSnapshotTimestamp) / proofInterval);
+        // update the index of the operator sets
+        operatorSetToOneBasedIndex[msg.sender][operatorSet.operatorSetId] = 0;
+        operatorSetToOneBasedIndex[msg.sender][substituteOperatorSet.operatorSetId] = operatorSetOneBasedIndex;


this should not be msg.sender, it should be the operatorSet.avs and substituteOperatorSet.avs respectively

yeah, definitely

gpsanant · 2024-09-11T19:14:15Z

src/contracts/core/StakeRootCompendium.sol

-        );
+    // updates the deposit balance for the operator set and returns the penalty if the operator set has fallen below the minimum deposit balance
+    function _updateDepositInfo(IAVSDirectory.OperatorSet memory operatorSet) internal returns (uint256) {
+        uint256 depositBalance = getDepositBalance(operatorSet);


we should remove them from the operatorSet if they fall below the min balance here?

we can, i reserved that logic where it is explicitly relevant eg processing withdrawals. . .

let me ponder, can put it back in, just thinking if there was a good reason i took it out

gpsanant · 2024-09-11T19:22:54Z

src/contracts/core/StakeRootCompendium.sol

-        uint224 operatorSetIndex = operatorSetToIndex[operatorSet.avs][operatorSet.operatorSetId].latest();
-        operatorSets[operatorSetIndex] = substituteOperatorSet;
-        operatorSets.pop();
+    function _updateTotalStrategies(uint256 numStrategiesBefore, IAVSDirectory.OperatorSet memory operatorSet) internal {


rename back to updateTotalCharge because we need to call this when charges change

gpsanant · 2024-09-11T19:31:32Z

src/contracts/core/StakeRootCompendium.sol

-            cumulativeContantChargeSnapshot.upperLookup(timestamp),
-            cumulativeLinearChargeSnapshot.upperLookup(timestamp)
+    function _removeOperatorSetFromStakeTree(IAVSDirectory.OperatorSet memory operatorSet) internal {
+        uint256 operatorSetOneBasedIndex = operatorSetToOneBasedIndex[operatorSet.avs][operatorSet.operatorSetId];


we need to update the totalCharge in this function in case this is a force removal that isn't a removal of strategies

gpsanant · 2024-09-11T19:32:40Z

src/contracts/core/StakeRootCompendium.sol


-        // remove the strategies and multipliers for the operator set
-        _removeStrategiesAndMultipliers(operatorSet, strategies);
+    function removeOperatorSetFromStakeTree(uint32 operatorSetId) external {


hm why do we have this?

not necessary I guess, could remove it if it doesn't matter for AVS'

gpsanant · 2024-09-11T19:32:57Z

src/contracts/core/StakeRootCompendium.sol

    }

    /// @inheritdoc IStakeRootCompendium
    function setOperatorSetExtraData(uint32 operatorSetId, bytes32 extraData) external {
-        (bool exists,, uint224 index) = operatorSetToIndex[msg.sender][operatorSetId].latestCheckpoint();


seems correct to revert here imo

yeah i'll put that back, it's a setter so used very rarely

gpsanant · 2024-09-11T19:47:47Z

src/contracts/core/StakeRootCompendium.sol

        operatorExtraDatas[msg.sender][operatorSetId][operator] = extraData;
    }

    /// POSTING ROOTS AND BLACKLISTING

    /// @inheritdoc IStakeRootCompendium
    function verifyStakeRoot(
-        uint32 calculationTimestamp,
+        uint64 calculationBlockNumber,


this should be a timestamp since everything is in timestamps in the slashing release

hmmmm this might be awkward since the zk proof is against a blockhash and blocks are sequential but calculationTimestamps can jump
BN: [ 0 ] [ 1 ] [ 2 ]
TS: 12 36 48

so there is a missing timestamp corresponding to timestamp 24. . . should a proof be skipped in this case?

gpsanant · 2024-09-11T19:48:58Z

src/contracts/core/StakeRootCompendium.sol

        );
        require(
-            block.timestamp < stakeRootSubmissions[submissionIndex].blacklistableBefore,
-            "StakeRootCompendium.blacklistStakeRoot: stakeRoot cannot be blacklisted"
+            latestBlockNumberProven < calculationBlockNumber,


calculationBlockNumber = latestBlockNumberProven + proofInterval

And this can only be if this is not the first submission

what do you mean?

from the first require statement, we have calculationBlockNumber % proofInterval == 0

and from latestBlockNumberProven < calculationBlockNumber

we have basically that

latestBlockNumberProven = latestBlockNumberProven + n * proofInterval

do you mean proofs should be calculated strictly sequentially?

gpsanant · 2024-09-11T20:03:12Z

src/contracts/core/StakeRootCompendium.sol

-                "StakeRootCompendium._postStakeRoot: calculationTimestamp must be greater than the last posted calculationTimestamp"
-            );
+        if (submission.stakeRoot == bytes32(0)) {
+            submission.stakeRoot = stakeRoot;


if this is not a proof post, the submission must be proven. confirmation must happen after proofs

I was kind of overloading this functionality for the desired feature that if the provers are offline, the same data structures can be used for AVS' which may choose to use a simpler confirmation rule (trust eigen labs)

or what is desired as a fallback if zkprovers are all offline?

but yeah, certainly can adjust this so confirmerss strictly confirm

gpsanant · 2024-09-11T20:07:32Z

src/contracts/core/StakeRootCompendium.sol

-        operatorSetToIndex[operatorSet.avs][operatorSet.operatorSetId].push(uint32(block.timestamp), operatorSetIndex);
+    function _setChargePerProof(uint64 _chargePerProofLinear, uint64 _chargePerProofConstant) internal {
+        _updateCumulativeCharges();
+        chargePerProofLinear = _chargePerProofLinear;


we need to update the total Charge here

yeah, i overlooked some total charging related logic, will put it in there

gpsanant · 2024-09-11T20:11:09Z

src/contracts/core/StakeRootCompendium.sol

+    function _updateDepositInfo(IAVSDirectory.OperatorSet memory operatorSet) internal returns (uint256) {
+        uint256 depositBalance = getDepositBalance(operatorSet);
+        depositInfo[operatorSet.avs][operatorSet.operatorSetId].balance = uint96(depositBalance);
+        depositInfo[operatorSet.avs][operatorSet.operatorSetId].snapshotTimestamp = uint64(block.timestamp);


we need to update the cumulatives here

gpsanant · 2024-09-11T20:16:59Z

src/contracts/core/StakeRootCompendium.sol

-        stakeRootSubmissions[submissionIndex].blacklisted = true;
+        // TODO: verify proof
+        _postStakeRoot(true, calculationBlockNumber, stakeRoot);
+        uint256 charge = totalChargeSnapshot.upperLookup(uint32(calculationBlockNumber));


should we just allow them to submit an index here to avoid lookup gas?

i need to put a bit more thought into the total charge, thats a component I overlooked a bit. . .

can't we avoid snapshots by having a view function for the totalCharge and doing a storage proof for that block? that's basically what the zk proof is doing anyways right? could add that to the proof? and to avoid zk tech stack failure enforce a MAX_TOTAL_CHARGE = 3 eth

alternatively to KISS we can do a little bit more accounting to have an O(1) lookup, I think there's a simple way to do it

gpsanant · 2024-09-11T20:45:35Z

src/contracts/core/StakeRootCompendiumStorage.sol

@@ -23,8 +23,6 @@ abstract contract StakeRootCompendiumStorage is IStakeRootCompendium, OwnableUpg
    uint32 public constant MAX_NUM_OPERATOR_SETS = 2048;
    /// @notice the maximum number of strategies that each operator set in the StakeTree can use to weight their operator stakes
    uint32 public constant MAX_NUM_STRATEGIES = 20;
-    /// @notice the placeholder index used for operator sets that are removed from the StakeTree
-    uint32 public constant REMOVED_INDEX = type(uint32).max;

    /// @notice the minimum balance that must be maintained for an operatorSet
    uint256 public constant MIN_DEPOSIT_BALANCE = 0.001 ether;


can we make this an immutable?

MIN_DEPOSIT_BALANCE? yeah

gpsanant · 2024-09-11T20:46:27Z

src/contracts/core/StakeRootCompendium.sol

-        stakeRootSubmissions[submissionIndex].blacklisted = true;
+        // TODO: verify proof
+        _postStakeRoot(true, calculationBlockNumber, stakeRoot);
+        uint256 charge = totalChargeSnapshot.upperLookup(uint32(calculationBlockNumber));


set latestBlockNumberProven

src/contracts/core/StakeRootCompendium.sol

+    function _isInStakeTree(IAVSDirectory.OperatorSet memory operatorSet) internal view returns (bool) {
+        (bool exists,,uint224 index) = operatorSetToIndex[operatorSet.avs][operatorSet.operatorSetId].latestCheckpoint();
+        return exists && index != REMOVED_INDEX;
+    }


src/contracts/core/StakeRootCompendiumStorage.sol

src/contracts/core/StakeRootCompendium.sol

+    function _updateTotalStrategies(uint256 _countStrategiesBefore, uint256 _countStrategiesAfter) internal {
+        totalStrategies = totalStrategies - _countStrategiesBefore + _countStrategiesAfter;
+        _updateChargePerProof();
+    }


src/contracts/core/StakeRootCompendium.sol

+        bytes32 _imageId    
+    ) StakeRootCompendiumStorage(_delegationManager, _avsDirectory, _maxTotalCharge, _minBalanceThreshold, _minProofsDuration, _verifier, _imageId) {}
+
+    function initialize(address _owner, address _rootConfirmer, uint32 _proofIntervalSeconds, uint96 _chargePerStrategy, uint96 _chargePerOperatorSet) public initializer {


src/contracts/core/StakeRootCompendium.sol

-        if (endIndex > stakeRootSubmissions.length) {
-            endIndex = stakeRootSubmissions.length;
-        }
+    function setProofIntervalSeconds(uint32 proofIntervalSeconds) public onlyOwner  {


src/contracts/core/StakeRootCompendium.sol

-            // total charge is the charge per strategy per proof times the number of strategies at the time of proof
-            totalCharge += totalChargeSnapshot.upperLookup(stakeRootSubmissions[i].calculationTimestamp);
-        }
+    function setRootConfirmer(address _rootConfirmer) public onlyOwner {


src/contracts/core/StakeRootCompendium.sol

+    function _updateChargePerProof() internal {
+        // note if totalStrategies is 0, the charge per proof will be 0, and provers should not post a proof
+        uint256 chargePerProof = operatorSets.length * chargePerOperatorSet + totalStrategies * chargePerStrategy;
+        require(chargePerProof <= MAX_TOTAL_CHARGE, "StakeRootCompendium._updateChargePerProof: charge per proof exceeds max total charge");
+        chargePerProofHistory.push(
+            uint32(block.timestamp), 
+            uint224(chargePerProof)
        );
+    }


src/contracts/core/StakeRootCompendiumStorage.sol

+    bytes32 public immutable imageId;
+
+    /// @notice the interval in seconds at which proofs can be posted
+    uint32 public proofIntervalSeconds;


src/contracts/core/StakeRootCompendiumStorage.sol

+    bytes32 public immutable imageId;
+
+    /// @notice the interval in seconds at which proofs can be posted
+    uint32 public proofIntervalSeconds;


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

View reviewed changes

shotaronowhere requested a review from gpsanant September 11, 2024 18:22

gpsanant reviewed Sep 11, 2024

View reviewed changes

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

View reviewed changes

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

View reviewed changes

gpsanant approved these changes Sep 16, 2024

View reviewed changes

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

View reviewed changes

squash

d0a6339

gpsanant force-pushed the shotaro-stake-root branch from 42b86cd to d0a6339 Compare September 16, 2024 04:45

receive function

b5da589

gpsanant merged commit 7023144 into stake-root Sep 16, 2024
4 of 10 checks passed

gpsanant deleted the shotaro-stake-root branch September 16, 2024 04:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stake Root Simplifications [WIP] #741

Stake Root Simplifications [WIP] #741

shotaronowhere commented Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024 •

edited

Loading

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024 •

edited

Loading

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

shotaronowhere Sep 11, 2024

gpsanant Sep 11, 2024

		@@ -26,170 +25,139 @@ contract StakeRootCompendium is StakeRootCompendiumStorage {

		/// @inheritdoc IStakeRootCompendium
		function depositForOperatorSet(IAVSDirectory.OperatorSet calldata operatorSet) external payable {

Stake Root Simplifications [WIP] #741

Stake Root Simplifications [WIP] #741

Conversation

shotaronowhere commented Sep 11, 2024

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 Sep 11, 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

gpsanant Sep 11, 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

gpsanant Sep 11, 2024 •

edited

Loading

gpsanant Sep 11, 2024 •

edited

Loading