Improve performance on Spark ZK proof verification #1718
Conversation
WalkthroughRefactors Spark spend proof verification in Changes
Sequence DiagramsequenceDiagram
participant Caller
participant State as state.cpp
participant Pool as Proof ThreadPool
participant Cache as gCheckedSparkSpendTransactions
rect rgb(200, 220, 250)
Note over State: New Asynchronous Flow
Caller->>State: Verify Spark Spend (non-stateful)
State->>State: Enter do/while (fRecheckNeeded)
alt Future exists in cache
State->>Cache: Check prior in-progress result
Cache-->>State: Future completed?
alt Result ready
State->>State: Mark fChecked=true, extract result
else Result pending
State->>State: fRecheckNeeded=true (retry)
end
else No cached future
State->>Pool: Enqueue verification task
Pool-->>Cache: Store future (fChecked=false)
State->>State: fRecheckNeeded=true (poll next iteration)
end
State->>State: Loop until !fRecheckNeeded
alt fChecked && result valid
State-->>Caller: Return verified
else Stateful required
State->>State: Direct synchronous verification
State-->>Caller: Return result
else Defer verification
State-->>Caller: Return true (async pending)
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/spark/state.cpp (1)
794-800: No critical issues found. The recheck logic is correctly bounded to a maximum of 2 loop iterations.The embedded analysis in the review comment was accurate:
fRecheckNeededis reset tofalseat the start of each do-block iteration (line 769), preventing infinite loops. When an async proof check fails and the entry is erased, the second iteration immediately falls through without re-entering the async path because the entry no longer exists. IffStatefulSigmaCheckis true, the subsequent synchronous verification at line 831 will properly validate or reject the proof.The assumption that failures may stem from an incomplete anonymity set is reasonable and documented in the code comments. However, the suggestion to add telemetry/logging for recheck frequency and success rates is valid—currently there is no logging for this specific path, only for the "already checked" scenario (line 778). This would improve production observability.
The retries are not "unnecessary"—they serve a legitimate purpose of re-validating once the anonymity set may have grown. The concern about invalid proofs passing through is addressed by the bounded retry and subsequent synchronous verification.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/spark/state.cpp(2 hunks)
🔇 Additional comments (1)
src/spark/state.cpp (1)
822-839: LGTM with a note on exception handling.The post-loop logic correctly handles the different verification paths:
- Cached verified proof → use cached result
- Stateful check needed → verify synchronously
- Non-stateful check → return true and defer verification
The exception handler at lines 837-839 sets
passVerify = false, which is appropriate. However, if an exception occurs while the lock is held, the cache state ingCheckedSparkSpendTransactionsmay be left inconsistent (e.g., an entry withfChecked=falsebut nocheckInProgressfuture). Subsequent checks of the same transaction might encounter unexpected states. Consider whether cleanup logic is needed in the exception path.
| // wait for the check to complete | ||
| auto future = checkState.checkInProgress; | ||
| cs_checkedSparkSpendTransactions.unlock(); | ||
| bool result = future->get(); | ||
| cs_checkedSparkSpendTransactions.lock(); | ||
|
|
||
| checkState.fChecked = true; | ||
| checkState.fResult = result; | ||
| checkState.checkInProgress = nullptr; |
There was a problem hiding this comment.
Critical: Race condition with dangling reference after unlock/relock.
Lines 786-788 unlock the critical section, wait for the future, then relock. However, lines 790-792 access the checkState reference that was obtained before unlocking. Between the unlock and relock, another thread could erase or modify the gCheckedSparkSpendTransactions[hashTx] entry, causing checkState to become a dangling reference. Accessing it afterward leads to undefined behavior.
Apply this fix to safely handle the state after relocking:
else if (checkState.checkInProgress && fStatefulSigmaCheck) {
// wait for the check to complete
auto future = checkState.checkInProgress;
cs_checkedSparkSpendTransactions.unlock();
bool result = future->get();
cs_checkedSparkSpendTransactions.lock();
- checkState.fChecked = true;
- checkState.fResult = result;
- checkState.checkInProgress = nullptr;
+ // Re-lookup entry as it may have changed while unlocked
+ if (gCheckedSparkSpendTransactions.count(hashTx)) {
+ auto& updatedCheckState = gCheckedSparkSpendTransactions[hashTx];
+ updatedCheckState.fChecked = true;
+ updatedCheckState.fResult = result;
+ updatedCheckState.checkInProgress = nullptr;
- if (!result) {
- // unfortunately, it's possible that the proof was checked and failed
- // because the anonymity set was incomplete at the time of checking. We need
- // to recheck the proof again
- fRecheckNeeded = true;
- gCheckedSparkSpendTransactions.erase(hashTx);
- }
- else
- fChecked = true;
+ if (!result) {
+ // unfortunately, it's possible that the proof was checked and failed
+ // because the anonymity set was incomplete at the time of checking. We need
+ // to recheck the proof again
+ fRecheckNeeded = true;
+ gCheckedSparkSpendTransactions.erase(hashTx);
+ }
+ else
+ fChecked = true;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
PR intention
Removes duplicate checks for ZK proofs, when spark spend verification fails gives it a second chance, because it might have failed due to incomplete anonymity set at the time of the check