fix: use validateTransaction instead of isConsistentOrThrow in Wallet…

[HashEngineering]

….receive

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

Summary by CodeRabbit

• Refactor
  • Streamlined transaction validation by narrowing its focus to transactions in current active pools, removing validation checks for certain transaction states to enhance consistency across validation methods.

[coderabbitai]

📝 Walkthrough

Walkthrough

The validateTransaction method in the Wallet class was changed from public to protected visibility, and its validation logic was narrowed to only validate transactions in spent or unspent pools, removing support for pending and dead transactions. Call sites were updated to invoke validateTransaction(tx) directly instead of relying on isConsistentOrThrow().

Changes

Cohort / File(s)	Summary
Validation Logic Refactor core/src/main/java/org/bitcoinj/wallet/Wallet.java	Changed validateTransaction from public to protected; removed pending/dead transaction validation paths; updated multiple call sites to use validateTransaction(tx) instead of isConsistentOrThrow(); validation now restricted to spent/unspent pool scenarios.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A rabbit once guarded each path with great care,
Then trimmed back the branches that floated in air—
Now pending and dead paths are cleared from the view,
Protected, not public, the method shines true! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main change: replacing isConsistentOrThrow with validateTransaction in Wallet, and changing the method's visibility from public to protected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/validate-transactions

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.41.0)

core/src/main/java/org/bitcoinj/wallet/Wallet.java

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

core/src/main/java/org/bitcoinj/wallet/Wallet.java (1)

1990-2005: ⚠️ Potential issue | 🔴 Critical

validateTransaction() drops the multi-pool invariant that isConsistentOrThrow() enforced.

isConsistentOrThrow() did more than validate spent/unspent outputs: it also detected the same tx appearing in multiple pools by comparing the summed pool sizes with getTransactions(true).size(). This replacement skips that entirely, so a tx can now live in spent and pending simultaneously and still pass validation, which weakens the safeguard exercised by core/src/test/java/org/bitcoinj/wallet/WalletTest.java:683-712.

Proposed fix

 protected boolean validateTransaction(Transaction tx) {
     lock.readLock().lock();
     try {
         Sha256Hash txHash = tx.getTxId();
+        EnumSet<Pool> pools = getContainingPools(tx);
+        if (pools.size() > 1) {
+            throw new IllegalStateException("Transaction appears in multiple pools: " + txHash + " " + pools);
+        }
 
         // Determine which pool the transaction is in and validate accordingly.
         // Pending/dead transactions are intentionally not checked here: their outputs
         // may all be spent by other pending/conflict transactions, which is valid.
         // This mirrors the behaviour of isConsistentOrThrow() which only checks

🤖 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/Wallet.java` around lines 1990 - 2005,
The new validateTransaction() implementation removed the multi-pool uniqueness
check that isConsistentOrThrow() enforced, allowing a tx to appear in multiple
pools; restore that invariant by adding a check in validateTransaction() (the
method shown) that verifies the total number of unique transactions across all
pools equals getTransactions(true).size() (or equivalently detect duplicates
across spent, unspent, pending, dead maps) and throw an IllegalStateException if
a transaction is present in more than one pool; locate the pools referenced
(spent, unspent, pending, dead) and perform the uniqueness/size comparison or
duplicate detection before returning so the same safeguard the WalletTest
expects is re-established.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@core/src/main/java/org/bitcoinj/wallet/Wallet.java`:
- Around line 1990-2005: The new validateTransaction() implementation removed
the multi-pool uniqueness check that isConsistentOrThrow() enforced, allowing a
tx to appear in multiple pools; restore that invariant by adding a check in
validateTransaction() (the method shown) that verifies the total number of
unique transactions across all pools equals getTransactions(true).size() (or
equivalently detect duplicates across spent, unspent, pending, dead maps) and
throw an IllegalStateException if a transaction is present in more than one
pool; locate the pools referenced (spent, unspent, pending, dead) and perform
the uniqueness/size comparison or duplicate detection before returning so the
same safeguard the WalletTest expects is re-established.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 735e3208-6f4c-4f5e-ac54-0054e9c5de8f

📥 Commits

Reviewing files that changed from the base of the PR and between 3777b7c and ac03dc0.

📒 Files selected for processing (1)

• core/src/main/java/org/bitcoinj/wallet/Wallet.java