[niteshbalusu11]

Summary

getblockheader and getblock on a held side block (a body in RAM with no header row) now report height and the cumulative chainwork, as Core does.

 getblockheader / getblock (held body)
-  height:    omitted
-  chainwork: this block's work only        # getblock: only when the parent is active
+  held_height_and_work(block)
+    parent active        -> its height and work
+    parent stored header -> off_chain_header(parent) height and work
+    parent held          -> walk to the next parent
+  height = parent + 1, chainwork = parent work + this block's work

held_chainwork_hex is replaced by the new helper, so both RPCs use one path.

Evidence

The same blocks were sent to bitcoind 31.1 and rbitcoin. Each held block below ties the tip at height 4, where Core's chainwork is 0x0a.

Held block's parent	Core	master	this PR
on the active chain (getblockheader)	4, 0x0a	no height, 0x02	4, 0x0a
held	4, 0x0a	no height, 0x02	4, 0x0a
stored side header	4, 0x0a	no height, 0x02	4, 0x0a

getblock matched the getblockheader row in each case. The one exception is getblock with an active parent, where master already had the right chainwork but no height.

Test: chain_ops_held_height_and_work, in the mature-chain RPC journey, submits one held block of each kind. It checks height and chainwork from both RPCs against the tip.

• Before: fails with Null != 144 on the first held block. It also fails with either new branch of held_height_and_work disabled.
• After: passes.

Merge Danger

Door: two-way

RPC output only. No consensus, store, or schema change.

Blast Radius: RPC JSON

Only held side blocks. Active and archived blocks take the same paths as before.

Known limit: the values still need the block's ancestors. Once a held ancestor is evicted from the 320-body cache, height and chainwork stay unknown for the blocks above it, as on master.

Found while working on #1089. Core's submitblock returns bad-prevblk for a block whose parent is invalid, while rbitcoin returns inconclusive. That is not fixed here: a fix that matches Core needs rbitcoin to stop caching CheckBlock rejects, and duplicate-invalid on resubmit is pinned by the journey.

🤖 Generated with Claude Code

[rearden-grok]

Verdict

Comment. Four passes (DoS, security, correctness, performance) found no merge blocker. held_height_and_work is the right read-only replacement for held_chainwork_hex: height is parent + steps, chainwork is the ancestor's cumulative work plus each held header above it, once. Active blocks and stored side headers stay on their existing paths. confirmations stays -1, and enrich_block_header_json is still called with height: None, so a held body does not pick up the active chain's chainwork or nextblockhash.

DoS

The walk is a for depth in 1..=OFF_CHAIN_WALK_MAX loop (10_000), not recursion. A real chain stops earlier: HeldBodies::CAP is 320, and held_body returning None aborts the whole result rather than publishing a partial sum. A prev-hash cycle that would burn all 10,000 steps is not something a peer can build, because the block hash commits to prev_blockhash.

The new cost is one held_body clone per held ancestor (see performance). That is bounded by the cache the node already keeps. The active-chain getblock / getblockheader path does not enter the helper. Verbose false and getblock verbosity 0 return before it.

if let Ok(Some(loc)) = off_chain_header(...) treats a store error and "side header walk exceeded" the same as a miss, then tries held_body. A stored header that ensure_header accepted is already within the same 10,000-step cap, so this does not repeat a failed walk in practice. Stopping on Err would match off_chain_header's own contract.

Security

RPC output only. No consensus, store, or validation change. A peer can only affect JSON for a body this node already parked. The response still has "confirmations": -1. getblockcount and getblockchaininfo chainwork stay on the active tip. An equal-work fork reporting the same height and chainwork as the tip is the same shape Core and the existing stored-side-header path already return; the active chain is confirmations > 0 and getchaintips status "active".

bitcoin::Work's + wraps in release and debug_asserts on overflow. This tree already avoids that operator in sum_work / checked_add_work (crates/rbitcoin-net/src/most_work.rs). Overflow is not reachable from public ingress (submitblock rejects high-hash; a target small enough to overflow a 256-bit sum cannot be ground). h.0 + depth is a plain u32 add; off_chain_header uses saturating_add for the same quantity. Both are safe at real heights. Prefer sum_work so a corrupt nBits cannot trip the debug assert.

TCP getblock / getblockheader stay authenticated. --rest (default off) already served these objects with no bearer token; this does not widen that.

Correctness

• Active parent. height_of_hash is the best-chain index. work_through_height is the prefix through that height. A side block whose parent is an ancestor of the tip uses that ancestor, not the tip.
• Stored side header. off_chain_header already includes that header's own work. The held child adds depth and the work accumulated above it, so the stored header is not added twice.
• Held parent. Each body is added once, then the walk moves to its prev. A miss or eviction drops the partial sum: height is omitted and chainwork falls back to this block's own work (same fallback as held_chainwork_hex on master). The RPC does not error.
• Both RPCs call held_height_and_work_hex. A block that itself has a header row still uses off_chain_header.

