fix(legacy): never announce block-originated transactions to legacy peers#1073
Merged
oskarszoon merged 3 commits intoJun 11, 2026
Merged
Conversation
Contributor
|
🤖 Claude Code Review Status: Complete Current Review: No blocking issues found. Clean, well-scoped, well-tested change. Re-traced each layer against the current diff:
One design note (already acknowledged in the PR description, not a bug): dropping all History:
|
Contributor
Benchmark Comparison ReportBaseline: Current: Summary
All benchmark results (sec/op)
Threshold: >10% with p < 0.05 | Generated: 2026-06-11 10:21 UTC |
ordishs
approved these changes
Jun 11, 2026
ordishs
left a comment
Collaborator
There was a problem hiding this comment.
Verified the fix end to end: all seven block-context Validate call sites carry WithInBlock(true), the flag is set only on the Kafka-published copy (after CreateInUtxoStore, so it never leaks into persisted UTXO records), bit 4 was previously unused and is inert for the subtree-validation cache, and rolling upgrades degrade gracefully in all old/new combinations. Ran the touched packages fresh with -race — meta, validator, and netsync all green; vet clean.
icellan
approved these changes
Jun 11, 2026
The txmeta Kafka topic is a cache-population stream: block validation and legacy sync deliberately validate mined-block txs through the validator to pre-warm the subtree-validation txmeta cache, so every mined tx lands on the topic alongside fresh mempool txs. Legacy netsync consumed that stream as its tx announce source with no origin marker, INVing every synced and newly-validated block tx to peers once the FSM reached RUNNING. Peers check tx invs against their mempool, not the chain, so they getdata essentially everything — flooding pushTxMsg with TX_NOT_FOUND for txs that are long mined and already pruned (spent txs get DeleteAtHeight and catchup races the chain height past it). Carry the origin on the wire: claim bit 4 of the txmeta flags byte as Mined. The validator sets it from SkipPolicyChecks — only ever set when validating transactions from an already-mined block — and netsync drops flagged entries from the announce batcher. Existing consumers parse the flags byte with per-bit ops, so the new bit is inert for them.
SkipPolicyChecks is also set by subtree validation for transactions from announced subtrees — block candidates that are not mined yet — so "Mined" overstated what the flag means. InBlock covers all setters: the tx arrived as part of a block or announced subtree rather than via mempool submission. Announce-filter semantics unchanged.
Deriving InBlock from SkipPolicyChecks was wrong: SkipPolicyChecks is a general option that also arrives from external callers (gRPC ValidateTransactionRequest, the Kafka validation message, propagation forwarding), so a fresh mempool tx submitted with it set would have been suppressed from relay — the inverse of the bug being fixed. Thread provenance explicitly instead: new Options.InBlock / WithInBlock(true), set only at the block-context call sites (subtreevalidation x4, legacy netsync handle_block x3), carried over gRPC as in_block and the HTTP fallback as inBlock. The Kafka validation path is mempool-only and defaults to false.
11eb7fa to
02761c5
Compare
|
oskarszoon
added a commit
to oskarszoon/teranode
that referenced
this pull request
Jun 11, 2026
…mber resolution flag to proto field 12 Conflicts resolved: - validator_api.proto: bsv-blockchain#1073 took field 11 for in_block (merged upstream first, owns the number); unconfirmed_parents_at_candidate_height moves to field 12. Both fields kept. - validator_api.pb.go: regenerated from the resolved proto. - validator/Client.go: buildValidateTxRequest carries both InBlock (upstream) and UnconfirmedParentsAtCandidateHeight (this branch). WIRE SKEW WARNING for the existing beta deployment: the beta build sends the resolution flag as field 11, which post-merge means in_block. A beta subtreevalidation talking to a post-merge validator would silently set the wrong option (wedge returns, wrong provenance). Replace beta deployments wholesale; do not mix beta and post-merge services.
oskarszoon
added a commit
that referenced
this pull request
Jun 11, 2026
oskarszoon
added a commit
to oskarszoon/teranode
that referenced
this pull request
Jun 11, 2026
…r with candidate-height resolution Replaces the per-block ParentMetadata accumulator (bsv-blockchain#1013) on the block-validation path with the WithUnconfirmedParentsAtCandidateHeight validator option (introduced for the legacy path in bsv-blockchain#1065), then deletes the accumulator machinery. In-block parents resolve the unconfirmedParentHeight sentinel to the candidate block height inside validateTransaction — identical outcome to the accumulator for genuine in-block parents, without the hot-path seeding, per-tx metadata filtering, per-level mutex merges, or per-tx ParentMetadata wire payload. On a 5.24M-tx chained block this removes ~34% end-to-end overhead. Safety: - PoW (nBits + HasMetTargetDifficulty) now runs before subtree blessing in ValidateBlock, so the fail-open option cannot be triggered by a no-PoW header. - Floater backstop: a parent that is unconfirmed and not in the block fails open at tx level, then block.Valid surfaces ErrBlockIncomplete. When caught up (FSM not CATCHINGBLOCKS/LEGACYSYNCING) that is a genuine floater (SetMinedMulti->MinedSet invariant), so the block is invalidated/rolled back at every acceptance sink (optimistic background, non-optimistic, revalidation worker); while syncing it stays incomplete and is retried, preserving bsv-blockchain#1031. - Recorded parent BlockHeights are untouched (sentinel exact-match only), so fork/revalidation height resolution is unchanged. Deprecates proto field 10 (parent_metadata) via reserved; deletes ParentTxMetadata. Preserves the in_block provenance option (bsv-blockchain#1073, field 11); unconfirmed-parents resolution stays field 12. Closes bsv-blockchain#1066
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #1067 (fix 3) per review there, with the provenance flag reworked per @ordishs' feedback.
Problem
After legacy catchup reaches the tip and the FSM enters RUNNING, peers flood the node with getdata and
pushTxMsglogs:The txmeta Kafka topic is a cache-population stream: block validation, subtree validation, and legacy sync deliberately validate block/subtree txs through the validator to pre-warm the subtree-validation txmeta cache, so every such tx lands on the topic alongside fresh mempool txs. Legacy netsync consumed that stream as its tx announce source with only an
IsCoinbasefilter, INVing every synced and newly-validated block tx to peers. Peers check tx invs against their mempool, not the chain, so they getdata essentially everything — most of it long mined and often already pruned (spent txs getDeleteAtHeight, and catchup races the chain height past it). The found ones were worse: served to peers as if fresh mempool relay.Fix
Carry provenance on the wire, explicitly:
validator.Options.InBlock/WithInBlock(true)— set only at the block-context call sites: subtreevalidation (Server.go×2,check_block_subtrees.go×2) and legacy netsync (handle_block.go×3). NOT inferred fromSkipPolicyChecks, which external gRPC/Kafka/propagation submitters may set on genuinely fresh transactions (the flaw in the first iteration of this change, caught in fix(legacy): prevent sync stalls from backpressure-triggered peer rotation and dropped orphan continuation #1067 review).optional bool in_block = 11onValidateTransactionRequest, plus theinBlockHTTP fallback query param — same propagation pattern ascandidate_parent_median_time. The Kafka validation message path is mempool-only and defaults to false.meta.Data.InBlock, bit 4 of the txmeta flags byte (both serializers). Existing consumers parse the flags byte with per-bit ops, so the new bit is inert for them; the subtree-validation cache stores the raw bytes unchanged.InBlockentries before the announce batcher: mined/subtree txs are never INVed as fresh mempool txs. Subtree-origin txs were already relayed via the propagation path on first validation, so this is dedup, not lost relay.Testing
TestOptionsFromValidateRequest_InBlockRoundTrip/_InBlockDefaultsFalse: gRPC propagation; explicitly pins thatSkipPolicyChecks=truealone does NOT setInBlock.TestHTTPHandlerPath_InBlock_EndToEnd: HTTP fallback query → server parse.Test_InBlockFlagRoundtrip: flag survives both txmeta serialization formats.TestProcessTXmetaBatchMessage_SkipsInBlockTx: wire batch with in-block + mempool entries → only the mempool tx reaches the announce batcher.go test -raceclean onservices/validator,services/legacy/netsync,stores/utxo/meta;services/subtreevalidationclean;go vetclean;golangci-lintreports only pre-existing prealloc findings in untouched subtreevalidation test files.