refactor: quorum creation is the divine right of the registry coordinator #28

wadealexc · 2023-10-26T16:26:13Z

Motivation:

fix word soup variable and function naming
take advantage of new quorum existence invariant due to RegistryCoordinator.createQuorum and its associated initializeQuorum methods in each registry
registries each keep "histories" of updates. The status quo is that every time anything changes, we push a new history update. this means that when updates happen multiple times in the same block, the history contains multiple updates from the same block number, making it harder to determine what the canonical state at a given block number is.

This PR:

Moves createQuorum to the registry coordinator, and adds quorum initialization logic for each registry contract
Replaces logic that handles "0-length histories" with simple checks, since we now have an invariant that initialized quorums have nonzero histories (see any registry's initializeQuorum method)
changes registry histories to update their "most recent entry" rather than pushing a new entry, if the entry was made in the current block
Renames state variables in the registry coordinator and registry contracts to be less wordy
adds a new mapping to BLSPublicKeyCompendium : operatorToPubkey, which maps operator addresses to G1Point
adds a method BLSPublicKeyCompendium.getRegisteredPubkey(address) which performs a pubkey lookup along with validation that the pubkey exists/isn't the zero pubkey.
- Validation logic here was previously performed in the BLSPubkeyRegistry
Removes the "pubkey" parameter from various functions, like in the BLSPubkeyRegistry and BLSRegistryCoordinatorWithIndices. The pubkey registry is able to look up pubkeys with just an operator address, now.

For reviewers, this order may work best:

Review IndexRegistry changes in the first commit, which contains the functional changes mentioned above, as well as readability/logical simplification. This is the worst commit to review, sorry! The diff looks intimidating, so I'd recommend opening the new/old IndexRegistry files side by side and comparing like that. 🙏
The rest of the commits are style/refactor commits. I'd recommend skimming the renaming changes below and only dipping into the commits themselves if you feel like it.

Renaming changes:

IndexRegistry state vars:
- operatorIdToIndex -> currentOperatorIndex
- _totalOperatorsHistory -> _operatorCountHistory
- _indexToOperatorIdHistory -> _indexHistory
IndexRegistry functions:
- _getTotalOperatorsForQuorumAtBlockNumber -> _operatorCountAtBlockNumber
- _getOperatorIdAtIndexForQuorumAtBlockNumber -> _operatorIdForIndexAtBlockNumber
- getOperatorIndexUpdateOfIndexForQuorumAtIndex -> getOperatorUpdateAtIndex
- getTotalOperatorsForQuorumAtBlockNumberByIndex -> getTotalOperatorsForIndexAtBlockNumber
- getOperatorListForQuorumAtBlockNumber -> getOperatorListAtBlockNumber
StakeRegistry state vars:
- operatorIdToStakeHistory -> operatorStakeHistory
- strategiesConsideredAndMultipliers -> strategyParams
StakeRegistry structs:
- StrategyAndWeightingMultiplier -> StrategyParams
StakeRegistry functions:
- strategiesConsideredAndMultipliersLength -> strategyParamsLength
- strategyAndWeightingMultiplierForQuorumByIndex -> strategyParamsByIndex
- getOperatorIdToStakeHistory -> getOperatorStakeHistory
- getLengthOfTotalStakeHistoryForQuorum -> getTotalStakeHistoryLength
- getTotalStakeUpdateForQuorumFromIndex -> getTotalStakeUpdateAtIndex
- getStakeUpdateIndexForOperatorIdForQuorumAtBlockNumber -> getStakeUpdateIndexForOperatorAtBlockNumber
- getTotalStakeIndicesByQuorumNumbersAtBlockNumber -> getTotalStakeIndicesAtBlockNumber
- getStakeUpdateForQuorumFromOperatorIdAndIndex -> getStakeUpdateForOperatorAtIndex
- getStakeForQuorumAtBlockNumberFromOperatorIdAndIndex -> getOperatorStakeAtBlockNumberAndIndex
- getStakeForOperatorIdForQuorumAtBlockNumber -> getOperatorStakeAtBlockNumber
- getLengthOfOperatorIdStakeHistoryForQuorum -> getOperatorStakeHistoryLength
BLSRegistryCoordinatorWithIndices state vars:
- _quorumOperatorSetParams -> _quorumParams
- _operatorIdToQuorumBitmapHistory -> _operatorBitmapHistory
- _operators -> _operatorInfo
BLSRegistryCoordinatorWithIndices state modifying functions:
- registerOperatorWithCoordinator -> registerOperator
- registerOperatorWithCoordinator -> registerOperatorWithChurn
- deregisterOperatorWithCoordinator -> deregisterOperator
- ejectOperatorFromCoordinator -> ejectOperator
BLSRegistryCoordinatorWithIndices view functions:
- getQuorumBitmapIndicesByOperatorIdsAtBlockNumber -> getQuorumBitmapIndicesAtBlockNumber
- getQuorumBitmapByOperatorIdAtBlockNumberByIndex -> getQuorumBitmapAtBlockNumberByIndex
- getQuorumBitmapUpdateByOperatorIdByIndex -> getQuorumBitmapUpdateByIndex
- getCurrentQuorumBitmapByOperatorId -> getCurrentQuorumBitmap
- getQuorumBitmapUpdateByOperatorIdLength -> getQuorumBitmapHistoryLength
BLSPubkeyRegistry state variable naming changes (these better match the rest of the contracts):
- quorumApk -> currentApk
- quorumApkUpdates -> apkHistory
BLSPubkeyRegistry view function naming changes:
- getApkIndicesForQuorumsAtBlockNumber -> getApkIndicesAtBlockNumber
- getApkForQuorum -> getApk
- getApkUpdateForQuorumByIndex -> getApkUpdateAtIndex
- getApkHashForQuorumAtBlockNumberFromIndex -> getApkHashAtBlockNumberAndIndex
- getQuorumApkHistoryLength -> getApkHistoryLength

IndexRegistry changes:

Refactored index registry history updates to have a more readable register/deregister flow:
- registerOperator is now _increaseOperatorCount + _assignOperatorToIndex. Functional changes:
  - If the most recent QuorumUpdate/OperatorUpdate is from this block, we update the last entry rather than pushing a new one.
- deregisterOperator is now _decreaseOperatorCount + _popLastOperator + _assignOperatorToIndex. Functional changes:
  - As above, histories are updated rather than pushed if the last entry was from the current block.
  - Previously, we assigned currentOperatorIndex[quorum][OPERATOR_DOES_NOT_EXIST_ID] to the removed index, and emitted an event for this update. This logic has been removed, since the currentOperatorIndex of the null id doesn't matter. See previous logic vs new logic

src/IndexRegistry.sol

gpsanant · 2023-10-26T16:45:22Z

src/interfaces/IStakeRegistry.sol

+        uint96 stake;
+    }
+
+    // EVENTS


