feat: keyregistrar #481

eigenmikem · 2025-05-19T13:24:27Z

Motivation:

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications:

Describe the modifications you've done.

Result:

After your change, what will change.

ypatil12

First pass

ypatil12 · 2025-05-19T13:52:27Z

src/KeyRegistrar.sol

+    }
+
+    /// @dev Maps (AVS, operator, curve type) to key info
+    mapping(address => mapping(address => mapping(CurveType => KeyInfo))) private avsOperatorKeyInfo;


Let's change to mapping from operatorSet to KeyInfo, then we can get rid of CurveType in this mapping

(Assume single key type per operatorSet)

ypatil12 · 2025-05-19T13:52:50Z

src/KeyRegistrar.sol

+    mapping(address => mapping(address => BN254.G2Point)) private avsOperatorToBN254KeyG2;
+
+    /// @dev Maps (AVS, operator, curve type) to key hash for quick lookup
+    mapping(address => mapping(address => mapping(CurveType => bytes32))) private avsOperatorToKeyHash;


TODO: identify need for using key hash

yeah im unclear on why we need this too

ypatil12 · 2025-05-19T13:53:12Z

src/KeyRegistrar.sol

+    mapping(address => mapping(uint32 => BN254.G1Point)) private avsOperatorSetToAggregateBN254Key;
+
+    /// @dev Maps AVS to their authorized registrar contract
+    mapping(address => address) public avsToRegistrar;


Let's remove this and just use PermissionController

why not just reuse registrar from AM?

ypatil12 · 2025-05-19T13:54:16Z

src/KeyRegistrar.sol

+     * @param pubkey Public key bytes
+     * @return alreadyRegistered True if the key was already registered
+     */
+    function registerKey(


If operators register their key first, I'm wondering if we can then just have a separate ECDSA and BN254 functions? Unsure what is cleaner...

yea I think there needs to be one register function that receives bytes and a curve type, and does a look up for the dedicated handler of that reg type

RonTuretzky · 2025-05-19T13:53:40Z

src/KeyRegistrar.sol

+    }
+
+    /// @dev Maps (AVS, operator, curve type) to key info
+    mapping(address => mapping(address => mapping(CurveType => KeyInfo))) private avsOperatorKeyInfo;


why not make KeyInfo bytes so it can support any curve type in the future without upgrading this contract?
Same arg could be made to use an int/string for the CurveType key , allows AVS teams to extend this functionality without EL intervention

This would also allow support for verifiable DKG in the future

RonTuretzky · 2025-05-19T13:55:22Z

src/KeyRegistrar.sol

+    mapping(address => mapping(CurveType => mapping(bytes32 => address))) private avsKeyHashToOperator;
+
+    /// @dev Maps (AVS, operatorSetId) to their aggregate BN254 G1 point
+    mapping(address => mapping(uint32 => BN254.G1Point)) private avsOperatorSetToAggregateBN254Key;


As this is specific to BLS, If we adopt the extensible pattern I suggested above, this would have to be in a "additional data" field or metadata field

RonTuretzky · 2025-05-19T13:56:29Z

src/KeyRegistrar.sol

+    mapping(address => address) public avsToRegistrar;
+
+    /// @dev Optional delay period before a key rotation takes effect (in blocks)
+    uint256 public rotationDelay;


What is the benefit of introducing a rotation delay? I fear that in 0 day scenarios , allowing the operator key to override the signing key with no delay might have some edge case advantage

RonTuretzky · 2025-05-19T13:59:51Z

src/KeyRegistrar.sol

+        bytes memory oldKey;
+        if (curveType == CurveType.ECDSA) {
+            oldKey = avsOperatorToECDSAKey[avs][operator];
+            _registerECDSAKey(avs, operator, newPubkey);
+        } else if (curveType == CurveType.BN254) {
+            BN254.G1Point memory oldKeyPoint = avsOperatorToBN254Key[avs][operator];
+            oldKey = abi.encode(oldKeyPoint.X, oldKeyPoint.Y);
+
+            // Register new key
+            BN254.G1Point memory newKeyPoint = _registerBN254Key(avs, operator, newPubkey);
+
+            // Update all specified operator sets
+            for (uint256 i = 0; i < operatorSetIds.length; i++) {
+                uint32 operatorSetId = operatorSetIds[i];
+                _updateOperatorSetAggregateBN254Key(avs, operatorSetId, oldKeyPoint, newKeyPoint);
+            }
+        } else {
+            revert InvalidCurveType();
+        }
+
+        // Update rotation timestamp
+        uint256 effectiveBlock = block.number + rotationDelay;
+        avsOperatorKeyInfo[avs][operator][curveType].lastRotationBlock = effectiveBlock;
+
+        emit KeyRotationInitiated(avs, operator, curveType, oldKey, newPubkey, effectiveBlock);


the way this could be handled in the extensible pattern is when someone registers a new curve type in the keyregistrar (permissionlessly) they also point to a handling function which then can decode/encode/validate the data as necessary and implements an interface.

Another option is to let the AVS handle this and simple not include any encoding/decoding logic in this contract, just storing arbitrary bytes. This is an anti-pattern to the desired behavior of not having AVS teams re-deploy the same code though.

I prefer the first approach I suggested, as it would still allow Eigen to support first class curves without the tradeoff of limiting extensibility

