fix: instantsend processing in background thread#293
Open
HashEngineering wants to merge 16 commits intomasterfrom
Open
fix: instantsend processing in background thread#293HashEngineering wants to merge 16 commits intomasterfrom
HashEngineering wants to merge 16 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughAdds a non-threaded execution mode to InstantSendManager, reworks pending-lock and wallet-update flows, introduces parallel wallet protobuf loading, updates TestNet masternode lists and an updater script, adjusts tests and logging, adds a CoinJoin forwarding example, and minor .run/.gitignore edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Peer as PeerGroup/Peers
participant ISM as InstantSendManager
participant DB as InstantSendDatabase
participant Wallet as Wallet(s)
participant Caller as Caller (DashSystem / Scheduler / Tests)
Note over Peer,ISM: ISLock arrives from network
Peer->>ISM: deliverInstantSendLock(isLock)
ISM->>DB: lookupOrStore(isLock)
alt isLock verified & runWithoutThread == true
ISM->>Caller: notify/processPending()
Caller->>ISM: processPendingInstantSendLocks()
else internal worker thread handles pending
ISM->>ISM: worker thread processes pending locks
end
ISM->>Wallet: getWalletTransaction(isLock.txid)
alt wallet tx found
ISM->>Wallet: set IX_LOCKED / attach InstantSendLock
ISM->>DB: persist applied state
else no wallet tx
ISM->>DB: keep pending / requeue isLock
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/quorums/InstantSendManager.java (1)
434-445:⚠️ Potential issue | 🟠 MajorLogic error:
&&short-circuits despite comment saying "Don't short circuit".On line 441, the comment explicitly states "Don't short circuit. Try to process deterministic and not deterministic islocks", but using
&&will short-circuit if the first call returnsfalse, preventing the second call from executing.🐛 Proposed fix to prevent short-circuit evaluation
} else { // Don't short circuit. Try to process deterministic and not deterministic islocks - return processPendingInstantSendLocks(false) && processPendingInstantSendLocks(true); + boolean nonDeterministic = processPendingInstantSendLocks(false); + boolean deterministic = processPendingInstantSendLocks(true); + return nonDeterministic || deterministic; }Alternatively, use bitwise OR
|which doesn't short-circuit:return processPendingInstantSendLocks(false) | processPendingInstantSendLocks(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java` around lines 434 - 445, The method InstantSendManager.processPendingInstantSendLocks() currently uses logical AND which short-circuits and can prevent the second call to processPendingInstantSendLocks(true) from running despite the comment; change the control flow so both calls always execute (e.g., call processPendingInstantSendLocks(false) and processPendingInstantSendLocks(true) unconditionally and combine their results non-short-circuitingly) so deterministic and non-deterministic islocks are both processed; update the return to reflect the combined boolean result while keeping the existing isInitialized() and DIP0024Active check logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.run/Tests in 'dashj-master-three.core.test'.run.xml:
- Line 13: The Gradle task path is using the project's display name
':dashj-core:test' which Gradle won't resolve; update the option value to use
the actual project path ':core:test' (as defined by project(':core').name =
'dashj-core' in settings.gradle) so the task reference points to the correct
subproject task; locate the option element with value=":dashj-core:test" and
replace it with the project path ':core:test'.
- Line 21: The test run configuration has RunAsTest set to false which disables
IDE test runner integration for the "Tests in..." configuration; change the
RunAsTest value to true so IntelliJ recognizes test results, shows the test
tree, and reports pass/fail status (update the RunAsTest element in the "Tests
in..." run configuration XML accordingly).
---
Outside diff comments:
In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java`:
- Around line 434-445: The method
InstantSendManager.processPendingInstantSendLocks() currently uses logical AND
which short-circuits and can prevent the second call to
processPendingInstantSendLocks(true) from running despite the comment; change
the control flow so both calls always execute (e.g., call
processPendingInstantSendLocks(false) and processPendingInstantSendLocks(true)
unconditionally and combine their results non-short-circuitingly) so
deterministic and non-deterministic islocks are both processed; update the
return to reflect the combined boolean result while keeping the existing
isInitialized() and DIP0024Active check logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 547524d8-813d-437c-aad8-89c70afa76ff
📒 Files selected for processing (4)
.run/Tests in 'dashj-master-three.core.test'.run.xmlcore/src/main/java/org/bitcoinj/manager/DashSystem.javacore/src/main/java/org/bitcoinj/quorums/InstantSendManager.javacore/src/main/java/org/bitcoinj/quorums/LLMQBackgroundThread.java
HashEngineering
force-pushed
the
fix/instantsend-processing
branch
from
1442a06 to
2c12ef5
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
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/quorums/InstantSendManager.java (1)
432-440:⚠️ Potential issue | 🔴 CriticalLine 439 still short-circuits the deterministic pass.
The comment says “Don't short circuit”, but
&&skipsprocessPendingInstantSendLocks(true)whenever the non-deterministic pass returnsfalse—which is the normal case after draining that queue. That leaves deterministic ISLocks unprocessed on the pre-DIP0024 path.Suggested fix
if (blockTip != null && context.getParams().isDIP0024Active(blockTip)) { return processPendingInstantSendLocks(true); } else { // Don't short circuit. Try to process deterministic and not deterministic islocks - return processPendingInstantSendLocks(false) && processPendingInstantSendLocks(true); + boolean moreNonDeterministicWork = processPendingInstantSendLocks(false); + boolean moreDeterministicWork = processPendingInstantSendLocks(true); + return moreNonDeterministicWork || moreDeterministicWork; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java` around lines 432 - 440, The current else branch in InstantSendManager.processPendingInstantSendLocks() uses "return processPendingInstantSendLocks(false) && processPendingInstantSendLocks(true);" which short-circuits and skips the deterministic pass; change it to call both passes unconditionally (e.g., boolean nonDet = processPendingInstantSendLocks(false); boolean det = processPendingInstantSendLocks(true);) and then return the combined result (use nonDet || det to indicate if any work was done), ensuring both processPendingInstantSendLocks(false) and processPendingInstantSendLocks(true) are invoked.
🧹 Nitpick comments (3)
tools/src/main/python/update_masternodes.py (2)
24-24: Specify explicit encoding for file operations.Opening files without explicit encoding relies on system defaults, which can vary across platforms and cause issues on non-UTF-8 systems (e.g., Windows with cp1252).
♻️ Proposed fix to add explicit encoding
- with open(inventory_path) as f: + with open(inventory_path, encoding='utf-8') as f:Apply the same pattern at lines 105 and 111:
- with open(java_path) as f: + with open(java_path, encoding='utf-8') as f:- with open(java_path, 'w') as f: + with open(java_path, 'w', encoding='utf-8') as f:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/src/main/python/update_masternodes.py` at line 24, The open() calls should specify an explicit encoding to avoid platform defaults; update the invocation that uses inventory_path (and the other two open(...) usages later in the file) to include encoding='utf-8' so all file reads/writes consistently use UTF-8; search for the open(inventory_path) call and the two subsequent open(...) calls around where files are read (the three open usages referenced) and add the encoding parameter to each.
70-76: Regex body match may fail if array values contain}characters.The
[^}]*pattern stops at the first}character. If any IP address string or future value contains a}, the regex would capture an incomplete body.For IP addresses this is currently safe, but consider a more robust pattern if the script might be reused for other arrays.
♻️ More robust pattern using non-greedy match with lookahead
pattern = re.compile( r'((?:public\s+|private\s+|protected\s+|static\s+|final\s+)*' r'String\s*\[\]\s*' + re.escape(array_name) + r'\s*=\s*\{)' - r'[^}]*' + r'.*?' r'(\s*\};)', re.DOTALL )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/src/main/python/update_masternodes.py` around lines 70 - 76, The current regex stored in pattern uses [^}]* which will prematurely stop if any value contains a } character; change the body matcher to a non-greedy DOTALL match so it captures everything until the closing brace sequence—update the pattern built around array_name (the variable pattern) to use a non-greedy match like .*? with re.DOTALL (or an explicit lookahead for \s*\};) between the opening group for the declaration and the closing group to robustly capture the array body.examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java (1)
272-285: Make sender parsing failures observable and less broadOn Line 283-Line 285, catching
Exceptionand suppressing it hides parsing failures and makes behavior opaque when sender derivation fails.Suggested fix
- } catch (Exception e) { - // skip this input, try the next + } catch (RuntimeException e) { + System.err.println("Skipping unparsable input script while inferring sender: " + e.getMessage()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java` around lines 272 - 285, In getSenderAddress, avoid the broad catch of Exception that hides parsing failures; instead catch specific expected exceptions thrown while parsing the scriptSig/pubKeyBytes (e.g., ScriptException, IndexOutOfBoundsException, IllegalArgumentException) and log a warning including the transaction id, input index and the exception message so failures are observable; do not swallow unexpected exceptions—either let them propagate or rethrow as a runtime exception after logging. Use the existing logging facility (or add one) to record details when handling failures from scriptSig, pubKeyBytes or ECKey.fromPublicOnly so debugging is possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java`:
- Around line 717-719: The deferred late-arrival path currently calls
updateWalletTransaction(islock.txid) which only flips IX_LOCKED on the first
matching wallet and never attaches the ISLock to the TransactionConfidence,
causing inconsistent confidence state and leaving other wallets stale; change
updateWalletTransaction (or add an overload) to accept the ISLock object
(islock) and, for every wallet that contains the txid, set
transaction.getConfidence().setInstantSendLock(islock) and apply the IX_LOCKED
flag to each one (i.e. fan out to all matching wallets), and update all call
sites (including the other deferred paths) to pass the ISLock so the confidence
is consistent across wallets.
- Around line 223-231: The code currently calls processPendingInstantSendLocks()
while holding InstantSendManager.lock, which pins the lock during heavy BLS
verification and callbacks; move the call so it runs after releasing the manager
lock: exit the synchronized/locked section first, then if runWithoutThread is
true invoke processPendingInstantSendLocks() outside the lock, but preserve the
existing exception mapping (catch RuntimeException, inspect getCause() for
BlockStoreException and rethrow a VerificationException or rethrow the original
RuntimeException) by performing the try/catch around the call made after the
lock is released; reference InstantSendManager.lock and
processPendingInstantSendLocks() when making the change.
In `@core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java`:
- Around line 1181-1185: The txMap.clear() call must be moved into a
method-level finally so the map is always cleared even if loadExtensions(...),
connectTransactionOutputs(...), or addWalletTransaction(...) throws; modify
readWallet in WalletProtobufSerializer so that any allocation/population of
txMap is cleaned in a finally block (ensure both the current txMap.clear() and
the similar cleanup near addWalletTransaction are removed from success paths and
placed into one outer finally that always executes), referencing txMap.clear(),
readWallet, loadExtensions, connectTransactionOutputs, and addWalletTransaction
to locate the affected code.
- Around line 944-957: The patch is calling the injected KeyChainFactory
concurrently when parallelLoad is true (see CompletableFuture.supplyAsync and
calls to KeyChainGroup.fromProtobufEncrypted/fromProtobufUnencrypted using
keyChainFactory), which breaks stateful custom factories set via
setKeyChainFactory(...); change the code so factory-backed parsing is performed
serially or protected by a private lock: either (A) execute all
KeyChainGroup.fromProtobuf* calls on a single-thread executor when parallelLoad
is true, or (B) wrap every use of the injected keyChainFactory in a synchronized
block on a private final lock object (e.g. factoryLock) so setKeyChainFactory
can remain public without requiring callers to be thread-safe; apply the same
protection to all occurrences referenced in the comment.
In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java`:
- Around line 547-548: The current assertions using getTransactionCount(true)
and getBalance(Wallet.BalanceType.ESTIMATED) are too weak; add an assertion that
verifies spend-link connectivity or pool assignment so a broken
connectTransactionOutputs(...) would fail. For example, assert a non-zero
getBalance(Wallet.BalanceType.AVAILABLE) or AVAILABLE_SPENDABLE, compare
wallet.getPoolSize(...) against loaded.getPoolSize(...) for at least one
relevant Transaction.Pool, or iterate transactions and call
getConnectedOutput(tx) / wallet.getConnectedOutput(tx) to assert expected
non-null connectivity; update the tests around the existing asserts (in
LargeCoinJoinWalletTest methods that reference getTransactionCount(true) and
getBalance(ESTIMATED)) to include one of these stronger invariants.
In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java`:
- Around line 171-173: The FileNotFoundException catch in
CoinJoinForwardingService currently swallows missing checkpoint files; change it
to log a warning (e.g., logger.warn or System.err.println) including the
exception message and the checkpoint filename/context rather than silently
ignoring it, so replace the empty catch block around FileNotFoundException with
a log that includes explanatory text and the caught exception (or at minimum
exception.getMessage()) to make missing checkpoint occurrences visible in
production; keep the current control flow (do not rethrow) if intended to
continue on missing files.
- Around line 105-109: The bounds checks around parsing optional CLI args in
CoinJoinForwardingService are wrong and misassign clientPath/confPath; update
the logic in the parsing block (the code that currently checks lastArg + 1 and
lastArg + 2) so that clientPath is assigned from args[lastArg + 1] when lastArg
+ 1 < args.length, and confPath is assigned from args[lastArg + 2] when lastArg
+ 2 < args.length (i.e., use +1/+2 indexing into args and strictly less-than
checks), ensuring you reference the existing variables lastArg, clientPath, and
confPath in your changes.
- Around line 201-203: In CoinJoinForwardingService's anonymous callback method
onFailure(Throwable t) you should not rethrow as a RuntimeException (which can
kill the callback thread); instead handle the error explicitly: log the
exception using the class logger, and propagate the failure via the appropriate
error channel (e.g., completeExceptionally on the associated CompletableFuture,
call an existing error callback/handler, or signal the parent task) so the
failure is observed without terminating the executor thread; replace the throw
new RuntimeException(t) with that logging + propagation pattern inside
onFailure(Throwable t).
---
Outside diff comments:
In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java`:
- Around line 432-440: The current else branch in
InstantSendManager.processPendingInstantSendLocks() uses "return
processPendingInstantSendLocks(false) && processPendingInstantSendLocks(true);"
which short-circuits and skips the deterministic pass; change it to call both
passes unconditionally (e.g., boolean nonDet =
processPendingInstantSendLocks(false); boolean det =
processPendingInstantSendLocks(true);) and then return the combined result (use
nonDet || det to indicate if any work was done), ensuring both
processPendingInstantSendLocks(false) and processPendingInstantSendLocks(true)
are invoked.
---
Nitpick comments:
In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java`:
- Around line 272-285: In getSenderAddress, avoid the broad catch of Exception
that hides parsing failures; instead catch specific expected exceptions thrown
while parsing the scriptSig/pubKeyBytes (e.g., ScriptException,
IndexOutOfBoundsException, IllegalArgumentException) and log a warning including
the transaction id, input index and the exception message so failures are
observable; do not swallow unexpected exceptions—either let them propagate or
rethrow as a runtime exception after logging. Use the existing logging facility
(or add one) to record details when handling failures from scriptSig,
pubKeyBytes or ECKey.fromPublicOnly so debugging is possible.
In `@tools/src/main/python/update_masternodes.py`:
- Line 24: The open() calls should specify an explicit encoding to avoid
platform defaults; update the invocation that uses inventory_path (and the other
two open(...) usages later in the file) to include encoding='utf-8' so all file
reads/writes consistently use UTF-8; search for the open(inventory_path) call
and the two subsequent open(...) calls around where files are read (the three
open usages referenced) and add the encoding parameter to each.
- Around line 70-76: The current regex stored in pattern uses [^}]* which will
prematurely stop if any value contains a } character; change the body matcher to
a non-greedy DOTALL match so it captures everything until the closing brace
sequence—update the pattern built around array_name (the variable pattern) to
use a non-greedy match like .*? with re.DOTALL (or an explicit lookahead for
\s*\};) between the opening group for the declaration and the closing group to
robustly capture the array body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d12e11cc-586f-4c8f-aee0-95b37dc94ef3
📒 Files selected for processing (14)
.gitignore.run/Tests in 'dashj-master-three.core.test'.run.xmlcore/src/main/java/org/bitcoinj/evolution/QuorumRotationState.javacore/src/main/java/org/bitcoinj/manager/DashSystem.javacore/src/main/java/org/bitcoinj/params/TestNet3Params.javacore/src/main/java/org/bitcoinj/quorums/InstantSendManager.javacore/src/main/java/org/bitcoinj/quorums/LLMQBackgroundThread.javacore/src/main/java/org/bitcoinj/wallet/DefaultRiskAnalysis.javacore/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.javacore/src/test/java/org/bitcoinj/coinjoin/CoinJoinSessionTest.javacore/src/test/java/org/bitcoinj/net/discovery/MasternodeSeedPeersTest.javacore/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.javaexamples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.javatools/src/main/python/update_masternodes.py
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- core/src/main/java/org/bitcoinj/evolution/QuorumRotationState.java
🚧 Files skipped from review as they are similar to previous changes (3)
- .run/Tests in 'dashj-master-three.core.test'.run.xml
- core/src/main/java/org/bitcoinj/quorums/LLMQBackgroundThread.java
- core/src/main/java/org/bitcoinj/manager/DashSystem.java
core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java
Outdated
Show resolved
Hide resolved
examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java
Outdated
Show resolved
Hide resolved
examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java
Show resolved
Hide resolved
examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java (1)
931-936:⚠️ Potential issue | 🟠 MajorSerialise access to the injected
KeyChainFactory; snapshotting is not enough.Freezing
effectiveFactoryunderfactoryLockonly prevents replacement mid-parse.keysFuture,friendRecvFuture, andfriendSendFuturestill invoke the same publicKeyChainFactoryconcurrently, which breaks existing stateful/custom factories onceparallelLoadis enabled.🔒 One safe direction
- KeyChainGroup kcg = hasEncryption ? - KeyChainGroup.fromProtobufEncrypted(params, walletProto.getKeyList(), keyCrypter, effectiveFactory) : - KeyChainGroup.fromProtobufUnencrypted(params, walletProto.getKeyList(), effectiveFactory); + KeyChainGroup kcg; + synchronized (factoryLock) { + kcg = hasEncryption ? + KeyChainGroup.fromProtobufEncrypted(params, walletProto.getKeyList(), keyCrypter, effectiveFactory) : + KeyChainGroup.fromProtobufUnencrypted(params, walletProto.getKeyList(), effectiveFactory); + }Apply the same guard to both friend-key futures, or move all factory-backed parsing onto a single-thread executor.
Also applies to: 955-1005
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java` around lines 931 - 936, The snapshot of keyChainFactory under factoryLock isn’t enough because keysFuture, friendRecvFuture, and friendSendFuture still call the (potentially stateful) public KeyChainFactory concurrently when parallelLoad is enabled; ensure all uses of the injected factory are serialized by either (a) acquiring factoryLock around every call that uses keyChainFactory (wrap the factory-access/parsing logic inside keysFuture, friendRecvFuture, friendSendFuture with synchronized(factoryLock)) or (b) move all parsing that uses keyChainFactory onto a single-thread executor and submit tasks from keysFuture/friendRecvFuture/friendSendFuture, so stateful/custom factories are never invoked concurrently (refer to keyChainFactory, factoryLock, keysFuture, friendRecvFuture, friendSendFuture, parallelLoad).examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java (2)
191-205:⚠️ Potential issue | 🟠 MajorKeep forwarding failures on the error path instead of throwing on callback threads.
onFailure(...)and theforwardCoins(...)catch both turn background failures into unchecked exceptions. Here that can tear down the confidence/event thread and stop later forwards from being processed.🛠️ Safer handling
`@Override` public void onFailure(Throwable t) { - throw new RuntimeException(t); + System.err.println("Failed waiting for tx confirmation: " + t.getMessage()); } @@ - } catch (KeyCrypterException | InsufficientMoneyException e) { - throw new RuntimeException(e); + } catch (KeyCrypterException | InsufficientMoneyException e) { + System.err.println("Failed to forward coins: " + e.getMessage()); }Also applies to: 257-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java` around lines 191 - 205, The callback currently throws runtime exceptions on failure and lets exceptions from forwardCoins(...) bubble out on the callback thread (created via Futures.addCallback on tx.getConfidence().getDepthFuture(1)), which can kill the confidence/event thread; instead catch and route errors to the service's error path: change onFailure(Throwable t) to not throw but call a dedicated error handler (e.g. handleForwardFailure(t, tx) or log + enqueue for retry/inspection), and wrap the onSuccess body (Context.propagate(...); forwardCoins(tx); txReport.printReport();) in a try/catch that forwards any thrown exception to the same error handler so failures are recorded/queued rather than thrown on the callback thread. Ensure you apply the same pattern for the other callback at lines 257-269.
91-109:⚠️ Potential issue | 🟠 MajorFix the devnet positional parsing before it selects the wrong network/config.
devnetstill requires at least 7 args here andlastArgnever advances past the devnet-specific fields, so a normaladdress devnet name spork port seedinvocation falls through to mainnet, while longer invocations feeddevnet-name/sporkaddressintoclientPath/confPath.💡 Minimal fix if devnet is meant to consume the remaining positional args
- } else if (args.length > 6 && args[1].equals("devnet")) { + } else if (args.length > 4 && args[1].equals("devnet")) { String[] dnsSeeds = new String[args.length - 5]; System.arraycopy(args, 5, dnsSeeds, 0, args.length - 5); params = DevNetParams.get(args[2], args[3], Integer.parseInt(args[4]), dnsSeeds); filePrefix = "coinjoin-forwarding-service-devnet"; + lastArg = args.length; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java` around lines 91 - 109, The devnet branch selects DevNetParams but doesn't advance lastArg, so subsequent clientPath/confPath parsing misinterprets devnet positional args; update the devnet branch (where DevNetParams.get(...) and filePrefix are set) to set lastArg to args.length (or otherwise to the index after the devnet-specific args) so no devnet arguments fall through into clientPath/confPath parsing and the network/config selection stays correct.
🧹 Nitpick comments (2)
core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java (2)
193-204: Consider clearing ISLock on verification failure.The ISLock is attached to confidence at line 200 before BLS verification. If verification fails (handled at lines 652-661),
IX_LOCK_FAILEDis set but the ISLock object remains attached viasetInstantSendLock(islock). This could be confusing for callers checkinggetInstantSendLock()on a failed lock.Consider clearing the ISLock when setting
IX_LOCK_FAILED:confidence.setInstantSendLock(null); confidence.setIXType(TransactionConfidence.IXType.IX_LOCK_FAILED);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java` around lines 193 - 204, The ISLock instance set on TransactionConfidence via setInstantSendLock(isLock) can remain attached when verification later fails; update the failure path in the BLS verification handling (the code that sets IX_LOCK_FAILED) to also clear the attached lock by calling TransactionConfidence.setInstantSendLock(null) before or when you call setIXType(TransactionConfidence.IXType.IX_LOCK_FAILED), and ensure any relevant listeners are queued (e.g., queueListeners(ChangeReason.IX_TYPE)) so callers seeing getInstantSendLock() won't be misled by a stale lock.
820-839: Consider calling updateWalletTransaction directly instead of re-queuing.When
syncTransactiondetects a late-arriving wallet transaction for an ISLock already in the DB, it re-queues the ISLock for processing. However,processInstantSendLock(line 716-720) will immediately detect the ISLock is already in DB and just callupdateWalletTransaction.This round-trip through pending queue adds overhead. A direct call would be simpler:
Suggested simplification
if (!isDisconnect && islockHash != null && !islockHash.isZero()) { InstantSendLock islock = db.getInstantSendLockByHash(islockHash); if (islock != null && tx.getConfidence().getIXType() != TransactionConfidence.IXType.IX_LOCKED) { - lock.lock(); - try { - if (!pendingInstantSendLocks.containsKey(islockHash)) { - pendingInstantSendLocks.put(islockHash, new Pair<>((long) -1, islock)); - } - } finally { - lock.unlock(); - } - if (runWithoutThread) { - processPendingInstantSendLocks(); - } + updateWalletTransaction(islock.txid, islock); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java` around lines 820 - 839, syncTransaction currently re-queues an already-stored InstantSendLock (found via db.getInstantSendLockByHash) into pendingInstantSendLocks and optionally calls processPendingInstantSendLocks, which duplicates work because processInstantSendLock would immediately call updateWalletTransaction; instead, detect the stored InstantSendLock and call updateWalletTransaction(tx, islock) directly (use the same updateWalletTransaction used by processInstantSendLock) and skip adding to pendingInstantSendLocks and processPendingInstantSendLocks; modify the block that checks islockHash/islock to invoke updateWalletTransaction when islock != null and IX type isn't IX_LOCKED, keeping the lock/unlock discipline if needed and removing the pendingInstantSendLocks.put and processPendingInstantSendLocks call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java`:
- Around line 955-966: The supplier lambdas used when creating
CompletableFutures (e.g., the keysFuture supplier that calls
KeyChainGroup.fromProtobufEncrypted / KeyChainGroup.fromProtobufUnencrypted)
catch UnreadableWalletException and wrap it in a RuntimeException, but callers
later only inspect the direct cause and thus lose the original
UnreadableWalletException; update the suppliers to rethrow the
UnreadableWalletException directly (or unwrap and throw it so that get() sees it
as the cause) instead of wrapping it in a generic RuntimeException, and apply
the same change to the similar suppliers used for friend/receiving/sending key
handling (the blocks referenced around lines 973-985, 992-1004, 1051-1086) so
the catch blocks that call future.get() can detect the true
UnreadableWalletException.
In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java`:
- Around line 132-136: The code calls WalletEx.initializeCoinJoin(0) which uses
the unencrypted overload and will improperly mutate an encrypted wallet; update
the logic that checks WalletEx.getCoinJoin().hasKeyChain(...) so that when the
wallet is encrypted you call initializeCoinJoin(KeyParameter, 0) with the
wallet's decrypt KeyParameter (or else fail-fast) instead of
initializeCoinJoin(0); apply the same change for the other initializeCoinJoin(0)
occurrence in this class so encrypted wallets are never initialized via the
unencrypted overload.
---
Duplicate comments:
In `@core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java`:
- Around line 931-936: The snapshot of keyChainFactory under factoryLock isn’t
enough because keysFuture, friendRecvFuture, and friendSendFuture still call the
(potentially stateful) public KeyChainFactory concurrently when parallelLoad is
enabled; ensure all uses of the injected factory are serialized by either (a)
acquiring factoryLock around every call that uses keyChainFactory (wrap the
factory-access/parsing logic inside keysFuture, friendRecvFuture,
friendSendFuture with synchronized(factoryLock)) or (b) move all parsing that
uses keyChainFactory onto a single-thread executor and submit tasks from
keysFuture/friendRecvFuture/friendSendFuture, so stateful/custom factories are
never invoked concurrently (refer to keyChainFactory, factoryLock, keysFuture,
friendRecvFuture, friendSendFuture, parallelLoad).
In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java`:
- Around line 191-205: The callback currently throws runtime exceptions on
failure and lets exceptions from forwardCoins(...) bubble out on the callback
thread (created via Futures.addCallback on
tx.getConfidence().getDepthFuture(1)), which can kill the confidence/event
thread; instead catch and route errors to the service's error path: change
onFailure(Throwable t) to not throw but call a dedicated error handler (e.g.
handleForwardFailure(t, tx) or log + enqueue for retry/inspection), and wrap the
onSuccess body (Context.propagate(...); forwardCoins(tx);
txReport.printReport();) in a try/catch that forwards any thrown exception to
the same error handler so failures are recorded/queued rather than thrown on the
callback thread. Ensure you apply the same pattern for the other callback at
lines 257-269.
- Around line 91-109: The devnet branch selects DevNetParams but doesn't advance
lastArg, so subsequent clientPath/confPath parsing misinterprets devnet
positional args; update the devnet branch (where DevNetParams.get(...) and
filePrefix are set) to set lastArg to args.length (or otherwise to the index
after the devnet-specific args) so no devnet arguments fall through into
clientPath/confPath parsing and the network/config selection stays correct.
---
Nitpick comments:
In `@core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java`:
- Around line 193-204: The ISLock instance set on TransactionConfidence via
setInstantSendLock(isLock) can remain attached when verification later fails;
update the failure path in the BLS verification handling (the code that sets
IX_LOCK_FAILED) to also clear the attached lock by calling
TransactionConfidence.setInstantSendLock(null) before or when you call
setIXType(TransactionConfidence.IXType.IX_LOCK_FAILED), and ensure any relevant
listeners are queued (e.g., queueListeners(ChangeReason.IX_TYPE)) so callers
seeing getInstantSendLock() won't be misled by a stale lock.
- Around line 820-839: syncTransaction currently re-queues an already-stored
InstantSendLock (found via db.getInstantSendLockByHash) into
pendingInstantSendLocks and optionally calls processPendingInstantSendLocks,
which duplicates work because processInstantSendLock would immediately call
updateWalletTransaction; instead, detect the stored InstantSendLock and call
updateWalletTransaction(tx, islock) directly (use the same
updateWalletTransaction used by processInstantSendLock) and skip adding to
pendingInstantSendLocks and processPendingInstantSendLocks; modify the block
that checks islockHash/islock to invoke updateWalletTransaction when islock !=
null and IX type isn't IX_LOCKED, keeping the lock/unlock discipline if needed
and removing the pendingInstantSendLocks.put and processPendingInstantSendLocks
call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2fba442-dcd8-4f8f-b952-323f8f46955b
📒 Files selected for processing (3)
core/src/main/java/org/bitcoinj/quorums/InstantSendManager.javacore/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.javaexamples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java
Comment on lines
+955
to
+966
| CompletableFuture<KeyChainGroup> keysFuture = CompletableFuture.supplyAsync(() -> { | ||
| log.info("readWallet: keys thread started ({} keys)", walletProto.getKeyCount()); | ||
| long start = System.currentTimeMillis(); | ||
| try { | ||
| KeyChainGroup kcg = hasEncryption ? | ||
| KeyChainGroup.fromProtobufEncrypted(params, walletProto.getKeyList(), keyCrypter, effectiveFactory) : | ||
| KeyChainGroup.fromProtobufUnencrypted(params, walletProto.getKeyList(), effectiveFactory); | ||
| keysTime[0] = System.currentTimeMillis() - start; | ||
| log.info("readWallet: keys thread done in {}ms", keysTime[0]); | ||
| return kcg; | ||
| } catch (UnreadableWalletException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
Unwrap the nested UnreadableWalletException in the key/friend futures.
These suppliers wrap UnreadableWalletException in RuntimeException, but the get() side only checks the direct cause. That collapses the real parse error into generic "Failed to parse ..." messages.
💡 Minimal unwrapping fix
- } catch (ExecutionException e) {
- Throwable cause = e.getCause();
- if (cause instanceof UnreadableWalletException) throw (UnreadableWalletException) cause;
+ } catch (ExecutionException e) {
+ Throwable cause = e.getCause();
+ if (cause instanceof RuntimeException && cause.getCause() instanceof UnreadableWalletException)
+ throw (UnreadableWalletException) cause.getCause();
+ if (cause instanceof UnreadableWalletException) throw (UnreadableWalletException) cause;
throw new UnreadableWalletException("Failed to parse key chain group", e);Apply the same unwrapping in the receiving/sending friend-key catch blocks.
Also applies to: 973-985, 992-1004, 1051-1086
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/main/java/org/bitcoinj/wallet/WalletProtobufSerializer.java` around
lines 955 - 966, The supplier lambdas used when creating CompletableFutures
(e.g., the keysFuture supplier that calls KeyChainGroup.fromProtobufEncrypted /
KeyChainGroup.fromProtobufUnencrypted) catch UnreadableWalletException and wrap
it in a RuntimeException, but callers later only inspect the direct cause and
thus lose the original UnreadableWalletException; update the suppliers to
rethrow the UnreadableWalletException directly (or unwrap and throw it so that
get() sees it as the cause) instead of wrapping it in a generic
RuntimeException, and apply the same change to the similar suppliers used for
friend/receiving/sending key handling (the blocks referenced around lines
973-985, 992-1004, 1051-1086) so the catch blocks that call future.get() can
detect the true UnreadableWalletException.
Comment on lines
+132
to
+136
| WalletEx walletEx = (WalletEx) kit.wallet(); | ||
| if (!walletEx.getCoinJoin().hasKeyChain( | ||
| org.bitcoinj.wallet.DerivationPathFactory.get(finalParams).coinJoinDerivationPath(0))) { | ||
| walletEx.initializeCoinJoin(0); | ||
| } |
There was a problem hiding this comment.
Don’t initialize CoinJoin with the unencrypted overload on a loaded encrypted wallet.
This path also runs for wallets loaded from disk via setWalletFactory(...). initializeCoinJoin(0) always takes the unencrypted path, so encrypted wallets need initializeCoinJoin(KeyParameter, 0) or a fail-fast before mutating the wallet.
Also applies to: 161-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/src/main/java/org/bitcoinj/examples/CoinJoinForwardingService.java`
around lines 132 - 136, The code calls WalletEx.initializeCoinJoin(0) which uses
the unencrypted overload and will improperly mutate an encrypted wallet; update
the logic that checks WalletEx.getCoinJoin().hasKeyChain(...) so that when the
wallet is encrypted you call initializeCoinJoin(KeyParameter, 0) with the
wallet's decrypt KeyParameter (or else fail-fast) instead of
initializeCoinJoin(0); apply the same change for the other initializeCoinJoin(0)
occurrence in this class so encrypted wallets are never initialized via the
unencrypted overload.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Behavior
Tests
Chores