2 event sections should be made 1

src/StakeRegistry.sol

… unneeded code

…te if last update was in current block

generally i just removed testing of specific stake histories in favor of testing net outcomes. we can revisit these tests in a few weeks.

…ator

… naming and logic

…c and comments for deregistration

…ry. also fix tests

…ator

... which means pubkey parameters for various functions/structs can be removed

…t with other registry contracts

ChaoticWalrus · 2023-11-02T19:49:01Z

src/BLSPubkeyRegistry.sol


            unchecked {
                ++i;
            }
        }
    }

+    // TODO - should this fail if apkUpdate.apkHash == 0? This will be the case for the first entry in each quorum


good question. relatedly, should we actually push the hash of the zero point in the first update, rather than zero per se?
I think this probably shouldn't revert in this case, but am not strongly opinionated

question for @gpsanant

good point, let's push the zero hash

so to confirm, the change is to push the zero hash for the first update, and have this method fail if apkHash == 0?

Should it also fail if apkHash == ZERO_HASH?

src/BLSRegistryCoordinatorWithIndices.sol

ChaoticWalrus · 2023-11-02T21:21:38Z

src/IndexRegistry.sol

+        _indexToOperatorIdHistory[quorumNumber][index].push(OperatorUpdate({
+            operatorId: operatorId,
+            fromBlockNumber: uint32(block.number)
+        }));


it's weird that elsewhere we use 24 bytes (as in the BLSPubkeyRegistry) for the equivalent of operatorId, and here we use 32 bytes. Not sure why this line in particular made me notice this, but it's just...a bit odd?
should operatorIds perhaps be shorter, so we can do the same storage packing everywhere?

ChaoticWalrus · 2023-11-02T21:31:44Z

src/StakeRegistry.sol

+            // TODO - not a huge fan of this dependency on the reg coord, but i do prefer this
+            //        over the stakereg also keeping its own count.


I think this makes sense. it's a bit ugly, but in my mind the registries are basically all "puppets" of the RegistryCoordinator, so calling into the coordinator to fetch a bit of data is fine.
I wouldn't be super opposed to just keeping count in the StakeRegistry either, as it'd be more efficient -- we could just increment a quorumCount variable in the initializeQuorum() function? Then the two can only be out-of-sync if the RegistryCoordinator either fails to call initializeQuorum appropriately or calls it inappropriately.

