refactor: misc instantsend refactors

[PastaPastaPasta]

Issue being fixed or feature implemented

Improve and modernize some instantsend code; see commits

What was done?

see commits

How Has This Been Tested?

Breaking Changes

NA

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

This pull request refactors the InstantSend system to introduce batch verification for ISLocks and enforce deterministic cycle height validation. The primary changes include: (1) a new PendingISLockEntry struct that extends PendingISLockFromPeer with an islock_hash member, replacing the previous pair-based storage; (2) five new validation and verification methods (ValidateIncomingISLock, ResolveCycleHeight, ValidateDeterministicCycleHeight, BuildVerificationBatch, ApplyVerificationResults) to decompose the monolithic ProcessMessage flow into modular stages; (3) a new BatchVerificationData struct to encapsulate batch verification state and results; and (4) refactored pending ISLock processing workflows to leverage batch verification with staged result application.

Sequence Diagram(s)

sequenceDiagram
    participant Node
    participant ProcessMessage as ProcessMessage<br/>(Handler)
    participant Validator as ValidateIncoming<br/>ISLock
    participant CycleResolver as ResolveCycle<br/>Height
    participant DeterministicCheck as ValidateDeterministic<br/>CycleHeight
    participant PendingQueue as Pending Queue

    Node->>ProcessMessage: Receive ISLock Message
    ProcessMessage->>Validator: ValidateIncomingISLock(islock, node_id)
    alt Validation Fails
        Validator-->>ProcessMessage: false (invalid)
        ProcessMessage->>Node: Apply misbehavior score, return
    else Validation Passes
        Validator-->>ProcessMessage: true
    end
    
    ProcessMessage->>CycleResolver: ResolveCycleHeight(cycle_hash)
    alt Height Not Found
        CycleResolver-->>ProcessMessage: std::nullopt
        ProcessMessage->>Node: Apply misbehavior score, return
    else Height Resolved
        CycleResolver-->>ProcessMessage: optional<int> height
    end
    
    ProcessMessage->>DeterministicCheck: ValidateDeterministicCycleHeight(height, llmq_params, node_id)
    alt Deterministic Check Fails
        DeterministicCheck-->>ProcessMessage: false
        ProcessMessage->>Node: Apply misbehavior score, return
    else Check Passes
        DeterministicCheck-->>ProcessMessage: true
    end
    
    ProcessMessage->>PendingQueue: Enqueue ISLock if new
    PendingQueue-->>ProcessMessage: Acknowledged

Loading

sequenceDiagram
    participant WorkThread as WorkThread<br/>Main
    participant BatchBuilder as BuildVerification<br/>Batch
    participant BatchVerifier as CBLSBatch<br/>Verifier
    participant ResultApplier as ApplyVerification<br/>Results
    participant RecoveredSigs as Recovered Sigs<br/>(Cache)
    participant Network as Network<br/>Propagation

    WorkThread->>WorkThread: Dequeue pending ISLocks (vector<PendingISLockEntry>)
    WorkThread->>BatchBuilder: BuildVerificationBatch(llmq_params, signOffset, pend)
    
    BatchBuilder->>BatchBuilder: Extract recovered sigs from pend entries
    BatchBuilder->>BatchVerifier: Add recovered sigs to batch
    BatchBuilder->>BatchBuilder: Increment counters
    BatchBuilder-->>WorkThread: unique_ptr<BatchVerificationData>
    
    WorkThread->>BatchVerifier: Verify batch (async/internal)
    BatchVerifier-->>WorkThread: Verification complete
    
    WorkThread->>ResultApplier: ApplyVerificationResults(llmq_params, ban, data, pend)
    
    ResultApplier->>ResultApplier: Iterate verification results
    alt Signature Verification Fails
        ResultApplier->>ResultApplier: Track misbehaving peer (ban=true)
    else Verification Succeeds
        ResultApplier->>RecoveredSigs: Cache reconstructed recovered sig
    end
    
    ResultApplier-->>WorkThread: Uint256HashSet (verified locks)
    
    WorkThread->>Network: Propagate verified recovered sigs
    Network-->>WorkThread: Propagation complete

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~70 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title "refactor: misc instantsend refactors" is vague and uses non-descriptive language ("misc") that does not convey meaningful information about the specific changes made.	Replace the vague "misc" with specific details about the main refactoring goal, such as "refactor: improve instantsend verification and validation logic" or mention the primary architectural change.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is related to the changeset, referencing instantsend code improvements and modernization, though it defers detail to individual commits.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into develop

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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 and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/instantsend/net_instantsend.h (1)

1-76: ⚠️ Potential issue | 🟡 Minor

CI failure: clang-format differences detected.

The pipeline reports formatting issues in this file. Please run clang-format-diff and apply the resulting patch to match the project style.

src/instantsend/net_instantsend.cpp (1)

1-332: ⚠️ Potential issue | 🟡 Minor

CI failure: clang-format differences detected.

The pipeline reports formatting issues in this file as well. Please run clang-format-diff and apply the resulting patch.

🧹 Nitpick comments (3)

src/instantsend/net_instantsend.cpp (3)

45-59: Consider whether ResolveCycleHeight should handle the misbehavior penalty itself.

Currently, ResolveCycleHeight returns std::nullopt on failure, and the caller (ProcessMessage, line 225) applies the misbehavior penalty. This is fine for the single call site today, but if ResolveCycleHeight is reused elsewhere, the penalty logic would need to be duplicated. A minor design consideration — not blocking.

---

61-73: Verify the comment accuracy for non-rotation quorums.

The comment on line 66 says "Deterministic islocks MUST use rotation based llmq," yet the validation only checks cycle_height % dkgInterval == 0 without verifying llmq_params.useRotation. This works because the method is only called for llmqTypeDIP0024InstantSend (which always uses rotation), but if this helper were ever reused for a non-rotation LLMQ type, the cycle-height divisibility check would be incorrect.

Consider adding an assertion for clarity:

Proposed assertion

 bool NetInstantSend::ValidateDeterministicCycleHeight(
     int cycle_height,
     const Consensus::LLMQParams& llmq_params,
     NodeId node_id)
 {
     // Deterministic islocks MUST use rotation based llmq
+    assert(llmq_params.useRotation);
     if (cycle_height % llmq_params.dkgInterval == 0) {
         return true;
     }

---

75-141: Batch returns nullptr when quorum selection fails — all pending locks in the batch are silently dropped.

When SelectQuorumForSigning / GetQuorum returns nullptr (line 123), BuildVerificationBatch returns nullptr and ProcessPendingInstantSendLocks (line 271) returns an empty Uint256HashSet. This means all locks in the batch — including those already pushed to the batch verifier and those not yet iterated — are neither processed nor retried.

The comment on line 124 says "if one fails to select, all others will also fail to select," which justifies the early return. However, locks already marked as alreadyVerified (line 101) or added to the batch verifier (line 128) before the failure point are also lost. The alreadyVerified locks in particular had valid recovered sigs and could have been processed.

This appears to be the pre-existing behavior, so it's not a regression, but worth noting.

[UdjinM6]

utACK 418dcda

[knst]

LGTM 418dcda