feat: experimental ecdsa registries #123

ChaoticWalrus · 2023-12-28T09:41:46Z

Extremely rough draft contracts that I threw together in ~90 minutes.

Do NOT use these contracts without some kind of review.

Comments in the contracts are definitely a bit of a mess -- see the commit messages for a brief summary of work here.

comments are in a poor state no interface or separated storage definitions eliminated historical storage behavior is otherwise changed ~minimally from BLS version

basically combines the RegistryCoordinator, IndexRegistry, and APK registry functionality removes historical storage comments and documentation are definitely incorrect

again no interface or historical storage comments are definitely wrong signatory record hash may no longer be useful implementation might have critical safety errors this just queries the current stakes and adds together signing stakes

classic i <> j mix up, ya know? there could definitely be more of these lurking.

samlaf

Seems like an awesome first start, thanks for this.
One big area of complexity that would be nice to get rid of is the churner, but unsure if there are alternatives that have been considered that could work better with this simpler registry?

samlaf · 2024-01-04T21:37:47Z

src/ECDSASignatureChecker.sol

+ * @notice Terms of Service: https://docs.eigenlayer.xyz/overview/terms-of-service
+ * @notice This is the contract for checking the validity of aggregate operator signatures.
+ */
+contract BLSSignatureChecker {    


Update contract name. also ctrl-f "aggregate" and "bls" and remove any reference.

Yes this would need a lot of cleanup w/r/t naming and comments, in particular

samlaf · 2024-01-05T21:29:54Z

src/ECDSARegistryCoordinator.sol

+    /// @notice the ServiceManager for this AVS, which forwards calls onto EigenLayer's core contracts
+    IServiceManager public immutable serviceManager;
+    /// @notice the Stake Registry contract that will keep track of operators' stakes
+    IStakeRegistry public immutable stakeRegistry;


ECDSAStakeRegistry does not implement IStakeRegistry. Should this be ECDSAStakeRegistry instead of IStakeRegistry? Or should ECDSAStakeRegistry implement IStakeRegistry?

I was super liberal with removing inheritance for the sake of expediency here.
It's very possible the integration between these contracts straight-up doesn't work rn, but it would be pretty quick to check if the ECDSAStakeRegistry implements all the functions we're calling from the IStakeRegistry interface (I think it does?)
If you want this developed further, then the path forward isn't immediately clear to me.
Perhaps making a new IECDSAStakeRegistry interface or something would make sense, or these changes could inform reducing the scope of the IStakeRegistry interface so both can fit into it, and then the more specific IECDSAStakeRegistry and IBLSStakeRegistry interfaces (if these need to exist at all!) can both inherit from this minimal interface.

samlaf · 2024-01-23T21:56:42Z

src/ECDSASignatureChecker.sol

+     * @return quorumStakeTotals is the struct containing the total and signed stake for each quorum
+     * @return signatoryRecordHash is the hash of the signatory record, which is used for fraud proofs
+     */
+    function checkSignatures(


Implementing the ecdsa version of inc-sq using these contracts and just realized this doesn't have a referenceBlockNumber like the bls version. Does this mean that the ecdsa stakeregistry doesn't track weight histories and can only validate against current weights?

in this quick + hacked-together version I completely removed histories, so yes, you are exactly correct

samlaf · 2024-01-23T22:12:58Z

src/ECDSASignatureChecker.sol

+    function checkSignatures(
+        bytes32 msgHash, 
+        bytes calldata quorumNumbers,
+        bytes32[] memory signerIds,


should we be calling signerIds signerAddresses instead?
They are verified in

checkSignature_EIP1271(address signer, bytes32 digestHash, bytes memory signature)

and passed as the signer (an address)

fine by me!

samlaf

Also realizing we might need an ECDSAOperatorStateRetriever?

ChaoticWalrus · 2024-01-24T00:22:53Z

Also realizing we might need an ECDSAOperatorStateRetriever?

yes, this would also likely be a practical or even necessary addition.

samlaf · 2024-01-25T01:46:23Z

src/ECDSARegistryCoordinator.sol

+        // the id of the operator, which is likely the keccak256 hash of the operator's public key if using BLSRegistry
+        bytes32 operatorId;


Still a bit confused as to whether this should be stored as a bytes32 or an address.

which is likely the keccak256 hash of the operator's public key if using BLSRegistry

shows that this was copied from the BLSRegistry. Pretty convinced this should be address here right? (hash of ecdsa pubkey IS the ecdsa address by defn)

samlaf · 2024-01-25T01:48:00Z

src/ECDSASignatureChecker.sol

+            }
+
+            // check the operator's signature
+            EIP1271SignatureUtils.checkSignature_EIP1271(address(uint160(uint256(signerIds[i]))), msgHash, signatures[i]);


related to my previous comments, if we store the operatorId as operatorAddr instead (literally type address instead of bytes32), we don't need to do this conversion here (address(uint160(uint256))). Afaik there's no other use for storing the operatorId as a bytes32 instead of an address?

Changed it in my branch and everything checked out.

samlaf

Sorry for bombarding you @ChaoticWalrus with my every passing thoughts :P

Seems like the main problem I'm facing right now is resulting from the combination of:

no "epoch" history (only the current state of oeprators is stored in contracts)
no "epoch" timeline - aka permissionless entry/exit, at any moment
So any avs would be totally broken if they start aggregating signatures offchain (think eigenDA) and then some operator joins/leaves.

This is related to the new avs-sync proposal I've done for churning operators. Basically there seems to be an epoch spectrum:
"fluid" (reactive) epochs -------------------------> hard epochs (after P&S)
current eigenDA bft chain with epoch-based consensus

If we are moving to a hard-epoch based system after P&S, feels like eigenDA will also need to follow suit and adapt (but maybe not?). I was thinking new operators joining could just be added to the next epoch's list, and we change the list pointer (similar to doing a green-blue deployment) at the epoch boundary... but maybe this doesn't work for smart contracts as it would require another tx for all the operators?

Would like some help thinking through this in more detail whenever you have time.

samlaf · 2024-01-25T22:00:25Z

src/ECDSARegistryCoordinator.sol

+ * @title A `RegistryCoordinator` that has three registries:
+ *      1) a `StakeRegistry` that keeps track of operators' stakes
+ *      2) a `BLSApkRegistry` that keeps track of operators' BLS public keys and aggregate BLS public keys for each quorum
+ *      3) an `IndexRegistry` that keeps track of an ordered list of operators for each quorum


