Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat(M2-Mainnet): StakeRegistry pull updates per quorum #62

Merged
merged 21 commits into from
Dec 8, 2023

Conversation

8sunyuan
Copy link
Collaborator

@8sunyuan 8sunyuan commented Nov 9, 2023

Description:
Due to gas costs for updating all operators in all quorums, we will implement pull updates on a per quorum basis.
More specifically we can first call updateOperatorsPerQuorum() for quorum0 anticipating a larger number of operators and strategies and then updateOperatorsPerQuorum() for the remaining quorums.
Note: With this in place for m2-mainnet, we will also remove all push updates in the DelegationManager

Changes:

  • RegistryCoordinator.updateOperatorsPerQuorum() which updates stakes for all operators for the specified quorums. Operators must be passed in based on ascending operatorId per quorum to ensure uniqueness for length checks.
  • The StakeRegistry now keeps track of the last update timestamps for each quorum in the mapping quorumUpdateTimestamp.
    This is only updated if all operators are updated at once from calling updateOperatorsPerQuorum() which calls StakeRegistry.updateQuorumTimestamp(uint8 quorumNumber)
  • BLSSignatureChecker.checkSignatures now ensures that for each quorum, the quorumUpdateTimestamp isn't staler than a week.

src/StakeRegistry.sol Outdated Show resolved Hide resolved
@wadealexc
Copy link
Collaborator

Overall, fine with these changes - but I wonder if updateOperators and updateOperatorsPerQuorum can be refactored to use a common helper method. They're almost identical.

Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

left a few questions, nothing blocking except for updating tests!

would not recommend writing new tests unless you want to dive into middleware tests in general. if you do want that, DM me and ill get you up to speed

Copy link
Contributor

@ChaoticWalrus ChaoticWalrus left a comment

Choose a reason for hiding this comment

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

Looking good!
Left some minor feedback in comments.

@wadealexc
Copy link
Collaborator

I think using the modifier makes it hard to use this check in other places, so I did a quick refactor and moved it to a helper. Here's my take; you're welcome to squash merge into this PR if you like it: #64

@wadealexc
Copy link
Collaborator

I'm otherwise fine with these changes for now, but may want to revisit when we start writing tests for the reg coord. I can't help but feel that with this additional method, we can shift some stuff around a bit.

updateOperators and updateOperatorsPerQuorum feel similar enough that I want to say there's a helper method that can support both.

And _deregisterOperator is now frequently called with a slice of 1 byte. I wonder if that can be improved somehow.

@8sunyuan
Copy link
Collaborator Author

I'm otherwise fine with these changes for now, but may want to revisit when we start writing tests for the reg coord. I can't help but feel that with this additional method, we can shift some stuff around a bit.

updateOperators and updateOperatorsPerQuorum feel similar enough that I want to say there's a helper method that can support both.

And _deregisterOperator is now frequently called with a slice of 1 byte. I wonder if that can be improved somehow.

Good points on potential refactoring, before going forward with these changes I've got to first get some gas estimations again on the new functionality.


// Update timestamp that all operators in quorum have been updated all at once
quorumUpdateBlocknumber[quorumNumber] = block.number;
emit QuorumBlocknumberUpdated(quorumNumber, block.number);
Copy link
Contributor

Choose a reason for hiding this comment

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

QuorumUpdateBlockNumberUpdated? lol idk

@8sunyuan 8sunyuan force-pushed the feat/update-operators-per-quorum branch from 6993116 to d1e4274 Compare December 1, 2023 21:05
Additionally
- renamed updateOperatorsPerQuorum to updateOperatorsForQuorum
- sort by operator addresses instead of operatorIds
@8sunyuan 8sunyuan force-pushed the feat/update-operators-per-quorum branch from 85cb66c to e313d34 Compare December 8, 2023 15:55
Comment on lines +553 to +555
if (operatorInfo.status != OperatorStatus.REGISTERED) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I made an earlier comment that we should have this check for both methods and that it didn't cost extra because we're already SLOADing the operatorId

But I'm realizing that's actually not the same slot - the operatorId is 32 bytes, so the status is its own SLOAD.