This is marked TODO because I was intending a future PR to move updateStakes into the registry coordinator. I'm working on that locally and will have something to share next week

ahhh OK that works!

ChaoticWalrus · 2023-11-02T21:33:55Z

src/StakeRegistry.sol

+     * @dev This function has no check to make sure that the strategies for a single quorum have the same underlying asset. This is a concious choice,
+     * since a middleware may want, e.g., a stablecoin quorum that accepts USDC, USDT, DAI, etc. as underlying assets and trades them as "equivalent".


I realize these probably weren't part of this PR (it probably just moves the comments?) but:
typo: 'concious' => 'conscious'
and I think 'trades' should be 'treats'

ChaoticWalrus · 2023-11-02T21:35:07Z

src/StakeRegistry.sol

+    /**
+     * @notice Remove strategies and their associated weights from the quorum's considered strategies
+     * @dev higher indices should be *first* in the list of @param indicesToRemove, since otherwise
+     * the removal of lower index entries will cause a shift in the indices of the other strategies to remove
+     */
+    function removeStrategies(
+        uint8 quorumNumber,
+        uint256[] memory indicesToRemove
+    ) public virtual onlyServiceManagerOwner quorumExists(quorumNumber) {
+        uint256 toRemoveLength = indicesToRemove.length;
+        require(toRemoveLength > 0, "StakeRegistry.removeStrategyParams: no indices to remove provided");
+
+        StrategyAndWeightingMultiplier[] storage strategyParams = strategiesConsideredAndMultipliers[quorumNumber];
+
+        for (uint256 i = 0; i < toRemoveLength; i++) {
+            emit StrategyRemovedFromQuorum(quorumNumber, strategyParams[indicesToRemove[i]].strategy);
+            emit StrategyMultiplierUpdated(quorumNumber, strategyParams[indicesToRemove[i]].strategy, 0);
+
+            // Replace index to remove with the last item in the list, then pop the last item
+            strategyParams[indicesToRemove[i]] = strategyParams[strategyParams.length - 1];
+            strategyParams.pop();
+        }
+    }


@wadealexc thoughts on the safety of this function?
it previously had a redundant parameter which it checked against, to ensure that the desired strategies were removed

What redundant parameter? Here's the master branch: https://github.com/Layr-Labs/eigenlayer-middleware/blob/master/src/VoteWeigherBase.sol#L62

I havent given any thought to the safety of the method... I think that'll come when we start working on integration tests. Essentially we should be testing that stake updates occur as expected even if we interweave strategy modifications in between updates.

sorry, wasn't related to this PR, probably should have asked elsewhere, but here's a link to the previous behavior I was thinking of https://github.com/Layr-Labs/eigenlayer-contracts/blob/bd211f12fe8ed84f691ca44db3f812412defcb1e/src/contracts/middleware/VoteWeigherBase.sol#L134-L138

this could be described as an "idiot-check", but I do have a feeling it'd be easy to call this function "incorrectly" (or at least, in a way that doesn't have the desired outcome) with the way it works now

gotcha. i think there's a footgun here if the method is called incorrectly, but since this is an admin function, i think it's ok. footguns inherently exist when performing "irregular state transitions" like this, and i'd rather keep the methods clean so it's clear what's going on - rather than adding checks that correct for mistakes we make off chain.

worst case we realize we did this incorrectly and we can pause/correct the mistake. but i think any time we call these we should do our due diligence and feel confident that we're doing it correctly.

ChaoticWalrus

Feels like a good change overall.
I made some smaller comments but don't think I have any major points.
Am in agreement with the small open items noted by @gpsanant

…nt with corresponding state lookup

refactor: simplify naming and logic in registries and registry coordinator

refactor: pubkey compendium stores pubkeys

gpsanant reviewed Oct 26, 2023

View reviewed changes

src/IndexRegistry.sol Outdated Show resolved Hide resolved

gpsanant reviewed Oct 26, 2023

View reviewed changes

src/interfaces/IStakeRegistry.sol

uint96 stake;

}

// EVENTS

Copy link

Contributor

gpsanant Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 event sections should be made 1

gpsanant reviewed Oct 26, 2023

View reviewed changes

src/StakeRegistry.sol Outdated Show resolved Hide resolved

wadealexc added 7 commits October 30, 2023 15:01

style: use for loop indices when the index is just an index

70cdc89

