Skip to content

fix(consensus): reject a block whose hash is above its target on connect - #23

Open
MeABzZz wants to merge 1 commit into
BTCDecoded:mainfrom
MeABzZz:criticals/connect-pow
Open

MeABzZz wants to merge 1 commit into
BTCDecoded:mainfrom
MeABzZz:criticals/connect-pow

Conversation

@MeABzZz

@MeABzZz MeABzZz commented Oct 21, 2026 •

Copy link
Copy Markdown

What

connect_block_inner checks that header.bits equals next_required_bits from the ancestor lookup. It never compares the header hash against that target. pow::check_proof_of_work exists, but nothing under src/block/ calls it.

The node hashes headers it ingests through headers messages. It does not hash the header of a full block it connects. As a result, these paths all accepted a block whose bits were right and whose hash missed the target:

  • a P2P block after IBD (handle_block_wire_message, then process_block)
  • submitblock, via connect_mined_block
  • reorg connect
  • parallel IBD
  • verifychain

Below the assume-valid height, scripts were skipped as well.

Fix

This calls check_proof_of_work inside the same difficulty_ancestor branch as the bits check. That matches Core: CheckBlockHeader, then CheckProofOfWork, run from CheckBlock inside ConnectBlock. Every node connect path builds this context: block_processor.rs, parallel_ibd/mod.rs, reorg_executor.rs, pruning.rs, and rpc/blockchain.rs. The check costs one double-SHA256 per block.

  • It runs after the BIP94 check. That way the existing BIP94 test keeps its reject reason, and both checks still reject.
  • Design choice, flagged for review: callers that pass no ancestor lookup are unchanged. That covers the library default, blvm-protocol::validate_and_connect_block, and most unit tests, which use fake headers. Checking unconditionally would match Core more closely, but it would need every fixture in this repo re-mined. A follow-up could make the context carry an explicit check_pow that defaults to on.

Evidence

The new tests/connect_pow_check.rs uses mainnet block 1, rebuilt from its coinbase. The test asserts the txid and merkle root 0e3e2357…, the hash 00000000839a8e68…, and that the block meets its target.

Test main This branch
vector_is_mainnet_block_1 ok ok
connect_accepts_mainnet_block_1 ok ok
connect_rejects_hash_above_target_with_correct_bits (block 1 with nonce 0: right bits, valid merkle root, hash above target) FAILED (accepted) ok
connect_regtest_pow_boundary (nonces 0..63, both sides of the regtest target) FAILED: "nonce 2: Valid" ok
  • cargo test (Rust 1.88): 2207 passed, 0 failed, 7 ignored. That is 2203 on main plus these 4.
  • cargo clippy --lib --bins -- -D warnings: clean.
  • cargo fmt --check: clean.
  • blvm-node full suite with this patched in: 3018 passed, 1 failed. The failing test, test_verify_chain_level4_with_undo_logs, has headers with no valid PoW. It is fixed in test(node): mine regtest proof of work for the verifychain level 4 fixture blvm-node#32, which must land before node CI builds against this.

Checklist

  • Can two implementations disagree? Yes before this change, since Core rejects these blocks. Now they agree on every path that has a context.
  • Attacker-controlled input? The header is. The check is constant work.
  • Not provided:
    • The unbounded queue_block deque (C2 in the review) is unchanged.
    • The submitblock path is admin-only, so the main exposure was P2P.

Review notes: merge order and open decisions (BLVM criticals set)

This PR is one of 7 fixing the four criticals from the BLVM three-lens review:
blvm-consensus#22 (undo log), blvm-consensus#23 (PoW vs target), blvm-node#32 (PoW test fixture), blvm-primitives#2 (min chainwork), blvm-node#31 (assume-valid), blvm-secp256k1#1 (no CUDA by default), blvm#10 (release build + no-CUDA check).

Merge order

  1. blvm-node#32 before blvm-consensus#23, otherwise node CI breaks (old fixture headers have no valid PoW).
  2. After the blvm-primitives release with Document mathematical proofs #2, regenerate the consensus golden file consensus_config_default.json, or its test fails.
  3. After the blvm-secp256k1 release with Add unit tests for edge cases #1, run cargo update -p blvm-secp256k1 in blvm, then cut a release with blvm#10. v0.1.63 and v0.1.64 stay broken (need libcudart) until then.

Decisions for the reviewer

Known, not fixed here: -assumevalid=<hash> CLI parses the hash in the wrong byte order; the GHCR image ships a different binary (blvm 0.1.0).

connect_block_inner checked that header.bits equals the retarget value
from the ancestor lookup, but never hashed the header against that
target. check_proof_of_work existed and nothing under src/block/ called
it. The node hashes headers it ingests, not the header of a full block
it connects, so a P2P block after IBD, submitblock, a reorg connect, and
parallel IBD all accepted a header whose hash missed the target, as long
as its bits were right.

Run check_proof_of_work (Core CheckBlockHeader -> CheckProofOfWork) in
the same branch as the bits check, which is the context every node
connect path builds. Callers that pass no ancestor lookup (library and
test paths) are unchanged. It runs after the BIP94 check only so that
test keeps its reject reason; both reject.

Test: tests/connect_pow_check.rs. Mainnet block 1 rebuilt from its
coinbase connects; the same header with nonce 0 has the required bits
and a valid merkle root and is rejected; on regtest a nonce grind hits
both sides of the target. The two reject cases fail on main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant