fix(spv): harden DIFF1 proof-header computation#4186
Conversation
Address review findings on the DIFF1-aware proof header walk: - Match the Bridge's skip predicate exactly by comparing the decoded header target to the minimum-difficulty target, instead of testing computed difficulty == 1 (a superset of the canonical DIFF1 target). - Distinguish the two skip causes: getProofInfo now returns a typed proofSkipReason so the caller emits a dedicated log and metric for 'outside relay range' (transient) versus 'exceeded max headers' (potentially permanent), instead of one generic warning. - Document the maxProofHeaders bound and its fixed-window limitation. - Compute the required total difficulty once, when the requested difficulty is bound, rather than on every header iteration. - Remove the unused difficultyEpochLength constant. - Fix swapped current/previous epoch difficulty args in the test setup and extend coverage: current-epoch binding on an asymmetric span, interior DIFF1 header accounting, the header-bound off-by-one, and chain tip reached before a decisive header.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSPV proof assembly now returns explicit skip reasons for relay-range mismatches and header-bound limits. Transaction proving handles these outcomes separately, while tests cover updated confirmation, difficulty, boundary, and chain-tip behavior. ChangesSPV proof skip classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant proveTransactions
participant getProofInfo
participant BitcoinHeaders
participant Metrics
proveTransactions->>getProofInfo: request proof confirmations
getProofInfo->>BitcoinHeaders: inspect header chain and targets
BitcoinHeaders-->>getProofInfo: header difficulty and position
getProofInfo-->>proveTransactions: confirmations and skipReason
proveTransactions->>Metrics: count classified proof skips
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/maintainer/spv/spv.go (1)
259-296: 🩺 Stability & Availability | 🔵 TrivialConsider log/compute cost for permanently-stuck transactions.
Since
proofStartBlockis anchored to the transaction's confirmation block and never shifts across cycles, a transaction that hitsproofSkipExceededMaxHeaderswill hit it again every future proving cycle (re-walking up to 144 headers and re-emitting anErrorfeach time), since the comment itself notes it "may be permanently unprovable." Over time this could produce persistent Error-level log/alert noise for a condition the maintainer can't resolve on its own.Worth considering a dedup/rate-limit (e.g., log once per transaction, or downgrade to periodic Warnf) for this specific case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/maintainer/spv/spv.go` around lines 259 - 296, The proofSkipExceededMaxHeaders handling should avoid repeated expensive scans and Errorf log noise for the same permanently stuck transaction. Update the proving-cycle state around proofStartBlock and the proofSkipExceededMaxHeaders branch so each transaction is deduplicated or rate-limited, while preserving the existing skip metric and behavior; use a periodic Warnf or equivalent for subsequent occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/maintainer/spv/spv.go`:
- Around line 259-296: The proofSkipExceededMaxHeaders handling should avoid
repeated expensive scans and Errorf log noise for the same permanently stuck
transaction. Update the proving-cycle state around proofStartBlock and the
proofSkipExceededMaxHeaders branch so each transaction is deduplicated or
rate-limited, while preserving the existing skip metric and behavior; use a
periodic Warnf or equivalent for subsequent occurrences.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe1dd57b-7373-41e6-859c-1db6ed143889
📒 Files selected for processing (2)
pkg/maintainer/spv/spv.gopkg/maintainer/spv/spv_test.go
Address review findings on the DIFF1-aware proof header walk: - Add a regression test pinning the skip predicate to exact target equality: a header with Difficulty()==1 but a target below the minimum-difficulty target must not be skipped. The test fails if the predicate regresses to computed-difficulty comparison. - Add coverage for proveTransactions' per-skip-reason handling, asserting that a skip never submits a proof, an assemblable proof is submitted, and the expected metric counter is incremented. - Guard the skip-reason switch with an explicit default so an unexpected reason surfaces as an error instead of silently falling through to proof submission. - Derive minDifficultyTarget from compact bits via CompactToBig, dropping the duplicated hex literal and discarded ok.
1 participant
fix(spv): harden DIFF1 proof-header computation
Follow-up to #4039, which introduced the DIFF1-aware SPV proof-header walk in
getProofInfo. A multi-agent review of that change surfaced edge-case, observability, and test-quality findings. This PR addresses them; the core algorithm from #4039 is unchanged.Changes
header.Target() == minDifficultyTarget), mirroring the Bridge'starget == MIN_DIFFICULTY_TARGET, instead of testing computeddifficulty == 1(which covers a superset of the canonical DIFF1 target and could otherwise let the maintainer assemble a proof the Bridge rejects).getProofInforeturns a typedproofSkipReason, so the caller emits a dedicated log and metric foroutside relay range(transient) versusexceeded max headers(potentially permanent, since the proof window is anchored at a fixed start block), instead of collapsing both into one generic warning. New counters:spv_proof_skipped_outside_relay_range_total,spv_proof_skipped_exceeded_max_headers_total.maxProofHeadersbound documented, including its fixed-window limitation.totalDifficultyRequiredcomputed once when the requested difficulty is bound, rather than on every header iteration.difficultyEpochLengthconstant.Testing
current/previousepoch-difficulty arguments in the test setup (they were stored inverted; the suite still passed, so it was not asserting the intended state).TestGetProofInfo: current-epoch binding on an asymmetric epoch span, an interior DIFF1 header being accounted for after the decisive header, themaxProofHeadersoff-by-one boundary (decisive header exactly at vs. just past the bound), and the chain tip reached before any decisive header is bound.go build ./...,go vet, staticcheck (-SA1019), andgofmt -lare clean;go test ./pkg/maintainer/spvpasses (30 tests).Summary by CodeRabbit
Bug Fixes
Tests