[bump] Derive corrected subtree-0 root from coinbase BUMP, not over-climbed STUMP

[rafa-js]

What Changed

• Delete computeCorrectedSubtreeRoot, whose stumpPath.ComputeRoot(coinbaseTxID) climbed every level of the subtree-0 STUMP with no height bound.
• Rewrite correctedSubtree0Root(coinbaseBUMP []byte, numSubtrees int) to derive the corrected subtree-0 root solely from the coinbase BUMP: internalHeight = len(cbPath.Path) − ceil(log2(numSubtrees)), then fold the coinbase exactly internalHeight levels via the existing bounded primitive subtree0RootFromCoinbaseBUMP.
• In BuildCompoundBUMP, derive the placement internalHeight from the coinbase BUMP when present (same formula), falling back to the first STUMP's height only when no coinbase BUMP is available.
• Relax the placement height guard from != to < so an over-tall STUMP is tolerated (only its first internalHeight levels are read); a STUMP shorter than internalHeight is still rejected.
• Add reproduction tests at the block-951360 shape and re-point the surviving unit test at subtree0RootFromCoinbaseBUMP.

Why It Was Necessary

merkle-service sometimes delivers subtree 0's STUMP at full block height instead of the subtree-internal height. The old unbounded climb then overshoots the subtree-0 root and lands on the block merkle root, poisoning subtreeHashes[0]; the compound's top tree is built over [block_root, s1, s2, …] and ValidateCompoundRoot rejects it. The transaction is never marked MINED, so it remains stuck and gets rebroadcast. Observed on mainnet block 951360 (tx 85598b7b2d994153487198f9b69b482471d496c50e694f17bb92dd611edadc97, subtreeCount = 3): block root 5ed5a7a9…ecbc, wrong computed root 49c8fd3a…4f23, STUMP treeHeight = 17 vs. true internalHeight = 15. The fix derives internalHeight from the coinbase BUMP (authoritatively full block height), making the correction immune to over-tall STUMPs.

Closes #183

Testing Performed

• go test ./bump/... — 89 tests pass, including two new reproductions: TestBuildCompoundBUMP_FullBlockSubtree0STUMP_Block951360 (over-tall subtree-0 STUMP + normal siblings; validates against the block root) and TestCorrectedSubtree0Root_Block951360_FromCoinbaseBUMP (corrected root equals the real subtree root, not the block root).
• TDD: both reproductions fail on the unmodified tree (placement height-mismatch / over-climb poison) and pass after the fix.
• go vet ./bump/... and gofmt -l bump/ are clean.

Impact / Risk

