ENGN-9226 Diego/query tx separation decoding path - #986
Conversation
…consensus path unchanged
f03e8c6 to
dac5510
Compare
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
helder-moreira
left a comment
There was a problem hiding this comment.
Checked the consensus claim rather than taking it on reading, since #985 looked fine on inspection too. Diffed both registries at runtime (query = app + exactly the two mint.v2 msgs, nothing missing), and ran a mint.v2 wrapped in MsgExec through FinalizeBlock on dev vs this branch with the Any hand-encoded so dev never links mintv2 — identical code, gas and state, so the new import doesn't move consensus. Also confirmed a v9 tx renders through the real gateway marshaler, which nothing here covers (the tests all call the decoder directly; the bug report is a REST endpoint).
Consensus safety holds. Comments are mostly about the tolerant decoder accepting more than historical payloads.
On rollout: agree on snapshot testing and the single-RPC canary. Worth hitting GetTx and GetBlockWithTxs too, not just txs?query= — different paths, and the shim's interfaces are satisfied structurally, so a mismatch only shows at runtime.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Problem
After the v10 proto bump, transaction query endpoints (e.g.
GET /cosmos/tx/v1beta1/txs) can no longer decode pre-upgrade payloads. The strict unknown-field check resolves nested (non-Any) message types by name through the global gogo proto registry, and several historical names are now registered as their v10 equivalents. As a result the whole transaction fails to decode and the query returns a parse error instead of the stored tx.Constraints (must not affect consensus)
The transaction decoder is shared between the query path and the consensus path (
CheckTx,ProcessProposal,FinalizeBlock). Any change that makes a node accept a transaction it previously rejected is therefore consensus-relevant: it changes the set of transactions a node will admit and partially process, which can diverge application state between nodes running different binaries during a rolling upgrade.Hard requirements for the fix:
Solution
Introduce a read-only decoding path used only by the historical-read methods of the tx service (
GetTx,GetTxsEvent,GetBlockWithTxs):mint.v2types, built without touching the app's own registry.TxDecode) keeps the app's standard strict behavior.The consensus decoder and the app's registry are left completely unchanged.
Details
app/querytx.go(new): the separate registry builder, the tolerant decoder plus a minimalsdk.Txshim for decoded historical txs, the composite tx service, and an override ofRegisterTxServicethat wires it in.app/app.go: small, behavior-preserving wiring — hoist the custom module basics into a reusable helper, and build/store the read-only registry and tx config at startup.mint.v2types resolve only on the query registry — never on the consensus registry or the global proto registry.mint.v2and the v7/v8 whitelist txs) decode over gRPC but don't render over the REST gateway, which resolves messages via the app registry; a page containing one fails to render. All other historical versions are unaffected.Summary by cubic
Restores decoding of pre‑v10 transactions on tx query endpoints without changing what consensus accepts. Previously these queries failed to parse stored pre‑v10 payloads; now
GetTx,GetTxsEvent, andGetBlockWithTxsdecode via a read‑only path while consensus decoding stays strict.InterfaceRegistryfor queries that mirrors the app registry and adds historicalmint.v2plus missing v7/v8 whitelist messages.Anywith empty or unresolvable type URLs.TxDecode) remain strict.authz‑wrapped), consensus rejection, registry superset checks, REST gateway behavior (only consensus‑registered types render), and a FinalizeBlock harness that proves no state changes for wrapped v9.mint.v2; gRPC queries are unaffected.Dependencies
golangci-lintworkflows to Go 1.23.5 and updates thego-install-hardenedworkflow reference.Written for commit c0a6015. Summary will update on new commits.