Was trying to add an OperatorStateRetriever for ecdsa inc-sq and realized its no longer possible (without the indexRegistry) to get a list of operators.

ahahahah 😅
OK guess I went a little too hard on removing things.
good to point out the need for this (or equivalent)
but if we are doing a "historyless" implementation then we could likely have a much-more-minimal version of the IndexRegistry
or you could just say "this is designed for users to have to index the operators off-chain" and the equivalent OperatorStateRetriever would require you to input specific operator addresses

ChaoticWalrus · 2024-01-25T23:00:38Z

Sorry for bombarding you @ChaoticWalrus with my every passing thoughts :P

Seems like the main problem I'm facing right now is resulting from the combination of:
1. no "epoch" history (only the current state of oeprators is stored in contracts)

2. no "epoch" timeline - aka permissionless entry/exit, at any moment
   So any avs would be totally broken if they start aggregating signatures offchain (think eigenDA) and then some operator joins/leaves.
This is related to the new avs-sync proposal I've done for churning operators. Basically there seems to be an epoch spectrum: "fluid" (reactive) epochs -------------------------> hard epochs (after P&S) current eigenDA bft chain with epoch-based consensus

If we are moving to a hard-epoch based system after P&S, feels like eigenDA will also need to follow suit and adapt (but maybe not?). I was thinking new operators joining could just be added to the next epoch's list, and we change the list pointer (similar to doing a green-blue deployment) at the epoch boundary... but maybe this doesn't work for smart contracts as it would require another tx for all the operators?

