fix(state): prefer the first-received chain on equal-work ties - #11341
fix(state): prefer the first-received chain on equal-work ties#11341arya2 wants to merge 4 commits into
Conversation
…work tie-break Pull the receipt-time tie-break key up from the tip block into a new Chain::received_time field, kept in sync with the tip by push and pop_tip, so Chain::cmp reads its ordering keys directly off the chain instead of looking up the tip block. The tip block hash remains the final tie-breaker when receipt times are equal.
Analyzed six files, diff |
conradoplg
left a comment
There was a problem hiding this comment.
Thanks! I like this one better.
I think this warrants a zebrad changelog entry.
Claude findings I agree with below. For C I think it would be best to avoid redundancy, but not a huge deal, your call.
A. The RPC mirror gets no first-received preference at all
zebra-rpc/src/sync.rs:375 and :495 build blocks with SemanticallyVerifiedBlock::with_hash, which sets received_time: None. Every chain in TrustedChainSync's non-finalized state therefore has received_time ==
None, all ties fall through to tip hash — exactly main's old behaviour — while the node it mirrors uses receipt time.
So on an equal-work fork, zebrad's getbestblockhash and the Zaino-facing mirror can name different tips. This is new: before this branch both used hash and always agreed. It's transient (resolves as soon as
work breaks the tie) but it's a real inconsistency between two views of the same node.
Cheap fix: stamp Some(Instant::now()) in sync.rs where the block is constructed. NonFinalizedBlocksListener streams best-chain-first (chain_iter() is chain_set.iter().rev()), so arrival order at the mirror
reproduces the source node's preference.
C. The denormalized Chain::received_time is correct today, but it's a new unenforced invariant
It duplicates the tip block's stamp and is a BTreeSet ordering key, so any path that changes a tip without updating it corrupts the set. I traced all of them:
- push — sets it after the fallible update_chain_tip_with, before inserting ✓
- pop_tip — sets it from the new tip ✓
- fork — loops pop_tip ✓
- Chain::invalidate_block — fork + pop_tip ✓
- reconsider_block — re-pushes stored blocks through push ✓
- pop_root — doesn't change the tip. Single-block case leaves the field stale, but the chain is then empty and finalize() guards re-insertion with !is_empty() ✓
The new expect("blocks is populated, asserted above") in pop_tip can't fire independently: the pre-existing assert!(!self.blocks.is_empty()) runs first, so the message is accurate.
So: no defect. But it's worth asking whether the denormalization earns its keep. cmp previously did one blocks.values().next_back() — a BTreeMap lookup on a chain, called O(log n) times per insert with n ≤ 10
forks. That's negligible, and reading the tip directly would make the invariant unfalsifiable by construction. Your call, but the trade is a correctness liability for a perf gain that won't be measurable.
Motivation
The Zcash protocol specification breaks ties between candidate chain tips with
equal cumulative work by preferring the block the node received first:
Sibling blocks on Zcash always have equal work (
nBitsis fully determined bytheir ancestors), so this is the common case for races at the chain tip. Zebra
previously broke equal-work ties by tip block hash, which let a later-arriving
equal-work sibling displace the already-adopted tip, causing avoidable reorgs
and divergence from zcashd's
nSequenceIdfirst-received behavior.Closes #11240.
Solution
(
zebra-consensus/src/block.rs), plumbed throughSemanticallyVerifiedBlockinto
ContextuallyVerifiedBlock. This is node-local, in-memory metadata:blocks from checkpoint sync, backup restore, and tests are unstamped (
None).Chaintracks its tip's stamp in a newChain::received_timefield, kept in sync byChain::pushandChain::pop_tip(which also coversfork,invalidate_block, andreconsideration replays).
Chain::cmpbreaks cumulative-work ties by preferring the earlier receipttime, keeping the tip block hash as the final tie-breaker so the order stays
total. Unstamped chains compare as received before stamped ones, like
zcashd's disk-loaded blocks, which all share
nSequenceId0. Reconsideredchains keep their original stamps, matching zcashd's behavior across
invalidate/reconsider.
Tests
equal_work_ties_prefer_first_received: commits twoequal-work siblings with controlled receipt times in both receipt orders and
checks the first-received sibling stays best both times (one order fails
under hash tie-breaking by construction), then checks strictly more work
still overrides receipt order.
chain_received_time_tracks_tip: the chain's receipt time mirrorsits tip block's stamp across pushes and fork truncation.
cargo test -p zebra-state --lib: 157 passed.cargo fmt --all -- --check,cargo clippy -p zebra-state --all-targets -- -D warnings, andcargo doc -p zebra-state --no-depsare all clean.Specifications & References
AI Disclosure
Chain::received_timerefactor,the tracking test, the changelog fragment, the doc-comment updates, and code
review. The initial receipt-time feature commit was written by the branch
author.
PR Checklist
type(scope): description