fix: make tx notify on blocks optional and other fixes #288
Conversation
📝 WalkthroughWalkthroughDefensive null-checks were added to several manager close() methods and a masternode sync guard. Transaction coinbase script-length validation was refactored to compute a dynamic minimum length. Wallet gained per-wallet manual confidence-change tracking, a control flag for block-driven notifications, and APIs to validate/update transaction depths. 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
🧪 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: 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
| /** | ||
| * Validates that a single transaction is internally consistent within the wallet. | ||
| * This is a lightweight alternative to {@link #isConsistentOrThrow()} for cases where | ||
| * the wallet is already known to be consistent and we only need to verify one | ||
| transaction. | ||
| * | ||
| * @param tx The transaction to validate | ||
| * @return true if the transaction is consistent with its current pool state | ||
| * @throws IllegalStateException if the transaction is in an inconsistent state | ||
| */ | ||
| // TODO: a test fails when using this in receive() | ||
| 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; | ||
| } finally { | ||
| lock.unlock(); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateTransaction misclassifies some pending/dead txs; either relax or narrow its scope
validateTransaction() currently treats any tx found in pending or dead as “unspent” by calling:
} 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);
}
}For pending transactions that are pure spends (no wallet-owned outputs), isTxConsistent(tx, false) will return false even though the tx state is perfectly valid, which matches the TODO comment about a test failing when using this from receive().
Given this method is public, throwing in such normal cases is confusing for callers. Two concrete options:
- Restrict it to SPENT/UNSPENT pools only (simplest and safest now):
- } 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);
- }
- }
+ }
+ // For PENDING/DEAD we currently skip pool-shape checks and rely on
+ // full-wallet validation via isConsistentOrThrow().- Or keep validating PENDING/DEAD but base the
isSpentexpectation on actual owned-output state, not pool name:
- } 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);
- }
- }
+ } else if (pending.containsKey(txHash) || dead.containsKey(txHash)) {
+ // For pending/dead, only assert internal spent/unspent invariants,
+ // not a specific pool shape.
+ boolean expectedSpent = tx.isEveryOwnedOutputSpent(this);
+ if (!isTxConsistent(tx, expectedSpent)) {
+ throw new IllegalStateException("Inconsistent pending/dead tx: " + txHash);
+ }
+ }In either case, until this API is fully exercised, keeping the call in receive() commented out (as it is now) is reasonable.
Also applies to: 2547-2547
🤖 Prompt for AI Agents
In core/src/main/java/org/bitcoinj/wallet/Wallet.java around lines 1975 to 2013,
validateTransaction incorrectly treats transactions in pending or dead as
unspent which causes false failures for pending pure-spend transactions; change
the logic to only validate transactions that are in the spent or unspent pools
(i.e., remove or skip the pending||dead branch), so pending/dead txs are not
validated here, and update the nearby TODO/comment to reflect this narrowed
scope.
There was a problem hiding this comment.
We will address this in the next PR
There was a problem hiding this comment.
@HashEngineering Sounds good—thanks for the heads‑up. Would you like me to open a follow‑up GitHub issue to track this for the next PR? I can include:
- Decision and change: either restrict validateTransaction() to SPENT/UNSPENT or derive expectedSpent for PENDING/DEAD.
- Javadoc/TODO update to reflect scope.
- Tests: pending pure-spend, mixed, dead, and SPENT/UNSPENT cases.
- Call site review (receive()): keep commented or re-enable once fixed.
If you say yes, I’ll open the issue referencing PR #288 and this discussion, and assign it to @HashEngineering.
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
| extension.processTransaction(tx, block, blockType); | ||
| } | ||
| isConsistentOrThrow(); | ||
| // validateTransaction(tx); |
There was a problem hiding this comment.
@HashEngineering you introduced validateTransaction, but it's never used. Is it needed?
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.