perf(node): accelerate and harden mainnet block sync - #8
Conversation
Drive the 0..150k replay through the same serialized apply entry used by live sync, avoiding a synthetic block-body reserialization in performance comparisons.
Seed the maintenance cursor per process and rotate both DNS seed order and each resolved address list. Fresh restarts no longer deterministically redial the same cached prefix while preserving the original first-success resolution and bounded queue behavior. Three fresh full-validation 0..150k runs reached the exact stop hash (416.62s, 1959.27s, 839.69s); the prior immediate repeat failed to finish in one hour at height 36421.
Convict the lowest expired request only after a second drain opportunity, then release the peer lease so DNS maintenance can replace it and the stripe can be re-requested in the same tick. Carry the actual delivery peer across the P2P boundary so a retry peer cannot absolve the original non-responder. Late delivery from the original peer clears suspicion, local apply backlog remains no-blame, and cooldown prevents immediate stripe reacquisition.
Race one bounded common prefix across eligible peers before committing the deep download window. Preserve one-shot and churn invariants without assigning striped height holes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c7737a9d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Match Bitcoin Core by omitting OP_RETURN outputs and scripts above the consensus size limit from chainstate. Invalidate checkpoints written under the old admission semantics so they resume from validated headers instead of restoring a divergent UTXO set.
Capture the strict MAX_SCRIPT_SIZE boundary and checkpoint codec cutover that keep provably unspendable outputs out of chainstate. Op: extend
Write snapshot v4 records with a u32 output count so valid high-fanout transactions checkpoint cleanly. Keep v2/v3 compatibility reads, preserve first-error ordering under adversarial input, and fall back to restored headers when a node checkpoint carries an older UTXO payload.
Hash-table probes and rehashes only need the txid prefix or txid, but the old accessors decoded output-count metadata on every call. Read those fixed identity bytes from the validated record header instead. A 100,000-record v4 snapshot fixture ran 1.97x to 2.06x faster in forward and reverse 20-run comparisons; Callgrind instruction reads fell 54.3%.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cfd3df819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| writer.file.seek(SeekFrom::Start(position.offset))?; | ||
| writer.file.write_all(&header)?; | ||
| writer.file.write_all(body)?; | ||
| writer.file.flush()?; |
There was a problem hiding this comment.
When power is lost after a clean checkpoint is published, flush() only transfers the appended block to the OS; unlike the checkpoint artifacts, it does not make the bytes durable. FlatFilePruneBodyStore::persist_block_body then commits the position to the KV backend, so after reboot the durable checkpoint/index can claim the block exists while recovery truncates or cannot read the missing flat-file tail, and the restored applied tip prevents that historical body from being downloaded again. Add a file durability barrier (and directory sync when creating a new file) before committing the index position.
AGENTS.md reference: AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cfd3df819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rayon::ThreadPoolBuilder::new() | ||
| .num_threads(available.min(MAX_SCRIPT_VERIFY_THREADS)) | ||
| .thread_name(|index| format!("script-verify-{index}")) | ||
| .build() | ||
| .unwrap_or_else(|error| panic!("failed to build script verification pool: {error}")) |
There was a problem hiding this comment.
When the OS refuses one of these worker-thread creations—for example under a container PID limit or temporary resource exhaustion—the first block with at least 16 script checks panics while initializing this LazyLock, terminating sync instead of returning a validation error or using the serial path. Build the pool through a fallible initialization and fall back to serial verification when construction fails.
AGENTS.md reference: AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| let position = self | ||
| .files | ||
| .persist(existing, height, *hash.as_byte_array(), body)?; |
There was a problem hiding this comment.
Once this path stores the body in a .dat file and places only a 16-byte position in the KV row, the public BlockPruner::prune_step and UtreexoOnlyCoordinator::block_processed paths no longer delete block bodies: both still delete only the KV value and even report that position's length as bytes freed. Consumers using either advertised pruning API therefore orphan the corresponding flat-file bytes indefinitely, which is especially damaging for Utreexo-only operation. Pass the flat-file store through these entry points or deprecate them in favor of the file-aware staging and reclamation path.
Useful? React with 👍 / 👎.
| if manifest.coinstats.version != COINSTATS_VERSION { | ||
| return Err(IncompatibleCheckpoint::UnsupportedVersion { | ||
| component: "CoinStats", | ||
| version: manifest.coinstats.version, | ||
| }); |
There was a problem hiding this comment.
When a checkpoint was written by a version using a different CoinStats artifact version, this branch returns IncompatibleCheckpoint, which propagates out of NodeState::open and prevents the node from starting at all. CoinStats is a chainstate payload just like UTXO data: the immediately preceding UTXO-version branch and the payload-codec branch both retain the already validated header tree and resume with HeadersOnly. Apply the same degradation here so an upgrade or downgrade with an incompatible CoinStats schema can resync chainstate instead of requiring manual checkpoint deletion.
Useful? React with 👍 / 👎.
| if expected_len > MAX_CHECKPOINT_PAYLOAD_BYTES { | ||
| return Err(CheckpointError::Invalid(format!( | ||
| "checkpoint artifact {name:?} exceeds the payload bound" | ||
| ))); |
There was a problem hiding this comment.
The writer has no corresponding enforcement of this 64 GiB artifact limit, so once a valid UTXO snapshot grows beyond it, clean shutdown can fully write and publish that generation as CURRENT, but the next startup rejects the same artifact here and falls back to a headers-only resync. Enforce the limit while writing, before generation publication, or remove/raise the read-side limit so the node cannot publish a checkpoint that its own loader will never accept.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9413d3384d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut remaining_bytes = total_bytes; | ||
| let mut outcome = PruneOutcome::default(); | ||
| for (key, row_bytes, selected_file) in candidates { | ||
| if !selected_file && remaining_bytes <= target_bytes { |
There was a problem hiding this comment.
When a nonzero PrunePolicy is used, every completed flat file whose maximum height is below the reorg horizon is marked selected_file, and this condition bypasses target_bytes entirely. Consequently even a store already below its configured target—or configured with a very large retention target—deletes all eligible historical files instead of only enough files to reach the requested footprint. Select whole files incrementally based on their actual sizes and stop once the target is satisfied.
Useful? React with 👍 / 👎.
| if pending.is_some_and(|request| request.peer_addr == source.addr) { | ||
| *pending = None; | ||
| } |
There was a problem hiding this comment.
When the selected highest peer answers with an empty headers message, or only repeats already-known headers, accept_headers succeeds and this clears its pending request even though the header tip did not advance. Because the inbound response wakes sync and the same peer remains the deterministic header_peer, the same tick immediately sends another getheaders; a stale or malicious peer can therefore create a continuous request/empty-response loop that consumes CPU and bandwidth. Clear the request only after verified progress, or demote/rotate a peer whose response makes none.
Useful? React with 👍 / 👎.
|
|
||
| fn header_record(&self, hash: Hash256) -> Option<BlockRecord> { | ||
| let tree = self.block_tree.read(); | ||
| let node = tree.node_by_hash(hash)?; |
There was a problem hiding this comment.
For a hash on a known side branch, this now synthesizes the same BlockRecord shape as for an active-chain block without retaining whether the node is active. The verbose getblockheader/getblock path subsequently computes confirmations solely from record.height and obtains nextblockhash from the active chain at that height, so a fork header below the applied tip is reported with positive confirmations and an unrelated active successor instead of side-chain status. Carry active membership through record resolution or have the handlers check the queried node against the active chain.
Useful? React with 👍 / 👎.
| let current_generation = match read_current(&root) { | ||
| Ok(Some(current)) => current.generation, | ||
| Ok(None) => 0, | ||
| Err(LoadStageError::Candidate(error)) => return Err(error), |
There was a problem hiding this comment.
If CURRENT is truncated or otherwise malformed, startup deliberately treats the parse failure as a rejected candidate and continues cold, but every later clean shutdown reaches this branch and returns the same error before allocating a replacement generation. The node can therefore fully resync yet can never publish another usable checkpoint, and every shutdown reports failure until the operator manually deletes CURRENT. Treat a candidate-level CURRENT error as having no usable current generation (while still preserving genuinely incompatible-version handling), or quarantine the bad pointer before publication.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c345292670
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
bitcoin-rs/crates/node/src/apply.rs
Lines 545 to 546 in 2697cfe
Verify preserved bytes before persisting them
When apply_block_with_serialized receives same-length bytes that do not encode block, release builds accept them because the equality check is debug-only. The decoded block is validated and its hash becomes the storage key, but the unrelated bytes are then persisted and passed to txindex, corrupting durable block lookup and transaction offsets; this can happen through any public/local caller that reuses a stale equal-sized buffer. Compare the bytes in release builds or fall back to canonical serialization on mismatch rather than treating length as an identity check.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Some(tree) = &self.block_tree { | ||
| let active_hash = tree.read().active_node_at_height(height)?.hash; | ||
| return self.resolve_block_by_hash(height, active_hash); |
There was a problem hiding this comment.
When a higher-work header-only fork branches below the applied tip, active_node_at_height follows that unvalidated fork even though txindex rows and persisted bodies still describe the applied chain. Because this source is wired into IndexerHistoryReader in state.rs, Electrum history and transaction resolution can return missing data—or resolve the wrong block once a fork body exists—until block application catches up. The fresh evidence beyond the previously reported RPC case is this separate NodeBlockSource path; give it the applied-tip handle and resolve heights from that ancestry instead.
Useful? React with 👍 / 👎.
| let live = peer_outbound.read().len(); | ||
| if live >= P2P_OUTBOUND_PEER_TARGET { | ||
| maintenance_delay = bootstrap_refill.next_delay(live, 0); | ||
| continue; | ||
| } | ||
| let deficit = P2P_OUTBOUND_PEER_TARGET - live; |
There was a problem hiding this comment.
When the listener has at least eight inbound connections, this treats them as satisfying the outbound target because peer_outbound contains leases for both connection directions. After the initial random outbound peers disconnect, an attacker—or merely sustained inbound traffic—can therefore prevent every DNS refill and leave sync dependent entirely on inbound-selected peers, enabling an eclipse or prolonged sync stall. Count registry entries with inbound == false, or maintain a separate live-outbound set, when computing this deficit.
Useful? React with 👍 / 👎.
1 participant
Summary
Mainnet initial block download now keeps the download window full, recovers from slow peers without discarding unrelated work, and accepts overlapping header batches idempotently. On the same six-peer cohort and 0→150,000 mainnet window, this branch completed in 167.339 seconds versus 502.417 seconds on
origin/main.Design
Performance
Identical command shape, storage backend (
fjall), six configured peers, release build withkernel-node, and validated start/stop hashes:origin/main(b32d30e)9c7737a)This is one matched public-network sample per revision. The fixed cohort removes DNS selection variance, but Internet peer throughput can still vary between sequential runs.
Validation
cargo fmt --all -- --checkcargo deny check advisoriescargo clippy -p bitcoin-rs --all-targets --no-default-features --features "rocksdb,fjall,redb,mdbx,bitcoinconsensus" -- -D warningscargo test --workspace --no-fail-fast— 1,051 passed; 13 ignoredcargo test -p bitcoin-rs --no-fail-fast --no-default-features --features "rocksdb,fjall,redb,mdbx,bitcoinconsensus"— 182 passed; 11 ignored