• Breaking change: None — public signatures of BuildCompoundBUMP / AssembleBUMP are unchanged.
• Behavioral risk: Low. For well-formed inputs the derived internalHeight equals the previous STUMP-derived value, so existing behavior is preserved; the change only affects over-tall STUMPs that previously failed.
• Caveat: The correction relies on a coinbase BUMP being present for multi-subtree blocks. With no coinbase BUMP, both the correction and the placement-height derivation fall back to the STUMP height (today's behavior), so a coinbaseBUMP == nil path with an over-tall STUMP would still hit the guard.

Notifications

• @mrz1836

[github-actions]

👋 Thanks, @rafa-js!

This pull request comes from a fork. For security, our CI runs in a restricted mode.
A maintainer will triage this shortly and run any additional checks as needed.

• 🏷️ Labeled: fork-pr, requires-manual-review
• 👀 We'll review and follow up here if anything else is needed.

Thanks for contributing to bsv-blockchain/arcade! 🚀

[oskarszoon]

Approve. Verified against live mainnet block 951360 (on-chain root 5ed5a7a9…, proof depth 17 → internalHeight 17−2=15): the bounded coinbase-BUMP fold yields the true subtree-0 root cbaaf149… instead of over-climbing to the block root. ceil(log2) has no float drift in range (nice touch pinning it with TestLog2Ceil_NoFloatDrift), bounds are guarded, 89 tests pass.

On the !=→< relaxation: it's safe. It loosens an input-shape check, not the acceptance gate — ValidateCompoundRoot still recomputes against the node's own chain-header merkle root, so a malformed or over-tall STUMP can only cause a loud rejection, never a forged accepted proof. Net it strengthens vs the old unbounded climb that caused this incident.

One thing to confirm before merge, not a blocker: the coinbase-BUMP-absent path for multi-subtree blocks isn't fixed (it falls back to the STUMP height and skips the subtree-0 correction). It still fails loud — the original stuck-tx symptom — so it's no worse than today, but it's not fixed and it's untested. Does ingestion guarantee a coinbase BUMP accompanies every multi-subtree block's STUMP set? If not, worth a follow-up for that residual + a test documenting the fallback.

Optional: the safety argument rests on ValidateCompoundRoot staying mandatory after BuildCompoundBUMP — a durable invariant/test guarding that would be cheap insurance.

[rafa-js]

Approve. Verified against live mainnet block 951360 (on-chain root 5ed5a7a9…, proof depth 17 → internalHeight 17−2=15): the bounded coinbase-BUMP fold yields the true subtree-0 root cbaaf149… instead of over-climbing to the block root. ceil(log2) has no float drift in range (nice touch pinning it with TestLog2Ceil_NoFloatDrift), bounds are guarded, 89 tests pass.

On the !=→< relaxation: it's safe. It loosens an input-shape check, not the acceptance gate — ValidateCompoundRoot still recomputes against the node's own chain-header merkle root, so a malformed or over-tall STUMP can only cause a loud rejection, never a forged accepted proof. Net it strengthens vs the old unbounded climb that caused this incident.

One thing to confirm before merge, not a blocker: the coinbase-BUMP-absent path for multi-subtree blocks isn't fixed (it falls back to the STUMP height and skips the subtree-0 correction). It still fails loud — the original stuck-tx symptom — so it's no worse than today, but it's not fixed and it's untested. Does ingestion guarantee a coinbase BUMP accompanies every multi-subtree block's STUMP set? If not, worth a follow-up for that residual + a test documenting the fallback.

Optional: the safety argument rests on ValidateCompoundRoot staying mandatory after BuildCompoundBUMP — a durable invariant/test guarding that would be cheap insurance.

Thanks for the careful review.

Coinbase-BUMP-absent multi-subtree path: confirmed — ingestion does not guarantee a coinbase BUMP. The datahub parser treats a zero-length coinbase-BUMP field as "absent" (bump/datahub.go:378), and coinbaseBUMPReconciles deliberately passes on an empty coinbase BUMP — "some peers legitimately omit it" (bump/datahub.go:201-206). So a multi-subtree block can reach BuildCompoundBUMP with coinbaseBUMP == nil.

In that case the subtree-0 root can't be corrected: the datahub computes it against the coinbase placeholder, and without the coinbase BUMP there's no real coinbase txid to fold and no full-block path to bound the climb. The compound root then can't match the header root, so the mandatory ValidateCompoundRoot after BuildCompoundBUMP (services/bump_builder/builder.go:481) rejects it and the tx stays non-MINED — the original stuck-tx symptom, but it never forges an accepted proof. So it's fail-safe and no worse than today; a correctness fix isn't possible without the coinbase data.

I've added TestBuildCompoundBUMP_NoCoinbaseBUMP_MultiSubtree_FailsSafe to lock that contract (compound builds, ValidateCompoundRoot rejects against the true block root — never silently accepts the placeholder root). I deliberately did not add an early-fail guard inside BuildCompoundBUMP: it would bypass the richer dumpBUMPFailureInputs diagnostic the builder emits on the validation rejection.

I'll open a separate follow-up issue to track "multi-subtree blocks without a coinbase BUMP can't be MINED" (an ingestion/datahub-guarantee question rather than a bump bug).

ValidateCompoundRoot stays mandatory: agreed it's load-bearing; the new test pins the "reject, don't forge" half of that invariant at the compound layer.

!=→< relaxation: agreed, thanks for confirming.

[Copilot]

Pull request overview

This PR fixes compound BUMP construction when merkle-service provides an over-tall (full-block-height) subtree-0 STUMP by deriving subtree-0 correction and placement height from the coinbase BUMP (authoritative full-block-height path) instead of trusting STUMP heights.

Changes:

• Remove the unbounded STUMP-based corrected-subtree-root computation and derive the corrected subtree-0 root from a bounded fold of the coinbase BUMP.
• In BuildCompoundBUMP, derive internalHeight from the coinbase BUMP when available and tolerate over-tall STUMPs by truncating to the required height (reject only under-tall STUMPs).
• Update/add tests to reproduce the block-951360 incident shape and to validate the bounded coinbase-based correction behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
bump/bump.go	Reworks subtree-0 correction and compound placement height derivation to use coinbase BUMP; relaxes STUMP height guard to tolerate over-tall STUMPs.
bump/bump_test.go	Enhances coinbase BUMP test helper to model full-block paths and adds/updates reproducer tests for the incident scenario.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.