If we want to save an extra SLOAD for each operator updated in updateOperatorsForQuorum, we could remove the REGISTERED check, put that solely in updateOperators, and just pass the operatorId into this helper method rather than the entire struct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm sorta ambivalent - I like the readability but IIRC gas was a concern so I'm fine with this concept if we feel it's worth it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For weekly avs-sync, 200 operators in quorum0 and 60 operators in 9 remaining quorums, that's ~150,000 gas from the SLOADS. Maybe worth considering removing the check but I also prefer the explicitness

Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

One major fix and a handful of minor tweaks

@@ -72,6 +84,12 @@ contract BLSSignatureChecker is IBLSSignatureChecker {
// loop through every quorumNumber and keep track of the apk
BN254.G1Point memory apk = BN254.G1Point(0, 0);
for (uint i = 0; i < quorumNumbers.length; i++) {
if (staleStakesForbidden) {
require(
registryCoordinator.quorumUpdateBlockNumber(uint8(quorumNumbers[i])) + delegation.withdrawalDelayBlocks() <= block.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we just store delegation.withdrawalDelayBlocks() as an immutable if we've taken out the ability to change it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DelegationManager could still get upgraded and reinitialize withdrawalDelayBlocks again so I'm assuming its better we just read off of DelegationManager

*/
function setStaleStakesForbidden(bool value) external onlyServiceManagerOwner {
staleStakesForbidden = value;
emit StaleStakesForbiddenUpdate(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

prefer rename value to _staleStakesForbidden

Copy link
Collaborator

Choose a reason for hiding this comment

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

why? it's public so it matches the getter

we're really inconsistent about this throughout both repos 😓

Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

LGTM after fixes!

@8sunyuan 8sunyuan merged commit 2f64af7 into m2-mainnet Dec 8, 2023
2 of 3 checks passed
@8sunyuan 8sunyuan deleted the feat/update-operators-per-quorum branch December 8, 2023 22:12
stevennevins pushed a commit that referenced this pull request Dec 18, 2023
* feat: initial implementation w/o timestamp

* feat: enforce stake updates for quorum in sigcheck

* fix: update operator stake for just one quorum

Additionally
- renamed updateOperatorsPerQuorum to updateOperatorsForQuorum
- sort by operator addresses instead of operatorIds

* fix: requested changes and optimizations

- sorted by ascending address instead of operatorId
- sanitization checks on quorumNumbers, added quorumsAllExist modifier
- performing `OperatorStatus.REGISTERED` checks now
- Removed uneccesary bitmapToBytes calls and doing bytes slicing for quorumNumbers
- using internal function and scoping to fix stack-too-deep

* refactor: move quorumUpdateTimestamp to reg coord

* refactor: remove modifier in favor of helper method (#64)

* fix: initializedQuorumBitmap fix

small off by 1 error, for example of quorumCount = 1,3
1 << 1 = 2, bit rep is 10
1 << 1 - 1 = 1, bit rep is 01

1 << 3 = 8, bit rep is 1000
1 << 3 - 1 = 7, bit rep is 0111
We should be subtracting by 1 instead of 2 here to get the bitmap

* fix: bitshifting error, needs to shift first before subtracting

* fix: moved 1 weeks to a constant for now

May change this in the future to be configurable

* chore: require statement

* refactor: move shared logic to `_updateOperator`

* refactor: added status check to `_updateOperator`

* feat: Delegation.withdrawalDelayBlocks in sigcheck

* fix: use blocknumber instead of timestamp

* fix: unused constant and require error

* feat: timestamp requirement is able to be toggled

* fix: interface imports

* chore: renamed to staleStakesForbidden

* chore: typo with BlockNumber

* fix: rebase remove deleted file

* fix: prevOperatorAddress not being updated

---------

Co-authored-by: Alex <18387287+wadealexc@users.noreply.github.com>
stevennevins added a commit that referenced this pull request Dec 18, 2023
* fix: remove husky command that only needs to be run once

* update nonSignerForQuorumIndex outside of loop

* chore: add MacOS indexing file to gitignore

* chore: correct addresses for M2 Goerli deployment

switch these addresses over from pre-prod environment to the addresses that EigenDA is using now

* chore: test out relative import

* chore: all relative imports

* chore: fix remaining relative imports

* fix: update missed import

* fix: additional missed imports

* Update README.md to avoid confusing phrasing (#75)

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

* refactor: Use a signed delta value in StakeRegistry to remove tons of unneeded code

* style: use  over , and simplify

* feat: only update total history for nonzero delta, and dont push update if last update was in current block

* style: use uint256 in registry coordinator

* test: fix broken tests for StakeRegistry changes

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 coordinator

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

* refactor: Use a signed delta value in StakeRegistry to remove tons of unneeded code

* style: use  over , and simplify

* feat: only update total history for nonzero delta, and dont push update if last update was in current block

* style: use uint256 in registry coordinator

* test: fix broken tests for StakeRegistry changes

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

* fix: fix compilation issues and tests

* refactor: wip refactor for index and blspubkey registries to simplify naming and logic

* style: pull out common logic to a helper method

* style: pulled additional logic out into a helper method

* refactor: simplify registry coord state variable names and clean logic and comments for deregistration

* fix: enforce invariant that existing indice have nonzero length history. also fix tests

* style: shorten state variable and function naming in registry coordinator

* style: shorten state variable and function naming in stake registry

* style: remove unused index registry function

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

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

* style: shorten state variable and function names to be more consistent with other registry contracts

* style: removed redundant check and swapped param order to be consistent with corresponding state lookup

* style: remove redundant checks from pubkey compendium

* fix: replace borked state variable

* refactor: move updateStakes to registry coordinator

* fix: fixes several compiler issues

still running into "stack too deep"

* fix: stack too deep issue addressed

also addressed review comments:
- natspec comments in IStakeRegistry
- incorrect boolean in StakeRegistry

* fix: actually commit natspec changes

* style: stakeregistry naming is more consistent with other contracts

* style: rename upper bound in bitmap utils, and rename Operator to OperatorInfo

* fix: operator has minimum if weight is equal to minimum

* test: update tests to support refactor

* style: reword comments and remove duplicate check

* style: remove unneeded parenthesis and fix _registerOperator naming

* style: rename OperatorStakeUpdate struct to StakeUpdate, and swap event to match (#61)

* cleanup imports (#78)

* chore: merge master into m2-mainnet (#79)


---------

Co-authored-by: steven <steven.nevins@eigenlabs.org>
Co-authored-by: gpsanant <gpsanant@gmail.com>
Co-authored-by: ChaoticWalrus <93558947+ChaoticWalrus@users.noreply.github.com>
Co-authored-by: Gautham Anant <32277907+gpsanant@users.noreply.github.com>

* build: point eigenlayer gitmodule to m2-mainnet (#80)

* ci: add rule to run CI on m2-mainnet branch

* chore: remove old tree file (#83)

Co-authored-by: steven <steven.nevins@eigenlabs.org>

* chore: rename several contracts and fix interfaces and tests (#95)

- BLSRegistryCoordinatorWithIndices is now RegistryCoordinator
- BLSOperatorStateRetriever is now OperatorStateRetriever
- BLSPubkeyRegistry is now BLSApkRegistry

* chore: copy BLSPublicKeyCompendium storage and functions into BLSApkRegistry

* chore: delete BLSPublicKeyCompendium and associated interface

requires changing a bunch of files to make things work.
I had to comment out a few calls in tests (now marked with a `TODO`) to get this past the compiler.
Will have to do additional cleanup to get this ready for review.

* feat: create harness for BLSApkRegistry and use it in tests

These changes appear to fix all existing tests.
I've done what I can to at least comment specifically where harnessed functions are used, as this pattern could give some false assurance and I'd like to avoid that.

* chore: delete unused mock file

I created this mock while trying to sort out compilation issues and test failures
This mock is no longer being used anywhere, so I am deleting it for cleanup

* chore: cleanup unused file and reduce mutability of test utils functions

this commit fixes a ton of compiler warnings on build, in particular.

* feat: make BLSApkRegistry. registerBLSPublicKey only callable by RegistryCoordinator

also make a version of `RegistryCoordinator. registerOperator`
that accepts these inputs
new function passes these inputs on appropriately

TODO:
remove old `registerOperator` function and fix associated tests

* chore: remove `RegistryCoordinator.registerOperator` method without pubkey registration inputs

also fix tests to use the new method

* feat: create struct for pubkey registration params

note: code is currently not compiling due to a stack-too-deep error
error here:
 src/RegistryCoordinator.sol:254:69:
    |
254 |                 _deregisterOperator(operatorKickParams[i].operator, quorumNumbers[i:i+1]);

* fix: address stack-too-deep issue in RegistryCoordinator

* chore: delete unused index

see reasoning here
#100 (comment)
this allows undoing a previous fix for a stack-too-deep error, which...
helps make the logic a little clearer / easier to parse here.

* feat: optimization for fetching operatorId

NOTE: this changes the IBLSApkRegistry interface
to make `registerBLSPublicKey` return a bytes32 object

* chore: shorten variable name that is quite clear from context

`pubkeyRegistrationParams` => `params`
see discussion here:
#100 (comment)

* feat: use EIP712 for the pubkeyRegistrationMessageHash

this commit also moves the definition to the RegistryCoordinator contract
and updates the function naming to be clearer

* chore: add NatSpec to getter function

* Feat(M2-Mainnet): StakeRegistry pull updates per quorum (#62)

* feat: initial implementation w/o timestamp

* feat: enforce stake updates for quorum in sigcheck

* fix: update operator stake for just one quorum

Additionally
- renamed updateOperatorsPerQuorum to updateOperatorsForQuorum
- sort by operator addresses instead of operatorIds

* fix: requested changes and optimizations

- sorted by ascending address instead of operatorId
- sanitization checks on quorumNumbers, added quorumsAllExist modifier
- performing `OperatorStatus.REGISTERED` checks now
- Removed uneccesary bitmapToBytes calls and doing bytes slicing for quorumNumbers
- using internal function and scoping to fix stack-too-deep

* refactor: move quorumUpdateTimestamp to reg coord

* refactor: remove modifier in favor of helper method (#64)

* fix: initializedQuorumBitmap fix

small off by 1 error, for example of quorumCount = 1,3
1 << 1 = 2, bit rep is 10
1 << 1 - 1 = 1, bit rep is 01

1 << 3 = 8, bit rep is 1000
1 << 3 - 1 = 7, bit rep is 0111
We should be subtracting by 1 instead of 2 here to get the bitmap

* fix: bitshifting error, needs to shift first before subtracting

* fix: moved 1 weeks to a constant for now

May change this in the future to be configurable

* chore: require statement

* refactor: move shared logic to `_updateOperator`

* refactor: added status check to `_updateOperator`

* feat: Delegation.withdrawalDelayBlocks in sigcheck

* fix: use blocknumber instead of timestamp

* fix: unused constant and require error

* feat: timestamp requirement is able to be toggled

* fix: interface imports

* chore: renamed to staleStakesForbidden

* chore: typo with BlockNumber

* fix: rebase remove deleted file

* fix: prevOperatorAddress not being updated

---------

Co-authored-by: Alex <18387287+wadealexc@users.noreply.github.com>

* feat: issue templates (#87)

Co-authored-by: steven <steven.nevins@eigenlabs.org>

* fix typo BLSPublicKeyCompendium.md

* fix typo BLSSignatureChecker.md

* fix typo StakeRegistry.md

* fix typo README.md

* chore: remove ServiceManagerBase and add RegistryCoordinator owner (#98)

* Feat: Add AVS/Operator Registration Support in RegistryCoordinator (#99)

* chore: fix merge artifacts / broken calls

these weren't caught in merging but were causing compiler errors

* chore: re-implement stack-too-deep fix

I previously implemented this fix, then reverted it as not needed.
Now upon merging with m2-mainnet, `registerOperatorWithChurn` has an additional input.
This is reintroducing the same stack-too-deep error
so I am reintroducing the same fix (one less local var)

* chore: de-duplicate code into an internal function

see suggestion here:
#102 (comment)

saves ~0.25kb in code size

* fix: actually use the `operator` input to `_getOrCreateOperatorId`

also reduce memory copying operations (I think, at least) in
the `registerOperator` function
specifically, only copy the `numOperatorsPerQuorum` returned by
the internal `_registerOperator` function

* feat: add optimizer runs count to foundry config

* chore: remove redundant check and return data

The `operatorId` in this check is already fetched from the `blsApkRegistry`
With the other changes, this return data is no longer necessary at all

* chore: fewer memory operations(?)

I believe this change cuts down on the memory copying being done here

* fix: reduce optimizer runs to meet contract code size limits

Likely not the ideal fix, but it gets the job done for the moment, at least.

* chore: rename file to reflect it only being used in tests

`RegistryCoordinatorHarness.sol` -> `RegistryCoordinatorHarness.t.sol`

* chore: delete unused (memory) variable

* fix: have the harness import Test so it gets ignored in build sizes

* fix: remove circular dependency (#108)

* fix: address multiple issues in BLSSignatureChecker, optimize, and clean (#104)

* fix: swap sign on sig checker withdrawal delay check

* fix: tiny_scalar_mul in BN254 and additional cleanup

- unify validateXAtBlockNumber style checks in registries
- checkSignatures changes below
- rename variables to be more readable
- validate input lengths for quorum and nonsigner params
- create signingQuorumBitmap using method with additional validation to avoid duplicates
- clarify commenting and style to be more consistent with rest of codebase
- cache state to avoid lookups in loop
- move total stake query to initial loop

* fix: refactor checkSignatures to remove an unneeded loop

- also optimizes BN254.hashG1Point
- also refactors checkSignatures to avoid negating pubkeys multiple times

* fix: stale stakes check should care about referenceBlockNumber

* fix: remove repeated storage read from loop

* chore: remove TODOs

* docs: add wip docs and clean main README (#96)

* docs: add docs very wip

* docs: wip add docs and main README

* docs: wip main README

* docs: more wip

* docs: get docs to a good ish state

* docs: address feedback

* chore: add eigenda goerli deployment (#111)

* feat: concentrate eigenlayer avs interactions into service manager (#109)

* feat: reintroduce IServiceManager & ServiceManagerBase

These are useful concepts to form the single interaction point
where AVSs push data to EigenLayer.
The previous version focused on (unused) Slashing interactions,
whereas this version focused on the newer registration interactions.
TODO: modify the RegistryCoordinator to call the ServiceManagerBase
instead of calling the DelegationManager directly.

* feat: integrate RegistryCoordinator with ServiceManager(Base)

achieves the goal of a single interaction point with EigenLayer (per AVS).
all calls which push data to EigenLayer core are now forwarded via the ServiceManager(Base)

* fix: use correct addresses in test setup + functions

simple follow-up commit to fix breaking tests from last commit

* chore: restore optimizer_runs to 200

other changes in this PR remove enough from the size
of the RegistryCoordinator contract to make this possible.
200 runs is a wider industry default +
this should slightly decrease gas costs for users.
we can switch back to 100 in the future if we ever need to

* chore: add `setMetadataURI` function to IServiceManager interface

* feat: add operator strategy indexing to service manager (#110)

---------

Co-authored-by: Yash Patil <40046473+ypatil12@users.noreply.github.com>

* fix: correctly set bit in stake registry (#112)

* feat: add restakeable strategies to service manager for indexing (#113)

* chore: resolve imports

* fix: remove eigenlayer deployer

* chore: additional relative imports

* chore: remove whitespace diff

* chore: clean up remappings

* chore: remove whitespace diff

* chore: stragglers

---------

Co-authored-by: steven <steven.nevins@eigenlabs.org>
Co-authored-by: Alex <18387287+wadealexc@users.noreply.github.com>
Co-authored-by: gpsanant <gpsanant@gmail.com>
Co-authored-by: ChaoticWalrus <93558947+ChaoticWalrus@users.noreply.github.com>
Co-authored-by: Gautham Anant <32277907+gpsanant@users.noreply.github.com>
Co-authored-by: Bowen Li <bowenli86@gmail.com>
Co-authored-by: wadealexc <pragma-services@proton.me>
Co-authored-by: quaq <56312047+0x0aa0@users.noreply.github.com>
Co-authored-by: Michael Sun <35479365+8sunyuan@users.noreply.github.com>
Co-authored-by: iwantanode <87604944+tudorpintea999@users.noreply.github.com>
Co-authored-by: Yash Patil <40046473+ypatil12@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants