Repository navigation
Remove the oracle module behind a v6.8 upgrade - #4319
Conversation
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4319 +/- ##
==========================================
- Coverage 67.60% 66.38% -1.23%
==========================================
Files 2192 2062 -130
Lines 168752 155488 -13264
==========================================
- Hits 114087 103216 -10871
+ Misses 54655 52262 -2393
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
devin-ai-integration
Bot
changed the title
devin-ai-integration
Bot
changed the title
…ecompiles' into masih/1790245567-remove-oracle-blockers
This reverts commit 974dd92.
This reverts commit 5367e10.
…ecompiles' into masih/1790245567-remove-oracle-blockers # Conflicts: # app/testdata/upgrade_v68_offline_source_test.go # app/upgrade_v68_offline_target_test.go # app/upgrade_v68_test.go
Precompile behaviour is selected by upgrade height, so the code every precompile ran as `v6.7` has to be frozen before `main` can carry `v6.8` behaviour. This is the `scripts/bump_version` output for that step, the same preparatory PR as sei-protocol#3625 (v6.6) and sei-protocol#3961 (v6.7): every generated precompile is archived into `legacy/v68`, and its `setup.go` and `versions` file gain the new entry. `app/tags` gains `v6.8` in the same PR, as sei-protocol#3961 did, because the two have to move together: once the `versions` file lists `v6.8`, the generated `GetVersioned` map carries both `latestUpgrade` and a literal `"v6.7"` key pointing at the archived snapshot, and with `app/tags` still at `v6.7` the snapshot shadows the live implementation. The archived `v6.7` code is not the current code (for example the pointer precompile still had its creation path, removed after the tag in sei-protocol#4230), so the shadowing changed historical `debug_traceTransaction` replays in `evmrpc/tests`. Appending to `app/tags` makes v6.7 to v6.8 the current boundary, so `make new-upgrade-test FROM=v6.7 TO=v6.8` scaffolds the tagged boundary tests. They assert the minimum for an upgrade that changes nothing yet: an un-upgraded binary halts at the plan height, the handler applies without changing the module version map, bank sends succeed on both sides, and the offline source, target and reopen phases see the same store set. The oracle removal in sei-protocol#4319 replaces them with its own assertions. The one hand-written generator change is in `archiveModule`, which now skips an existing module that has no `versions` file instead of creating one and archiving it. The retired `ibc` precompile is maintained by hand as a tombstone (sei-protocol#3981) and has no `versions` file, so without the guard the generator rewrote its `setup.go` down to a single `v6.8` entry and `TestVersionedPrecompilesAreAllTombstones` failed. Brand-new modules still get a `versions` file, since that path is keyed on `isNew`. Re-running `go run ./scripts/bump_version` on this branch produces no further changes.
PR SummaryHigh Risk Overview Historical compatibility is preserved via Infrastructure tweaks: Reviewed by Cursor Bugbot for commit 72a42fd. 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.
The PR removes the oracle module cleanly: a v6.8 handler, StoreUpgrades.Deleted for the store, decode-only retired Msg types, and storev2 now refusing unmounted store names. But the new tests don't cover the main app-hash-breaking behaviour, the actual deletion of the oracle store. The genesis-export assertion also always passes.
Findings: 3 blocking | 2 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
oracleis dropped frommaccPermsandallowedReceivingModAcc, but the handler does nothing with any balance the oracle module account still holds (for example its reward pool). After v6.8 those funds are stranded under an account no module owns, and the address is no longer blocked. Either confirm the mainnet balance is zero, or sweep it in the v6.8 handler and pin that in a test. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
On the module-account balance suggestion: the oracle module account |
|
@seidroid review |
There was a problem hiding this comment.
All three earlier blockers are fixed. The offline target now opens through UpgradeStoreLoader using the upgrade-info.json that v6.7 wrote, and checks that the memiavl oracle tree is gone after both a clean run and a crash replay. The reopen phase pins that v6.7 can no longer open the migrated database, and the genesis-export test now decodes the module map correctly. The storev2 no such store guard, the v6.8 handler and the decode-only retiredoracle types look correct; the only open item is from the last review and is non-blocking: nothing happens to any balance left in the oracle module account.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] Carried over from the previous review, still not addressed:
oracleis removed frommaccPerms, but the v6.8 handler does nothing with any balance the oracle module account still holds, such as the reward pool or funds sent to it. Its address stays inallowedReceivingModAccsemantics (not blocked), so after v6.8 those funds sit under an account that no module owns and nothing can spend. Either confirm the mainnet balance is zero, or sweep it in the v6.8 handler (for example to the community pool or fee collector) and pin that in a test.
Superseded: latest AI review found no blocking issues.
…ve-oracle-blockers # Conflicts: # CHANGELOG.md
| if upgradeInfo.Name == "1.0.4beta" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | ||
| storeUpgrades := storetypes.StoreUpgrades{ | ||
| Added: []string{oracletypes.StoreKey}, | ||
| Added: []string{"oracle"}, |
There was a problem hiding this comment.
nit - this change is probably not needed, but also maybe we should use some const strings somewhere for all deprecated stores
There was a problem hiding this comment.
Done in 72a42fd: added retiredoracle.ModuleName = "oracle" and used it here, in the v6.8 Deleted entry, in DeleteModuleVersion and for the codespace/route inside retiredoracle, so the name is spelled once. The change to this line was needed because oracletypes no longer exists; dex/aclaccesscontrol keep their existing local names.
|
|
||
| if upgradeInfo.Name == "v6.8" && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) { | ||
| storeUpgrades := storetypes.StoreUpgrades{ | ||
| Deleted: []string{"oracle"}, |
There was a problem hiding this comment.
same as above re. using a variable rather than raw string (worried about typos in the future)
| require.Panics(t, func() { app.RunBlock(nil) }) | ||
| // TestV68RejectsOracleTxsWithoutCharging pins that retired oracle transactions | ||
| // are refused before fees are charged. | ||
| func TestV68RejectsOracleTxsWithoutCharging(t *testing.T) { |
There was a problem hiding this comment.
do we need to still have tests covering oracle tx behavior if the intention is to remove oracle?
There was a problem hiding this comment.
I'd keep it: the two oracle Msg types are deliberately retained (decode-only) so historical blocks still replay under debug_trace*, and mainnet price feeders will still be submitting MsgAggregateExchangeRateVote when v6.8 lands. This test pins the one behaviour of those types that is a v6.8 decision (ValidateBasic refuses them, so they never reach fee deduction) — it's covering the decode-only shim, not the module. It goes away when the shim does, i.e. once historical replay no longer needs the types. Happy to drop it if you'd rather not carry it.
| @@ -2,5 +2,4 @@ | |||
|
|
|||
| Sei implements the following custom modules: | |||
| * `dex` - | |||
…op dex from x/README
* main: (21 commits) Backport evmonly parse, app-hash and changeset perf fixes from giga-1 (#4345) Remove the oracle module behind a v6.8 upgrade (#4319) fix(seidb): report only the current migration boundary on the snapshot gauge (#4327) fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count (#4322) Backport Autobahn execute-loop and produced-tx metrics from giga-1 (#4330) Add giga.storage.receipts to toggle the Autobahn receipt store (#4333) optimize gather phase (#4326) Regenerate the Unreleased changelog as a plain PR list (#4336) Bump sei-protocol/go-ethereum to v1.15.7-sei-21 (#4332) Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call (#4318) Add dashboard and topology option for Autobahn e2e (#4167) Add Giga fetch/serve and BlockDB prune metrics (#4329) Add eth_getLogs to the EVM-only Giga RPC (#4308) Generate v6.8 precompiles (#4320) Add [giga] app.toml section and honor it on the Autobahn node (#4323) feat(evmonly): add eth_estimateGas via existing libraries (#4325) Use Pebble batch directly in SS (#4300) Fix pruning issue in SS causing huge disk spike (#4321) Make Autobahn always run the EVM-only executor, disable/remove some integration tests (#4316) reduce seal lock contention (#4314) ...
v6.7 rejects `MsgCreateVestingAccount` once its upgrade has run, but kept the module wired in so existing vesting accounts still decode. The only ones left are test accounts: three on pacific-1 holding 2.1 SEI locked until the year 3000 (a full scan of the account store on 24 September found no others, and the scan is repeated before the upgrade), and a few dozen already fully vested ones on atlantic-2. Keeping the module means bank reads and decodes an account on every debit, delegation and EVM balance lookup just to learn that nothing is locked. This deletes `x/auth/vesting`, its protos, `seid tx vesting` and the `add-genesis-account --vesting-*` flags, and adds a `v6.8` upgrade. Its handler runs the new auth 3 to 4 migration, `Migrator.Migrate3to4`, then deletes the `vesting` module version. The migration scans the account store once and rewrites every account stored under a vesting type as the `BaseAccount` it embeds, keeping address, public key, account number and sequence; balances are untouched, so locked coins become spendable. It parses the raw bytes because the vesting account types are gone, and it returns an error, halting the upgrade, rather than leave behind an account the new binary cannot decode. A full scan beats a hardcoded address list because atlantic-2's tx index does not cover its whole history; through the real store stack it costs about 235 ns per account, roughly 25 s of CPU for pacific-1's 102M accounts plus disk reads. `MsgCreateVestingAccount` itself moves to a decode-only `app/retiredvesting` package with its type URL and fields, as `app/retiredibc` and the oracle removal in sei-protocol#4319 do for their types, so transactions from before v6.8 still decode. Bank's `LockedCoins` stays as the one place that used to read the account. It now returns no coins and performs the read only when `RetracesLockedCoinsLookup` reports a re-trace of a block from before v6.8, the same pattern as distribution's `ReadOnlyRewardsUpgrade`, and delegation keeps its account lookup behind the same gate. Without this, the evmrpc mainnet regression replays fail: traces of old precompile calls change gas, and one historical out-of-gas transaction starts succeeding. The gate is only as precise as the `ClosestUpgradeName` the RPC context carries, and `GetClosestUpgrade` returns the next upgrade rather than the one in effect, so once v6.8 is applied most v6.7-era blocks re-trace without the read; v6.7 freeze nodes serve those traces. Live execution skips the read, so transactions and precompile calls that move coins use less gas (1,405 per bank debit in the updated wasm tests), and delegating or undelegating no longer requires the delegator to have an account. This is state-machine breaking and ships behind `v6.8`, which also runs gov's 3 to 4 migration from sei-protocol#4000, so the upgrade moves auth and gov to version 4 and drops `vesting` from the version map. After the upgrade `MsgCreateVestingAccount` still decodes, but its `ValidateBasic` returns v6.7's deprecation error (codespace `vesting`, code 2), so it is refused at CheckTx and in DeliverTx before any fee, where v6.7 charged the fee first. The vesting account types are deliberately not retired: the migration leaves none in state, and registering them would let a genesis file create vesting accounts nothing enforces, so v6.8 nodes cannot decode vesting accounts from before the upgrade and v6.7 freeze nodes serve those historical queries. `x/auth/keeper/legacy_vesting.go` and the gate in `x/bank/keeper/view.go` deserve the closest look. Validated with the new v6.8 in-process tests, the offline source, target and reopen phases run against `release/v6.7`, `make upgrade-test-vet`, the affected package suites, and the live v6.7 to v6.8 cross-version run in CI's Minor release boundary job. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Since #3944 no oracle vote can enter the chain, but the module still ran every block:
oracle.MidBlockerfound no ballots each vote period and incrementedAbstainCountfor every bonded validator, andoracle.EndBlockerranSlashAndResetCountersevery slash window, so on a chain with a non-zeromin_valid_per_windowit would slash and jail every validator for abstaining from votes the chain rejects. The module is never coming back, so this removes it outright behind a new coordinatedv6.8upgrade instead of only stopping the blockers.v6.8is already inapp/tagsfrom #4320, which also carries the generated precompile snapshots and a placeholder v6.8 boundary test that this PR replaces; the v6.8 handler runs migrations andDeleteModuleVersion("oracle"), andSetStoreUpgradeHandlersdeletes theoraclestore at the upgrade height viaStoreUpgrades.Deleted, the same waydex(v5.8.0) andaccesscontrol(v6.3.0) went.x/oracle,proto/oracle, the oracle store key and memstore, params subspace, module account permissions, wasm query route, and seidb/dump tooling entries are all gone. The one thing that stays is decoding: oracle votes sit in nearly every historical mainnet block, so the two Msg types move to a decode-onlyapp/retiredoraclepackage with the same type URLs, amino names, codespaceoracleand code 25, keepingdebug_trace*, historical tx queries andutils.IsTxPrioritizedworking. TheirValidateBasicreturnsErrDeprecated, so a leftover price feeder's vote is refused at CheckTx without paying a fee. This reverses the v6.7 "rejected but still charged" behaviour, and is safe because a tx that failsValidateBasicnever enters the mempool or a block, so there is nothing to charge for; the v6.7-tagged tests that pinned the charge are updated accordingly. The retired oracle precompile keeps its address and versioned legacy implementations for historical EVM replay.Raw
/store/oracle/*ABCI queries against the deleted store previously hit storev2's unchecked store lookup (empty success on the SS fast path, a recovered panic on the commitment path), so storev2Querynow refuses unmounted store names withno such store, matching legacy rootmulti, and the cross-version test asserts that response.app/upgrade_v68_test.gopins that the handler drops the oracle version-map entry and is idempotent, that an un-upgraded binary halts at the plan height, that the store is really deleted (the offline target opens through the store loader with theupgrade-info.jsonthe v6.7 halt wrote and checks the memiavloracletree is gone, and the v6.7 reopen phase asserts the old binary can no longer open the migrated database), that retired oracle txs are refused without charging, that oracle is absent from exported genesis, plus the cross-version and offline boundaries. This is app-hash breaking at the v6.8 upgrade height.