[niteshbalusu11]

Summary

getblocktemplate proposal mode (check_block_proposal_with, also behind ChainHub::check_block_proposal) passed three kinds of block that Core's TestBlockValidity rejects, and that rbitcoin's own submitblock also rejects.

 check_block_proposal_with
   bad-diffbits
-  mtp = median_time_past(tip).unwrap_or(tip time)
+  mtp = median_time_past_store(tip)?                 # now the BIP113 cutoff and the BIP68 block median
   time-too-old
-  time-too-new (inline 2 h check)
+  check_header_version_and_future_time under `now`   # + bad-version (BIP34/66/65 floors)
   validate_block_structure
   proposal_connect
-    is_final_tx(tx, h, mtp.max(header.time))          # always header.time: pre-BIP113
+    is_final_tx(tx, h, assemble_lock_time_cutoff(..)) # the connect path's cutoff (parent MTP after CSV)
     resolve inputs, maturity, values, fees
+    reject_sequence_locks(coin heights, coin MTPs)    # BIP68, Core's ConnectBlock order; coin MTPs via median_time_past_store

The rules come from the helpers the connect path already uses. assemble_lock_time_cutoff is now exported from rbitcoin-query. The coin heights come from the inputs the proposal already resolves: the block height for a coin created in the same block, and the create height for a confirmed coin. Every median comes from confirmed headers through median_time_past_store, the helper connect's structural_bip68 uses. A store fault reading one is reported as that store error. It is not a reject reason, and the tip's own timestamp is never used in its place.

This matters more once #976 lands, because that PR validates declared Stratum v2 custom jobs through check_block_proposal.

Evidence

Both nodes were given the same proposals: bitcoind v31.1 and rbitcoin regtest, with recent block times and the proposal built on the tip.

Proposal	Core 31.1	master	this PR
tx nLockTime = tip MTP + 100, header time MTP + 1000	bad-txns-nonfinal	null	bad-txns-nonfinal
tx nLockTime = tip MTP	bad-txns-nonfinal	null	bad-txns-nonfinal
tx nLockTime = tip MTP − 1	null	null	null
v2 spend of a coin confirmed at the tip, nSequence = 10 blocks	bad-txns-nonfinal	null	bad-txns-nonfinal
same coin, nSequence = 1 block	null	null	null
same coin, nSequence = 5 × 512 s	bad-txns-nonfinal	null	bad-txns-nonfinal
block nVersion = 1	bad-version(0x00000001)	null	bad-version(0x00000001)
block nVersion = 3	bad-version(0x00000003)	null	bad-version(0x00000003)

Tests, extending the existing proposal journeys:

• rpc_regtest_from_genesis (chain_ops_empty_template_and_proposals): version 3 is bad-version(0x00000003). Time u32::MAX is time-too-new, which pins the check this PR replaced.

• rpc_regtest_mature_chain_ops (chain_ops_proposal_spends):

  • BIP113: a lock time equal to the tip MTP is bad-txns-nonfinal; one second below is valid.
  • BIP68 height lock: met exactly is valid; one block short is bad-txns-nonfinal.
  • BIP68 time lock: zero units is valid, which needs the coin's median time; u16::MAX units is bad-txns-nonfinal.
  • Same-block coin: a child of a parent in the same proposal with a 0-block lock is valid; with a 1-block lock it is bad-txns-nonfinal.

• check_block_proposal_reads_medians_from_confirmed_headers (rbitcoin-net): blocks are 600 s apart and the coin is the height-12 coinbase.

  • 118 × 512 s is valid and 119 is bad-txns-nonfinal. 118 would fail if the median of the coin's own block were used instead of the block before it.
  • Height 11 is blanked in the confirmed index: the coin-median read returns the store error, not bad-txns-nonfinal.
  • Height 110 is blanked and the fence rebuilt, which leaves the MTP ring cold: the tip-median read returns the store error and does not accept on the tip's timestamp.

• Before: rpc_regtest_mature_chain_ops fails with BIP113: left: Null, right: "bad-txns-nonfinal". rpc_regtest_from_genesis fails with left: Null, right: "bad-version(0x00000003)".
  After: both pass.

• Before the review fix: check_block_proposal_reads_medians_from_confirmed_headers fails with coin median: bad-txns-nonfinal. With only the coin fix applied, it fails on the tip median with unwrap_err() on Ok(0).

• Hand mutations (no cargo mutants): each of these fails the journey on its own assert:

  • skip the BIP68 call → BIP68 height lock one block short
  • same-block coins at height 0 → a same-block coin is zero blocks deep
  • never read the coin median time → a met time lock reads the coin's median time
  • coin median at the creating height instead of the height before it → 118 intervals is bad-txns-nonfinal
  • lock-time cutoff of 0 → BIP113: one second below the median is final

Local: fmt, clippy -D warnings, ast-grep, cargo test --workspace --exclude rbitcoin-bench, ci-os-smoke.sh. cargo deny is not installed here; this PR changes no dependencies.

