fix: authenticate cached InstantSend proof bytes

[PastaPastaPasta]

Issue being fixed or feature implemented

When an ISDLOCK arrives for a transaction whose recovered signature this node already has, the verifier skipped checking the lock's own signature and cycle. The cached recovered signature proves that the quorum signed this txid. It does not prove that the bytes in the received lock are that signature. The unchecked lock was then stored, relayed by its hash and returned by getislocks. A lock with the same txid that arrives later is rejected once one is stored. So a peer that sends an on-curve but invalid signature first, or a different known cycle hash, can keep the authentic proof from being stored and relayed. This does not authorize any new spend or cause a consensus split. It needs the recovered signature to be cached while no lock is stored, for example on a -watchquorums node.

What was done?

BuildVerificationBatch now resolves the quorum from the received lock's cycle before it consults the cache. It skips verification only when the cached recovered signature came from that same quorum and has the same signature bytes. Any other lock goes through normal batch verification.

missing_quorum_does_not_drop_batch now uses a real mined non-rotating quorum, because a cache hit now needs a quorum that can be resolved. Rotating missing-quorum coverage stays in received_genesis_cycle_has_no_quorum.

This is a Dash-specific fix. Bitcoin Core has nothing to backport.

How Has This Been Tested?

macOS arm64, existing depends build:

• Full make -j1 passed.
• test/functional/p2p_instantsend.py passed. It uses non-rotating quorums.
• test/functional/rpc_verifyislock.py passed. It uses rotating quorums, so masternodes that already hold the recovered signature receive the network locks.
• lint-includes.py, lint-circular-dependencies.py and git diff --check passed.

Linux x86_64 debug build, after the tests were simplified:

• ./src/test/test_dash --run_test=evo_islock_tests passed all 14 cases, on 3 runs in a row.

New test cached_recovered_sig_does_not_authenticate_other_lock:

• It adds mined commitments for two non-rotating quorums and caches a genuine recovered signature for a mempool transaction. It then passes a changed lock straight to ProcessPendingInstantSendLocks: once signed by another key, and once with the other cycle hash. It checks that the changed lock is not stored, and that the authentic lock processed afterwards is stored byte for byte.
• With net_instantsend.cpp reverted to the base, both cases failed: the changed lock was stored and the authentic lock was not (4 failed checks, exit 201).

An independent review approved the change. Beyond rpc_verifyislock.py, rotating-quorum cache hits are covered by reasoning, not by a dedicated test. No end-to-end P2P timing demonstration was done.

Breaking Changes

None. The wire format, quorum selection and signature validity rules are unchanged.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 092fb823-be77-42cf-9d61-15a1c39f8a96

---

📥 Commits

Reviewing files that changed from the base of the PR and between 9a51d11 and ecae783.

---

📒 Files selected for processing (1)

• src/test/evo_islock_tests.cpp

---

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

---

---

Walkthrough

BuildVerificationBatch now skips verification only when a recovered signature matches both the lock’s quorum hash and signature. Tests cover unavailable quorums, retries, and cached signatures that do not match a lock.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Merge Risk: ⚪ Minimal · up to ecae7

The change makes InstantSend verify a lock's own signature bytes and cycle hash instead of trusting a cached recovered signature. The test simplification adds no production risk, and no merge-blocking issues were found.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly and concisely describes the main change: authenticating cached InstantSend proof bytes.
Description check		The description is directly related to the changeset. It explains the cached-signature issue, the verification fix, the regression tests, and validation results.

---

---

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

---

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

✅ Final review complete — no blockers (commit ecae783) · triage: low

[thepastaclaw]

Final validation — Phase 1 + Phase 2

Static verification of head 9a51d11 found no actionable in-scope issues and supports both reviewers' clean assessments. The cache shortcut requires a recovered signature matching the transaction, request ID, resolved quorum, and received signature; the regression test checks rejection of altered proofs followed by storage of the authentic lock. No builds or tests were run in this lane; the supplied CI snapshot shows formatting, merge, and title checks passing, with container builds queued and independent build/test confirmation pending.

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The production change in NetInstantSend::BuildVerificationBatch is small and contained, requiring matching quorum and signature bytes before skipping verification, with regression tests covering mismatches.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 88% left, weekly 94% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

[knst]

fix for net_instantsend.cpp looks legit.

can you do something with this regressions tests? They are un-readable and non-supportable. For single case scenarious hundreds line of fixture is definitely something wrong with testing that.

[PastaPastaPasta]

Thanks. Simplified the tests in ecae783 (force-pushed; the production change is identical):

• The regression test no longer starts the NetInstantSend worker thread or polls with a sleep loop. It calls ProcessPendingInstantSendLocks directly, the same way missing_quorum_does_not_drop_batch does.
• The custom CachedISLockSetup fixture and its extra chain args are gone. Both tests share NetInstantSendTest (TestChain100Setup) with small helpers: AddMinedQuorum, MakeLock, Sign, CacheRecoveredSig and Process.
• The regression test is now about 30 lines: cache the authentic recovered sig, process a forged lock (re-signed with another key, or pointing at another cycle), check that it isn't stored, then check that the authentic lock is stored byte for byte.

It still fails without the fix (both cases), and all 14 evo_islock_tests cases pass with it.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[knst]

LGTM ecae783

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The complete diff at the exact head correctly restricts the InstantSend cache shortcut to locks whose resolved quorum and signature match the cached recovered proof; mismatches undergo normal verification. The regression test checks rejection of both a forged signature and an altered cycle, followed by storage of the authentic lock, and no actionable in-scope defects were found. Validation was static only: the supplied CI snapshot shows successful completed builds and lint checks, with five test jobs and the multiprocess build still pending.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The production change is a small, contained cache-validation fix in BuildVerificationBatch that requires matching quorum and signature bytes before skipping verification, so it does not meet the large-or-intricate requirement for critical.
• Phase 1 reviewers: gemini-3.8-flash-high — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: gemini-3.8-flash-high — antigravity quota: weekly 20% left, 5h 76% left
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.