Accounts Refactor PR 1: Adding account -> [credential_id] mapping#2770
Conversation
theodorebugnet
left a comment
There was a problem hiding this comment.
Given that this is going to be part of a hard fork upgrade, do we actually need to keep the legacy mapping going forward? The PR description mentions a "DrainLegacyAccounts migration" as follow-up changes, but the code goes to great lengths to define proper handling for the legacy map.
If we're gonna keep the legacy map around that makes sense, but just checking what the plan is because if we're gonna have a migration to remove it then IMO this PR can be massively simplified, as the intermediate steps on the theodore/multisig-upgrade branch don't need to be backwards compatible, as long as we finish adding the migration before we merge this into dev.
That's good question. I kept backwards compatibility to make a change less painful for Zeta's testnet/mainnet, because it is used by hyperlane register module. But that's good point and worth considering a full removal, but then we need to a migration. But maybe migration is simpler long-term. Let's discuss it on daily. |
theodorebugnet
left a comment
There was a problem hiding this comment.
Non-blocker for this PR, but I wonder if the migration can be made truly rollup-generic, and then maybe we can have a standalone sov-migrations collection of either binaries or libraries, to make it as easy as possible for any rollup to set up its migration?
Right now, rollup-starter will also need to define a similar migration (esp. if we want to test the upgrade using the starter, which we will going forward IMO), and it'd need to copy a lot of the code currently implemented in demo-rollup.
I think it'd be really great if we could:
- Make the migration helper generic on the entire rollup, if possible. This might be cumbersome because there are a lot of types (Runtime, Storage, etc.).
- Make a
sov-migrationslibrary, or similar. - Rollups (including
demo-rollup) just define a thin binary that imports the migration with their own STF types in the generics and runs it, with minimal boilerplate. Similar to howsov-buildright now can be invoked by 10-line build scripts orsov-soak-testing-libcan be used to set up soak tests with very little boilerplate.
I think it should be double to make migration fully generic and actual rollup just runs a single function with concrete types. I will do this in the last PR. For sov-migrations, I think this can be done as a separate issue after this workstream is done |
ef63098
into
theodore/multisig-upgrade
Description
PR 1 of the accounts refactor series. Replaces the legacy credential→address index with an explicit authorization map and an offline data migration, so the new code path is the only authoritative one. Existing chains drain pre-upgrade state in-place via the migration binary before booting this code; nothing in the production read path consults the legacy map anymore.
What changed
Module
account_owners: StateMap<(address, credential_id), bool>— the authorization set.sov_accounts::migrations— two-phase API (collect_legacy_account_entries+apply_legacy_account_migration) operators wire into their own migration tooling.InsertCredentialIdwriteaccount_ownersonly.is_authorized(addr, cred)andis_authorized_for(addr, cred).is_authorized_forconsults canonical-address +account_ownersonly — the legacy map is not read.hyperlane-solana-registerusesauthorize_credentialdirectly.account_ownersvalue isboolinstead of()because of rockbound limitations: Refactoring for more explicit sentinel values handling rockbound#50Removed
Accounts::resolve_sender_addressandAccounts::resolve_sender_address_read_only— both became no-op pass-throughs after the layer-1 drop; the pass-through is now inlined at the twosov-capabilitiescall sites and the twoRuntime::resolve_addressimpls.get_accountRPC and theResponse<Addr>type — the legacy map is no longer queryable from production code.sov-accounts/tests/integration/main.rs.Tombstone
Accounts::accountsis renamed to_accountsand kept as apub(crate)tombstone. It is not removed: it is the first#[state]field, so removing it would shift the discriminants ofenable_custom_account_mappings(1→0) andaccount_owners(2→1) and corrupt their on-disk data on every chain that has been upgraded through this code. Onlycrate::migrationsmay touch it.Migration tooling
examples/demo-rollup/src/migrations/legacy_accounts.rs— newlegacy-accounts-migrateCLI binary, modelled onmockda-to-celestia-migrate. Iterates the legacy prefix via NOMT, writes each(credential_id, addr)pair toaccount_ownersas(addr, credential_id) -> true, deletes the source row, and commits at the current head version. NOMT-only; JMT backends are not supported.examples/test-data/genesis/migration-test/accounts.json+ README — 6 diverse fixture entries (treasury, lone non-canonical sov1, lone non-canonical EVM, sentinel) for end-to-end testing.Verification REST API
GET /modules/accounts/authorizations/{address}/{credential_id}→{"authorized": bool}(readsaccount_owners).Was done manually and them removedGET /modules/accounts/legacy/{credential_id}→{"legacy_address": addr | null}(reads the tombstone so operators can confirm the source map is drained).Operator upgrade procedure
Existing chains with pre-upgrade
accountsentries must run the migration before booting this binary.legacy-accounts-migrate --rollup-config-path … --dry-run— inspectentries_migratedand confirmpre_state_root != post_state_rootif there were any entries.--dry-runto commit.examples/test-data/genesis/migration-test/README.md).The migration is idempotent: a second run reports
entries_migrated: 0with the state root unchanged.Trade-offs / behavioral deltas
InsertCredentialId(X)from senderAno longer makesXroute toA. It only authorizesXforAinaccount_owners. WhenXlater signs a V0 tx, the resolver returnsX.into()(stateless canonical). Fund the canonical address ifXneeds to draw gas fromA.exit_if_credential_existschecks(sender, credential)inaccount_ownersinstead of global credential uniqueness. Authorizing someone else's credential for your own address now succeeds — harmless, since they still need the private key to sign.is_authorized_forno longer consults the legacy map. Operators that skiplegacy-accounts-migrateon a chain with pre-upgrade entries will silently lose access for those credentials. This is the intended trade-off: one explicit operator step in exchange for a single authoritative read path.accounts[C] = Awas authoritative:is_authorized_for(X, C)returned strictlyA == Xand bypassed the canonical fallback. Post-upgrade,is_authorized_for(X, C)istrueiffX == credential_id.into::<S::Address>()oraccount_owners[(X, C)] = true, with no suppression. The migration writesaccount_owners[(A, C)] = truefor each legacy row, but for any row whereA != canonical(credential_id), the credential gains authorization forcanonical(credential_id)after migration in addition to keeping authorization forA; the canonical fallback is computed, not stored, so it cannot be revoked throughaccount_owners. Intended — the simpler "canonical OR explicit" model is the design target. Themigration-testfixture exercises this case (multiple credentials sharing one address; one credential bound to an EVM-format address). Seesov-accounts/README.mdfor operator guidance.get_accountRPC is gone. Tooling that queried(credential_id) → addressin the legacy map must move off it.Backward compatibility
accountsentries are no longer authoritative. Operators must runlegacy-accounts-migrate.AccountConfig,CallMessage, and tx schema unchanged.get_accountremoved;Response<Addr>removed.Known gaps / deferred
Runtime::resolve_addresstrait method (sov-modules-api/src/runtime/mod.rs:157) has no callers in the workspace and both impls are forceddefault_addresspass-throughs. TODO comments at all three sites point to the final PR of the accounts-refactor stack as the place to drop it.chain_state.true_slot_numberto satisfy the historical-state DB invariant that User-namespace writes must be paired with Kernel-namespace writes (sov-db/src/historical_state.rs:288). The state root is unchanged by the round-trip; the proper long-term fix is to relax that invariant for the migration commit path.account_2is preserved in the integration-test fixture with#[allow(dead_code)]for the upcomingtarget_addressPR.I have updated `CHANGELOG.md` with a new entry if my PR makes any breaking changes or fixes a bug. If my PR removes a feature or changes its behavior, I provide help for users on how to migrate to the new behavior.Linked Issues
sov-accountsrefactoring and extending #2772Testing
sov-accounts/src/migrations.rs:apply_migration_moves_entries_to_owners,apply_migration_empty_input,apply_migration_is_safe_to_repeat.sov-accountsintegration tests updated for the new model (get_account/Responsereferences removed; resolve-no-op tests dropped).legacy-accounts-migratereportsentries_migrated: 6, pre/post state roots differ.(address, credential)pairs return{"authorized": true}post-migration.{"legacy_address": null}post-migration.{"authorized": false}.sov-accounts,sov-capabilities,sov-hyperlane-register-module,sov-ethereum,integration-tests— all green.Docs
sov-accounts/README.mdrewritten for the post-migration model (two relations: stateless canonical andaccount_owners), plus an operator upgrade-procedure section and a tombstone explanation. The migration-test fixture has its own README documenting the end-to-end flow.