Not changed: proposal mode still accepts a header time equal to the parent MTP (time < mtp). The comment there says this is deliberate, for the harness cache. Core returns time-too-old.

Merge Danger

Door: two-way

RPC verdict only. No consensus, store, or schema change; block connect is untouched. rbitcoin-net turns on the rbitcoin-store test-hooks feature for its own tests only (dev-dependency).

Blast Radius: proposals

A proposal that used to return null can now return bad-txns-nonfinal or bad-version(…). In each case, submitblock would have rejected that block anyway.

🤖 Generated with Claude Code

[rearden-grok]

Discard this empty note. The review is the request-changes review on this pull request.

[rearden-grok]

Verdict

Requesting changes. The three rules are wired through the connect helpers, and the journey pins the happy path (BIP113 equality, BIP68 height at the exact depth and one short, same-block depth 0 vs 1, version 3, u32::MAX → time-too-new). Two median reads can still accept a lock that submitblock rejects. That is the failure mode this PR is closing, and it is the verdict #976 will trust for Stratum v2 custom jobs.

Required checks on 6fb9c764 are green (qc, test, windows, macos, coverage).

What matches connect

• Absolute finality uses assemble_lock_time_cutoff: header time before CSV and at height 0, parent MTP once CSV is active. is_final_tx keeps the strict <.
• Relative locks run only under csv_active_at, then bip68_active_for_tx (unsigned version ≥ 2).
• A same-block coin is recorded at the proposal height. A confirmed coin uses the tip-index create height. The coin time is the median at create_height.saturating_sub(1), which is also what structural_bip68 loads. Height 0 stays a lookup of block 0.
• Sequence::is_time_locked is the disable bit clear and the type bit set, same as the mask in structural_bip68.
• Height math matches Core EvaluateSequenceLocks: coin_height + rel - 1 >= block_height rejects. The age / age+1 pair and the same-block 0 / 1 pair pin that, including the off-by-one.
• Version floors and the two-hour cap are check_header_version_and_future_time under the caller clock. The predicate is still time > now + 2h. BadVersion formats as bad-version(0x{:08x}), same as submit. Future-time is still checked before the version floor, so a far-future version-1 block stays time-too-new.
• The old inline future-time check is gone, so there is one clock path.

Changes

1. mtp is still median_time_past(tip).unwrap_or(tip_rec.timestamp). That value is now the BIP113 cutoff and the BIP68 block median. On a chain whose timestamps move forward, the tip time sits above the median, so a failed median read accepts nLockTime in (real MTP, tip time]. That is the hole the success path just closed. Fail the proposal when the median read fails. Do not substitute the tip timestamp once this value is a consensus cutoff.
2. Coin medians go through median_time_past, which will fill a missing header from a confirm header plan. Connect uses median_time_past_store and will not connect that block. A lower plan timestamp shrinks min_time and can accept a relative time lock. map_err(|_| "bad-txns-nonfinal") also turns a store fault into a consensus needle. sequence_locks_satisfied already rejects a zero median. Use the store helper. Leave a store error as an RPC error.

Not blocking

• time < mtp still accepts a header time equal to the parent median. The comment and the PR body say this is deliberate for the harness cache. It is still a false accept against Core and against submitblock (time <= mtp there). A cached template at exactly the new median is not connectable. #976 should not treat a null proposal as submit-equivalent while this stands. Matching <=, or confining the exception to the harness, can be a follow-up.
• The 0-interval relative time lock only proves the coin median is non-zero. A raw median of 0 fails closed inside sequence_locks_satisfied, so "never read the median" does trip that assert. Any other non-zero stamp also passes, and u16::MAX intervals (~388 days) fails for every plausible median. Neither case pins which block's median was read. A lock that is final for the real previous-block median and non-final for 1, or for this block's own median, would.
• BIP113 never asserts the accept side (tip_mtp - 1 → null). The equality reject is what catches the old max(mtp, header.time) cutoff. A cutoff of 0 would still reject equality and hide on the rest of this journey, because the other spends use lock time 0.
• The coinbase-maturity fence is re-read before the height is pushed, so a memo/fence mismatch becomes bad-txns-inputs-missingorspent and does not reach BIP68. A reorg that moves both together is still a race. Not worth a separate path.

[niteshbalusu11]

Thanks. Both changes are in 9360b913, rebased onto master.

1. Tip median fallback: fixed. mtp is now median_time_past_store(query, tip_h).map_err(|e| e.to_string())?, with no fallback to the tip timestamp. To make that read fail, the new test blanks height 110 under a tip at 112 and rebuilds the fence, which leaves the MTP ring cold. The old code then returned Ok(0) for a coinbase-only proposal. It now returns the store error.

