fix: make tx notify on blocks optional and other fixes #288
Conversation
📝 WalkthroughWalkthroughDefensive null-checks added in several close()/sync methods; coinbase input script-length validation refactored to compute a dynamic minimum; Wallet gains per-wallet manual confidence-change tracking, a notify-on-next-block flag and APIs to validate/update transaction depths; one test annotated to be ignored. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 3
🧹 Nitpick comments (2)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
271-271: Clarify and de-duplicate depth-update logic aroundnotifyTxOnNextBlock/ manual tx listThe new
notifyTxOnNextBlockflag and manual list give good control over which transactions get depth updates on each best block, but there are a few points worth tightening:
- When
notifyTxOnNextBlockisfalse, most BUILDING transactions will not have their depth incremented innotifyNewBestBlock, andignoreNextNewBlockentries for those txs are not cleared until the flag is re-enabled. Callers must explicitly keepmanualConfidenceChangeTransactionsin sync or invokeupdateTransactionDepth()to bring all depths back in line withlastBlockSeenHeight. This contract is subtle and should be documented in the new setter/javadoc so future callers understand they’re opting out of automatic depth maintenance.- The depth-update logic (BUILDING check, event-horizon clear,
confidenceChanged.put(...)) is now duplicated three times:
notifyNewBestBlockfull-wallet branchnotifyNewBestBlockmanual-list branchupdateTransactionDepth()
Extracting a small helper likeupdateDepthForTransactions(Iterable<Transaction> txs, boolean useIncrement)would reduce the risk of future divergence.Consider adding brief Javadoc for:
setNotifyTxOnNextBlock(boolean)describing how it interacts withnotifyNewBestBlock,ignoreNextNewBlock, andupdateTransactionDepth().updateTransactionDepth()clarifying when callers should use it (e.g. after long sync or when auto notifications are suppressed).Also applies to: 1869-1875, 2632-2677, 2693-2720
1975-2012: Validate per-transaction consistency vs full-wallet checks – confirm coverage and remove dead commentReplacing
isConsistentOrThrow()inreceive(...)withvalidateTransaction(tx)is a good performance win, but it also changes what invariants are checked:
validateTransactiononly checks the single tx’s pool membership vsisTxConsistent(...).- It no longer verifies global invariants that
isConsistentOrThrow()handled (e.g., duplicate tx hashes across pools, overall map sizes).Because
isConsistentOrThrow()is still called in other hot paths (maybeCommitTx,cleanup,reorganize), this is probably fine, but it’d be good to confirm you’re comfortable with not running a full wallet consistency check on everyreceiveFromBlock/notifyTransactionIsInBlockpath anymore, and that tests cover regressions here.Also, the commented-out call left in place is just noise now:
// isConsistentOrThrow(); validateTransaction(tx);It can be safely removed.
Also applies to: 2545-2547
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/org/bitcoinj/core/Transaction.javacore/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.javacore/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.javacore/src/main/java/org/bitcoinj/quorums/InstantSendManager.javacore/src/main/java/org/bitcoinj/wallet/Wallet.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 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.
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 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.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🧬 Code graph analysis (2)
core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java (1)
core/src/main/java/org/bitcoinj/core/MasternodeSync.java (1)
MasternodeSync(30-616)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
core/src/main/java/org/bitcoinj/core/TransactionConfidence.java (1)
TransactionConfidence(66-714)
⏰ 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 macOS-latest Gradle
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
🔇 Additional comments (4)
core/src/main/java/org/bitcoinj/quorums/ChainLocksHandler.java (1)
133-135: LGTM! Defensive null check prevents NPE during cleanup.The null check on
peerGroupbefore removing the listener is appropriate and prevents potentialNullPointerExceptionifclose()is called whenpeerGroupwas never initialized or already set to null.core/src/main/java/org/bitcoinj/quorums/InstantSendManager.java (1)
148-150: LGTM! Null guard prevents NPE during listener cleanup.The null check on
chainLocksHandlerbefore removing the chain lock listener is appropriate, aschainLocksHandleris conditionally initialized insetBlockChain()(line 130-133) and may be null whenclose()is called.core/src/main/java/org/bitcoinj/evolution/SimplifiedMasternodeListManager.java (2)
303-304: LGTM! Null guard prevents NPE when checking sync flags.The null check on
masternodeSyncbefore evaluating sync flags is appropriate. SincemasternodeSyncis initialized externally viasetBlockChain()(line 422), this guard preventsNullPointerExceptionifshouldProcessMNListDiff()is called before initialization.
446-448: LGTM! Defensive null check prevents NPE during cleanup.The null check on
peerGroupbefore removing the listener is appropriate and consistent with similar cleanup patterns inChainLocksHandler.javaandInstantSendManager.java. This prevents potentialNullPointerExceptionduring resource cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
239-239: GuardmanualConfidenceChangeTransactionswithlockto avoid races/ConcurrentModificationException
manualConfidenceChangeTransactionsis a plainArrayListwritten fromaddManualNotifyConfidenceChangeTransaction/removeManualNotifyConfidenceChangeTransactionwithout takinglock, but it is iterated innotifyNewBestBlockwhile the wallet lock is held. If these methods are ever called from a different thread than the block-processing thread, this is a data race and can throwConcurrentModificationExceptionor yield stale views.This list should follow the same locking discipline as the rest of the wallet state.
Proposed fix: guard list with wallet lock
- private final ArrayList<Transaction> manualConfidenceChangeTransactions = Lists.newArrayList(); + @GuardedBy("lock") + private final List<Transaction> manualConfidenceChangeTransactions = Lists.newArrayList(); @@ - public void addManualNotifyConfidenceChangeTransaction(Transaction tx) { - manualConfidenceChangeTransactions.add(tx); - } + public void addManualNotifyConfidenceChangeTransaction(Transaction tx) { + lock.lock(); + try { + manualConfidenceChangeTransactions.add(tx); + } finally { + lock.unlock(); + } + } @@ - public void removeManualNotifyConfidenceChangeTransaction(Transaction tx) { - manualConfidenceChangeTransactions.remove(tx); - } + public void removeManualNotifyConfidenceChangeTransaction(Transaction tx) { + lock.lock(); + try { + manualConfidenceChangeTransactions.remove(tx); + } finally { + lock.unlock(); + } + }Also applies to: 2633-2676, 6585-6591
2694-2721: Fix off‑by‑one and negative depth risk inupdateTransactionDepth
updateTransactionDepth()currently sets:confidence.setDepthInBlocks(lastBlockSeenHeight - confidence.getAppearedAtChainHeight());This yields depth 0 when a tx has just appeared in the block at
appearedHeight == lastBlockSeenHeight, and is one less than expected for existing BUILDING transactions. It can also go negative iflastBlockSeenHeightis behindappearedAtChainHeight. Depth semantics elsewhere treat the appearance height as depth 1.Align this with the usual formula and guard against out-of-order heights.
Proposed fix for depth calculation
- confidence.setDepthInBlocks(lastBlockSeenHeight - confidence.getAppearedAtChainHeight()); + int appearedHeight = confidence.getAppearedAtChainHeight(); + if (lastBlockSeenHeight >= appearedHeight) { + confidence.setDepthInBlocks(lastBlockSeenHeight - appearedHeight + 1); + } if (confidence.getDepthInBlocks() > context.getEventHorizon()) confidence.clearBroadcastBy();
🧹 Nitpick comments (2)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
1975-2013: MakevalidateTransactionoperate on the canonical wallet transaction instance
validateTransactiondetermines pool membership bytxHashbut runsisTxConsistent(...)on theTransactioninstance passed in. If a caller provides a logically equivalentTransactionthat is not the same instance as the one stored in the pools, the spent/unspent wiring (getOutputs(),getSpentBy(),isAvailableForSpending()) may not reflect actual wallet state, leading to spuriousIllegalStateExceptions or missed inconsistencies.To make this helper robust and self-contained, resolve the canonical transaction from the appropriate pool map before validating, and fall back early when the hash is not tracked by the wallet.
Proposed refactor to use canonical pool instance
public boolean validateTransaction(Transaction tx) { lock.lock(); try { - Sha256Hash txHash = tx.getTxId(); - - // Determine which pool the transaction is in and validate accordingly - if (spent.containsKey(txHash)) { - if (!isTxConsistent(tx, true)) { - throw new IllegalStateException("Inconsistent spent tx: " + txHash); - } - } else if (unspent.containsKey(txHash)) { - if (!isTxConsistent(tx, false)) { - throw new IllegalStateException("Inconsistent unspent tx: " + txHash); - } - } else if (pending.containsKey(txHash) || dead.containsKey(txHash)) { - // For pending/dead transactions, check as unspent - if (!isTxConsistent(tx, false)) { - throw new IllegalStateException("Inconsistent pending/dead tx: " + - txHash); - } - } - // If not in any pool, nothing to validate - - return true; + Sha256Hash txHash = tx.getTxId(); + + Transaction walletTx = spent.get(txHash); + if (walletTx != null) { + if (!isTxConsistent(walletTx, true)) { + throw new IllegalStateException("Inconsistent spent tx: " + txHash); + } + return true; + } + + walletTx = unspent.get(txHash); + if (walletTx != null) { + if (!isTxConsistent(walletTx, false)) { + throw new IllegalStateException("Inconsistent unspent tx: " + txHash); + } + return true; + } + + walletTx = pending.get(txHash); + if (walletTx != null) { + if (!isTxConsistent(walletTx, false)) { + throw new IllegalStateException("Inconsistent pending tx: " + txHash); + } + return true; + } + + walletTx = dead.get(txHash); + if (walletTx != null) { + if (!isTxConsistent(walletTx, false)) { + throw new IllegalStateException("Inconsistent dead tx: " + txHash); + } + return true; + } + + // If not in any pool, nothing to validate against this wallet + return true; } finally { lock.unlock(); } }
2547-2547: Avoid leaving commented-outvalidateTransaction(tx)callThe commented-out
// validateTransaction(tx);inreceive(...)is easy to forget and can become stale. Either remove it entirely or replace it with a more explicit TODO referencing the failing test or issue so future maintainers know why it’s intentionally disabled.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 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.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
⏰ 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)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
271-271:notifyTxOnNextBlockflag and accessors look correctThe new
notifyTxOnNextBlocktoggle defaults totrue, preserving existing behavior, and the simple getter/setter cleanly expose control over whethernotifyNewBestBlockwalks all transactions or only the manual list. No issues with this change as written.Also applies to: 1869-1875
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
239-239: Manual confidence-change tracking now correctly locked; consider Set + @GuardedBy for stronger invariantsThe race/
ConcurrentModificationExceptionrisk aroundmanualConfidenceChangeTransactionsis addressed by takinglockinaddManualNotifyConfidenceChangeTransaction/removeManualNotifyConfidenceChangeTransaction, and by iterating underlockinnotifyNewBestBlock(). That aligns this state with the rest of the wallet’s locking model.Two small robustness improvements you may want to fold in:
- Mark the field as guarded by
lockto document the invariant and help static analysis:
@GuardedBy("lock") private final … manualConfidenceChangeTransactions- Use a
Set<Transaction>instead of aListto avoid accidental duplicate registrations, which would currently double-increment depth per block for that tx whennotifyTxOnNextBlock == false.Proposed refactor for `manualConfidenceChangeTransactions`
- private final ArrayList<Transaction> manualConfidenceChangeTransactions = Lists.newArrayList(); + @GuardedBy("lock") + private final Set<Transaction> manualConfidenceChangeTransactions = Sets.newHashSet(); @@ - manualConfidenceChangeTransactions.add(tx); + manualConfidenceChangeTransactions.add(tx); @@ - manualConfidenceChangeTransactions.remove(tx); + manualConfidenceChangeTransactions.remove(tx);Also applies to: 2633-2675, 6585-6602
2694-2721: Depth recomputation fix is correct; optional defensive guard on heights
updateTransactionDepth()now uses:confidence.setDepthInBlocks(lastBlockSeenHeight - confidence.getAppearedAtChainHeight() + 1);which matches the
setAppearedAtChainHeight()semantics (depth 1 at appearance height) and fixes the previous off‑by‑one.To make this more robust against any pathological state (e.g., if
lastBlockSeenHeightwere ever behindappearedAtChainHeight), you might add a simple guard:- confidence.setDepthInBlocks(lastBlockSeenHeight - confidence.getAppearedAtChainHeight() + 1); + int appeared = confidence.getAppearedAtChainHeight(); + if (lastBlockSeenHeight >= appeared && appeared > 0) { + confidence.setDepthInBlocks(lastBlockSeenHeight - appeared + 1); + }Functionally, though, the current change resolves the main correctness issue.
🧹 Nitpick comments (1)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
271-271:notifyTxOnNextBlockflagging looks good; consider memory-visibility tighteningThe new
notifyTxOnNextBlockflag and branching innotifyNewBestBlock()behave as intended: existing users retain the original “update all BUILDING txs per block” path, and callers can opt into manual per-tx updates viamanualConfidenceChangeTransactions.If
setNotifyTxOnNextBlock(boolean)can be called from arbitrary threads while block processing is running, you may want to either:
- Guard the flag with
lockin both setter and reads, or- Declare it
volatileto guarantee timely visibility across threads. The rest of the logic is fine as-is.
Also applies to: 1869-1875, 2633-2675
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 288
File: core/src/main/java/org/bitcoinj/core/Transaction.java:1572-1577
Timestamp: 2025-12-26T19:03:14.265Z
Learning: In Dash, TRANSACTION_COINBASE (type 5, CbTx) coinbase transactions require only 1 byte minimum scriptSig length (not 2), because the block height is encoded in the CbTx special payload rather than the coinbase scriptSig. Regular coinbase transactions still require 2–100 bytes.
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 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.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🧬 Code graph analysis (1)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
core/src/main/java/org/bitcoinj/core/TransactionConfidence.java (1)
TransactionConfidence(66-714)
⏰ 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: Analyze (java)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (1)
99-99: Track this ignored test for future resolution.Ignoring the test is a reasonable short-term fix for the OutOfMemoryError. However, ensure this is tracked so the underlying issue can be addressed later—either by optimizing the
toString()implementation for large wallets, increasing the test's heap allocation, or redesigning the test approach.Would you like me to open a GitHub issue to track this test failure, or help investigate potential fixes such as JVM configuration for this specific test or memory optimization in the
toString()methods?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 288
File: core/src/main/java/org/bitcoinj/core/Transaction.java:1572-1577
Timestamp: 2025-12-26T19:03:14.265Z
Learning: In Dash, TRANSACTION_COINBASE (type 5, CbTx) coinbase transactions require only 1 byte minimum scriptSig length (not 2), because the block height is encoded in the CbTx special payload rather than the coinbase scriptSig. Regular coinbase transactions still require 2–100 bytes.
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 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.
Applied to files:
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java
⏰ 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: Analyze (java)
- GitHub Check: JAVA 11 OS ubuntu-latest Gradle
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (2)
1975-2013:validateTransactionscope/semantics already tracked in follow‑up issueThe current
validateTransaction()still treats any tx inpending/deadas “unspent” (isTxConsistent(tx, false)), which is the behavior already discussed in this PR and captured in the follow‑up issue you requested (re: pending pure‑spend txs). Given that, it’s fine to keep this helper as‑is in this PR and leave the semantic change to that dedicated issue.If you haven’t already, please confirm that the new issue remains open and link it from this Javadoc/TODO so future readers understand why this method isn’t used from
receive()yet.
2547-2548: Commented‑outvalidateTransaction(tx)call aligns with current TODOThe TODO here correctly reflects that
validateTransaction()isn’t yet robust enough to replaceisConsistentOrThrow()(as discussed earlier in the PR and captured in the follow‑up issue). Keeping this call commented out inreceive()is appropriate until that work lands; just ensure the TODO or Javadoc onvalidateTransaction()cross‑references the tracking issue so it doesn’t get forgotten.
🧹 Nitpick comments (5)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (5)
239-239: Lock-guarded manual list looks good; consider documenting the guard
manualConfidenceChangeTransactionsis only accessed underlocknow (including the new add/remove methods andnotifyNewBestBlock), which fixes the earlier race. For future maintainers, you may want to annotate this field with@GuardedBy("lock")likespentOutpointsIndexto make the intended synchronization explicit.
271-271: SynchronizenotifyTxOnNextBlockaccess for consistency with other wallet flags
notifyTxOnNextBlockis written/read withoutlockorvolatile, but is consumed insidenotifyNewBestBlockunderlockand may be toggled from other threads viasetNotifyTxOnNextBlock(). To avoid a (low‑risk) data race and match patterns likesetAcceptRiskyTransactions()andsetRiskAnalyzer(), consider guarding the setter/getter withlock(or making the fieldvolatile) and optionally using the accessor fromnotifyNewBestBlockas well.Also applies to: 1869-1875
2634-2677: Manual vs automatic depth updates: clarify interaction withignoreNextNewBlockandupdateTransactionDepthThe new
notifyTxOnNextBlockswitch andmanualConfidenceChangeTransactionspath correctly avoid scanning all txs when you only care about a subset, while preserving the “skip once afterreceive()” behavior for those manual txs viaignoreNextNewBlock.One subtle edge case: for BUILDING transactions not in
manualConfidenceChangeTransactions,ignoreNextNewBlockentries are left untouched whilenotifyTxOnNextBlock == false, so when you later flip it back totrue, those txs will skip a single depth increment (until you callupdateTransactionDepth()). If that matters for callers, consider either:
- Documenting that the expected pattern when switching modes is: disable → run
updateTransactionDepth()→ re‑enable; or- Clearing any stale entries in
ignoreNextNewBlockwhen running in manual mode (e.g.,ignoreNextNewBlock.clear()at the end of theelsebranch) so future automatic updates can proceed cleanly.
2695-2722: Depth recomputation logic is now correct; optional defensive guard
updateTransactionDepth()now useslastBlockSeenHeight - appearedAtChainHeight + 1, which matches the intended BUILDING‑depth semantics and fixes the earlier off‑by‑one. To be extra defensive against any future call sites wherelastBlockSeenHeightmight lagappearedAtChainHeight(e.g., unusual restore/reorg flows), you could wrap thesetDepthInBlockscall with a simple guard:Optional safety guard
- if (confidence.getConfidenceType() == ConfidenceType.BUILDING) { + if (confidence.getConfidenceType() == ConfidenceType.BUILDING) { // ... - confidence.setDepthInBlocks(lastBlockSeenHeight - confidence.getAppearedAtChainHeight() + 1); + int appeared = confidence.getAppearedAtChainHeight(); + if (lastBlockSeenHeight >= appeared) { + confidence.setDepthInBlocks(lastBlockSeenHeight - appeared + 1); + }Not mandatory if you’re confident the invariant always holds, but it would prevent any accidental negative depths.
6587-6603: Manual confidence‑change list now thread‑safe; consider set semantics to avoid duplicatesWrapping
addManualNotifyConfidenceChangeTransaction/removeManualNotifyConfidenceChangeTransactioninlockfixes the earlier race on this list and makes its use fromnotifyNewBestBlocksafe.One behavioral edge case: because
manualConfidenceChangeTransactionsis anArrayList, callers can add the sameTransactionmultiple times, which would causeincrementDepthInBlocks()to run more than once per block for that tx in manual mode. If you want to harden this API against misuse, consider switching the field to aLinkedHashSet<Transaction>(or checkingcontains()before add/remove) so each tx appears at most once.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 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.
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 288
File: core/src/main/java/org/bitcoinj/core/Transaction.java:1572-1577
Timestamp: 2025-12-26T19:03:21.760Z
Learning: In Dash, TRANSACTION_COINBASE (type 5, CbTx) coinbase transactions require only 1 byte minimum scriptSig length (not 2), because the block height is encoded in the CbTx special payload rather than the coinbase scriptSig. Regular coinbase transactions still require 2–100 bytes.
📚 Learning: 2025-08-25T15:26:22.477Z
Learnt from: HashEngineering
Repo: dashpay/dashj PR: 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.
Applied to files:
core/src/main/java/org/bitcoinj/wallet/Wallet.java
🧬 Code graph analysis (1)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)
core/src/main/java/org/bitcoinj/core/TransactionConfidence.java (1)
TransactionConfidence(66-714)
⏰ 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)
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
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.