fix: optimize large wallets #284
Conversation
WalkthroughAdds persistence for per-outpoint CoinJoin rounds and a coinJoinSalt in CoinJoinExtension (protobuf + I/O); refactors WalletEx rounds computation and caching with stricter locking and deterministic randomness using the extension salt; adds a persistence unit test and adjusts test utilities and build/publishing configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as App
participant Wallet as WalletEx
participant CJ as CoinJoinExtension
participant TX as TxGraph
Caller->>Wallet: getRealOutpointCoinJoinRounds(outPoint)
activate Wallet
Wallet->>Wallet: acquire lock
Wallet->>Wallet: check mapOutpointRoundsCache
alt cached
Wallet-->>Caller: cached rounds
else not cached
Wallet->>TX: locate tx by outPoint
alt invalid/missing
Wallet-->>Caller: error code (-1/-2/-3/-4)
else valid
loop traverse wallet-owned input graph
Wallet->>TX: follow input -> parent tx
Wallet-->>Wallet: compute candidate rounds
end
Wallet->>Wallet: bound rounds, cache result
Wallet-->>Caller: rounds
end
end
deactivate Wallet
sequenceDiagram
participant Wallet as WalletEx
participant CJ as CoinJoinExtension
participant PB as Protobuf
Wallet->>CJ: getCoinJoinSalt()
alt coinJoinSalt == ZERO_HASH
CJ->>CJ: calculateCoinJoinSalt() (SecureRandom -> Sha256Hash)
end
CJ-->>Wallet: coinJoinSalt
Wallet->>PB: serialize CoinJoin (outpoint_rounds_cache, coinjoin_salt)
PB-->>Wallet: deserialize CoinJoin
Wallet->>CJ: restore outpoint rounds cache and/or coinJoinSalt (or CJ generates new)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
147-199: Generalize and simplify numeric placeholder formatting in formatMessageThe advanced placeholder handling only supports 1 or 2 decimals. A minimal change can support any N with a single String.format call and reduce branching. Also avoids surprising defaults for values other than 1 or 2.
Apply this diff to simplify decimals handling:
- if (arg instanceof Number && formatSpec.endsWith("f")) { + if (arg instanceof Number && formatSpec.endsWith("f")) { // Handle decimal formatting like .2f, .1f - try { - int decimals = Integer.parseInt(formatSpec.substring(1, formatSpec.length() - 1)); - double value = ((Number) arg).doubleValue(); - if (decimals == 1) { - replacement = String.format("%.1f", value); - } else if (decimals == 2) { - replacement = String.format("%.2f", value); - } else { - replacement = String.format("%.2f", value); // default - } - } catch (NumberFormatException e) { - replacement = arg.toString(); - } + try { + int decimals = Integer.parseInt(formatSpec.substring(1, formatSpec.length() - 1)); + double value = ((Number) arg).doubleValue(); + replacement = String.format("%." + decimals + "f", value); + } catch (NumberFormatException e) { + replacement = arg == null ? "null" : arg.toString(); + }core/src/main/proto/wallet.proto (1)
567-580: Consider documenting size constraints for coinjoin_salt and output_index semantics
- coinjoin_salt is expected to be 32 bytes (Sha256Hash). A brief comment helps future maintainers and avoids accidental misuse.
- output_index is uint64; in practice indices won’t exceed 32-bit, but the wider type is fine. A note could clarify it mirrors TransactionOutPoint index.
Would you like me to add the doc comments?
core/src/test/java/org/bitcoinj/wallet/CoinJoinExtensionTest.java (1)
19-19: Use a neutral name for params fieldUNITTEST is a bit misleading for TestNet3Params. Consider renaming to PARAMS or TESTNET for clarity.
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (3)
204-211: Be robust to malformed salt length during deserializationSha256Hash.wrap() will throw if the byte array isn’t 32 bytes. Consider catching IllegalArgumentException and falling back to a freshly generated salt to avoid breaking wallet load on a corrupted extension.
- if (coinJoinProto.hasCoinjoinSalt()) { - coinJoinSalt = Sha256Hash.wrap(coinJoinProto.getCoinjoinSalt().toByteArray()); - } else { - // if there is no coinJoinSalt, then add it. - calculateCoinJoinSalt(); - } + if (coinJoinProto.hasCoinjoinSalt()) { + try { + coinJoinSalt = Sha256Hash.wrap(coinJoinProto.getCoinjoinSalt().toByteArray()); + } catch (IllegalArgumentException e) { + // Malformed length; generate a fresh salt to keep the wallet usable. + calculateCoinJoinSalt(); + } + } else { + // If there is no coinJoinSalt, then generate one lazily. + calculateCoinJoinSalt(); + }
270-283: Thread-safety of lazy salt initializationgetCoinJoinSalt() lazily initializes coinJoinSalt without synchronization. Two concurrent calls could both generate and race to assign different values. This is low risk but easy to harden by marking coinJoinSalt volatile or synchronizing the initialization block.
I can provide a double-checked locking variant if desired.
698-730: Efficient logging: avoid String.format in hot pathslog.info(COINJOIN_EXTRA, "... {}", ...) defers formatting until enabled, but a few lines use String.format(...) eagerly (e.g., the lines above use String.format in other methods too). Consider switching to parameterized logging to avoid building strings unnecessarily in hot loops on large wallets.
I can prepare a focused diff replacing String.format usage within COINJOIN logs.
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (1)
351-358: Cycle guard sentinel (-10) is sufficient but could be explicitThe -10 sentinel indicates “computing.” While cycles are not expected in the UTXO graph, a small comment explaining the purpose of the sentinel improves readability.
Would you like me to add a brief comment at the initialization site?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java(7 hunks)core/src/main/java/org/bitcoinj/wallet/WalletEx.java(3 hunks)core/src/main/proto/wallet.proto(2 hunks)core/src/test/java/org/bitcoinj/wallet/CoinJoinExtensionTest.java(2 hunks)core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
core/src/test/java/org/bitcoinj/wallet/CoinJoinExtensionTest.java (4)
core/src/main/java/org/bitcoinj/core/Sha256Hash.java (1)
Sha256Hash(38-315)core/src/main/java/org/bitcoinj/core/TransactionOutPoint.java (1)
TransactionOutPoint(34-228)core/src/main/java/org/bitcoinj/params/TestNet3Params.java (1)
TestNet3Params(31-299)core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java (1)
WalletProtobufSerializer(86-1108)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (2)
core/src/main/java/org/bitcoinj/core/Sha256Hash.java (1)
Sha256Hash(38-315)core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientOptions.java (1)
CoinJoinClientOptions(35-120)
⏰ 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). (3)
- GitHub Check: Analyze (java)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: JAVA 11 OS macOS-latest Gradle
🔇 Additional comments (5)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
147-199: Edge cases: unmatched or extra placeholders
- Unmatched "{:.Nf" without a closing "}" will leave the loop early but the message will still include the unprocessed fragment. This is fine for tests but worth noting.
- If there are more "{}" placeholders than remaining args, they'll be left in the result; if there are extra args, they will be ignored. This behavior matches many logging frameworks.
Please confirm this behavior is intentional for test logs. If you want stricter checks (e.g., assert on mismatched counts), I can draft a guard.
core/src/main/proto/wallet.proto (1)
560-565: Protobuf additions for CoinJoin look consistent and non-breakingThe new optional fields and messages (OutpointRoundsCache/Entry, coinjoin_salt) use fresh tags and won’t break existing parsers. Using bytes for transaction_hash and uint64 for output_index is appropriate.
core/src/test/java/org/bitcoinj/wallet/CoinJoinExtensionTest.java (1)
60-67: No changes needed: default serializer instantiates WalletEx and deserializes its extensionsThe no-arg WalletProtobufSerializer constructor uses a factory that calls new WalletEx(...), and readWallet(...) invokes loadExtensions(wallet, wallet.getExtensions(), …), so any extension registered in WalletEx (including CoinJoinExtension) is automatically serialized and then deserialized via wallet.deserializeExtension. Thus your roundTrip(...) already returns a WalletEx with its CoinJoinExtension populated—no extra wiring or explicit extensions array is required.
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (2)
300-309: Add a lock-guarded wrapper for getRealOutpointCoinJoinRounds — goodThe public wrapper ensures the wallet lock is held and relies on the internal recursion path. This is correct and avoids deadlocks.
437-461: isFullyMixed randomness sources the extension salt correctlyUsing the persisted coinJoinSalt ensures deterministic decisions across sessions, given the salt is properly initialized and serialized. With the earlier fix in CoinJoinExtension to always persist a non-zero salt, this looks good.
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 (1)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (1)
286-293: Fix NPE and incorrect summation in getUnmixableTotalThe current implementation of
getUnmixableTotalin CoinJoinExtension.java
• Always callsgetOutputs().get(-1), butgetOutputs()never populates key –1 →get(-1)is null, causing an NPE.
• Usessum.add(...)without assignment; since Coin is immutable,sumremainsCoin.ZERO.Locations to update:
- File: core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java
- Method: getUnmixableTotal (lines 286–293)
Proposed diff:
- public Coin getUnmixableTotal() { - Coin sum = Coin.ZERO; - getOutputs().get(-1).forEach(outPoint -> { - if (((WalletEx) wallet).getRealOutpointCoinJoinRounds(outPoint.getOutPointFor()) == -2) - sum.add(outPoint.getValue()); - }); - return sum; - } + public Coin getUnmixableTotal() { + Coin sum = Coin.ZERO; + // Iterate all unspent outputs and select those with no mix rounds (–2) + for (TransactionOutput output : wallet.getUnspents()) { + TransactionOutPoint outPoint = + new TransactionOutPoint(output.getParams(), output.getIndex(), output.getParentTransactionHash()); + if (((WalletEx) wallet).getRealOutpointCoinJoinRounds(outPoint) == -2) { + sum = sum.add(output.getValue()); + } + } + return sum; + }Optional: if “unmixable” should refer to collateral outputs, filter on return value –3 instead of –2.
♻️ Duplicate comments (3)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (2)
55-55: Secure PRNG: change to SecureRandom is correct and resolves the prior concernUsing java.security.SecureRandom for salt generation is appropriate here.
Also applies to: 81-81
154-159: Initialize salt before persisting — good catchEnsuring coinJoinSalt is non-zero before serialization fixes non-determinism across restarts.
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (1)
939-944: Invalidate rounds cache only on reorg — aligned with design choiceClearing mapOutpointRoundsCache in reorganize() and not elsewhere matches the stated performance/correctness tradeoff for large wallets.
🧹 Nitpick comments (4)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (1)
85-85: Minor: make salt field volatile and use constants for readability
- coinJoinSalt is read/written across methods; mark it volatile for safe publication.
- Prefer Sha256Hash.isZero() over comparing to ZERO_HASH.
- Use Sha256Hash.LENGTH instead of literal 32.
Apply this diff:
- protected Sha256Hash coinJoinSalt = Sha256Hash.ZERO_HASH; + protected volatile Sha256Hash coinJoinSalt = Sha256Hash.ZERO_HASH; @@ - public Sha256Hash getCoinJoinSalt() { - if (coinJoinSalt.equals(Sha256Hash.ZERO_HASH)) { + public Sha256Hash getCoinJoinSalt() { + if (coinJoinSalt.isZero()) { calculateCoinJoinSalt(); } return coinJoinSalt; } @@ - private void calculateCoinJoinSalt() { - byte [] newCoinJoinSalt = new byte[32]; + private void calculateCoinJoinSalt() { + byte [] newCoinJoinSalt = new byte[Sha256Hash.LENGTH]; random.nextBytes(newCoinJoinSalt); coinJoinSalt = Sha256Hash.wrap(newCoinJoinSalt); }Also applies to: 272-277, 279-283
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (3)
312-314: Remove unused helper or annotate it for testsgetRealOutpointCoinJoinRoundsInternal(...) is currently unused. Remove it or mark @VisibleForTesting and use in tests.
Apply this diff to remove:
- private int getRealOutpointCoinJoinRoundsInternal(TransactionOutPoint outPoint) { - return getRealOutpointCoinJoinRounds(outPoint, 0); - }
361-370: Adjust log severity and marker for expected conditions“No such tx” and “bad index” can occur during scanning and shouldn’t be log.error. Recommend logging with COINJOIN_EXTRA at info/debug to reduce noise.
Apply this diff:
- log.error(String.format("FAILED %-70s %3d (no such tx)", outpoint.toStringCpp(), -1)); + log.info(COINJOIN_EXTRA, String.format("FAILED %-70s %3d (no such tx)", outpoint.toStringCpp(), -1)); @@ - log.error(String.format("FAILED %-70s %3d (bad index)", outpoint.toStringCpp(), -4)); + log.info(COINJOIN_EXTRA, String.format("FAILED %-70s %3d (bad index)", outpoint.toStringCpp(), -4));Also applies to: 373-379
438-462: Deterministic extra-rounds decision looks good; minor clarity nitThe salt and outpoint bytes feed a double-SHA256 and then a 50/50 decision via readInt64 % 2. Consider using getBytes() (not reversed) for the salt to keep endianness consistent, or add a comment explaining the reversal for future maintainers.
Would you like me to prepare a small comment/docstring clarifying the byte order rationale here?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java(7 hunks)core/src/main/java/org/bitcoinj/wallet/WalletEx.java(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dashj#284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.451Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
📚 Learning: 2025-08-25T15:26:22.451Z
Learnt from: HashEngineering
PR: dashpay/dashj#284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.451Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.javacore/src/main/java/org/bitcoinj/wallet/WalletEx.java
🧬 Code graph analysis (2)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (1)
core/src/main/java/org/bitcoinj/core/Sha256Hash.java (1)
Sha256Hash(38-315)
core/src/main/java/org/bitcoinj/wallet/WalletEx.java (4)
core/src/main/java/org/bitcoinj/coinjoin/CoinJoinConstants.java (1)
CoinJoinConstants(24-71)core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientOptions.java (1)
CoinJoinClientOptions(35-120)core/src/main/java/org/bitcoinj/coinjoin/CoinJoin.java (1)
CoinJoin(45-394)core/src/main/java/org/bitcoinj/core/Utils.java (1)
Utils(64-862)
⏰ 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). (3)
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: Analyze (java)
🔇 Additional comments (7)
core/src/main/java/org/bitcoinj/wallet/CoinJoinExtension.java (3)
206-213: Salt deserialization fallback looks fineOn absence of coinjoin_salt, generating a new one is reasonable.
138-153: The script above will dump the serialization context for all extensions. Let’s see whetherserializeWalletExtension()is invoked under any wallet-level lock. Once we have that, we can confirm if an additional lock inCoinJoinExtensionis necessary.
193-205: Deserialization should clear and lock the cacheClear the cache before loading to avoid stale entries being mixed with persisted ones, and guard writes with the wallet lock for thread-safety.
Apply this diff:
// Deserialize outpoint rounds cache for WalletEx if (containingWallet instanceof WalletEx && coinJoinProto.hasOutpointRoundsCache()) { - WalletEx walletEx = (WalletEx) containingWallet; - Protos.OutpointRoundsCache cacheProto = coinJoinProto.getOutpointRoundsCache(); - for (Protos.OutpointRoundsEntry entryProto : cacheProto.getEntriesList()) { - Sha256Hash txHash = Sha256Hash.wrap(entryProto.getTransactionHash().toByteArray()); - long outputIndex = entryProto.getOutputIndex(); - int rounds = entryProto.getRounds(); - TransactionOutPoint outPoint = new TransactionOutPoint(containingWallet.params, outputIndex, txHash); - walletEx.mapOutpointRoundsCache.put(outPoint, rounds); - } + WalletEx walletEx = (WalletEx) containingWallet; + Protos.OutpointRoundsCache cacheProto = coinJoinProto.getOutpointRoundsCache(); + wallet.lock.lock(); + try { + walletEx.mapOutpointRoundsCache.clear(); + for (Protos.OutpointRoundsEntry entryProto : cacheProto.getEntriesList()) { + Sha256Hash txHash = Sha256Hash.wrap(entryProto.getTransactionHash().toByteArray()); + long outputIndex = entryProto.getOutputIndex(); + int rounds = entryProto.getRounds(); + if (rounds == -10) continue; // skip in-progress sentinel if ever serialized + TransactionOutPoint outPoint = new TransactionOutPoint(containingWallet.params, outputIndex, txHash); + walletEx.mapOutpointRoundsCache.put(outPoint, rounds); + } + } finally { + wallet.lock.unlock(); + } }⛔ Skipped due to learnings
Learnt from: HashEngineering PR: dashpay/dashj#284 File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429 Timestamp: 2025-08-25T15:26:22.451Z Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.core/src/main/java/org/bitcoinj/wallet/WalletEx.java (4)
304-310: Public wrapper enforces lock discipline — goodProviding a wrapper that acquires/release the wallet lock ensures safe access to the caching method.
344-352: Good: require caller to hold lock and cap roundscheckState(lock.isHeldByCurrentThread()) prevents unsynchronized access; roundsMax caps recursion depth.
352-359: Cache bootstrap pattern is fineUsing a sentinel (-10) to mark in-progress avoids redundant work under the same lock.
471-475: Nice: test-only cache clearer@VisibleForTesting clearCachesForTests() will help prevent test interdependence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
72-73: Don’t comment out assertions; assert a deterministic invariant instead.coinJoinSalt may vary across runs, but we can still assert stability properties that don’t depend on the exact salt. In particular, the COINJOIN balance should be stable across consecutive reads in the same process and wallet state. This aligns with the retrieved learning that the outpoint rounds cache is not cleared during normal operations, only on reorgs. Replace the commented-out assert with a salt-agnostic check below.
Apply this diff:
- // the coinJoinSalt changed and this value is not constant - // assertEquals(Coin.valueOf(13634336342L), wallet.getBalance(Wallet.BalanceType.COINJOIN)); + // Assert that the COINJOIN balance is stable across consecutive reads for the same wallet state. + Coin cjBalance1 = wallet.getBalance(Wallet.BalanceType.COINJOIN); + Coin cjBalance2 = wallet.getBalance(Wallet.BalanceType.COINJOIN); + assertEquals("COINJOIN balance should be stable across calls", cjBalance1, cjBalance2);Optional follow-up (keeps existing imports): add another relationship-based assertion if desired:
- COINJOIN balance should not exceed ESTIMATED balance.
- COINJOIN balance should be >= 0.
If you want, I can propose a small helper to assert those without adding new imports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
core/src/test/java/org/bitcoinj/wallet/CoinJoinExtensionTest.java(2 hunks)core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/org/bitcoinj/wallet/CoinJoinExtensionTest.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dashj#284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.451Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
⏰ 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). (3)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
149-200: No mixed‐placeholder usages found—please manually verify and add testsI ran the suggested grep checks and didn’t find any callsites where a simple
{}appears before an advanced{:.Nf}, or vice versa, so it looks like the old two-phase formatter wouldn’t actually mis-order any existing messages.That said, absence of evidence isn’t proof of absence. Please:
- Manually inspect any logging or message-formatting calls to confirm no mixed
{}and{:.Nf}placeholders occur.- Consider adding a focused unit test covering mixed placeholder strings (e.g.
"keys: {}; progress: {:.2f}") to lock in correct behavior and guard against regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
core/build.gradle (2)
148-151: Avoid double-signing ambiguity between Gradle and JReleaserYou enable JReleaser signing (active=ALWAYS) and also sign Gradle publications. That’s workable if JReleaser simply reuses .asc files, but it’s easy to drift. Prefer one source of truth.
Minimal options:
- Let Gradle sign artifacts (current setup) and set JReleaser signing to NEVER.
- Or remove Gradle signing and let JReleaser sign during deploy.
Example to rely on Gradle-only:
jreleaser { ... - signing { - active = 'ALWAYS' - armored = true - } + signing { + active = 'NEVER' // Gradle signs; JReleaser will reuse *.asc from staging + } }Also document required env vars for Gradle signing (e.g., ORG_GRADLE_PROJECT_signingKey, ORG_GRADLE_PROJECT_signingPassword).
Also applies to: 191-194
205-212: Jacoco XML report config is commented out but tests still finalize on jacocoTestReport
test.finalizedBy jacocoTestReportruns, but without explicit XML output path some CI consumers (e.g., Codecov) may miss coverage. Re-enable XML output (Gradle 7/8 syntax shown).-//jacocoTestReport { -// dependsOn test // tests are required to run before generating the report -// reports { -// xml.enabled true -// xml.destination file("${project.projectDir}/build/reports/jacoco/jacoco.xml") -// } -//} +jacocoTestReport { + dependsOn test + reports { + xml.required.set(true) + xml.outputLocation.set(file("${project.projectDir}/build/reports/jacoco/jacoco.xml")) + html.required.set(false) + csv.required.set(false) + } +}If you need legacy syntax, keep the original (uncommented) block instead.
Also applies to: 61-76
README.md (1)
76-79: Document required credentials and signing details for publish + jreleaserDeployThe new flow is correct, but readers need which env vars to export for a successful deploy (both SNAPSHOT and release), plus a note that
publishfills build/staging-deploy andjreleaserDeployperforms the remote upload.Suggested additions after the commands:
- For releases (Maven Central via Publisher): JRELEASER_MAVENCENTRAL_USERNAME, JRELEASER_MAVENCENTRAL_PASSWORD
- For SNAPSHOTs (OSSRH nexus2): JRELEASER_NEXUS2_OSSRH_USERNAME, JRELEASER_NEXUS2_OSSRH_PASSWORD
- For Gradle signing (if kept): ORG_GRADLE_PROJECT_signingKey (ASCII-armored), ORG_GRADLE_PROJECT_signingPassword
- Clarify: publish writes artifacts and .asc files to build/staging-deploy; jreleaserDeploy uploads them.
core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientManager.java (2)
330-334: Don’t gate queue-start on sessionResult; base it on post-loop state snapshot to avoid missed startsRelying on sessionResult to decide whether to schedule startAsync can miss the case where a session transitions to POOL_STATE_QUEUE but doAutomaticDenominating returns false (e.g., internal reasons/edge cases). A more robust approach is to compute needStartAsync from the final session states after all sessions have been processed (still under the lock).
Proposed refactor:
- boolean sessionResult = session.doAutomaticDenominating(finishCurrentSessions, dryRun); - if (sessionResult && session.getState() == PoolState.POOL_STATE_QUEUE) { - needStartAsync = true; - } - fResult &= sessionResult; + boolean sessionResult = session.doAutomaticDenominating(finishCurrentSessions, dryRun); + fResult &= sessionResult;And immediately after the for-loop, but still before releasing the lock:
for (CoinJoinClientSession session: deqSessions) { ... - fResult &= sessionResult; } + // Decide outside per-session result to avoid missing transitions to QUEUE. + if (!needStartAsync) { + for (CoinJoinClientSession s : deqSessions) { + if (s.getState() == PoolState.POOL_STATE_QUEUE) { + needStartAsync = true; + break; + } + } + }This ensures a single startAsync is triggered whenever at least one session is queued at the end of this cycle, irrespective of doAutomaticDenominating’s return value semantics.
340-345: Wrap startAsync in a safe boundary and add trace logging for diagnosabilitySince startAsync now runs outside the critical section, consider adding lightweight tracing and exception safety to ease diagnosing racey queueing issues and prevent unexpected runtime exceptions from escaping this path.
- if (needStartAsync) { - coinJoinManager.startAsync(); - } + if (needStartAsync) { + try { + log.debug("CoinJoinClientManager: starting CoinJoinManager due to queued session(s)."); + coinJoinManager.startAsync(); + } catch (RuntimeException e) { + log.warn("CoinJoinClientManager: failed to start CoinJoinManager", e); + } + }If CoinJoinManager exposes a run-state check (e.g., isRunning()), it can be added to avoid redundant starts; otherwise, the try/catch + debug log is sufficient and low risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
README.md(1 hunks)core/build.gradle(4 hunks)core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientManager.java(2 hunks)core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientSession.java(0 hunks)
💤 Files with no reviewable changes (1)
- core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientSession.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dashj#284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
⏰ 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). (2)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (java)
🔇 Additional comments (8)
core/build.gradle (5)
131-136: Staging to build/staging-deploy aligns with JReleaser’s stagingRepositoryPublishing to a local directory is a sound handoff to JReleaser. This reduces credential sprawl in Gradle config and centralizes deployment in JReleaser. LGTM.
169-173: Maven Central publisher path is fine; confirm credentials and workflowThe publisher endpoint and stagingRepository are consistent with the local staging approach. Ensure CI sets JRELEASER_MAVENCENTRAL_xxx credentials and that README instructs running
publish(to fill staging) beforejreleaserDeploy.
82-90: New archiveClassifier API usage is correct; compatible with modern GradleSwitching to
archiveClassifier.set(...)is aligned with Gradle’s property API. No issues.
1-6: Verified Gradle wrapper version supports the plugins DSLThe project’s
gradle/wrapper/gradle-wrapper.propertiesspecifies Gradle 7.1, which satisfies the minimum Gradle 6.8 requirement for the plugins DSL and JReleaser 1.17.0. No changes to the wrapper are needed.
166-186: JReleaser nexus2 configuration looks invalid and snapshot endpoints are likely wrong
- Block shape:
nexus2 { app { ... } }does not match JReleaser’s expectednexus2 { create("name") { ... } }.- Endpoints: Uploading SNAPSHOTs usually targets Sonatype OSSRH (s01.oss.sonatype.org), not central.sonatype.com browsing URLs. As-is, SNAPSHOT deploys will likely fail.
Proposed fix (uses OSSRH s01 endpoints and a named server):
- nexus2 { - app { - active = 'SNAPSHOT' - url = 'https://central.sonatype.com/repository/maven-snapshots/' - snapshotUrl = 'https://central.sonatype.com/repository/maven-snapshots/' - applyMavenCentralRules = true - snapshotSupported = true - closeRepository = true - releaseRepository = true - stagingRepository("$buildDir/staging-deploy") - } - } + nexus2 { + create('ossrh') { + active = 'SNAPSHOT' + // Nexus2 service endpoint (not the repository browse URL) + url = 'https://s01.oss.sonatype.org/service/local/' + snapshotUrl = 'https://s01.oss.sonatype.org/content/repositories/snapshots/' + applyMavenCentralRules = true + snapshotSupported = true + closeRepository = true + releaseRepository = true + stagingRepository("$buildDir/staging-deploy") + } + }Make sure credentials are provided via env vars that JReleaser recognizes (e.g., JRELEASER_NEXUS2_OSSRH_USERNAME / PASSWORD).
⛔ Skipped due to learnings
Learnt from: HashEngineering PR: dashpay/dashj#279 File: core/build.gradle:129-130 Timestamp: 2025-06-09T18:42:57.206Z Learning: The correct snapshot repository URL for the Central Publishing system is `https://central.sonatype.com/repository/maven-snapshots/`, not the legacy OSSRH URL `https://oss.sonatype.org/content/repositories/snapshots/`. The Central Portal URLs should be used instead of legacy OSSRH URLs as OSSRH will be sunset on June 30th, 2025.README.md (2)
70-70: Outputs under build/ acknowledgedThis aligns with the updated Gradle/JReleaser staging strategy. LGTM.
85-85: Minor wording fix is correctThe sentence now reads properly. Thanks for tightening this up.
core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientManager.java (1)
300-300: Confirmed:startAsync()Is Idempotent and Thread-Safe
- In CoinJoinClientManager (core/src/main/java/org/bitcoinj/coinjoin/CoinJoinClientManager.java), the
needStartAsyncflag is computed under the manager lock and, once set, results in exactly one call tocoinJoinManager.startAsync()(around lines 341–343). Subsequent invocations seeneedStartAsync == false, so no further calls occur.- In CoinJoinManager (core/src/main/java/org/bitcoinj/coinjoin/utils/CoinJoinManager.java), the override of
startAsync()acquires its own lock and only callsmasternodeGroup.startAsync()if!masternodeGroup.isRunning(), preventing duplicate starts and avoiding exceptions from Guava’sAbstractService(around lines 282–286).Together, these guarantees ensure that
CoinJoinManager.startAsync()is safe to invoke even if the manager is already running.
HashEngineering
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
gradle/wrapper/gradle-wrapper.properties (2)
1-6: Regenerate Gradle Wrapper JAR to match updated distribution URLThe wrapper JAR in gradle/wrapper/gradle-wrapper.jar still bears a 2021 timestamp and hasn’t been regenerated to align with Gradle 8.7, even though the properties file was updated. A mismatched JAR can lead to build failures:
• gradle/wrapper/gradle-wrapper.jar
– Last modified: Feb 11 2021 – needs regeneration to include the 8.7 wrapper code.
• gradlew & gradlew.bat
– Already updated (Aug 25 2025), but they rely on the JAR’s internal wrapper classes.Please run:
./gradlew wrapper --gradle-version 8.7then commit the regenerated gradle-wrapper.jar (alongside the updated scripts) to ensure consistency.
1-6: Pin Gradle wrapper checksum and enable URL validation
The official SHA-256 forgradle-8.7-all.zipis194717442575a6f96e1c1befa2c30e9a4fc90f701d7aee33eb879b79e7ff05c0(gradle.org). URL validation is enabled by default in the wrapper task (viavalidateDistributionUrl=true) (docs.gradle.org).Please update
gradle/wrapper/gradle-wrapper.propertiesas follows:distributionUrl=https\://services.gradle.org/distributions/gradle-8.7-all.zip +validateDistributionUrl=true +distributionSha256Sum=194717442575a6f96e1c1befa2c30e9a4fc90f701d7aee33eb879b79e7ff05c0.github/workflows/codeql-analysis.yml (1)
41-41: Update checkout to v4 to align with Node20-based actions and reduce warnings.- uses: actions/checkout@v3 + uses: actions/checkout@v4
🧹 Nitpick comments (4)
gradle/wrapper/gradle-wrapper.properties (1)
1-1: Consider -bin distribution for faster CI unless you need sources.If IDE source downloads aren’t required in CI, switching to “-bin.zip” reduces download time.
-distributionUrl=https\://services.gradle.org/distributions/gradle-8.7-all.zip +distributionUrl=https\://services.gradle.org/distributions/gradle-8.7-bin.zip.github/workflows/codeql-analysis.yml (3)
71-73: Optionally enable extended query packs for broader coverage.If build times allow, add security-and-quality queries.
- name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@v3 + uses: github/codeql-action/analyze@v3 + with: + category: "/language:${{ matrix.language }}" + queries: security-extended,security-and-quality
35-36: Consider dropping Python from the CodeQL matrix to speed up scansWe discovered only two Python files in the repo (outside of venvs,
.git, andnode_modules):
examples/src/main/python/forwarding.pytools/src/main/python/generate-seeds-dashj.pyIf you do not need CodeQL analysis on these scripts, you can safely remove Python from the workflow to shave off the extra engine startup time. Otherwise, keep it enabled to cover this tooling code.
Files to update:
.github/workflows/codeql-analysis.yml(lines 35–36)Suggested optional diff:
- language: [ 'java', 'python' ] + language: [ 'java' ]
58-60: Suggest pinning JDK & enabling Gradle cache (optional)Your CodeQL workflow’s
ubuntu-latestrunners now default to Java 17, so Gradle 8.7 builds should succeed without extra setup (github.com). However, explicitly installing a specific JDK and enabling the Gradle cache will make your diagnostics more reproducible and speed up analysis runs.• No existing
actions/setup-javastep in.github/workflows/codeql-analysis.ymlaround the Autobuild job.
• On Ubuntu 24.04 (the currentubuntu-latest), Java 17 is the default (JAVA_HOME_17_X64) (github.com).
• Addingactions/setup-java@v4lets you pin Temurin 17 (or 21) and enablescache: gradlefor faster builds.Example patch:
- name: Autobuild uses: github/codeql-action/autobuild@v3 + + # Pin JDK 17 and cache Gradle dependencies for reproducible, faster builds + - name: Set up JDK 17 + uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: '17' + cache: gradleFeel free to adjust
java-versionto'21'if you’ve configured your Gradle toolchains accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/codeql-analysis.yml(3 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dashj#284
File: core/src/main/java/org/bitcoinj/wallet/WalletEx.java:342-429
Timestamp: 2025-08-25T15:26:22.477Z
Learning: In WalletEx.java, the mapOutpointRoundsCache for CoinJoin rounds should only be cleared during blockchain reorganizations via the reorganize() method, not during normal wallet operations like adding transactions or moving transactions between pools. This design choice prioritizes performance for large wallets while ensuring correctness when the blockchain structure changes.
⏰ 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). (3)
- GitHub Check: Analyze (java)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
- GitHub Check: JAVA 11 OS macOS-latest Gradle
🔇 Additional comments (1)
.github/workflows/codeql-analysis.yml (1)
45-46: Good: CodeQL actions bumped to v3.
| @@ -1,4 +1,4 @@ | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-7.1-all.zip | |||
| distributionUrl=https\://services.gradle.org/distributions/gradle-8.7-all.zip | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Gradle 8.7 requires JDK 17+ to run — verify CI/local toolchains or builds will break.
Upgrading wrapper from 7.1 → 8.7 bumps the minimum runtime JDK for the build to 17. Ensure all workflows (and dev docs) pin Java 17 or 21, and that Gradle toolchains are configured accordingly.
Run this script to locate Java setup across workflows and Gradle toolchain hints:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Workflows using actions/setup-java =="
rg -n -C2 'actions/setup-java@' .github/workflows || true
echo "== Declared Java versions in workflows =="
rg -n -C2 'java-version\s*:\s*' .github/workflows || true
echo "== Gradle toolchain / source/target compatibility =="
rg -n -C3 'toolchain|sourceCompatibility|targetCompatibility' -g '!**/build/**' -S || true
echo "== Wrapper artifacts present? =="
ls -l gradle/wrapper/gradle-wrapper.jar || true
ls -l gradlew gradlew.bat || trueLength of output: 2885
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== matrix Java versions in tests.yml ==="
rg -n "matrix\.java" -C3 .github/workflows/tests.yml || true
echo "=== strategy matrix section ==="
sed -n '1,200p' .github/workflows/tests.yml | sed -n '/strategy:/,/steps:/p'Length of output: 1043
Ensure CI and local toolchains target JDK 17+ for Gradle 8.7
Our workflows and builds are still pinned to Java 11, which will break once the wrapper is upgraded to Gradle 8.7 (which requires JDK 17+ to run). Please update the following before merging:
- .github/workflows/tests.yml
- The matrix only includes
java: ['11']→ update to at least['17'](or include both 17 and 21 if you wish to test multiple runtimes).
- The matrix only includes
- Developer docs / README
- Explicitly state that JDK 17 (or 21) is required to run Gradle tasks locally.
- (Optional, but recommended) Configure a Gradle toolchain in your
build.gradlefiles to enforce Java 17 for compilation and execution:java { toolchain { languageVersion = JavaLanguageVersion.of(17) } }
Once these changes are made, our CI and local builds will be compatible with Gradle 8.7’s JDK requirements.
🤖 Prompt for AI Agents
In gradle/wrapper/gradle-wrapper.properties (lines 1-1) the wrapper was bumped
to Gradle 8.7 which requires JDK 17+, but CI and docs still target Java 11;
update .github/workflows/tests.yml matrix to use at least java: ['17'] (or
include ['17','21'] if testing multiple runtimes), update developer docs/README
to state JDK 17 (or 21) is required for local Gradle tasks, and optionally add a
Java toolchain block to your build.gradle files to enforce languageVersion = 17
so compilation and execution use JDK 17.
Issue being fixed or feature implemented
Cache some coinjoin related information in the wallet to speed up other operations such as spending and mixing.
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
Chores