fix(spv): compute required proof headers from actual header difficulties#4039
Merged
piotr-roslaniec merged 1 commit intoJul 22, 2026
Conversation
getProofInfo assumed every proof header carries the relay epoch difficulty, so with txProofDifficultyFactor=1 it assembled single-header proofs. On testnet4 (BIP94), sweeps mined in minimum-difficulty blocks produced proofs containing only a DIFF1 header, which the Bridge rejects with "Not at current or previous difficulty". Mirror the Bridge's BitcoinTx logic instead: skip leading DIFF1 headers when both relay epochs are above minimum, bind the requested difficulty to the first decisive header matching the relay's current or previous epoch difficulty, and accumulate headers until their total observed difficulty covers requestedDifficulty * txProofDifficultyFactor.
|
Warning Review limit reached
More reviews will be available in 17 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
piotr-roslaniec
added a commit
that referenced
this pull request
Jul 22, 2026
## Summary - remove the client-only 144-header cap from the SPV proof walk - keep walking through mined headers until the proof has enough work or reaches the Bitcoin chain tip - skip only headers whose decoded target exactly matches the Bridge DIFF1 target - reject non-positive header targets before calculating difficulty ## Regression coverage - 144 DIFF1 headers followed by a decisive difficulty-16 header - non-DIFF1 bits 0x1d00aaaa, whose integer difficulty rounds to one, remain decisive - zero-target compact bits return a proof error instead of panicking ## Testing - go test -count=1 ./pkg/maintainer/spv - go test ./pkg/maintainer/... Stacked on #4039. The temporary base branch mirrors #4039 at 832c116; retarget this PR to main after #4039 merges.
lrsaturnino
approved these changes
Jul 22, 2026
Collaborator
|
Follow-up: a multi-agent review of this change surfaced some edge-case, observability, and test-quality findings. Since this PR is already merged, they are addressed in a separate follow-up PR: #4186 ( |
lrsaturnino
added a commit
to lionakhnazarov/keep-core
that referenced
this pull request
Jul 24, 2026
Resolve conflicts by final behavior, preserving the candidate's OOB hardening and the no-cap / exact-target SPV semantics while taking main's threshold-network#4039 work-accumulation model, threshold-network#4165 script-type isolation, threshold-network#4172 sweep fee floor, and the threshold-network#3998 v1-random-beacon extraction. SPV conflict resolution: - pkg/maintainer/spv/spv.go: keep candidate side (no 144-header cap, header cache, exact decoded-target DIFF1 comparison + zero-target guard). Removed the `const maxProofHeaders = 144` that the 3-way merge silently re-added from main (merge-base lacked it, so the candidate's iteration-1 removal was invisible to the merge). - pkg/maintainer/spv/deposit_sweep.go: combine candidate's bounds-checked OutputAt accessor with main's bitcoin.GetScriptType classification (threshold-network#4165). - pkg/maintainer/spv/spv_test.go: keep candidate's superset of cases (non-DIFF1 rounding, zero-target, 145- and 201-header long-run cases); drop main's removed "minimum difficulty run exceeds header bound" cap test. Accepted main's deletion of the extracted v1 random beacon and dead Ropsten infra. NOTE: this drops two candidate-only v1 hardenings that main moved out of this repo — require(entryCreatedSuccess) in KeepRandomBeaconOperator.sol and the registry-approval modifier in KeepRandomBeaconServiceImplV1.sol. If those are in security scope they must be ported to the keep-core-v1 repo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lrsaturnino
added a commit
to lionakhnazarov/keep-core
that referenced
this pull request
Jul 24, 2026
Resolve conflicts by final behavior, preserving the candidate's OOB hardening and the no-cap / exact-target SPV semantics while taking main's threshold-network#4039 work-accumulation model, threshold-network#4165 script-type isolation, threshold-network#4172 sweep fee floor, and the threshold-network#3998 v1-random-beacon extraction. SPV conflict resolution: - pkg/maintainer/spv/spv.go: keep candidate side (no 144-header cap, header cache, exact decoded-target DIFF1 comparison + zero-target guard). Removed the `const maxProofHeaders = 144` that the 3-way merge silently re-added from main (merge-base lacked it, so the candidate's iteration-1 removal was invisible to the merge). - pkg/maintainer/spv/deposit_sweep.go: combine candidate's bounds-checked OutputAt accessor with main's bitcoin.GetScriptType classification (threshold-network#4165). - pkg/maintainer/spv/spv_test.go: keep candidate's superset of cases (non-DIFF1 rounding, zero-target, 145- and 201-header long-run cases); drop main's removed "minimum difficulty run exceeds header bound" cap test. Accepted main's deletion of the extracted v1 random beacon and dead Ropsten infra. NOTE: this drops two candidate-only v1 hardenings that main moved out of this repo — require(entryCreatedSuccess) in KeepRandomBeaconOperator.sol and the registry-approval modifier in KeepRandomBeaconServiceImplV1.sol. If those are in security scope they must be ported to the keep-core-v1 repo.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(spv): compute required proof headers from actual header difficulties
Problem
The SPV maintainer computed the number of block headers for a proof assuming every header carries the relay epoch difficulty: with
txProofDifficultyFactor = 1it always assembled a single-header proof. On testnet4 (BIP94), sweep transactions are regularly mined in minimum-difficulty (DIFF1) blocks, so the proof contained only a DIFF1 header. The Bridge skips DIFF1 headers when binding to the relay difficulty, finds no decisive header, and reverts with:Because the error is deterministic, the maintainer crash-looped on the same transaction every cycle (
error while maintaining SPV: ... restarting maintainer), and the affected wallet's main UTXO could never be registered on the host chain — blocking all subsequent sweeps for that wallet.Fix
getProofInfonow mirrors the Bridge'sBitcoinTx.determineRequestedDifficulty/evaluateProofDifficultylogic instead of assuming a fixed per-header difficulty:requestedDifficulty × txProofDifficultyFactor,maxProofHeaders = 144and report "wait for more confirmations" when the chain tip is reached first.The old epoch-boundary block-count arithmetic is removed; epoch spans are handled implicitly by summing actual header difficulties.
Testing
TestGetProofInforewritten for the new semantics: current/previous-epoch proofs, epoch-spanning proofs with difficulty drop/raise, leading DIFF1 headers, relay at minimum difficulty, decisive-header mismatch, DIFF1 run exceeding the header bound, and chain tip reached early. Headers are generated with real compact-bits encoding (blockHeaderWithDifficulty).go test ./pkg/maintainer/spvpasses.876510fd...62c8, mined in a DIFF1 block at height 137542) was proven on the first cycle after deploying this fix —submitDepositSweepProoftx0xfeeefad32c7116727949483edfb1e38f8117d923521fd3467886a05226b82577, status 1 — and the maintainer completed its round cleanly.Notes
determineRequestedDifficulty(deployed via the DIFF1 library upgrade). This changes maintainer behavior only for transactions whose proof would begin with minimum-difficulty (DIFF1) headers: those are now skipped rather than the maintainer deterministically failing on them, and a leading DIFF1 run exceedingmaxProofHeadersis reported with a distinct log and metric. Transactions whose proof begins with a real-difficulty header are handled as before.