Repository navigation
[DO NOT MERGE YET] Remove the authz module behind the v6.9 upgrade - #4569
alexander-sei wants to merge 3 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
alexander-sei
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4569 +/- ##
==========================================
+ Coverage 59.47% 59.87% +0.39%
==========================================
Files 2040 2022 -18
Lines 160200 157778 -2422
==========================================
- Hits 95278 94463 -815
+ Misses 64917 63310 -1607
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
c659c17 to
328b6d0
Compare
Delete x/authz, its protos and the Go behaviour of the bank SendAuthorization and staking StakeAuthorization types, and unwire the module from the app, the sei-wasmd test app, the tools and the sei-db store keys. app/tags gains v6.9: its store loader deletes the authz store and its handler drops the authz version-map entry. The v6.8 handler is unchanged. MsgGrant, MsgExec and MsgRevoke move to a decode-only app/retiredauthz package with the same type URLs, so historical blocks still decode. Their ValidateBasic returns ErrDeprecated, so new authz transactions are refused before any fee is charged. The authz precompile becomes a tombstone that always reverts, and the authorization methods leave the staking, gov, distribution and slashing precompiles. The ante checks on messages nested in MsgExec go with the module. The v6.8 -> v6.9 boundary tests cover the handler, the refusal of every authz message, the persisted deletion of a store holding a grant, and a live chain upgraded with a grant on it. Co-authored-by: Cursor <cursoragent@cursor.com>
328b6d0 to
e571657
Compare
The store loader deletes the authz store only when upgrade-info.json names v6.9 at startup, but the handler runs either way, so a node that reaches the upgrade height without that file would drop the authz version-map entry, keep the authz tree and fork its AppHash. #4538 made the v6.8 handler halt in that case. deletedStoresByUpgrade now holds the stores each upgrade deletes. The store loader reads it, and every upgrade handler first requires those stores to be gone, so v6.8 keeps its check, v6.9 gains it, and a later upgrade that deletes stores gets both by adding one entry. The offline v6.9 target phase covers the halt by dropping upgrade-info.json and checks that the recovery the error names reaches the clean upgrade's hash. The snapshot subtest now writes the file before applying the upgrade. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview Adds Retires EVM authz surfaces: the Tests add v6.9 unit, offline upgrade, and cross-version paths plus Reviewed by Cursor Bugbot for commit c73def9. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Removes the authz module behind the v6.9 upgrade. The store is deleted at the upgrade height, the version-map entry is dropped, MsgGrant/MsgExec/MsgRevoke stay decodable through a retiredauthz package that fails ValidateBasic before any fee is charged, the authz precompile becomes a revert-only tombstone, and the authorization methods are removed from the staking, gov, distribution and slashing precompiles. I approve: the inlined direct staking and distribution paths match the removed code, ValidateBasic still rejects authz messages in the CosmosStatelessChecks path of both CheckTx and DeliverTx, and RunMigrations rebuilds the version map from registered modules only, so DeleteModuleVersion holds. Codex's reading found nothing, which matches mine apart from the stale-description note; I had nothing of theirs to keep or drop.
Non-blocking
- The PR description no longer matches the diff. It describes a v6.8 upgrade using
v68DeletedStores, edits tolegacy/v67andlegacy/v68, and a hold on #4551. The code targets v6.9 withv69DeletedStoresanddeletedStoresByUpgradeand touches no legacy precompile copies. Reviewers and release notes that rely on the description will get the wrong upgrade name.
seidroid review · decision approve · session c4c47111e74844bd8304b6adec8f160b · turn resp_claude_3b0d7aef3e8222b7559930e836fad751 · item 11ec0e0edb2d556e8656ec8683151604
Findings: 0 blocking | 1 non-blocking | 0 posted inline
alexander-sei
changed the title
`MultiTree.ApplyUpgrades` drops a deleted tree by swapping it past the end of `t.trees` and never closes it, so its snapshot stays mapped with its three files open, and the vacated slot in the backing array keeps the tree reachable. Changelog replay makes it worse. `Catchup` hands every tree a background writer through `ApplyChangeSetAsync`, and its closing loop only stops the writers of trees still listed, so a tree deleted mid-replay keeps a goroutine blocked on its channel, and that goroutine pins the tree, its mappings and its file descriptors for the life of the process. This came up in review on sei-protocol#4569, whose v6.9 upgrade deletes the `authz` tree. Every node hits this at the upgrade height, and again on each restart or snapshot reload that replays from a snapshot taken before it. It becomes repeatable on nodes that read past heights from state commitment, which are nodes with `sc-enable-state-proof` on and nodes with SS off. There a query at a past height opens a fresh read-only memiavl through `LoadVersion(version, true)` and replays the changelog from the nearest snapshot, so every query at a height past the upgrade that is served from a pre-upgrade snapshot leaks one goroutine and three mapped files per deleted tree, until that snapshot is pruned 10,000 to 20,000 blocks later with the default snapshot settings. The historical-proof limit, one query per second by default, slows that without bounding it, and with six trees deleted at v6.8 each such query there can leak up to 18 mappings and 18 file descriptors. Nodes on the default configuration reject state proofs and serve past heights from SS, so they only see the upgrade-height and replay cases. v5.8.0 and v6.3.0 went through the same path with one store each. `ApplyUpgrades` now deletes through `deleteTree`, which clears the vacated slot, waits for the tree's background writer and closes it. The wait comes first because `Tree.Close` closes the writer's channel without waiting for it, and a replay can leave the writer mid-changeset. A failure to close is logged rather than returned, so the change can only release resources and can never fail a store loader at an upgrade height. Anything else holding the tree's snapshot keeps it, since snapshots are reference-counted and `Tree.Copy` takes its own reference. Nothing persisted changes, neither state nor hashes nor changelog entries, so this is not app-hash breaking. It is labeled for backport to `release/v6.8` because the v6.8 binary is the one that replays v6.8's six deletions after its upgrade height, and a fix that only lands on main first ships in v6.9. `TestCatchupReleasesTreeDeletedByUpgrade` replays a changelog through a delete and requires the deleted tree's writer to have stopped, its snapshot to be closed and the multitree to hold no reference to it; it fails on main. A scratch run of 50 read-only loads past a delete leaked 50 goroutines and 150 open files before this change and none after. Validated with the memiavl, composite, migration and `sei-cosmos/storev2` packages under `-race`, golangci-lint, `golangci-lint fmt --diff`, `go vet`, and the three `upgrade-test-offline` phases from `release/v6.7` against this commit, whose target phase applies v6.8's six deletions through the store loader, including after a simulated crash, and reopens each result by replaying the changelog through them. Co-authored-by: Cursor <cursoragent@cursor.com>
Authz is still a fully live module on main. Accounts can grant, execute and revoke authorizations, two ante checks reject a
MsgExecthat wraps an EVM message or nests more than five levels deep, theauthzprecompile at0x…100Eserves grant queries, and the staking, gov, distribution and slashing precompiles carry 18 scoped authorization methods that execute through the authz keeper. This removes the module outright behind thev6.9upgrade, the same way #4319 and #4536 removed oracle and tokenfactory behindv6.8.x/authzand its protos are deleted, along with the Go behaviour of bank'sSendAuthorizationand staking'sStakeAuthorization(their generated types stay) and giga's unused copy ofSendAuthorization. The module is unwired fromapp.New, the sei-wasmd test app andkeys.MemIAVLStoreKeys.app/tagsgainsv6.9, andretiredauthz.ModuleNameis the only entry in the newv69DeletedStores, so the store loader deletes theauthzstore at the upgrade height, taking every outstanding grant with it, and the v6.9 handler drops its version-map entry.MsgGrant,MsgExecandMsgRevokemove to a decode-onlyapp/retiredauthzpackage with the same type URLs, which also keeps the three authorization types registered becauseRejectUnknownFieldsresolves theAnyinside a historicalMsgGrant. TheirValidateBasicreturnsErrDeprecated(codespaceauthz, code 2, because code 3 keeps its historical meaning), so a new authz transaction is refused in CheckTx and in a block before any fee is charged, andretiredauthz_test.gopins encodings and amino sign bytes captured from the removed module and decodes two real pacific-1 transactions. The ante checks go with the module, since aMsgExecno longer passesValidateBasic.The store loader deletes the
authzstore only whenupgrade-info.jsonnamesv6.9at startup, but the handler runs either way, so a node that reached the upgrade height without that file would drop the version-map entry, keep theauthztree and fork its AppHash. #4538 made v6.8 halt in that case. The stores each upgrade deletes now live indeletedStoresByUpgrade, which the store loader reads and every upgrade handler checks first, so v6.8 keeps its halt and v6.9 gains the same one, with the same recovery: write{"name":"v6.9","height":H}todata/upgrade-info.jsonand restart.The
authzprecompile stays registered as a tombstone: its three grant queries keep their ABI, but every call reverts withauthz precompile is retired; authz grant queries are disabled, andAuthz.solis deleted. The authorization methods leave the staking, gov, distribution and slashing precompiles (Go,abi.jsonand the Solidity interfaces), so calling one reverts as an unknown selector, whichTestAuthorizationMethodsAreRetiredchecks for all 18. The direct paths behave as before:delegate,redelegateandundelegatecall the staking keeper directly, and the distribution withdrawals keep their scoped panic recovery. The docker cluster's authz YAML suites are deleted,authz.spec.tsasserts the retirement, and the other precompile specs drop their authorization cases.This is app-hash breaking at the v6.9 height. From then on authz transactions are refused and grants are gone, so REStake-style auto-compounding stops (pacific-1 has 174
MsgExecand 17MsgGranttransactions). Because #4551 removed the height-versioned precompile copies, a trace or call on this binary at a pre-v6.9 height that reaches the authz precompile or an authorization method reverts; #4551 already requires operators to route those heights to frozen nodes running the release that executed them. memiavl refuses to delete a tree it does not have, so the upgrade relies on every network carrying anauthztree, which holds because v6.8 mounts it and pacific-1 and atlantic-2 gained it in the 3.0.6 store upgrade.Spamming grants before the upgrade height doesn't make the deletion slower: the store loader drops the
authzmemiavl tree in one step whatever its size, and unlike v6.8 the handler runs nodeleteByPrefixprune, so there is nothing for aRetiredPrefixDeleteLimit-style cap to bound. That holds only whileauthzlives in memiavl. v6.8 setsMigrateCosmosEnabled, so onceNumKeysToMigratePerBlockis raised and the EVM migration completes, auto nodes entermigrate_cosmos, which on v6.8 moves every store butevminto FlatKV,authzincluded. Inmigrate_cosmosorflatkv_onlythe composite store refuses the delete andMayHoldStorereportsauthzas present, so no node in either mode gets past the v6.9 height, and FlatKV has no store-level delete to fall back on. v6.9 therefore requires every network to be atevm_migratedor earlier at its upgrade height: governance has to keepNumKeysToMigratePerBlockat 0 until v6.9 activates, or setMigrateCosmosEnabledback to false before raising it. As of 2026-10-26 pacific-1 and atlantic-2 have the rate at 0 and have not applied v6.8, and noupgrade_v69test runs with a non-zero rate.The
upgrade_v69tests require authz to be the only module version the upgrade removes, in process, offline and on a live chain. Validated by CI at c73def9:make upgrade-testandmake upgrade-test-vet, the persisted offline boundary fromrelease/v6.8(clean, crash-replay and halt-then-recover target paths, then a reopen that v6.8 must refuse), the cross-version boundary fromrelease/v6.8with a send grant on chain, the sharded Go tests, golangci-lint withgolangci-lint fmt --diff, and the full docker integration matrix.