x/bor: remove unused MsgBackfillSpans plumbing#598
Conversation
MsgBackfillSpans has no consumer: its side handler is a constant VOTE_NO no-op, the post handler is unreachable, and the msg-server handler only runs input validation and returns success without state writes. The feature was intentionally disabled in bc90777… and never re-wired. Keeping the registration around makes ExtendVoteHandler walk these txs and allocate one of the 50 maxSideTxResponsesCount slots per registered msg regardless of vote, so a block packed with these admissible no-ops can delay or skip the sideTx response of legitimate side-tx-bearing messages later in the block. Dropping the registration is enough to close that behavior — but since the entire plumbing is dead, this PR removes it end-to-end (proto + generated code + msg-server + side handler + post handler + metrics + tests + the case in the ABCI stress test). When the feature is genuinely needed it can be reintroduced with a real side handler and post handler. Touch points: - proto/heimdallv2/bor/tx.proto: drop BackfillSpans rpc + messages - api/* + x/bor/types/tx.pb.go: regenerated via `make proto-gen` - x/bor/keeper/msg_server.go: drop BackfillSpans handler - x/bor/keeper/side_msg_server.go: drop SideHandle/PostHandle + dispatch cases + FillMissingSpansMsgTypeURL - x/bor/types/{msg,events}.go: drop Type/Route/event constants - metrics/api/bor.go: drop BackfillSpansMethod and PostHandleMsgBackfillSpansMethod - *_test.go: drop tests pinning the removed APIs; rebalance the abci stress test from 5-way to 4-way msg cycling
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #598 +/- ##
=======================================
Coverage 45.2% 45.2%
=======================================
Files 172 172
Lines 18663 18582 -81
=======================================
- Hits 8437 8407 -30
+ Misses 9019 8967 -52
- Partials 1207 1208 +1
... and 3 files with indirect coverage changes
🚀 New features to boost your workflow:
|
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Address review feedback on #598: - Re-add MsgBackfillSpans (no rpc, no handlers) to proto and register it explicitly in codec.go so historical blocks containing a MsgBackfillSpans tx remain decodable post-upgrade. Removing the rpc method also removed the type from the interface registry via RegisterMsgServiceDesc, which would have broken TxDecode during replay for any chain whose history included such a tx. - Drop orphan helpers/symbols left behind by the original cleanup: GenerateBorCommittedSpans and CalcCurrentBorSpanId (+ tests), ErrInvalidLastBorSpanID (+ test entries), and the stale "we need to backfill before proposing a new span" comment in the side handler -- all unreachable now that the handlers are gone.
|
@claude Review again |
… 2 historical txs)
Audit of mainnet + Amoy heimdall RPC tx_search for /heimdallv2.bor.MsgBackfillSpans
found 2 historical txs on Amoy: heights 8790181 (hash AE3D46F7..) and 8790209 (hash
2FC9428B..), both Code=0 with gas_used=81753. Removing the msg-server handler in the
prior commits would have made these txs unroutable post-upgrade -- msgServiceRouter
would return nil, runTx would short-circuit with ErrUnknownRequest+gas_used=0, and
the resulting ExecTxResult divergence would halt replay via LastResultsHash mismatch
for any node syncing from before height 8790181 on Amoy.
Scope-reduce: keep the side-tx response-slot censorship fix (the real win from this
PR, also tracked in the equivalent private draft) but restore the bits required for
historical replay:
- Restore `BackfillSpans` rpc method + msg-server handler (validation only, no state
writes -- exactly matches pre-PR ExecTxResult for the 2 historical Amoy txs).
- Restore BackfillSpansMethod metric, ErrInvalidLastBorSpanID, CalcCurrentBorSpanId
+ its test (all called by the restored handler).
- Restore msg_server_test.go (TestBackfillSpans), errors_test.go entries, and the
abci stress test's 5-way msg cycling (now exercises a routable msg again).
- Drop the explicit RegisterImplementations(&MsgBackfillSpans{}) from codec.go --
redundant now that the rpc is back in _Msg_serviceDesc (RegisterMsgServiceDesc
handles it).
Stays removed (orphan after the slot-censorship fix, unreachable in production):
- SideHandleMsgBackfillSpans + PostHandleMsgBackfillSpans funcs + their dispatch
in SideTxHandler/PostTxHandler (this IS the slot-censorship fix).
- FillMissingSpansMsgTypeURL const, GenerateBorCommittedSpans helper (only post
handler used them), PostHandleMsgBackfillSpansMethod metric.
- MsgBackfillSpans.Type() / .Route() methods + their test (deprecated in modern
cosmos-sdk, not required for routing).
- EventTypeBackfillSpans + AttributesKeyLatest{Span,BorSpan}Id constants + tests
(only PostHandleMsgBackfillSpans emitted them).
- Stale "we need to backfill before proposing a new span" comment in side handler.
|
There was a problem hiding this comment.
Thanks for running the on-chain audit and going with full restoration of the msg-server handler — that's the right call for preserving ExecTxResult on the 2 known Amoy historical txs. Deferring final sign-off to a human reviewer since this still removes side-handler registration for a msg type with on-chain history and touches consensus-critical side-tx wiring.
Extended reasoning...
Overview
PR removes the side handler / post handler / dispatch for MsgBackfillSpans (the actual goal: closing the side-tx response-slot censorship vector) while keeping the rpc method, msg-server handler, codec registration, and supporting helpers (CalcCurrentBorSpanId, ErrInvalidLastBorSpanID) intact. Two prior review rounds flagged (1) TxDecode failure on replay if the proto type went away entirely, and (2) ExecTxResult divergence if the msg-server handler went away while leaving codec registration. Author addressed both by auditing Amoy archive (tx_search on /heimdallv2.bor.MsgBackfillSpans returned txs at heights 8790181 and 8790209 with code=0, gas_used=81753) and restoring the full handler.
Security risks
No direct security risk in the diff itself — it's a removal, and the remaining wiring keeps the msg-server handler doing identical work (validation + DB reads, no state writes), so the per-tx ExecTxResult shape is preserved for the historical txs. tallyVotes in app/vote_ext_utils.go tallies votes from extVoteInfo without consulting GetSideHandler, so historical VOTE_NO votes in old VEs will still tally the same way; the post handler being absent doesn't matter because the tx never reached the approved set in the first place. SideTxDecorator only enforces ≤1 side handler per tx, so 0 side handlers is fine at ante time.
Level of scrutiny
This is consensus-critical territory: side_msg_server.go changes, removing side-handler registration for a msg type with confirmed on-chain history, and a per-tx ExecTxResult invariant that depends on the msg-server handler continuing to do exactly the same work. Per the consensus-critical and hardfork-rollout rules, even surgical removals here warrant expert human sign-off — particularly someone who can confirm the audit also covered any production devnet that ran v0.6.0+ besides Amoy (the rollout note explicitly couldn't verify mainnet from the available infra).
Other factors
Bug hunting system found no new bugs. Both of my prior inline-comment bugs are marked resolved with substantive fixes. SonarQube clean, Codecov reports all modified lines covered. CI green. Author's responses have been technically detailed and correct (including the catch that a stub no-op handler wouldn't replicate the original gas_used=81753 from real DB reads). This is high-quality work that just happens to live in a part of the codebase where I should not be the one signing off.
|
@claude review once. So no other findings remaining? Ensure this is a non-breaking change and that it just remove this unused message while keeps retrocompatibility |
There was a problem hiding this comment.
Backwards-compatibility looks preserved: msg-server handler, proto/codec registration, gRPC registration, and CalcCurrentBorSpanId are all retained so the 2 historical Amoy txs (heights 8790181, 8790209) replay with matching ExecTxResult; the only behavior change is dropping the side-handler dispatch (the intended slot-censorship fix, covered by the regression test). Deferring final approval to a human since this touches ABCI side-tx/vote-extension semantics and consensus replay paths.
Extended reasoning...
Overview
This PR closes a side-tx response-slot censorship vector around MsgBackfillSpans by removing its side-handler / post-handler dispatch entries in x/bor/keeper/side_msg_server.go, while retaining the msg-server handler, proto definitions, codec/gRPC registration, and the CalcCurrentBorSpanId helper so historical blocks replay with matching ExecTxResult. Orphan scaffolding that only served the removed post-handler is also cleaned up: GenerateBorCommittedSpans, PostHandleMsgBackfillSpansMethod metric, post-handler-only event constants (EventTypeBackfillSpans, AttributesKeyLatestSpanId, AttributesKeyLatestBorSpanId), and deprecated Type()/Route() methods. A stale comment in SideHandleMsgSpan was rewritten.
Security risks
The change closes a slot-censorship vector (a no-op MsgBackfillSpans previously reserved one of the 50 maxSideTxResponsesCount slots regardless of its constant VOTE_NO vote, allowing crowding of legitimate side-tx-bearing msgs in the same block). There is a regression test (app/abci_backfill_spans_slot_test.go) covering 50 MsgBackfillSpans + 1 MsgCheckpoint → exactly 1 SideTxResponse. No new attack surface is introduced — the retained msg-server handler is the same validation-only path it has been since bc907770.
Level of scrutiny
This is consensus-critical: the change alters side-tx response budgeting (vote-extension shape) and any historical MsgBackfillSpans tx must replay deterministically. The author conducted an Amoy archive audit (pos-amoy-pbss-archive, tx_index: on) and found 2 historical txs (heights 8790181, 8790209, both Code=0, gas_used=81753), then deliberately chose option (a) from my prior review — keeping the msg-server handler routable so Code and GasUsed (which depends on real DB reads GetLastSpan / GetLastMilestone / GetSpan) match on replay. A no-op stub would not have preserved GasUsed. Mainnet audit was not feasible from the team's infra, so a conservative assumption was adopted — appropriate for v0.9.0.
Other factors
- Two prior detailed review cycles (codec.go:23, tx.proto:19, side_msg_server.go:32) identified real replay-safety bugs that the author addressed by reverting their initial "full removal" approach to the safer "keep handler routable + drop dispatch" approach.
- Codecov shows coverage holding (45.2%), SonarQube quality gate passed, tests passing including
TestABCI_StressTestWith100Blockscycling 5 msg types includingMsgBackfillSpans. - No bugs found by the bug hunting system this round.
- I'm deferring rather than approving because the change touches ABCI side-tx handlers and vote-extension slot accounting — exactly the surface my consensus-critical review guidance flags as needing a human signoff. The work itself looks correct; this is about the policy that consensus-critical surfaces are not eligible for shadow-approval.
|
Hey @lucca30 I have a doubt that this change would require a HF 😓
|
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
2 participants
Summary
Closes the side-tx response-slot censorship vector around
MsgBackfillSpansand removes the dead scaffolding around it that becomes orphan once the dispatch is gone, while preserving the msg-server handler so historical txs still replay correctly. Targeting v0.9.0.The side handler was a constant
VOTE_NOand the post handler was therefore unreachable, but every msg with a registered side handler reserves one of the 50maxSideTxResponsesCountslots inExtendVoteHandlerregardless of the vote it returns. A block heavy with no-opMsgBackfillSpanstxs could crowd the side-tx response budget and delay responses for legitimate side-tx-bearing msgs (MsgCheckpoint,MsgEventRecord, validator-join, ...) later in the same block. Removing the dispatch drops the registration and removes that pressure source.The msg-server handler is intentionally retained as validation-only (no state writes) so the 2 historical Amoy txs (see audit below) replay with the same
ExecTxResult.Side benefit, called out by @mardizzone on the equivalent private draft.
Touch points
Slot-censorship fix:
x/bor/keeper/side_msg_server.go— dropFillMissingSpansMsgTypeURLdispatch fromSideTxHandlerandPostTxHandler, drop the now-orphanSideHandleMsgBackfillSpans/PostHandleMsgBackfillSpansfunctions, drop theFillMissingSpansMsgTypeURLconstant.Orphan cleanup (post slot-fix):
x/bor/types/util.go— dropGenerateBorCommittedSpans(only the post handler used it).x/bor/types/msg.go— dropMsgBackfillSpans.Type()/.Route()(deprecated in modern cosmos-sdk).x/bor/types/events.go— dropEventTypeBackfillSpans,AttributesKeyLatestSpanId,AttributesKeyLatestBorSpanId(only the post handler emitted them).metrics/api/bor.go— dropPostHandleMsgBackfillSpansMethod.x/bor/keeper/side_msg_server.go:197— rewrite the stale "we need to backfill before proposing a new span" comment that referenced the now-removed recovery path.*_test.go— drop tests pinning the removed APIs (TestGenerateBorCommittedSpans,TestMsgBackfillSpans_Type,TestEventTypeBackfillSpans,TestAttributesKeyLatest*).Retained:
BackfillSpansrpc method, msg-server handler,BackfillSpansMethodmetric,ErrInvalidLastBorSpanID,CalcCurrentBorSpanId,MsgBackfillSpans+MsgBackfillSpansResponseproto messages, and corresponding tests. See "Replay safety audit" below.Replay safety audit
The pre-PR msg-server handler ran with no state writes but consumed ~81k gas via 2-3 DB reads (
GetLastSpan,GetLastMilestone,GetSpan). Removing it would replace historicalExecTxResult{Code=0, GasUsed=81753}withExecTxResult{Code=ErrUnknownRequest, GasUsed=0}during replay, causingLastResultsHashmismatch and halting sync-from-genesis on any chain where such a tx ever landed in a block.Audited via CometBFT
tx_searchformessage.action='/heimdallv2.bor.MsgBackfillSpans':pos-amoy-pbss-archive,tx_index: on, blocks 8788501..35935529): 2 txs found — heights 8790181 (AE3D46F7…) and 8790209 (2FC9428B…), bothCode=0, gas_used=81753, submitted by0x93feed2cc3d58c2b1bb62ce63125fa7fcaae7177.tx_index: onon our observation infra (all hosts run bor against the public heimdall API). Conservative assumption: behave as if mainnet has at least one such tx too.Hence: keep the msg-server handler routable. The PR drops the dispatch (slot-fix) and the orphan post-handler-only scaffolding, not the handler itself.
Executed tests
go test ./x/bor/... ./app/... ./metrics/... -count=1— 655 tests pass.make proto-gen— clean.TestABCI_StressTestWith100Blocks— exercises 5 txs per block across 5 distinct msg types (including the retainedMsgBackfillSpans) over 100 blocks; passes.app/abci_backfill_spans_slot_test.go, ported from the private draft): 50MsgBackfillSpanstxs followed by aMsgCheckpointyield exactly 1SideTxResponse— for the checkpoint.Rollout notes
MsgBackfillSpanstxs (none are expected; the msg has been a no-op sincebc907770always voted NO).ExecTxResultfor the 2 historical txs.