Would like some help thinking through this in more detail whenever you have time.

You have a ton of ideas here; agreed we should chat about them.
I'll try to give the bulk of your ideas thought here -- I've read what you've been writing but haven't poured more effort into this.
To respond to one thing in particular:

   So any avs would be totally broken if they start aggregating signatures offchain (think eigenDA) and then some operator joins/leaves.

Why do you think this? If you submit a bunch of ECDSA signatures at once and an operator isn't registered, just add 0 to the total "stake/weight" here (or otherwise skip some calculation steps). If an operator newly registered and you don't have their signature...there's no issue I can see at all.
These race condition kind of problems are actually way worse with the BLS scheme we're using and were a motivator for a ton of the historical storage of our contracts, which is part of why I aggressively ripped historical storage out in this PR.

samlaf · 2024-01-25T23:13:49Z

Why do you think this? If you submit a bunch of ECDSA signatures at once and an operator isn't registered, just add 0 to the total "stake/weight" here (or otherwise skip some calculation steps). If an operator newly registered and you don't have their signature...there's no issue I can see at all.

wdym by "just add 0"? where would this be? in the contract? or in the aggregator offchain code?
The way inc-sq works is that a task request is submitted to the contract, and the task response is accepted only if the threshold is met wrt the stake weight of operators at the time the task was submitted. Given that we don't have history of these updates, if a new operator joins with a bunch of stake, then that totally changes the distrbution, so a signature that would have been accepted might get rejected.

ChaoticWalrus · 2024-01-26T01:34:24Z

wdym by "just add 0"? where would this be? in the contract? or in the aggregator offchain code?
The way inc-sq works is that a task request is submitted to the contract, and the task response is accepted only if the threshold is met wrt the stake weight of operators at the time the task was submitted. Given that we don't have history of these updates, if a new operator joins with a bunch of stake, then that totally changes the distrbution, so a signature that would have been accepted might get rejected.

ahhhh I missed this latter point but you're totally right. But TBH I think these issues might be acceptable -- just gather 60% of the signatures instead of 50% or w/e and the probability that your tx reverts is tiny

samlaf · 2024-01-29T19:17:57Z

I implemented an ecdsa aggregation service in services/ecdsa/aggregation/agg.go and started wondering whether there's a way to do ecdsa verification without having a centralized aggregator, aka every operator just signs and sends their signature to the contract. Seems like this would require more gas because each time a new signature comes the contract needs to calculate and store the current stake, until it meets threshold. With update histories like I used in that PR (which uses the ecdsa-with-history-poc branch) the contract would also need to be passed for each validator its update history indices to fetch its correct stake, and we'd have to make sure all operators are using the same referenceBlockNumber.

Just curious in general what are people's thoughts around this, since aggregating in the smart contract seems like an easy way to get rid of the centralized aggregator role (wihout having to deal with a p2p network and leader aggregating - what happens if a task is not finished aggregating and then leader changes, etc). I think gnosis safes also work with a centralized aggregator though?

ChaoticWalrus · 2024-01-29T19:26:12Z

I implemented an ecdsa aggregation service in services/ecdsa/aggregation/agg.go and started wondering whether there's a way to do ecdsa verification without having a centralized aggregator, aka every operator just signs and sends their signature to the contract. Seems like this would require more gas because each time a new signature comes the contract needs to calculate and store the current stake, until it meets threshold. With update histories like I used in that PR (which uses the ecdsa-with-history-poc branch) the contract would also need to be passed for each validator its update history indices to fetch its correct stake, and we'd have to make sure all operators are using the same referenceBlockNumber.

Just curious in general what are people's thoughts around this, since aggregating in the smart contract seems like an easy way to get rid of the centralized aggregator role (wihout having to deal with a p2p network and leader aggregating - what happens if a task is not finished aggregating and then leader changes, etc). I think gnosis safes also work with a centralized aggregator though?