2. Coin medians: fixed. They go through median_time_past_store, the helper mtp_at uses. A store fault is that store error, not bad-txns-nonfinal. With height 11 blanked under a coin at height 12, the proposal used to return bad-txns-nonfinal. It now returns store: corrupt record: confirm: write MTP missing confirmed header (carry prev_mtp).

On "stay an RPC error": the proposal path has one Err(String) channel today, and gbt_proposal returns every Err as the BIP22 result string. The tip-header read and the two fence reads in coinbase_spend_is_immature already report store faults as e.to_string() there. The two median reads now do the same, so a fault reads as a store error and not as a consensus reject. Raising every fault in the proposal path as RPC -25, like submitblock and Core's state.IsError(), would change the contract for all of those sites. That fits better as its own change, sized for #976, than in this PR.

Not blocking:

• Which median: pinned in check_block_proposal_reads_medians_from_confirmed_headers. Blocks are 600 s apart and the coin is the height-12 coinbase. 118 × 512 s is null and 119 is bad-txns-nonfinal. A hand mutation that reads the coin's own block median (Height(h)) turns 118 into bad-txns-nonfinal. It is a hub test because the journey's coins sit on blocks about a second apart.
• BIP113 accept side: added. tip_mtp - 1 must be null. With the cutoff forced to 0, the journey fails on that assert.
• time < mtp: agreed. The < is older than this PR and is documented as deliberate for the harness cache. The PR body lists it as not changed. A Core-equal <=, or an exception limited to the harness, can be a follow-up.
• Fence race: agreed, no separate path.

The new test needs set_raw_for_test, so rbitcoin-net turns on rbitcoin-store's test-hooks feature as a dev-dependency only. Local: fmt, clippy -D warnings, rbitcoin-net (924) and rbitcoin-rpc (125) lib tests.

[niteshbalusu11]

CI note: on 9360b913, test, qc, windows and macos passed, but coverage failed on ibd::events::confirm_reject_tests::verdict_applied_after_the_tip_moved_asks_for_the_body_again. The assert at confirm_reject_tests.rs:3715 (apply_header_rewind(..) returned false) is in the IBD rewind rig, which this PR does not touch. The same file failed the same way in master's coverage job this morning (run 39422897442, spend_of_a_reorged_out_tx_is_invalid, maybe_rewind_to_best_work(..) returned false). It passes 40 of 40 runs locally on this branch. I cannot rerun jobs here; a rerun of the coverage job should clear it.

[reardencode]

haha, already reran :)

[rearden-grok]

Verdict

Approving 9360b913. The two changes from the last review are in, and they close the false-accept paths. A separate security review of this diff found no medium, high, or critical issue.

qc, test, windows, and macos are green on this head. coverage was still running.

Previous requests

1. The parent median is median_time_past_store(tip)?. A failed read is the store error. The tip timestamp is no longer the BIP113 cutoff or the BIP68 block median.
2. Coin medians use median_time_past_store as well, the same helper structural_bip68 uses through mtp_at. A missing confirmed header stays a store error, not bad-txns-nonfinal.

check_block_proposal_reads_medians_from_confirmed_headers pins which median. Blocks are 600 s apart. A relative lock of 118×512 s is final for the median of the block before the creating block, and 119 is not. 118 would fail if the creating block's own median were used. Blanking height 11 fails the coin read and still lets an empty proposal through. Blanking height 110 fails the tip read. The journey now also accepts nLockTime == tip MTP − 1.

What still matches connect

• Cutoff is assemble_lock_time_cutoff: header time before CSV and at height 0, parent MTP after CSV. is_final_tx is strict <.
• BIP68 runs only when csv_active_at, then the unsigned version ≥ 2 gate.
• Same-block coins are at the proposal height. Confirmed coins use the tip-index create height. Coin time is the median at create_height.saturating_sub(1).
• Sequence::is_time_locked is the disable bit clear and the type bit set.
• Height locks match EvaluateSequenceLocks. The journey's age / age+1 pair and the same-block 0 / 1 pair pin the off-by-one.
• Version floors and the two-hour cap are check_header_version_and_future_time under the proposal clock. The predicate is still time > now + 2h. bad-version(0x{:08x}) matches submit. A far-future low version is still time-too-new, because the future check runs first.
• test-hooks on rbitcoin-store is a dev-dependency. set_raw_for_test stays behind that feature.

Security review

No medium or higher finding. The old proposal false accepts (BIP113 cutoff max(parent MTP, header time), no BIP68, no version floor) are gone. Store and median failures return Err and do not accept the block. SubmitSolution still goes through accept_block, not this preflight.

Left as-is

• time < mtp still accepts a header time equal to the parent median. Connect and Core use <=. The comment says this is for the harness cache. It is a preflight lie: that block is not connectable. GBT mintime is mtp + 1, so an honest template does not hit it. Not a chain-accept bypass.
• expected_next_bits(...).unwrap_or(tip bits) is unchanged and still fail-open on a store error. Out of this diff.