Extend tx decoder canonical-size check to AuthInfo (CON-343) - #3839
Extend tx decoder canonical-size check to AuthInfo (CON-343)#3839wen-coding wants to merge 2 commits into
Conversation
Apply the same wire-vs-remarshal size check already used for TxBody to AuthInfo, so non-canonical AuthInfo encodings are rejected on live decode paths. Historical-replay and debug-trace lenient decoders remain unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
wen-coding
changed the title
|
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 #3839 +/- ##
==========================================
- Coverage 61.29% 60.41% -0.89%
==========================================
Files 2351 2259 -92
Lines 197491 186900 -10591
==========================================
- Hits 121060 112913 -8147
+ Misses 65579 63987 -1592
+ Partials 10852 10000 -852
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The AuthInfo canonical-size check itself is correct and well-targeted (it closes the amino-sign-mode malleability gap that the TxBody check already covers), but the strictness is added with no corresponding leniency window for the debug_trace* path, whose lenient-decoder gate is still keyed to v6.5 only. Test coverage also lacks a positive control that a fully-signed AuthInfo still decodes.
Findings: 1 blocking | 8 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Ecosystem-compatibility risk: the check now rejects any client that explicitly encodes proto3 defaults inside AuthInfo (e.g.
SignerInfo.sequence = 0on an account's first tx, or an explicitFee.gas_limit = 0). Standard proto3 libraries omit these, but this is anapp-hash-breakingchange on a widely-integrated surface — worth confirming against a recent mainnet block sample that zero legitimate txs would newly fail before cutting the release. - The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material findings but noted it could not run tests (read-only Go module cache), so neither external pass provides test-execution signal. - I could not execute
go test ./sei-cosmos/x/auth/tx/...in this environment (command approval denied), so the new tests are reviewed statically only. - No prompt-injection attempts were found in the PR title, description, diff, or commit messages.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
|
|
||
| if rejectBloat { | ||
| if err := rejectBloatedProto(raw.AuthInfoBytes, &authInfo, "tx auth info"); err != nil { |
There was a problem hiding this comment.
[blocker] This new strictness is unconditional in DefaultTxDecoder, but the one place in the tree that already models "which strictness was in effect at height H" is not updated.
evmrpc/trace_tx_decoder.go:23 selects the lenient decoder only when !isV65ActiveAtHeight(height) (evmrpc/simulate.go:305-307, keyed to the literal "v6.5"). That gate was correct when the only strict check was TxBody bloat, introduced in v6.5. With AuthInfo bloat rejection added now, every height at or after the v6.5 upgrade but before this release will be traced with a decoder that is stricter than the one that originally accepted those txs.
Concrete failure: a tx signed with SIGN_MODE_LEGACY_AMINO_JSON is malleable in exactly the way this PR fixes — the amino sign bytes are rebuilt from decoded fields, not from raw.AuthInfoBytes — so a post-v6.5 block can legitimately contain a committed tx with a duplicated Fee field. On the new binary, debug_traceTransaction / debug_traceBlockByNumber for that block returns a decode error instead of a trace. The same applies to evmrpc/tx.go:157 and evmrpc/info.go:355,390, which call txConfigProvider(height).TxDecoder() with no compat wrapper at all, so getEthTxForTxBz yields nil and the tx silently drops out of block responses.
The PR description says "leave debug-trace lenient decoders unchanged" — but the debug-trace decoder is only lenient below v6.5, so leaving it unchanged is what creates the gap. This needs a second gate (a new upgrade constant threaded through traceCompatTxConfig, per the app/tags convention) so tracing a height in the v6.5-to-now range still uses body-strict/auth-info-lenient decoding. Per REVIEW_GUIDELINES §1 the tag not existing yet is fine; the issue is that no gate is introduced and the existing v6.5 gate is now the wrong one for this check.
| } | ||
| } | ||
|
|
||
| func TestDefaultTxDecoderRejectsAuthInfoBloat(t *testing.T) { |
There was a problem hiding this comment.
[suggestion] Missing positive control. Every case here asserts rejection; nothing asserts that a realistic canonical AuthInfo still decodes under the strict decoder. A false positive in rejectBloatedProto on AuthInfo is far more damaging than a false negative — it would reject valid txs at CheckTx/DeliverTx.
The risky shapes are exactly the ones this test never builds: SignerInfos with an Any-packed pubkey (whose Value bytes must survive UnpackInterfaces → proto.Marshal byte-identically), ModeInfo_Multi with a CompactBitArray, and Fee.Amount coins carrying custom-marshaled sdk.Int. builder_test.go:55-60 constructs such an AuthInfo but only marshals it — it never round-trips through DefaultTxDecoder. Please add a case that encodes a fully-signed multi-signer tx (including one multisig signer) and asserts DefaultTxDecoder returns no error.
| }, | ||
| }, | ||
| { | ||
| name: "payer explicitly encoded as default empty string inside fee", |
There was a problem hiding this comment.
[nit] This case is not really distinct from "fee field encoded twice" — both append a whole second Fee (tag 2) to authInfoBytes, so both are duplicate-known-field cases and both would still be caught if the nested explicit-default handling regressed. To actually cover "explicit default inside a single, non-duplicated field", mutate the existing fee submessage in place (splice 0x1a 0x00 into its payload and fix the length prefix) rather than appending a second one.
| if len(rawBodyBytes) != len(canonicalBytes) { | ||
| return fmt.Errorf("tx body wire size (%d) exceeds canonical size (%d)", len(rawBodyBytes), len(canonicalBytes)) | ||
| if len(rawBytes) != len(canonicalBytes) { | ||
| return fmt.Errorf("%s wire size (%d) exceeds canonical size (%d)", name, len(rawBytes), len(canonicalBytes)) |
There was a problem hiding this comment.
[nit] The guard is != but the message says "exceeds". Since this is now shared by two callers and its text is asserted on in tests, consider %s wire size (%d) does not match canonical size (%d) so a hypothetical raw-smaller-than-canonical mismatch does not produce a misleading error.
| // DefaultTxDecoderWithoutBodyBloatRejection returns a protobuf TxDecoder that | ||
| // preserves pre-v6.5 decode behavior for historical tooling. Do not use this for | ||
| // preserves pre-v6.5 decode behavior for historical tooling: it does not reject | ||
| // non-canonical TxBody or AuthInfo wire encodings. Do not use this for |
There was a problem hiding this comment.
[nit] The doc comment is updated to cover AuthInfo, but the identifiers still say Body: DefaultTxDecoderWithoutBodyBloatRejection and NewTxConfigWithoutBodyBloatRejection (config_historical_replay.go:18) now skip both checks. Since rejectBodyBloat → rejectBloat was already renamed in this PR for the same reason, renaming these two exported symbols to ...WithoutBloatRejection would keep the naming honest — there are only three call sites (evmrpc/trace_tx_decoder.go:33, app/params/proto_historical_replay.go:23,39).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this extends a consensus-critical tx decoder check (AuthInfo canonical-size rejection) with no upgrade-height gate of its own, so I'd like a human to confirm the rollout plan (coordinated binary upgrade, same as the existing TxBody check) before this lands.
What was reviewed:
- The generalization of
rejectBloatedBodyintorejectBloatedProtofor both TxBody and AuthInfo — logic is unchanged, just parameterized. - The historical-replay and debug-trace lenient decoder paths (gated by
historical_replaybuild tag /isV65ActiveAtHeight) — confirmed they still skip both checks together, so trace/replay tooling behavior is consistent for both fields. - New test coverage for non-canonical AuthInfo encodings (duplicate fee field, explicit-default payer) mirroring the existing TxBody bloat tests.
- Whether reusing the v6.5 boundary flag for the new AuthInfo check could reject previously-valid historical transactions during trace/replay of the v6.5-to-now range — this mirrors the deployment model described in this repo's review guidelines (binaries roll out in lockstep per coordinated upgrade), so it's consistent with how the existing TxBody check was deployed, but it's the kind of judgment call I'd rather a human confirm given the consensus-safety stakes.
Extended reasoning...
Overview
This PR generalizes the existing TxBody wire-vs-canonical-remarshal bloat check in sei-cosmos/x/auth/tx/decoder.go (rejectBloatedBody → rejectBloatedProto) so it also applies to AuthInfo in DefaultTxDecoder. The lenient decoder used for historical replay (historical_replay build tag) and for debug-trace of pre-v6.5 blocks (evmrpc/trace_tx_decoder.go, gated by isV65ActiveAtHeight) is updated in lockstep to keep skipping both checks. Two comment-only files and a new test (TestDefaultTxDecoderRejectsAuthInfoBloat) round out the change. Scope is narrow and the diff is small.
Security risks
This is squarely consensus/security-sensitive code: DefaultTxDecoder sits on the mempool/CheckTx/DeliverTx path, and this change tightens what AuthInfo encodings validators accept (rejecting non-canonical/bloated wire encodings, e.g. duplicated fee fields or explicit-default sub-fields). Tightening validation is directionally safe against malleability/bloat-style issues, but any node/binary divergence on when this check activates could, in principle, cause an AppHash mismatch across validators. The change carries no explicit height gate of its own — it relies on the same "binaries roll out in coordinated lockstep" deployment model documented in this repos review guidelines, which is also how the original TxBody check was deployed.
Level of scrutiny
Given the file is directly in the consensus tx-decode path, I applied high scrutiny here despite the diff being small and mechanical. I specifically checked whether the AuthInfo check reusing the existing v65-era boundary/build-tag structure could cause the lenient/strict decoder split to diverge for the two fields, or could reject legitimately-included historical transactions during archival trace/replay if AuthInfo canonicality was not enforced on-chain since v6.5. Per this repos review guidelines, this class of concern (a not-yet-released version boundary, and version-gated logic assuming lockstep binary rollout) is explicitly called out as expected/non-actionable here, and the bug-hunting systems finder/verifier pass already investigated and ruled out the closest concrete instance of this concern.
Other factors
The change is well tested — it adds two new AuthInfo bloat-encoding test cases that mirror the existing TxBody tests exactly, and the PR description lists the specific test commands run locally. No design ambiguity, no CODEOWNER-only paths beyond the normal sei-cosmos/x/auth/tx ownership, and no unresolved reviewer comments in the timeline. My hesitation is purely that this is a consensus-affecting rule change to production tx validation with no dedicated upgrade-height gate visible in the diff itself, which I think warrants a human confirming the rollout plan even though nothing in the code itself looks wrong.
Split body vs AuthInfo decoder checks so debug_trace* can stay AuthInfo-lenient from v6.5 until v6.8, matching historical acceptance while live DefaultTxDecoder remains fully strict. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview
Tests cover AuthInfo bloat rejection, the v6.5–v6.8 trace window, and updated canonical-size error strings. Reviewed by Cursor Bugbot for commit 6c2ec0a. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
The AuthInfo canonical-size check and the three-way trace decoder split are correct and well-tested; I found no blocking defects. Remaining notes are naming drift on the now-fully-lenient decoder, an upgrade-constant question (v6.8 vs the tree's in-flight v6.7), per-tx decoder construction inside hot loops, and a couple of test/doc gaps.
Findings: 0 blocking | 10 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage:
TestDefaultTxDecoderAcceptsCanonicalAuthInfobuilds an unsigned tx, soAuthInfo.SignerInfosis empty and theAnypublic key path is never exercised by the strict decoder. Since a re-marshal ofcodectypes.Anyis the most likely place for a false-positive rejection on real traffic, add a positive case with a signed tx (SignerInfos with a pubkeyAny+ signatures, ideally both SIGN_MODE_DIRECT and LEGACY_AMINO_JSON) assertingDefaultTxDecoderaccepts it. - Test coverage:
TestTraceBlockByNumberUsesCompatDecoderForHistoricalAuthInfocovers the v6.5–v6.8 window and v6.8+, but not a pre-v6.5 height where the fully lenient decoder should apply. The existing body-bloat test is table-driven over heights; consider matching that shape here. - Non-trace historical read paths still use the strict decoder for pre-v6.8 heights —
evmrpc/tx.go:157,evmrpc/info.go:355/:390, andfilterTransactions(evmrpc/utils.go:216). A historical tx with non-canonical AuthInfo will now silently fail to decode there (getEthTxForTxBzreturns nil;filterTransactionscontinues). Impact is likely nil in practice because EVM txs are wrapped server-side with an SDK-encoded (canonical) AuthInfo, but it's worth confirming the asymmetry with the trace paths is intentional. - Second-opinion passes: Codex reported no material findings;
cursor-review.mdis empty, i.e. the Cursor pass produced no output for this PR. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| // mempool, CheckTx, or DeliverTx paths. | ||
| func DefaultTxDecoderWithoutBodyBloatRejection(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { | ||
| return defaultTxDecoder(cdc, false) | ||
| return defaultTxDecoder(cdc, false, false) |
There was a problem hiding this comment.
[suggestion] Naming drift: DefaultTxDecoderWithoutBodyBloatRejection now disables both the body and the AuthInfo check, so the name no longer describes the behavior — and it reads as "AuthInfo checks are still on," which is the opposite of what it does. Same for NewTxConfigWithoutBodyBloatRejection in config_historical_replay.go (and the MakeEncodingConfig comments in app/params/proto_historical_replay.go already had to be reworded around it).
Given the misuse of a lenient decoder on a live path is consensus-unsafe, the name is the main guardrail here. Suggest renaming the fully-lenient pair to something like DefaultTxDecoderWithoutCanonicalSizeChecks / NewTxConfigWithoutCanonicalSizeChecks, which also reads better alongside the new DefaultTxDecoderWithoutAuthInfoBloatRejection.
|
|
||
| func (b *Backend) isV68ActiveAtHeight(height int64) bool { | ||
| ctx := b.ctxProvider(LatestCtxHeight).WithGasMeter(sdk.NewInfiniteGasMeter(1, 1)) | ||
| return b.keeper.UpgradeKeeper().IsUpgradeActiveAtHeight(ctx, "v6.8", height) |
There was a problem hiding this comment.
[suggestion] Worth double-checking the version this gate should name. app/tags currently tops out at v6.6, and the tree's in-flight (not yet tagged) constants are v6.7 — sei-cosmos/x/auth/vesting/msg_server.go:17 (DeprecationUpgradeName) and sei-cosmos/x/distribution/keeper/delegation.go:20 (ReadOnlyRewardsUpgrade). If this change ships in the v6.7 release, the live decoder starts enforcing AuthInfo strictness at the v6.7 upgrade height while the trace path stays lenient until v6.8. That is conservative rather than wrong (lenient accepts everything strict does, and blocks after v6.7 can't contain bloated AuthInfo), but the constant should match the release it ships in.
Separately, "v6.5" and "v6.8" are bare string literals here. Consider exported constants next to the decoder variants they gate (as ReadOnlyRewardsUpgrade does), so the decoder split and the height gate can't drift apart.
| } | ||
| for i := range tmBlock.Block.Txs { | ||
| decoded, err := traceCompatTxDecoder(b.txConfigProvider(tmBlock.Block.Height), b.isV65ActiveAtHeight(tmBlock.Block.Height))(tmBlock.Block.Txs[i]) | ||
| decoded, err := traceCompatTxDecoder( |
There was a problem hiding this comment.
[suggestion] The decoder is rebuilt on every iteration, and each isV6xActiveAtHeight call constructs a fresh context via b.ctxProvider(LatestCtxHeight) and does an upgrade-keeper store read — so this is now 2 ctx creations + 2 store reads per transaction, doubled by this PR. traceTxConfigProvider is already built above at line 439; hoist the decoder out of the loop:
traceDecoder := traceTxConfigProvider(tmBlock.Block.Height).TxDecoder()
for i := range tmBlock.Block.Txs {
decoded, err := traceDecoder(tmBlock.Block.Txs[i])The height is loop-invariant, so this is behavior-preserving.
| return nil, nil, emptyRelease, err | ||
| } | ||
| sdkTx, err := traceCompatTxDecoder(b.txConfigProvider(block.Number().Int64()), b.isV65ActiveAtHeight(block.Number().Int64()))(tx) | ||
| sdkTx, err := traceCompatTxDecoder( |
There was a problem hiding this comment.
[suggestion] Same loop-invariant issue as in BlockByNumber: block.Number().Int64() doesn't change across iterations, so the decoder and both upgrade lookups can be computed once before the for idx, tx := range tmBlock.Block.Txs loop instead of per transaction.
| return b.keeper.UpgradeKeeper().IsUpgradeActiveAtHeight(ctx, "v6.5", height) | ||
| } | ||
|
|
||
| func (b *Backend) isV68ActiveAtHeight(height int64) bool { |
There was a problem hiding this comment.
[nit] isV68ActiveAtHeight is a verbatim copy of isV65ActiveAtHeight with a different string. Consider a single helper and two thin wrappers (or just pass the name through to traceCompatTxConfigProvider):
func (b *Backend) isUpgradeActiveAtHeight(name string, height int64) bool {
ctx := b.ctxProvider(LatestCtxHeight).WithGasMeter(sdk.NewInfiniteGasMeter(1, 1))
return b.keeper.UpgradeKeeper().IsUpgradeActiveAtHeight(ctx, name, height)
}This keeps the next strictness tier from adding a third copy.
| } | ||
|
|
||
| // rejectBloatedBody rejects tx bodies where the raw wire encoding is larger | ||
| // rejectBloatedProto rejects messages where the raw wire encoding is larger |
There was a problem hiding this comment.
[nit] The doc comment still says "larger than the canonical re-marshal," but the check is len(rawBytes) != len(canonicalBytes) and the error message was correctly updated in this PR to "does not match canonical size." Suggest "rejects messages whose raw wire encoding does not match the size of the canonical re-marshal of the decoded struct" so the comment, the error text, and the code agree.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Deferring to a human reviewer: no bugs were found this run, but this PR is labeled app-hash-breaking and changes the live tx decoder used on CheckTx/DeliverTx paths, so it warrants a manual look regardless.
What was reviewed: the new AuthInfo canonical-size check (rejectBloatedProto) added to DefaultTxDecoder; the three-tier debug_trace* decoder selection (pre-v6.5 lenient / v6.5-v6.8 body-strict-only / v6.8+ fully strict) and its wiring through evmrpc/simulate.go and trace_tx_decoder.go; and the historical-replay lenient config. I also confirmed that the v6.8 tracing gate an earlier automated reviewer (seidroid) flagged as missing is present in the current diff, along with a positive-control decode test and a corrected non-duplicate-field test case addressing that reviewer's other comments.
Extended reasoning...
Overview
The PR extends the existing TxBody wire-vs-canonical-remarshal size check to also cover AuthInfo in sei-cosmos/x/auth/tx/decoder.go's DefaultTxDecoder, which is used on the live CheckTx/DeliverTx path. It also splits debug_trace* decoder selection into three height-gated tiers (pre-v6.5, v6.5-to-v6.8, v6.8+) via a new isV68ActiveAtHeight gate threaded through evmrpc/simulate.go and evmrpc/trace_tx_decoder.go, and updates the historical-replay lenient decoder/config and error-message text ("exceeds" -> "does not match") accordingly. Tests were added/updated in encode_decode_test.go and simulate_test.go.
Security risks
The core risk class here is consensus-relevant, not classic injection/auth: a bug in the new AuthInfo canonicality check could either (a) cause CheckTx/DeliverTx to reject a previously-valid tx shape (liveness/compatibility issue), or (b) mis-gate the trace-time decoder tiers such that historical blocks are decoded with a stricter-than-original decoder, producing spurious errors in debug_trace*/block responses for pre-upgrade heights. The PR is explicitly labeled app-hash-breaking, confirming this is a state-machine-affecting change.
Level of scrutiny
This warrants the highest level of scrutiny this repo has: it touches the shared tx decoder used by every CheckTx/DeliverTx call, is app-hash-breaking, and has non-trivial height-gated branching logic (three decoder tiers) that is easy to get subtly wrong, as evidenced by the earlier seidroid[bot] blocker comment about the missing v6.8 gate. The current diff appears to already incorporate that fix and the associated test suggestions, but given the blast radius of a decoder bug (chain halt or apphash divergence), a human familiar with the upgrade-gating conventions in app/tags/app/upgrades.go should confirm the v6.8 tag naming and gating logic before merge.
Other factors
No bugs were surfaced by the bug hunting system in this run, and the one candidate it investigated (non-trace RPC paths using the fully-strict decoder for historical blocks) was ruled out. Test coverage looks reasonably thorough (positive-control canonical AuthInfo decode, rejection cases for both trace-compat tiers, a dedicated v6.5-to-v6.8 window test). None of this changes the fact that the change is consensus-critical and explicitly flagged as apphash-breaking, which is reason enough to route it through a human reviewer rather than an automated approval.
1 participant
Summary
DefaultTxDecoder(live CheckTx/DeliverTx)debug_trace*can use:Test plan
GOWORK=off go test github.com/sei-protocol/sei-chain/sei-cosmos/x/auth/tx -count=1 -run 'TestDefaultTxDecoder|TestUnknownFields'GOWORK=off go test github.com/sei-protocol/sei-chain/evmrpc -count=1 -run 'TestTraceBlockByNumberUsesCompatDecoder'