Wallet: Lock Spark address state - #1879
Conversation
Walkthrough
ChangesSpark wallet thread safety
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
8cd6fa6 to
87348ae
Compare
🤖 CodeAnt AI — Review Status
|
codeant-ai
Bot
added
the
size:S
User descriptionPR intentionFixes #1878.
Code changes brief
Testing
CodeAnt-AI DescriptionPrevent wallet crashes while Spark addresses are accessed concurrently What Changed
Impact
💡 Usage GuideChecking Your Pull RequestEvery time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later. Talking to CodeAnt AIGot a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask: This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code. ExamplePreserve Org Learnings with CodeAntYou can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input: This helps CodeAnt AI learn and adapt to your team's coding style and standards. ExampleRetrigger reviewAsk CodeAnt AI to review the PR again, by typing: Check Your Repository HealthTo analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| public: | ||
| // to protect coinMeta | ||
| // Protects lastDiversifier, addresses, and coinMeta. |
There was a problem hiding this comment.
Suggestion: The synchronization comment claims that cs_spark_wallet protects coinMeta, but coinMeta is not annotated with GUARDED_BY(cs_spark_wallet). Clang thread-safety analysis therefore cannot enforce the stated protection for this member, allowing future unlocked accesses to go undetected. Annotate coinMeta as guarded or remove it from the comment. [comment mismatch]
Severity Level: Major ⚠️
- ❌ Unlocked coin metadata access can race wallet updates.
- ⚠️ Clang analysis misses violations on transaction handling paths.
- ⚠️ Future coinMeta accesses may bypass synchronization undetected.Steps of Reproduction ✅
1. Build with Clang thread-safety analysis enabled; `GUARDED_BY` expands to Clang's
`guarded_by` attribute in `src/threadsafety.h:9-19`.
2. Follow the production wallet transaction path from `CWallet::HandleSparkTransaction()`
at `src/wallet/wallet.cpp:5580-5598`, which calls `CSparkWallet::UpdateSpendState()` at
line 5592.
3. `CSparkWallet::UpdateSpendState()` accesses `coinMeta` at
`src/spark/sparkwallet.cpp:497-499` without taking `cs_spark_wallet`.
4. Because `coinMeta` at `src/spark/sparkwallet.h:177` lacks
`GUARDED_BY(cs_spark_wallet)`, Clang cannot diagnose this unlocked access even though the
class comment at line 160 claims the map is protected. Annotating the member would expose
this current access and protect against future unlocked accesses.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/spark/sparkwallet.h
**Line:** 160:160
**Comment:**
*Comment Mismatch: The synchronization comment claims that `cs_spark_wallet` protects `coinMeta`, but `coinMeta` is not annotated with `GUARDED_BY(cs_spark_wallet)`. Clang thread-safety analysis therefore cannot enforce the stated protection for this member, allowing future unlocked accesses to go undetected. Annotate `coinMeta` as guarded or remove it from the comment.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/spark/sparkwallet.h (1)
160-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the
coinMetaguarding annotation and access contract.
coinMetais protected bycs_spark_walletin the comment, but the declaration lacksGUARDED_BY(cs_spark_wallet). Add the annotation and wrapUpdateSpendState(..., uint256, bool)(which reads/writescoinMeta) withLOCK(cs_spark_wallet)so the annotation reflects the actual contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spark/sparkwallet.h` around lines 160 - 174, Annotate the coinMeta declaration with GUARDED_BY(cs_spark_wallet), then update UpdateSpendState(..., uint256, bool) to acquire LOCK(cs_spark_wallet) before accessing coinMeta, preserving its existing read/write behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/spark/sparkwallet.h`:
- Around line 160-174: Annotate the coinMeta declaration with
GUARDED_BY(cs_spark_wallet), then update UpdateSpendState(..., uint256, bool) to
acquire LOCK(cs_spark_wallet) before accessing coinMeta, preserving its existing
read/write behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a987c0d-114b-4e71-b179-218f776bb92c
📒 Files selected for processing (2)
src/spark/sparkwallet.cppsrc/spark/sparkwallet.h
Bring in the six commits master gained since the previous merge: Mac nap inhibitor (#1867), Spark address state locking (#1879), prevector destruction backport (#1882), GUI freeze fixes (#1883), Spark wallet scan/lookup speedups (#1885, #1886). No conflicts, and no semantic interaction with this branch's changes: isAddressMine() now takes cs_spark_wallet internally (#1879), which both SignMessage() call paths acquire after cs_wallet -- the same order the existing RPC path already established. Refreshing the merge base also makes the PR diff show this branch's own 14 files again instead of master's already-merged work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0131ZL8NntcsaPuYepdwsx2C
2 participants
PR intention
Fixes #1878.
CSparkWalletaccessed its Spark address map andlastDiversifierfrom concurrent RPC and GUI paths withoutcs_spark_wallet. An address insertion can rehash thestd::unordered_mapwhile another thread copies or iterates it, causing undefined behavior and potentially crashing the wallet process.Review also found one existing unlocked
coinMetaaccess inUpdateSpendState(), reachable from wallet transaction handling while Spark worker threads use the same map.Code changes brief
cs_spark_walletin every address-state accessor, the two diversifier database helpers, and the sharedUpdateSpendState()implementation.lastDiversifier,addresses, andcoinMetawithGUARDED_BY(cs_spark_wallet)so Clang thread-safety analysis can detect future unlocked access.Testing
git diff --checkcs_main, thencs_wallet, thencs_spark_wallet.masterand the merged result retains every lock and annotation.