Deprecate oracle message and query handlers - #3944
Conversation
|
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 #3944 +/- ##
==========================================
- Coverage 59.51% 58.47% -1.04%
==========================================
Files 2326 2225 -101
Lines 198890 187658 -11232
==========================================
- Hits 118367 109738 -8629
+ Misses 69279 67564 -1715
+ Partials 11244 10356 -888
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview Ante and mempool path: Tests/integration: Oracle integration scenarios assert deprecation; wasm oracle queries expect Reviewed by Cursor Bugbot for commit c4e1164. 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.
Clean, well-scoped deprecation: both oracle msg handlers and all ten gRPC query handlers now return the newly registered ErrOracleDeprecated, legacy SDK routes are dropped (safe — runMsgs prefers the msg service router and RegisterRoutes skips empty routes), and the tests were rewritten to seed keeper state directly. No correctness bugs found; the remaining notes are about deprecation leftovers (gasless carve-out, simulation ops, orphaned NewHandler, repurposed integration files).
Findings: 0 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The oracle gasless carve-out in
app/antedecorators/gasless.gois now permanently open.oracleVoteIsGaslesstreats a vote as gasless wheneverGetAggregateExchangeRateVotefinds nothing — and nothing writes an aggregate vote anymore, so theErrAggregateVoteExistbranch is dead and every well-formedMsgAggregateExchangeRateVoteis fee-free and then always fails withErrOracleDeprecated.CheckAndSetSpamPreventionCounterbounds this to one tx per validator per block, so it is not a serious spam vector, but the carve-out now only buys free blockspace for a message that can never succeed. Consider dropping the oracle branch fromIsTxGasless(and the now-vestigialSpammingPreventionDecorator/VoteAloneDecorator) in the same change. - Callers do not uniformly get the deprecation error the PR describes.
GaslessDecoratorpropagates theIsTxGaslesserror, so a vote from a non-feeder is rejected in ante withErrNoVotingPermission/ErrNoValidatorFoundbefore reaching the msg server, andSpammingPreventionDecorator.CheckOracleSpammingstill writes the spam-prevention counter (mem store) on CheckTx for every incoming vote. Worth aligning if "explicit deprecation result" is the goal. x/oracle/simulation/operations.gostill returns weighted operations forMsgAggregateExchangeRateVoteandMsgDelegateFeedConsent, andAppModule.WeightedOperationsstill wires them in. Both callapp.Deliver, which now returnsErrOracleDeprecated, so the operation returns a non-nil error and any simulation run fails. These operations should return no-ops (orWeightedOperationsshould return nil) alongside the handler deprecation.- Both oracle integration files were repurposed into near-duplicate deprecation checks (
set_feeder_test.yamlfor msgs,verify_penalty_counts.yamlfor queries) whileMidBlocker/EndBlockerstill run penalty accounting andSlashAndResetCounterson chain. That end-to-end path now has no integration coverage. Slashing is inert today only becauseDefaultSlashFraction/DefaultMinValidPerWindoware zero — worth a note in the PR description that the deprecation relies on that. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
x/oracle/client/rest/query.gobuilds legacycustom/oracle/...queries, butAppModule.LegacyQuerierHandleralready returnednilon the base branch, so those REST endpoints were registered against a nil querier and were already non-functional. This PR makes them fail cleanly instead of nil-panicking;RegisterRESTRoutescould be dropped entirely as follow-up.
|
@seidroid review |
There was a problem hiding this comment.
The deprecation itself is clean and the previous round's notes were addressed, but the oracle mempool-priority carve-out survived while every gate that limited who could earn it was deleted, so any funded account can buy top-of-block priority with a message that always fails. The new message-deprecation integration test also can't pass as written.
Findings: 2 blocking | 7 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
x/oracle/simulation/operations.go(WeightedOperations,SimulateMsgAggregateExchangeRateVote,SimulateMsgDelegateFeedConsent) now has no caller sinceAppModule.WeightedOperationsreturns nil. Delete the file, and with it theaccountKeeper/bankKeeperfields onAppModule(and theNewAppModuleparams) that only fed it.Keeper.ValidateFeeder,CheckAndSetSpamPreventionCounter,getSpamPreventionCounter/setSpamPreventionCounterandErrAggregateVoteExistnow have no production callers — onlyx/oracle/keeper/keeper_test.go. That is the same "exported production code alive solely to satisfy tests" shape flagged forNewHandlerlast round, and the spam-prevention mem-store writes are now unreachable. Worth removing in this change while the context is fresh.- "Is this an oracle tx" is now spelled three different ways: the inline loop in
CosmosStatelessChecks(includesMsgDelegateFeedConsent),antedecorators.isOracleTx(vote only), andapp/prioritizer.go'sisOracleTx(vote only). If the priority carve-out is removed as suggested, all three disappear; if any is kept, fold them into one named helper. Relatedly, the empty-bodiedcaseclauses in the rewrittenCosmosStatelessChecksloop read as accidental no-ops — the predicate is "every message is an oracle message" and would be clearer stated that way. - The oracle CosmWasm bindings (
x/oracle/client/wasm/query.go) now returnErrOracleDeprecatedwhere they previously returned data —GetExchangeRatesin particular used to return an empty list on fresh state. Existing contracts that query exchange rates or TWAPs will abort rather than get a result. This is the PR's intent, but it is a contract-visible break worth naming explicitly in release notes alongside the msg/query changes. verify_penalty_counts.yamlis gone, so theMidBlocker/EndBlockerpenalty-accounting andSlashAndResetCounterspath — which still runs on chain — now has no integration coverage. The PR description's note thatDefaultSlashFraction/DefaultMinValidPerWindoware zero is the only thing standing behind it (confirmed inx/oracle/types/params.go:42-43); worth keeping some end-to-end assertion that no validator is jailed across a slash window.- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
x/oracle/client/rest/query.gobuilds legacycustom/oracle/...queries, butAppModule.LegacyQuerierHandleralready returnednilon the base branch, so those REST endpoints were registered against a nil querier and were already non-functional. WithQuerierRoute()now returning "" they are definitively dead;RegisterRESTRoutesandtypes.QuerierRoutecould be dropped as follow-up. - [suggestion] On the base branch, a transaction containing only
MsgDelegateFeedConsentalready receivedantedecorators.OraclePriorityfromDecoratePrioritywith no feeder/validator check at CheckTx (CheckMessageonly validatedMsgAggregateExchangeRateVote). The blast radius was smaller because the message still had to name a real validator to succeed, but the priority boost itself was unguarded.
| } | ||
| oracleVote := false | ||
| otherMsg := false | ||
| oracleVote := len(tx.GetMsgs()) > 0 |
There was a problem hiding this comment.
[blocker] Oracle transactions keep top-tier mempool priority while this PR removes every gate that limited who could earn it.
oracleVote still flows into DecoratePriority (line 125), which sets antedecorators.OraclePriority = MaxInt64-100 — above the MaxPriority = MaxInt64-1000 ceiling that any fee-paying transaction can reach. Previously that boost was gated by the MsgAggregateExchangeRateVote case in CheckMessage, deleted in this diff, which ran ValidateFeeder (sender must be the delegated feeder of an existing validator) and CheckAndSetSpamPreventionCounter (one per validator per block). Both are now gone, and MsgAggregateExchangeRateVote.ValidateBasic only requires a parseable bech32 feeder, a parseable bech32 validator, and parseable rates — no validator has to exist.
Failure scenario: an attacker funds any account, signs a vote naming its own address as Feeder and any well-formed seivaloper... string as Validator, and pays the minimum fee. CosmosStatelessChecks returns oracleVote == true, CheckAndChargeFees succeeds (the message is no longer gasless), DecoratePriority assigns OraclePriority, and CheckMessage no longer objects — so the tx is admitted to the mempool ahead of every legitimate transaction and, per the updated TestCheckTotalBlockGas_OracleVoteCountsTowardLimit, now also consumes block gas. It then fails in delivery with ErrOracleDeprecated. Repeated at minimum fee, that is cheap priority-lane block space for a message that can never succeed.
Since oracle messages can no longer do anything, the carve-out has no remaining purpose. Suggest dropping it wholesale: the oracleVote return here and the oracleVote branch in DecoratePriority, isOracleTx/priority = OraclePriority in app/antedecorators/priority.go:36, and isOracleTx(feeTx) in app/prioritizer.go:155.
| env: ADMIN_ADDR | ||
| - cmd: printf "12345678\n" | seid keys show node_admin --bech val --output json | jq -r ".address" | ||
| env: ADMIN_VAL_ADDR | ||
| - cmd: seid tx oracle set-feeder $ADMIN_ADDR --from $ADMIN_ADDR --chain-id sei --dry-run 2>&1 |
There was a problem hiding this comment.
[blocker] These two seid tx oracle ... --dry-run commands can't reach the node, so the verifiers below will never see the deprecation error.
node_admin lives in the default password-protected keyring — docker/localnode/scripts/step1_configure_init.sh:48 creates it with printf "12345678\n12345678\ny\n", and every other access in this repo pipes the passphrase (including lines 3 and 5 of this very file, and submit_tx_and_wait in integration_test/utils/_tx_helpers.sh:237). --from $ADMIN_ADDR plus --dry-run requires a keyring lookup to build the simulation tx, so both commands stop at the passphrase prompt with stdin closed. SET_FEEDER_ERROR/VOTE_ERROR end up holding a keyring error, the oracle module is deprecated regexes don't match, and TestOracleModule fails.
Either pipe the passphrase (printf "12345678\n" | seid tx oracle ...) or follow the existing --dry-run precedent in integration_test/bank_module/simulation_tx.yaml:13 and add --keyring-backend test against a key created in that backend.
1 participant
Summary
ErrOracleDeprecatederror from both oracle message handlersWhy
The oracle module is deprecated, but its Cosmos message and query services still accepted incoming requests. Keeping the gRPC services registered while returning a stable module error gives callers an explicit deprecation result and prevents further oracle message state mutations. The legacy SDK routes are no longer registered.
Impact
Incoming aggregate exchange-rate votes, feeder delegation messages, and oracle queries now fail with oracle error code 25 (
oracle module is deprecated). Oracle transactions are no longer gasless and are treated as fee-paying transactions before dispatch.Oracle mid-block/end-block processing remains registered for state compatibility. Under the default oracle parameters, both
DefaultSlashFractionandDefaultMinValidPerWindoware zero, so missed oracle votes do not produce slashing. Networks that override those parameters must set them to zero before or with this deployment.Validation
go test -race ./app ./app/ante ./app/antedecorators ./x/oracle/...go vet ./app/... ./x/oracle/...gofmt,goimports, andgit diff --checkon all touched files