Check Spark spend ZK proofs on acceptance into the mempool and cache the result, so it's not re-checked later#1665
Conversation
WalkthroughAdds asynchronous, cached Spark spend-proof verification with a background thread pool and per-transaction check state, exposes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Node
participant Wallet as CSparkWallet
participant Spark as spark::state
participant Pool as gCheckProofThreadPool
rect rgb(240,248,255)
Node->>Wallet: Begin shutdown (FinishTasks)
Wallet->>Wallet: Shutdown wallet thread pool
Wallet->>Spark: spark::ShutdownSparkState()
Spark->>Pool: Request pool shutdown
Pool-->>Spark: Pool stops accepting tasks / drains
Note over Wallet,Spark: Ensures background proof tasks stop before exit
end
sequenceDiagram
autonumber
participant Validator as Tx Validation
participant Cache as gCheckedSparkSpendTransactions
participant Pool as gCheckProofThreadPool
participant Verifier as Proof Verifier
Validator->>Cache: Check tx state
alt previously failed
Cache-->>Validator: Reject immediately
else previously passed
Cache-->>Validator: Accept, skip verify
else in-progress
alt needs stateful sigma check
Validator->>Cache: Wait for completion
Cache-->>Validator: Return result
else non-stateful path
Cache-->>Validator: Soft-accept (true)
end
else not checked
alt non-stateful path
Validator->>Pool: Enqueue async verification
Pool->>Verifier: Verify proofs (async)
Verifier-->>Cache: Update pass/fail
else stateful required
Validator->>Verifier: Verify synchronously
Verifier-->>Cache: Update pass/fail
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 (3)
src/spark/sparkwallet.cpp (1)
3-3: Remove duplicate include of "state.h"The file "state.h" is included on line 3 and again on line 11. This duplicate include should be removed.
Apply this diff to remove the duplicate:
#include "../liblelantus/threadpool.h" #include "sparkwallet.h" -#include "state.h" #include "../wallet/wallet.h"src/spark/state.cpp (2)
12-27: Thread-safety concern with shared_ptr usage in ProofCheckStateThe
checkInProgressfield is accessed from multiple threads but the shared_ptr itself may not be thread-safe for concurrent read/write operations. While the underlyingboost::futureis thread-safe, updating the shared_ptr pointer itself requires synchronization.The current code does lock
cs_checkedSparkSpendTransactionswhen accessing the map, which should provide the necessary synchronization. However, consider documenting this requirement clearly.Consider adding a comment to clarify the synchronization requirements:
struct ProofCheckState { // if this is true, then the proof was already checked, no need to check again bool fChecked; // result of the check (if fChecked is true) bool fResult; - // if this is non-null, then the proof is being checked right now + // if this is non-null, then the proof is being checked right now + // Note: Access to this field must be protected by cs_checkedSparkSpendTransactions std::shared_ptr<boost::future<bool>> checkInProgress; };
932-934: Minimal shutdown implementationThe
ShutdownSparkState()function only shuts down the thread pool. Consider whether the cache (gCheckedSparkSpendTransactions) should also be cleared on shutdown to free memory.void ShutdownSparkState() { gCheckProofThreadPool.Shutdown(); + { + LOCK(cs_checkedSparkSpendTransactions); + gCheckedSparkSpendTransactions.clear(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/spark/sparkwallet.cpp(2 hunks)src/spark/state.cpp(7 hunks)src/spark/state.h(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/spark/sparkwallet.cpp (1)
src/spark/state.cpp (2)
ShutdownSparkState(932-934)ShutdownSparkState(932-932)
src/spark/state.h (1)
src/spark/state.cpp (2)
ShutdownSparkState(932-934)ShutdownSparkState(932-932)
src/spark/state.cpp (2)
src/libspark/spend_transaction.cpp (4)
verify(228-233)verify(228-230)verify(238-408)verify(238-241)src/batchproof_container.cpp (2)
verify(48-56)verify(48-48)
⏰ 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). (15)
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-mac-cmake
- GitHub Check: build-linux-cmake
- GitHub Check: Create Source Distribution
- GitHub Check: build-windows-cmake
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: Create Source Distribution
🔇 Additional comments (10)
src/spark/state.h (1)
86-87: LGTM!The addition of the
ShutdownSparkState()declaration is appropriate for graceful shutdown of the new thread pool used for asynchronous proof verification.src/spark/sparkwallet.cpp (1)
89-90: LGTM!The addition of
spark::ShutdownSparkState()after shutting down the wallet's thread pool ensures proper cleanup of the global proof verification thread pool.src/spark/state.cpp (8)
1-1: LGTM!The addition of
threadpool.hand<set>includes is appropriate for the new asynchronous proof verification functionality.Also applies to: 8-8
608-611: Ownership transfer to shared_ptr for async verificationThe change from
unique_ptrtoshared_ptrwithstd::make_sharedis necessary for the asynchronous verification model where the spend object needs to be accessed from the thread pool after the function returns.
675-682: Soft error handling for non-stateful checksThe change to return
trueinstead of failing whenfStatefulSigmaCheckis false allows mempool acceptance to proceed with deferred verification. This is consistent with the PR's goal of avoiding redundant checks.
742-743: Consistent soft error return for missing cover setsThe conditional return based on
fStatefulSigmaCheckis consistent with the soft error approach for mempool acceptance.
784-790: Cache update after verificationThe code correctly updates the cache with the verification result, but only when
!fChecked. This ensures the cache is populated only once per transaction.
793-795: Early return optimization for non-stateful checksThe early return when
!fStatefulSigmaCheckavoids unnecessary work for mempool acceptance, relying on the asynchronous verification to complete in the background.
689-693: Confirm mempool soft-error handling (fStatefulSigmaCheck == false)The code returns true when index->GetBlockHash() != idAndHash.second && !fStatefulSigmaCheck (src/spark/state.cpp, ~lines 689–693). I found only this occurrence — locate where fStatefulSigmaCheck is set and verify the mempool acceptance path performs the full stateful-sigma validation before accepting the tx; if it does not, add stricter validation here or reject in mempool.
555-586: No deadlock — unlock/get/lock sequence verified safeSearched the repo: gCheckedSparkSpendTransactions is declared at src/spark/state.cpp:24 and used at ~557–586 (the unlock/get/lock path) and ~763–789 (posting the future). No erase/clear/swap or other future->get() calls were found. The code copies checkState.checkInProgress into a local shared_ptr before unlocking, so waiting on future outside the lock is safe; the only remaining invalidation risk would be an explicit erase of the map entry — none exists.
| if (fChecked) { | ||
| // if we are here, then the proof was already checked and it passed | ||
| passVerify = true; | ||
| } | ||
| else { | ||
| if (fStatefulSigmaCheck) { | ||
| // we need the answer now, so verify and execute | ||
| passVerify = spark::SpendTransaction::verify(*spend, cover_sets); | ||
| } | ||
| else { | ||
| LOCK(cs_checkedSparkSpendTransactions); | ||
| // put the proof into the thread pool for verification | ||
| auto future = gCheckProofThreadPool.PostTask([spend, cover_sets]() { | ||
| try { | ||
| return spark::SpendTransaction::verify(*spend, cover_sets); | ||
| } catch (const std::exception &) { | ||
| return false; | ||
| } | ||
| }); | ||
| auto &checkState = gCheckedSparkSpendTransactions[hashTx]; | ||
| checkState.fChecked = false; | ||
| checkState.checkInProgress = std::make_shared<boost::future<bool>>(std::move(future)); | ||
| // return true for now, the result will be processed later | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify thread pool task completion before shutdown
The asynchronous proof verification posts tasks to gCheckProofThreadPool but doesn't guarantee they complete before shutdown. The ShutdownSparkState() function should ensure all pending tasks are completed or cancelled properly.
Consider ensuring that the thread pool's Shutdown() method waits for all pending tasks to complete or provides a mechanism to cancel them gracefully.
🏁 Script executed:
#!/bin/bash
# Check the implementation of ParallelOpThreadPool::Shutdown
rg "class ParallelOpThreadPool" -A 30 --type cpp
rg "Shutdown\(\)" src/liblelantus/threadpool.h -A 10Length of output: 3333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/liblelantus/threadpool.h ==="
if [ -f src/liblelantus/threadpool.h ]; then
sed -n '1,400p' src/liblelantus/threadpool.h || true
else
echo "MISSING: src/liblelantus/threadpool.h"
fi
echo
echo "=== src/spark/state.cpp (first 480 lines) ==="
if [ -f src/spark/state.cpp ]; then
sed -n '1,480p' src/spark/state.cpp || true
else
echo "MISSING: src/spark/state.cpp"
fi
echo
echo "=== repo search for symbols ==="
rg -n "gCheckProofThreadPool" || true
rg -n "gCheckedSparkSpendTransactions" || true
rg -n "ShutdownSparkState" || true
rg -n "ShutdownSpark" || trueLength of output: 22036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/spark/state.cpp:520-820 ==="
sed -n '520,820p' src/spark/state.cpp || true
echo
echo "=== src/spark/state.cpp:820-1000 ==="
sed -n '820,1000p' src/spark/state.cpp || true
echo
echo "=== usages ==="
rg -n "checkInProgress" src/spark/state.cpp || true
rg -n "gCheckedSparkSpendTransactions" src/spark/state.cpp || true
rg -n "ShutdownSparkState" -n || trueLength of output: 19338
Reject new postings to the thread pool after shutdown and ensure in-flight tasks are drained.
ParallelOpThreadPool::Shutdown() (src/liblelantus/threadpool.h) joins existing worker threads and will drain tasks already enqueued, but PostTask() does not check the pool's shutdown flag and can enqueue/start new tasks after Shutdown() is called. CheckSparkSpendTransaction posts tasks at src/spark/state.cpp:~765; ShutdownSparkState() only calls gCheckProofThreadPool.Shutdown() at src/spark/state.cpp:932.
Fix: make PostTask() refuse/reject new submissions when shutdown==true (return a ready failed/empty future or throw) or add an explicit "accepting_tasks" gate and flip it before calling ShutdownSparkState(); ensure no code posts to gCheckProofThreadPool after that gate is set.
🤖 Prompt for AI Agents
In src/spark/state.cpp around lines 753-778, CheckSparkSpendTransaction posts
verification tasks to gCheckProofThreadPool without preventing new submissions
after shutdown; update the codebase so PostTask refuses new submissions when the
pool is shutting down (e.g., check a shutdown flag and return an
immediately-ready failed/empty future or throw) or add an explicit
accepting_tasks gate that is flipped off before ShutdownSparkState() calls
gCheckProofThreadPool.Shutdown(); then update callers (including this block) to
handle a rejected PostTask (do not assume a task was enqueued — either fall back
to synchronous verification or return false/appropriate state) and ensure
ShutdownSparkState() flips the gate before calling Shutdown() so no new tasks
are posted and in-flight tasks are drained by the existing Shutdown logic.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/liblelantus/threadpool.h (2)
9-9: Missing header for std::find_if
ThreadProc()usesstd::find_ifbut<algorithm>is commented out. Restore it to avoid non‑portable builds.-//#include <algorithm> +#include <algorithm>
81-95: Reject tasks after shutdown in PostTaskPostTask must not restart threads or enqueue work when shutdown==true; make it return an exceptional future or throw instead.
boost::future<Result> PostTask(std::function<Result()> task) { boost::packaged_task<Result> packagedTask(std::move(task)); boost::future<Result> ret = packagedTask.get_future(); boost::mutex::scoped_lock lock(task_queue_mutex); + if (shutdown) { + // Prevent scheduling after shutdown + // Option A: throw (simple, clear) + throw std::runtime_error("ParallelOpThreadPool is shut down"); + // Option B (if exceptions undesired): return an exceptional/ready future instead. + } + // lazy start threads on first request or after shutdown if (threads.size() < number_of_threads) StartThreads(); task_queue.emplace(std::move(packagedTask)); task_queue_condition.notify_one(); return std::move(ret); }Audit callers that post tasks (many do not check IsPoolShutdown): src/spark/sparkwallet.cpp:490,503,638,648; src/liblelantus/lelantus_prover.cpp:155; src/batchproof_container.cpp:212,315; src/spark/state.cpp:769 (IsPoolShutdown checked at 761–766).
♻️ Duplicate comments (1)
src/spark/state.cpp (1)
764-766: Race condition: PostTask after shutdown check.There's a potential race condition between checking
IsPoolShutdown()and callingPostTask(). The thread pool could shut down between these two operations, potentially causing issues.Consider adding a mechanism in the thread pool to reject new task submissions when shutdown is in progress, or ensure that
PostTask()handles shutdown state gracefully.
🧹 Nitpick comments (3)
src/liblelantus/threadpool.h (3)
118-121: Make IsPoolShutdown const/noexcept and avoid lock-order pitfallsAccessor should be const; locking inside a non‑const method is surprising. Also, calling it while the pool already holds the mutex would deadlock. Either make
shutdownatomic or keep the lock but mark the mutex mutable. Minimal change below.- boost::mutex task_queue_mutex; + mutable boost::mutex task_queue_mutex; @@ - bool IsPoolShutdown() { - boost::mutex::scoped_lock lock(task_queue_mutex); - return shutdown; - } + bool IsPoolShutdown() const noexcept { + boost::mutex::scoped_lock lock(task_queue_mutex); + return shutdown; + }Also applies to: 33-33
97-99: Return size_t (and noexcept) from GetNumberOfThreadsMatch the member type and signal no-throw.
- int GetNumberOfThreads() const { - return number_of_threads; - } + std::size_t GetNumberOfThreads() const noexcept { return number_of_threads; }
94-95: Avoid redundant std::move on local returnLet NRVO do its job; keeps intent clear.
- return std::move(ret); + return ret;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/liblelantus/threadpool.h(1 hunks)src/spark/state.cpp(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/spark/state.cpp (2)
src/libspark/spend_transaction.cpp (4)
verify(228-233)verify(228-230)verify(238-408)verify(238-241)src/batchproof_container.cpp (2)
verify(48-56)verify(48-48)
⏰ 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). (18)
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-mac-cmake
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-windows-cmake
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-x86_64-linux-gnu
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: Create Source Distribution
- GitHub Check: build-linux-cmake
- GitHub Check: build-guix-x86_64-apple-darwin
- GitHub Check: build-guix-aarch64-linux-gnu
- GitHub Check: build-guix-x86_64-w64-mingw32
- GitHub Check: build-guix-arm64-apple-darwin
- GitHub Check: build-windows-cmake
- GitHub Check: build-mac-cmake
- GitHub Check: Create Source Distribution
- GitHub Check: build-linux-cmake
- GitHub Check: build-guix-x86_64-linux-gnu
🔇 Additional comments (12)
src/spark/state.cpp (12)
1-1: Include order for thread pool.The thread pool header is included first, which is appropriate given its foundational role in the new asynchronous verification system.
8-8: Necessary include for transaction state tracking.The
<set>header is appropriately added to support the new caching and verification state management structures.
12-21: Well-designed proof check state structure.The
ProofCheckStatestructure provides a clean interface for tracking verification state with appropriate fields for completion status, results, and in-progress checks.
23-27: Thread-safe caching infrastructure.The global map and critical section properly protect the cached verification states. The use of transaction hash as the key provides appropriate granularity for caching.
555-586: Proper cache lookup and wait handling.The cache lookup logic correctly handles both completed and in-progress verifications. The synchronization when waiting for stateful checks is appropriate - unlocking before the blocking wait and re-locking after.
608-608: Ownership change for thread pool compatibility.The change from
unique_ptrtoshared_ptris necessary for thread pool task lifetime management, allowing the lambda capture to maintain ownership during asynchronous execution.
675-682: Soft error handling for non-stateful checks.The differentiation between stateful and non-stateful checks is well-implemented. For non-stateful checks (mempool acceptance), missing coin groups result in soft errors that return
true, deferring verification to later stages.Also applies to: 689-693
742-744: Consistent soft error pattern.The cover set validation follows the same soft error pattern, appropriately handling cases where verification cannot proceed during non-stateful checks.
753-782: Well-structured asynchronous verification logic.The verification flow properly handles three cases:
- Already verified (cache hit)
- Stateful check (synchronous execution)
- Non-stateful check (asynchronous thread pool execution)
The task submission to the thread pool includes proper exception handling within the lambda.
787-794: Proper result caching.The caching of verification results after completion ensures that repeated checks for the same transaction will be fast. The cleanup of the
checkInProgresspointer is appropriate.
797-800: Early return for non-stateful checks.The early return for non-stateful checks is correct - these checks only need to validate that verification can be deferred, not that it passes immediately.
936-938: Clean shutdown implementation.The
ShutdownSparkState()function provides a simple interface to shut down the thread pool. The underlying thread pool'sShutdown()method handles joining threads appropriately.
3 participants
PR intention
When a lot of Spark spend transaction are mined the miner has a problem with performance. ZK proofs for a Spark spend are checked once on acceptance into the mempool, and then every time a block is updated and
getblocktemplateis called every transaction is checked once again making the complexity of multiple checks O(N^2) where N is the number of Spark spend transaction in the block. This PR caches the result of the first check and makes the check multithreaded.Code changes brief
Use thread pool to defer the checks and cache the result.