fix(llmq): retain signHash marker on recSig truncation and drop late sigShares #6998
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes make recovered-signature cleanup conditional and add a pre-lock early-rejection in sigshare processing. RemoveRecoveredSig now erases the k4 (sign-key) DB entry and invalidates the hasSigForSessionCache only when Sequence Diagram(s)sequenceDiagram
participant Peer
participant SigShareHandler
participant DB as Cache/DB
participant Lock as CriticalSection
Peer->>SigShareHandler: sends sigShare
rect rgba(120,180,240,0.12)
Note over SigShareHandler: Pre-lock checks
SigShareHandler->>SigShareHandler: compute signHash
SigShareHandler->>DB: query alreadyRecovered(sig id / session)
DB-->>SigShareHandler: alreadyRecovered? (yes/no)
alt alreadyRecovered == yes
SigShareHandler->>SigShareHandler: log and drop sigShare
SigShareHandler-->>Peer: return
end
end
rect rgba(160,220,160,0.12)
Note over Lock: Critical section
SigShareHandler->>Lock: acquire
alt alreadyRecovered inside lock
SigShareHandler->>SigShareHandler: return early
else
SigShareHandler->>DB: has sigShare key?
alt duplicate
SigShareHandler->>SigShareHandler: skip duplicate
else
SigShareHandler->>SigShareHandler: store/process sigShare
end
end
SigShareHandler->>Lock: release
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-07-29T14:32:48.369ZApplied to files:
📚 Learning: 2025-10-05T20:38:28.457ZApplied to files:
🪛 GitHub Actions: Clang Diff Format Checksrc/llmq/signing_shares.cpp[error] 527-532: Clang-format differences found. Run 'clang-format-diff.py -p1' or 'clang-format' to fix code style issues in this file. ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (5)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
…sigShares
- Problem: After we process a recovered signature, TruncateRecoveredSig removes the id/signHash indexes (rs_r/rs_s). A very late sigShare for
that session then sees HasRecoveredSigForId as false, recreates a session, and eventually times out, even though we already finished the
session.
- Behavior change: Keep the rs_s entry (signHash -> 1) when truncating recovered sigs so HasRecoveredSigForSession(signHash) stays true after
truncation. Cleanup still removes the remaining keys when appropriate.
- Handling late shares: CSigSharesManager::ProcessMessageSigShare now also checks HasRecoveredSigForSession and logs/drops sigShares for sessions
already completed, preventing “zombie” sessions/timeouts.
- Notes: This leaves the recSig hash path (rs_h) unchanged; full deletions (conflict/cleanup) still remove all indexes. No extra caching
insertions to keep behavior parallel to rs_h.
PastaPastaPasta
force-pushed
the
fix/late-sigShare
branch
from
d15ce14 to
8f6f075
Compare
2 participants
Issue being fixed or feature implemented
Before this change on testnet, I saw ~1/5 recSigs having a "late sigShare" appear resulting in wasted processing and book keeping. I don't believe this is a significant performance boost (maybe moderate boost), but it will make the logs a lot clearer about when there is a true session timeout.
It seems like what was happening was 1 masternode peer was very slow / behind, resulting in them sending us their sigShares minutes late.
How Has This Been Tested?
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.