RonTuretzky · 2025-05-19T14:00:59Z

src/KeyRegistrar.sol

+    function _updateAggregateBN254Key(
+        address avs,
+        uint32 operatorSetId,
+        BN254.G1Point memory key,
+        bool isAddition
+    ) internal {
+        BN254.G1Point memory currentApk = avsOperatorSetToAggregateBN254Key[avs][operatorSetId];
+        BN254.G1Point memory newApk;
+
+        if (isAddition) {
+            newApk = currentApk.plus(key);
+        } else {
+            newApk = currentApk.plus(key.negate());
+        }
+
+        avsOperatorSetToAggregateBN254Key[avs][operatorSetId] = newApk;
+
+        emit AggregateBN254KeyUpdated(avs, operatorSetId, newApk);
+    }


This would be code that would sit outside this contract in the pattern I suggested above , maintained by eigen, and when someone registers a "Key Type" enum/identifier , they would be able to point to an external contract with this handling logic (that would receive a bytes array and decode it to BN254.G1Point, in this case)

gpsanant · 2025-05-20T18:39:42Z

src/KeyRegistrar.sol

+    mapping(address => mapping(CurveType => mapping(bytes32 => address))) private avsKeyHashToOperator;
+
+    /// @dev Maps (AVS, operatorSetId) to their aggregate BN254 G1 point
+    mapping(address => mapping(uint32 => BN254.G1Point)) private avsOperatorSetToAggregateBN254Key;


why store the aggregate in the contract instead of calculating it in the calculator?

this would introduce an "updateOperators" like pattern for maintaining the apk... imo better to do it in here , but keep it in an extraBytes field

Was requested in the TDD

Was requested in the TDD

I am highly opposed to introducing another updateOperators pattern as that one has been fairly unadopted and has cost hundreds of thousands of dollars cc @ypatil12 @gpsanant

gpsanant · 2025-05-20T18:50:15Z

src/KeyRegistrar.sol

+     * @param pubkey BN254.G1Point and BN254.G2Point encoded as bytes
+     * @return The registered BN254.G1Point
+     */
+    function _registerBN254Key(


they need to provide a signature here as well to prevent https://xn--2-umb.com/22/bls-signatures/#rogue-key-attack

Yea that's going in the 2nd draft, I can port over most of the code from the certVerifier

gpsanant · 2025-05-20T18:51:00Z

src/KeyRegistrar.sol

+     * @param operator Address of the operator
+     * @param pubkey ECDSA public key bytes
+     */
+    function _registerECDSAKey(


why isn't the pubkey decoded to address?

gpsanant · 2025-05-20T18:54:07Z

src/KeyRegistrar.sol

+    mapping(address => address) public avsToRegistrar;
+
+    /// @dev Optional delay period before a key rotation takes effect (in blocks)
+    uint256 public rotationDelay;


unclear why the core needs to enforce a rotation delay? any AVS that cares about a rotation delay knows enough to implement it themselves?

This was a feature requested by AVS devs

Key rotation is very useful for TEE security best practices , key compromises, machine failures etc

key rotation makes sense, but it's not clear to me why a rotation delay needs to be enforced by the core protocol?

gpsanant · 2025-05-20T18:55:23Z

src/KeyRegistrar.sol

+     * @param pubkey Public key bytes
+     * @return alreadyRegistered True if the key was already registered
+     */
+    function registerKey(


can we have the operator call these functions directly and have the AVS just view from this contract? reduces middleware scope of concern and codebase size...

2nd draft is going to do this :)

8sunyuan · 2025-05-27T20:29:27Z

src/KeyRegistrar.sol

+
+        // Check global uniqueness for new key
+        bytes32 newKeyHash = keccak256(newPubkey);
+        if (globalKeyRegistry[newKeyHash]) {


nit: replace revert Error() with just a single require statement

Also noticing multiple code blocks where you have the following code:

if (globalKeyRegistry[newKeyHash]) { revert KeyAlreadyRegistered(); } globalKeyRegistry[newKeyHash] = true;

Maybe just place this in a _setKeyHash(bytes32 keyhash) internal func

8sunyuan · 2025-05-27T20:30:50Z

src/KeyRegistrar.sol

+     * @param operator Address of the operator
+     * @return True if the key is registered
+     */
+    function isRegistered(


nit: rename isKeyRegistered for clarity?

8sunyuan · 2025-05-27T20:59:13Z

src/KeyRegistrar.sol

+                break;
+            }
+        }
+        if (isZero) revert ZeroPubkey();


could we just have some internal constant storing the hash of bytes pubkey[1:] = [0x00, ..., 0x00]
and compare if keccak256(abi.encode(pubkey)) is equal to that? Would be a lot more efficient vs this check

feat: keyregistrar

eaa1da9

ypatil12 reviewed May 19, 2025

View reviewed changes

RonTuretzky reviewed May 19, 2025

View reviewed changes

gpsanant reviewed May 20, 2025

View reviewed changes

eigenmikem added 2 commits May 22, 2025 22:22

refactor: refactoring KeyRegistrar

2f20952

refactor: fix return values

75c6db9

8sunyuan reviewed May 27, 2025

View reviewed changes

feat: keyregistrar #481

Are you sure you want to change the base?

feat: keyregistrar #481

Uh oh!

Conversation

eigenmikem commented May 19, 2025

Uh oh!

ypatil12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!