chain_ops_held_height_and_work locks the three parent kinds only for blocks that tie the tip. Regtest work is 2 per block, so a missed add would fail — but copying the tip's height and chainwork into every held block would pass. held_parent is submitted and never read. Its parent is the active block at tip-2, so its own height is tip-1 and its chainwork is not the tip's. Asserting that (and "confirmations": -1) is the check the tip comparison cannot see. Also untested: eviction (height absent, chainwork is this block only) and more than one held ancestor.

The changelog's "stay unknown" matches height. On that path chainwork is still present, as this block's own work.

Performance

Active-chain lookups are unchanged. A held block with an active parent is one height-index lookup, one work_through_height, and one header.work() — same order as the old helper.

A header-less ancestor pays, per step: one height-index lookup, one get_header_by_hash miss (no second ancestry walk), and held_body, which clones the whole Block under the held-body read lock:

pub fn held_body(&self, hash: &BlockHash) -> Option<Block> {
    self.held_bodies.read().unwrap().get(hash).cloned()
}

The map stores Arc<Block>, but the clone copies txdata. The lock drops after the copy; peak extra memory is one block. Depth is at most 320, and the work is nBits arithmetic, not proof-of-work. getblock runs the helper once for both fields. A header-only peek would drop the clone. Not required to merge at this cap.

Follow-ups (not blocking)

1. Assert held_parent: height tip - 1, chainwork equal to the active block at tip - 1, and confirmations == -1.
2. On the held walk, read the header (or clone the Arc) instead of the full body.
3. Add work with sum_work instead of Work's +.

[niteshbalusu11]

@rearden-grok Thanks. I took the small ones in 262d7fe:

1. The journey now checks held_parent: its height and chainwork match the active block at tip - 1, and every held block has confirmations: -1 from both RPCs. I checked that copying the tip's height and work into every held block fails it.
2. Held work is summed with rbitcoin_net::sum_work instead of Work's +. A store Err from off_chain_header now ends the walk, instead of being treated as a miss.
3. The changelog says what eviction leaves: height is omitted and chainwork is that block's own work.

I left the header-only peek on the held walk (your item 2) as is, since it is bounded by the 320-body cap. Eviction and longer held chains are still only covered by the harness.

Please re-review 262d7fe.

[rearden-grok]

Verdict

Approve. Re-reviewed 262d7fe8 against the call sites, the parent walk, and the journey. The earlier blockers I had were test and arithmetic nits, and they are fixed. What is left is cost on an authenticated RPC, which is the right place for this work.

held_height_and_work is only used from held_header_value (getblockheader) and disconnected_block_value (getblock). Active blocks return in getblockheader before that, through height_of_hash. A stored header returns through off_chain_header. Verbose false and getblock verbosity 0 return raw bytes before the walk. This does not run on the peer or validation path.

What the new commit fixes

• Held work is rbitcoin_net::sum_work of the ancestor base plus each held header above it. bitcoin::Work's + debug-asserts on overflow; this path no longer uses it. height.checked_add(depth) fails closed the same way: no height, and chainwork falls back to this block.
• off_chain_header's Err now aborts the walk (.ok()?). A miss (Ok(None)) still falls through to held_body. A store error cannot be reread as a held parent.
• chain_ops_held_height_and_work reads held_parent and requires its height and chainwork to match the active block at tip - 1, and confirmations == -1 on both RPCs. Copying the tip into every held block fails that row. on_held still locks a two-step walk (active grandparent, one held parent).
• The changelog matches the eviction fallback: height omitted, chainwork is this block's own work.

Adversarial checks

Accounting. works starts as this block and gains a held parent only in the continue branch. The ancestor that stops the walk is not pushed again. An active parent contributes work_through_height (best-chain prefix through that height). A stored side header contributes off_chain_header's cumulative work, which already includes that header. Height is ancestor + depth, and depth counts from the parent, so a chain active P, held A, held B is height h+2 and work W(P)+A+B.

Wrong chain. height_of_hash is the best-chain index. A side header is not given the active chain's work at the same height. getblock still passes None into enrich_block_header_json, so the new height field does not replace chainwork with the active prefix at that height and does not attach the active nextblockhash. confirmations stays -1.

Failure modes. A missing or evicted ancestor, a sum_work overflow, or a store error returns None. The RPC still succeeds, with no height and only this block's work. It does not publish a partial sum. A prev-hash cycle cannot be built by a peer (the hash commits to prev_blockhash); the loop cap is 10,000 and the held cache is 320.

Scope. Filling that cache is existing p2p behavior. The new work is one RPC that then walks it: per held step, a height-index lookup, a header miss, and held_body's full Block clone under the read lock. The lock drops after the copy. Peak extra memory is one block, depth at most 320. That memcpy is real, and it is an authenticated getblock / getblockheader, not a public ingest path. Leaving the header-only peek out is reasonable at this cap.

Required checks on 262d7fe8 are green (qc, test, windows, macos, coverage).

Residual, not blocking

mediantime for a held body still comes from median_time_from_wire, which only follows stored header rows. A held parent adds no ancestor times. That is unchanged, and confirmations: -1 still marks the block off the active chain. Eviction and a held chain longer than two are not in the journey; the miss path is the held_body()? abort.