Migrate fep ecdsa by alrevuelta · Pull Request #556 · agglayer/agglayer-contracts

alrevuelta · 2025-10-21T09:59:05Z

Explore FEP->ECDSA migration path
Closes https://github.com/agglayer/protocol-team-kanban/issues/719

alrevuelta · 2025-10-23T12:12:58Z

contracts/aggchains/AggchainECDSAMultisig.sol

 * The addresses and threshold are managed by the aggchainManager.
 */
-contract AggchainECDSAMultisig is AggchainBase {
+contract AggchainECDSAMultisig is AggchainBase, MigrationFEPToECDSASlots {


do you both @invocamanman and @krlosMata agree on this approach?

I think it is not needed, although is does not hurt.
A comment should be enough since the storage slots fulfilled are just not used.
It is needed a deeper analysis if the chain goes again to FEP. Check if the storage slots are rewritten or can potentially imply a security issue

alrevuelta · 2025-10-23T12:14:50Z

contracts/aggchains/AggchainECDSAMultisig.sol


    /// @notice Aggchain version
-    string public constant AGGCHAIN_ECDSA_MULTISIG_VERSION = "v1.0.0";
+    string public constant AGGCHAIN_ECDSA_MULTISIG_VERSION = "v1.0.1";


unsure if this makes sense. breaks a lot of stuff. but since the contract is changing we need versioning? using semver, +0.0.1 since its not breaking

it does not. I'd increase also to v1.0.1

ignasirv · 2025-10-23T15:51:36Z

contracts/lib/MigrationFEPToECDSASlots.sol

+    address public pendingOptimisticModeManager;
+    mapping(bytes32 => AggchainFEP.OpSuccinctConfig) public opSuccinctConfigs;
+    bytes32 public selectedOpSuccinctConfigName;
+    bytes32 public lastRollupExitRoot;


Why are you adding this storage that is not from FEP? There is an old deployed version of aggchainFEP with this storage? 🤔 (same for lastMainnetExitRoot, globalExitRootMap)

good catch. i messed it with the copy paste. fixed 2818965

ignasirv · 2025-10-23T15:52:25Z

contracts/lib/MigrationFEPToECDSASlots.sol

+    bytes32 public selectedOpSuccinctConfigName;
+    bytes32 public lastRollupExitRoot;
+    bytes32 public lastMainnetExitRoot;
+    mapping(bytes32 => uint256) public globalExitRootMap;


I would consider adding a _gap[50] for future upgrades of FEP

invocamanman · 2025-10-24T08:51:26Z

check contract LegacyZKEVMStateVariables {
specially:
e.g

    /// @custom:oz-renamed-from multiplierBatchFee
    uint16 internal _legacyMultiplierBatchFee;

I woudl do a very similar approach here, changing names and adig the custom ox renamed from
( This can be veyr easily done with IA :P)

invocamanman · 2025-10-24T08:53:48Z

Remmeber to:
Clear possible storage slots from FEP
check if FEP uses something aggchainBase that's not used in ECDSA multisig

alrevuelta · 2025-10-28T14:32:07Z

There is one issue with this feature. Due to how initializers work when using upgradable contracts, there is an implicit assumption that the only allowed path is ECDSA->FEP (not the opposite way), see reinitializer(3) in AggchainFEP and reinitializer(2) in AggchainECDSAMultisig

This means that as it is, if we add a new path FEP->ECDSA, we will create an inconsistency in a migration that goes FEP->ECDSA->FEP. In the ECDSA->FEP transition initializeFromECDSAMultisig can't be called anymore, due to the modifier, leading to an inconsistency. This makes this function work only sometimes (when state n-2 was not FEP). This doesn't seem ok to me.

One option would be to remove the reinitializer(3) modifier from initializeFromECDSAMultisig but if we do so, then the function can be called multiple times, so it wont be an initializer per se anymore.

krlosMata · 2025-10-28T14:52:49Z

I think it depends on the design that we want to take id a chain decides to move from FEP --> ECDSA --> FEP:

Design A
- the chain should prove all state-transition from the last time that it was an FEP
- no changes required in the SC
Design B
- new SC call reinitializel2Output that only the aggchainManager can call (same securoty assumption since it can effectivellt set up a new aggVKEy, rangeVKey and rollupConfigHash)
  - the function just adds a new l2Output element

alrevuelta · 2025-10-28T15:10:06Z

@krlosMata agree with your approaches, but wanted to also address this concern.

This means that as it is, if we add a new path FEP->ECDSA, we will create an inconsistency in a migration that goes FEP->ECDSA->FEP. In the ECDSA->FEP transition initializeFromECDSAMultisig can't be called anymore, due to the modifier, leading to an inconsistency.

krlosMata · 2025-10-28T15:14:51Z

@krlosMata agree with your approaches, but wanted to also address this concern.

This means that as it is, if we add a new path FEP->ECDSA, we will create an inconsistency in a migration that goes FEP->ECDSA->FEP. In the ECDSA->FEP transition initializeFromECDSAMultisig can't be called anymore, due to the modifier, leading to an inconsistency.

I think it is not an inconsistency. This function can be called the first time that we move from ECDSA --> FEP so certain parameters can be properly initialized. If those parameters are already initialized and the chain goes again from ECDSA --> FEP, all those parameters can be reset again, each one with its own setter....expect the initial l2Output.

alrevuelta · 2025-10-28T15:28:45Z

I think it is not an inconsistency. This function can be called the first time that we move from ECDSA --> FEP so certain parameters can be properly initialized. If those parameters are already initialized and the chain goes again from ECDSA --> FEP, all those parameters can be reset again, each one with its own setter....expect the initial l2Output.

yep. more than inconsistency i mean that is not idempotent. you can use the setters but you can no longer call initializeFromECDSAMultisig the second time. If that fine, i think your Design B makes sense.

krlosMata · 2025-10-29T16:03:40Z

contracts/aggchains/AggchainFEP.sol


+    function reinitializel2Outputs() external {
+        // Clear the array by popping all elements
+        while (l2Outputs.length > 0) {


just setting the length to 0 is enough

cant directly set it to 0.

TypeError: Member "length" is read-only and cannot be used to resize arrays. --> contracts/aggchains/AggchainFEP.sol:446:9: | 446 | l2Outputs.length = 0; | ^^^^^^^^^^^^^^^^

But i can call delete. fixed a27bfd6

older solidity versions allowed it

krlosMata · 2025-10-29T16:05:39Z

contracts/aggchains/AggchainFEP.sol

        }
    }

+    function reinitializel2Outputs() external {


missing onlyRollupManager

fixed a27bfd6

using onlyAggchainManager as discussed privately

krlosMata · 2025-10-29T16:14:18Z

contracts/AgglayerManager.sol

+        // If update from FEP to ECDSA, clear l2Outputs before upgrade.
+        // This ensures if it migrates back to FEP, the l2Outputs are empty.
+        if (
+            rollup.rollupVerifierType == VerifierType.ALGateway &&


I think adding chain specific configuration in the AgglayerManager.sol should not be done....it should be at Chains SC level.
Also, we lost history of previous verified states which may be useful.

I think it better to manage this exact same function, but on the chain implementation. I think it is easier on the long-term rather than managing all of them in the Manager

i see. removed here. a27bfd6
the function call can be passed now as rollupData

a27bfd6#diff-5c5d2e61f08010833196980538de8ebd806d00d53326e4ba06319f3474d7d30bR440

alrevuelta · 2025-10-31T11:17:39Z

Summary of the FEP->ECDSA migration:

Without modifying the contracts we could perform the FEP->ECDSA migration by calling just updateRollup. However, adding this new migration adds a new whole dimension to migration paths:
- Before: There was an implicit assumption that migrations where "linear". ECDSA->FEP. Without ever coming back.
- Now: If we allow FEP->ECDSA, migration can now be "circular". Meaning one can in theory do FEP->ECDSA->FEP. This opens up new problems.
The main problem we can face in the FEP->ECDSA->FEP case is having stale FEP slots that may not make sense anymore after we migrate again to FEP. To solve this we have two approaches. i) When migrating to ECDSA clean up all slots or ii) keep the original slots and just clean up l2Outputs. We have chosen approach ii) in this PR, see reinitializel2Outputs.
Other side effects of this new path is that some initializers won't work (due to the reinitialize(x)). Eg initializeFromECDSAMultisig won't work in the FEP-ECDSA-FEP case.
So in short, FEP->ECDSA is possible and relatively easy. We have taken some decissions on how FEP->ECDSA->FEP can behave but most likely this path will be quite rare.

Initial tests for FEP to ECDSA migration

f0f0ed6

alrevuelta changed the base branch from main to feature/v12 October 21, 2025 09:59

alrevuelta added 5 commits October 21, 2025 13:58

Draft working test with update

f219ae9

Misc changes

e820aff

Some refactors

4459e56

Last refactors

dd99ada

Fix link

fb0e231

alrevuelta commented Oct 23, 2025

View reviewed changes

ignasirv suggested changes Oct 23, 2025

View reviewed changes

alrevuelta added 2 commits October 23, 2025 18:12

Fixes 1

2818965

Fix tests due to version bump

db602ea

alrevuelta added 2 commits October 29, 2025 11:55

Add test fep-ecdsa-fep

2540d11

Clean l2outputs in FEP to ECDSA migration

856c53d

krlosMata reviewed Oct 29, 2025

View reviewed changes

alrevuelta added 3 commits October 30, 2025 13:34

Fix comments

a27bfd6

Fix tests

3706454

Use aggchain manager as role

5dea7f0

Base automatically changed from feature/v12 to develop December 19, 2025 14:56

Conversation

alrevuelta commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

invocamanman commented Oct 24, 2025

Uh oh!

invocamanman commented Oct 24, 2025

Uh oh!

alrevuelta commented Oct 28, 2025

Uh oh!

krlosMata commented Oct 28, 2025

Uh oh!

alrevuelta commented Oct 28, 2025

Uh oh!

krlosMata commented Oct 28, 2025

Uh oh!

alrevuelta commented Oct 28, 2025

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!

alrevuelta commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alrevuelta commented Oct 21, 2025 •

edited

Loading