Technically Gnosis Safes support both, but their UI uses a centralized aggregator (at least by default).
I think "aggregatorless" solutions would be great to develop though! I can see some of how it interacts with other architecture choices too.
Overall I would expect this type of thing to cost more gas pretty much regardless, so in terms of prioritization I'd be curious to understand the demand here.

src/ECDSARegistryCoordinator.sol

ChaoticWalrus · 2024-03-15T22:23:21Z

I think this was a useful exercise but am not seeing taking this particular branch/PR further, at least for months+.
Seems like #194 accomplishes the goals of this PR, but more fully (albeit with other changes).
So I will close this PR for the moment; the git history will continue to exist, should anyone ever want to pick this particular direction up again.

ChaoticWalrus added 4 commits December 28, 2023 00:31

feat: (very) rough draft ECDSA StakeRegistry

f2c6b5c

comments are in a poor state no interface or separated storage definitions eliminated historical storage behavior is otherwise changed ~minimally from BLS version

feat: (very) rough draft ECDSA RegistryCoordinator

eb5b4da

basically combines the RegistryCoordinator, IndexRegistry, and APK registry functionality removes historical storage comments and documentation are definitely incorrect

feat: (very) rough draft ECDSA SignatureChecker

3635f6a

again no interface or historical storage comments are definitely wrong signatory record hash may no longer be useful implementation might have critical safety errors this just queries the current stakes and adds together signing stakes

fix: use correct index

c95f663

classic i <> j mix up, ya know? there could definitely be more of these lurking.

samlaf reviewed Jan 4, 2024

View reviewed changes

samlaf reviewed Jan 5, 2024

View reviewed changes

samlaf reviewed Jan 23, 2024

View reviewed changes

samlaf reviewed Jan 25, 2024

View reviewed changes

samlaf mentioned this pull request Jan 25, 2024

Feat ecdsa sigs Layr-Labs/eigensdk-go#104

Closed

samlaf reviewed Jan 25, 2024

View reviewed changes

samlaf reviewed Jan 31, 2024

View reviewed changes

src/ECDSARegistryCoordinator.sol Show resolved Hide resolved

ChaoticWalrus closed this Mar 15, 2024

stevennevins deleted the feat-experimental-ecdsa-registries branch November 25, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: experimental ecdsa registries #123

feat: experimental ecdsa registries #123

ChaoticWalrus commented Dec 28, 2023

samlaf left a comment

samlaf Jan 4, 2024

ChaoticWalrus Jan 6, 2024

samlaf Jan 5, 2024

ChaoticWalrus Jan 6, 2024

samlaf Jan 23, 2024

ChaoticWalrus Jan 24, 2024

samlaf Jan 23, 2024

ChaoticWalrus Jan 24, 2024

samlaf left a comment

ChaoticWalrus commented Jan 24, 2024

samlaf Jan 25, 2024

samlaf Jan 25, 2024

samlaf Jan 25, 2024

samlaf left a comment

samlaf Jan 25, 2024

ChaoticWalrus Jan 25, 2024

ChaoticWalrus commented Jan 25, 2024 •

edited

Loading

samlaf commented Jan 25, 2024

ChaoticWalrus commented Jan 26, 2024

samlaf commented Jan 29, 2024

ChaoticWalrus commented Jan 29, 2024

ChaoticWalrus commented Mar 15, 2024

		// the id of the operator, which is likely the keccak256 hash of the operator's public key if using BLSRegistry
		bytes32 operatorId;

feat: experimental ecdsa registries #123

feat: experimental ecdsa registries #123

Conversation

ChaoticWalrus commented Dec 28, 2023

samlaf left a 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

samlaf left a comment

Choose a reason for hiding this comment

ChaoticWalrus commented Jan 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samlaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaoticWalrus commented Jan 25, 2024 • edited Loading

samlaf commented Jan 25, 2024

ChaoticWalrus commented Jan 26, 2024

samlaf commented Jan 29, 2024

ChaoticWalrus commented Jan 29, 2024

ChaoticWalrus commented Mar 15, 2024

ChaoticWalrus commented Jan 25, 2024 •

edited

Loading