refactor: Use a signed delta value in StakeRegistry to remove tons of…

7be39b3

… unneeded code

style: use over , and simplify

27568d2

feat: only update total history for nonzero delta, and dont push upda…

d5f0241

…te if last update was in current block

style: use uint256 in registry coordinator

5f49736

test: fix broken tests for StakeRegistry changes

a0d2c34

generally i just removed testing of specific stake histories in favor of testing net outcomes. we can revisit these tests in a few weeks.

refactor: wip refactor to move "createQuorum" to the registry coordin…

b4709cb

…ator

wadealexc force-pushed the alex/refactor-quorums branch from 596f20f to b4709cb Compare October 30, 2023 15:04

wadealexc added 13 commits October 30, 2023 15:39

merge into m2-mainnet

50e4bca

chore: update core contract submodule

066a6dc

fix: fix compilation issues and tests

12b09de

refactor: wip refactor for index and blspubkey registries to simplify…

aa1d513

… naming and logic

style: pull out common logic to a helper method

27a3cba

style: pulled additional logic out into a helper method

708acb5

refactor: simplify registry coord state variable names and clean logi…

21f97b1

…c and comments for deregistration

fix: enforce invariant that existing indice have nonzero length histo…

60ccc13

…ry. also fix tests

style: shorten state variable and function naming in registry coordin…

88abb15

…ator

style: shorten state variable and function naming in stake registry

5cd0d96

style: remove unused index registry function

698d818

refactor: pk compendium stores operator pubkeys and can look them up

b883fd0

... which means pubkey parameters for various functions/structs can be removed

style: shorten state variable and function names to be more consisten…

5968dcf

…t with other registry contracts

wadealexc mentioned this pull request Nov 2, 2023

refactor: pubkey compendium stores pubkeys #51

Merged

wadealexc marked this pull request as ready for review November 2, 2023 19:33

ChaoticWalrus reviewed Nov 2, 2023

View reviewed changes

src/BLSRegistryCoordinatorWithIndices.sol Outdated Show resolved Hide resolved

ChaoticWalrus reviewed Nov 2, 2023

View reviewed changes

ChaoticWalrus approved these changes Nov 2, 2023

View reviewed changes

wadealexc and others added 5 commits November 6, 2023 15:38

style: removed redundant check and swapped param order to be consiste…

231bbe3

…nt with corresponding state lookup

Merge pull request #43 from Layr-Labs/alex/refactor-registries

c5d5dc8

refactor: simplify naming and logic in registries and registry coordinator

Merge branch 'alex/refactor-quorums' into alex/refactor-pubkey-registry

1adb1d6

style: remove redundant checks from pubkey compendium

bb4011e

Merge pull request #51 from Layr-Labs/alex/refactor-pubkey-registry

9a060e5

refactor: pubkey compendium stores pubkeys

wadealexc merged commit a4b039e into m2-mainnet Nov 6, 2023
1 check passed

wadealexc deleted the alex/refactor-quorums branch November 6, 2023 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: quorum creation is the divine right of the registry coordinator #28

refactor: quorum creation is the divine right of the registry coordinator #28

wadealexc commented Oct 26, 2023 •

edited

Loading

gpsanant Oct 26, 2023

ChaoticWalrus Nov 2, 2023

wadealexc Nov 2, 2023

gpsanant Nov 6, 2023

wadealexc Nov 6, 2023

ChaoticWalrus Nov 2, 2023

ChaoticWalrus Nov 2, 2023

wadealexc Nov 2, 2023

ChaoticWalrus Nov 2, 2023

ChaoticWalrus Nov 2, 2023

ChaoticWalrus Nov 2, 2023

wadealexc Nov 2, 2023

wadealexc Nov 2, 2023

ChaoticWalrus Nov 2, 2023

wadealexc Nov 6, 2023

ChaoticWalrus left a comment

		// TODO - not a huge fan of this dependency on the reg coord, but i do prefer this
		// over the stakereg also keeping its own count.

		* @dev This function has no check to make sure that the strategies for a single quorum have the same underlying asset. This is a concious choice,
		* since a middleware may want, e.g., a stablecoin quorum that accepts USDC, USDT, DAI, etc. as underlying assets and trades them as "equivalent".

refactor: quorum creation is the divine right of the registry coordinator #28

refactor: quorum creation is the divine right of the registry coordinator #28

Conversation

wadealexc commented Oct 26, 2023 • 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

ChaoticWalrus left a comment

Choose a reason for hiding this comment

wadealexc commented Oct 26, 2023 •

edited

Loading