feat: add greedy algorithm for CoinJoinCoinSelector (don't return change) #290
Conversation
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
…oinjoin-greedy
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional multi‑phase greedy coin‑selection path to CoinJoinCoinSelector (new flag and constructor), multiple greedy vs non‑greedy tests and helpers, a Wallet fee calculation tweak when no change is returned, a WalletEx test helper, a test heap increase, and minor .gitignore/formatting edits. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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: 8
🤖 Fix all issues with AI agents
In `@core/src/main/java/org/bitcoinj/coinjoin/CoinJoinCoinSelector.java`:
- Around line 239-255: Phase 3 in CoinJoinCoinSelector currently picks
availableOutputs.get(0) without removing it or updating fees/remaining; to fix,
when you add the selected output in the Phase 3 loop (the branch that does
selection.add(availableOutputs.get(0))), remove that TransactionOutput from
workingDenomMap (or mark it consumed) so later phases cannot reuse the same
UTXO, and then recompute the fee and remaining amount (e.g. call the same fee
calculation used elsewhere or update calculatedFee and remaining after adding
the input) before breaking; update the logic around selection.add(...) and
remaining/ calculatedFee so the selection reflects the increased fee from the
added input.
In `@core/src/main/java/org/bitcoinj/wallet/Wallet.java`:
- Around line 5773-5775: The fee recomputation uses the original `value` (the
requested outputs) which ignores any reductions when `req.recipientsPayFees` is
true and can reset `fee` to zero; update the logic in `Wallet.java` where `fee`
is set (currently to `result.bestCoinSelection.valueGathered.subtract(value)`)
to compute the fee from the actual transaction outputs (the `tx` object) rather
than from `value` so the loop converges correctly—i.e., replace the subtraction
against `value` with a subtraction against the sum of the current `tx` outputs
(taking into account any reduced recipient amounts and excluding change if
`req.returnChange` applies) using the transaction output-sum accessor or by
summing `tx` outputs directly.
In `@core/src/test/java/org/bitcoinj/coinjoin/CoinJoinCoinSelectorTest.java`:
- Around line 113-212: The test mutates the same WalletEx (realWallet) by
calling completeTx twice so the normalReq runs on a wallet already spent by
greedyReq and returnChange differs; fix by cloning or reloading the wallet
before building each transaction and ensure both SendRequest instances use
consistent settings (e.g., set returnChange the same and same feePerKb).
Specifically, for each sendAmount create two independent wallet instances (or
re-read the wallet file) and run completeTx separately for greedyReq and
normalReq, and align properties on greedyReq and normalReq (returnChange,
feePerKb, coinSelector instantiation) so the comparison uses identical starting
state and settings.
In `@core/src/test/java/org/bitcoinj/coinjoin/CoinJoinSelectorGreedyTest.java`:
- Around line 232-240: The test builds expectedGreedyOutputs but never asserts
it against the algorithm result; add an assertion that compares the gathered
greedySelection results to expectedGreedyOutputs by converting
greedySelection.gathered to a sorted list of values (actualGreedyValues) and
comparing it to a sorted list of expectedGreedyOutputs values (expectedSorted)
using the test framework's assertEquals/assertThat in CoinJoinSelectorGreedyTest
so each parameterized case verifies the intended selection.
- Around line 256-269: The test creates coinjoin outputs but leaves the CoinJoin
marker commented out, so CoinJoinCoinSelector (which checks
output.isCoinJoin(...)) will ignore them; update createCoinJoinTransaction to
mark the TransactionOutput as a CoinJoin output (e.g., call
output.markAsCoinJoin() or set the rounds using the same approach as other
tests) before adding it to tx so that CoinJoinCoinSelector will recognize the
output.
In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java`:
- Around line 641-644: The "Change amount" log incorrectly prints tx.getFee();
update LargeCoinJoinWalletTest to compute actual change using the existing
variables (e.g., change =
totalInputValue.subtract(sendAmount).subtract(tx.getFee())) and log
change.toFriendlyString() instead of tx.getFee(), keeping the other info logs
(totalInputValue, sendAmount, fee) intact for clarity.
- Around line 654-659: The test uses the default selector so it never exercises
the greedy path; before calling wallet.completeTx(req) set the greedy selector
(e.g. call wallet.setCoinSelector(new GreedyCoinSelector()) or set
req.coinSelector = new GreedyCoinSelector() if available) so the SendRequest req
uses the greedy algorithm, then call wallet.completeTx(req) and restore the
original selector afterward if needed; reference SendRequest req,
wallet.completeTx(req) and GreedyCoinSelector to locate where to make the
change.
Comment on lines
113
to
212
| @Test | ||
| public void testRealWalletGreedySelection2() throws IOException, UnreadableWalletException, InsufficientMoneyException { | ||
| WalletEx realWallet; | ||
| Context.propagate(new Context(TestNet3Params.get(), 100, Coin.ZERO, false)); | ||
|
|
||
| try (InputStream is = getClass().getResourceAsStream("/org/bitcoinj/wallet/coinjoin-large.wallet")) { | ||
| if (is == null) { | ||
| System.out.println("Wallet file not found, skipping test"); | ||
| return; | ||
| } | ||
| realWallet = (WalletEx) new WalletProtobufSerializer().readWallet(is); | ||
| } | ||
|
|
||
| // Test multiple amounts: 0.5, 0.05, 5 DASH | ||
| Coin[] testAmounts = { | ||
| Coin.valueOf(50000L), // 0.0005 DASH | ||
| Coin.valueOf(5000000L), // 0.05 DASH | ||
| Coin.valueOf(50000000L), // 0.5 DASH | ||
| Coin.valueOf(5000000L), // 0.05 DASH | ||
| Coin.COIN, | ||
| Coin.COIN.plus(Coin.CENT.multiply(27)), | ||
| Coin.valueOf(500000000L), // 5 DASH | ||
| Coin.FIFTY_COINS // 50 DASH | ||
| }; | ||
|
|
||
| for (int i = 0; i < testAmounts.length; i++) { | ||
| Coin sendAmount = testAmounts[i]; | ||
| System.out.println("\n=== Testing Real Wallet Transaction " + (i+1) + " ==="); | ||
| System.out.println("Send amount: " + sendAmount.toFriendlyString()); | ||
| System.out.println("Wallet balance: " + realWallet.getBalance().toFriendlyString()); | ||
|
|
||
| try { | ||
| // Create transaction with greedy algorithm | ||
| Address toAddress = realWallet.freshReceiveAddress(); | ||
| SendRequest greedyReq = SendRequest.to(toAddress, sendAmount); | ||
| greedyReq.coinSelector = new CoinJoinCoinSelector(realWallet, false, true); | ||
| greedyReq.feePerKb = Coin.valueOf(1000L); // Set fee | ||
| greedyReq.returnChange = false; | ||
|
|
||
| // Clone wallet state for parallel testing | ||
| WalletEx greedyWallet = realWallet; // Use same wallet for now | ||
| greedyWallet.completeTx(greedyReq); | ||
| Transaction greedyTx = greedyReq.tx; | ||
|
|
||
| // Create transaction with normal algorithm | ||
| SendRequest normalReq = SendRequest.to(toAddress, sendAmount); | ||
| normalReq.coinSelector = new CoinJoinCoinSelector(realWallet, false, false); | ||
| normalReq.feePerKb = Coin.valueOf(1000L); // Set same fee | ||
|
|
||
| realWallet.completeTx(normalReq); | ||
| Transaction normalTx = normalReq.tx; | ||
|
|
||
| // Calculate input totals and change | ||
| Coin greedyInputTotal = greedyTx.getInputs().stream() | ||
| .map(input -> input.getConnectedOutput().getValue()) | ||
| .reduce(Coin.ZERO, Coin::add); | ||
| Coin normalInputTotal = normalTx.getInputs().stream() | ||
| .map(input -> input.getConnectedOutput().getValue()) | ||
| .reduce(Coin.ZERO, Coin::add); | ||
|
|
||
| Coin greedyChange = greedyInputTotal.subtract(sendAmount).subtract(greedyTx.getFee()); | ||
| Coin normalChange = normalInputTotal.subtract(sendAmount).subtract(normalTx.getFee()); | ||
|
|
||
| System.out.println("Greedy Transaction:"); | ||
| System.out.println(" Inputs: " + greedyTx.getInputs().size()); | ||
| System.out.println(" Total input: " + greedyInputTotal.toFriendlyString()); | ||
| System.out.println(" Fee: " + greedyTx.getFee().toFriendlyString()); | ||
| System.out.println(" Change: " + greedyChange.toFriendlyString()); | ||
| greedyTx.getInputs().forEach(input -> | ||
| System.out.println(" Input: " + input.getConnectedOutput().getValue().toFriendlyString())); | ||
|
|
||
| System.out.println("Normal Transaction:"); | ||
| System.out.println(" Inputs: " + normalTx.getInputs().size()); | ||
| System.out.println(" Total input: " + normalInputTotal.toFriendlyString()); | ||
| System.out.println(" Fee: " + normalTx.getFee().toFriendlyString()); | ||
| System.out.println(" Change: " + normalChange.toFriendlyString()); | ||
| normalTx.getInputs().forEach(input -> | ||
| System.out.println(" Input: " + input.getConnectedOutput().getValue().toFriendlyString())); | ||
|
|
||
| System.out.println("Comparison:"); | ||
| System.out.println(" Greedy change: " + greedyChange.toFriendlyString()); | ||
| System.out.println(" Normal change: " + normalChange.toFriendlyString()); | ||
| System.out.println(" Greedy fee: " + greedyTx.getFee().toFriendlyString()); | ||
| System.out.println(" Normal fee: " + normalTx.getFee().toFriendlyString()); | ||
| System.out.println(" Greedy uses more inputs: " + (greedyTx.getInputs().size() > normalTx.getInputs().size())); | ||
| System.out.println(" Greedy has higher fee: " + greedyTx.getFee().isGreaterThan(normalTx.getFee())); | ||
|
|
||
| // Verify that greedy algorithm produces efficient results | ||
| // assertTrue("Greedy algorithm should minimize change for " + sendAmount.toFriendlyString(), | ||
| // greedyChange.isLessThanOrEqualTo(normalChange)); | ||
|
|
||
| } catch (InsufficientMoneyException e) { | ||
| System.out.println("Insufficient funds for " + sendAmount.toFriendlyString()); | ||
| System.out.println("Available: " + realWallet.getBalance().toFriendlyString()); | ||
| } catch (Exception e) { | ||
| System.out.println("Exception creating transaction for " + sendAmount.toFriendlyString() + ": " + e.getMessage()); | ||
| e.printStackTrace(); | ||
| // Don't fail the test, just log and continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Greedy vs normal comparison mutates the same wallet and mixes settings.
completeTx spends inputs, so the “normal” transaction is built from a mutated wallet after the greedy spend. In addition, returnChange is disabled only for greedy. This makes the comparison unreliable.
Consider reloading/cloning the wallet per comparison (or per iteration) and aligning returnChange/fee settings between greedy and normal requests.
🤖 Prompt for AI Agents
In `@core/src/test/java/org/bitcoinj/coinjoin/CoinJoinCoinSelectorTest.java`
around lines 113 - 212, The test mutates the same WalletEx (realWallet) by
calling completeTx twice so the normalReq runs on a wallet already spent by
greedyReq and returnChange differs; fix by cloning or reloading the wallet
before building each transaction and ensure both SendRequest instances use
consistent settings (e.g., set returnChange the same and same feePerKb).
Specifically, for each sendAmount create two independent wallet instances (or
re-read the wallet file) and run completeTx separately for greedyReq and
normalReq, and align properties on greedyReq and normalReq (returnChange,
feePerKb, coinSelector instantiation) so the comparison uses identical starting
state and settings.
core/src/test/java/org/bitcoinj/coinjoin/CoinJoinSelectorGreedyTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/bitcoinj/coinjoin/CoinJoinSelectorGreedyTest.java
Show resolved
Hide resolved
Comment on lines
641
to
644
| info("Total input value: {}", totalInputValue.toFriendlyString()); | ||
| info("Send amount: {}", sendAmount.toFriendlyString()); | ||
| info("Change amount: {}", tx.getFee().toFriendlyString()); | ||
| info("Greedy algorithm test completed in: {}", watch); |
There was a problem hiding this comment.
Change amount is logged as the fee.
This log line is misleading; it prints the fee instead of actual change. Consider computing change from inputs to avoid confusion.
💡 Suggested fix
- info("Change amount: {}", tx.getFee().toFriendlyString());
+ Coin change = totalInputValue.subtract(sendAmount).subtract(tx.getFee());
+ info("Change amount: {}", change.toFriendlyString());🤖 Prompt for AI Agents
In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java` around
lines 641 - 644, The "Change amount" log incorrectly prints tx.getFee(); update
LargeCoinJoinWalletTest to compute actual change using the existing variables
(e.g., change = totalInputValue.subtract(sendAmount).subtract(tx.getFee())) and
log change.toFriendlyString() instead of tx.getFee(), keeping the other info
logs (totalInputValue, sendAmount, fee) intact for clarity.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@core/src/main/java/org/bitcoinj/wallet/WalletEx.java`:
- Around line 343-346: markAsFullyMixed mutates mapOutpointRoundsCache without
holding the class lock and uses a hard-coded 16; fix it by acquiring the
existing lock before updating the cache (e.g., surround the
mapOutpointRoundsCache.put call with lock.lock()/lock.unlock() or the
appropriate synchronized block used elsewhere) and replace the literal 16 with
the canonical rounds value (use the existing constant or accessor used elsewhere
— e.g., MAX_MIX_ROUNDS or a getMaxMixRounds()/getDefaultRounds() method) so the
method uses the same configured limit as the rest of the class.
In `@core/src/test/java/org/bitcoinj/coinjoin/CoinJoinCoinSelectorTest.java`:
- Around line 82-106: The test currently logs and continues when greedySelection
or normalSelection is null, hiding failures; update CoinJoinCoinSelectorTest to
assert that both greedySelection and normalSelection are non-null (use
assertNotNull on greedySelection and normalSelection before computing
greedyChange/normalChange) so the test fails immediately if selection creation
fails, then proceed with the existing change comparisons and assertions (remove
or replace the else branch that only prints "One or both coin selections
failed").
In `@core/src/test/java/org/bitcoinj/coinjoin/CoinJoinSelectorGreedyTest.java`:
- Around line 198-202: Replace the silent skip in CoinJoinSelectorGreedyTest
where candidates.isEmpty() currently prints and returns; instead call fail(...)
so the test fails when the setup yields no candidates — locate the if
(candidates.isEmpty()) block in the test method and change
System.out.println("No candidates available - skipping test"); return; to a
single fail("No candidates available - test setup failed") (or similar) so the
subsequent assertions run and test failures are reported.
core/src/test/java/org/bitcoinj/coinjoin/CoinJoinCoinSelectorTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/bitcoinj/coinjoin/CoinJoinSelectorGreedyTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@core/src/main/java/org/bitcoinj/coinjoin/CoinJoinCoinSelector.java`:
- Around line 88-89: The hardcoded feePerKb = Coin.valueOf(1000L) in
CoinJoinCoinSelector causes mismatches with SendRequest.feePerKb; update
CoinJoinCoinSelector to accept the effective fee rate from the caller (e.g., add
a Coin feePerKb parameter to the CoinJoinCoinSelector constructor or to
findBestCombination) and use that value instead of the hardcoded 1000L when
computing fees; change references to the local feePerKb variable in
findBestCombination (and any helper methods) to use the injected field/parameter
so the selector uses the same fee rate as
Wallet.completeTx()/SendRequest.feePerKb.
- Around line 262-336: The bug is that after a successful consolidation you
assign selection = optimizedSelection (making them the same reference), so a
subsequent failed consolidation mutates the already-adopted selection; to fix,
perform each consolidation on a fresh trial copy instead of mutating
optimizedSelection in place—i.e., before removing 10 small outputs and adding a
large output, create a tempList = new ArrayList<>(optimizedSelection) and
tempWorkingMap = deep copy of optimizedWorkingMap, apply the removals/additions
to those temps, compute newCalculatedFee and optimizedRemaining from the temps,
and only if optimizedRemaining <= 0 copy tempList into selection and
tempWorkingMap into workingDenomMap and set calculatedFee; do not assign
selection = optimizedSelection directly and ensure selectionCounts updates are
done against the trial copy or reverted from the trial, referencing symbols:
selection, optimizedSelection, optimizedWorkingMap, workingDenomMap,
selectionCounts, calculatedFee in CoinJoinCoinSelector.
In `@core/src/test/java/org/bitcoinj/coinjoin/CoinJoinGreedyCustomTest.java`:
- Around line 248-249: The inline comment next to the test value is incorrect:
Coin.valueOf(12000000L) (assigned to sendAmount in CoinJoinGreedyCustomTest)
represents 0.12 DASH, not 0.012; update the comment text to "0.12 DASH" (or
otherwise make the comment reflect the actual value) so the sendAmount
declaration and its comment are consistent.
- Around line 52-72: In addCoinJoinTransaction, remove the unused local
TransactionInput variable (the declaration "TransactionInput input = ...") since
you create and add inputs later with tx.addSignedInput; also delete any
now-unused import(s) tied to TransactionInput to keep imports clean; ensure the
rest of the method (creation of outputs, inputScript, and addSignedInput calls)
remains unchanged.
- Around line 143-146: In CoinJoinGreedyCustomTest replace the literal
Coin.valueOf(100000000L) entry with the standard denomination constant
Denomination.ONE.value so the test uses the canonical 1.0 DASH CoinJoin
denomination; update the list where Denomination.THOUSANDTH.value,
Denomination.HUNDREDTH.value, Denomination.TENTH.value are used to include
Denomination.ONE.value instead of Coin.valueOf(100000000L).
In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java`:
- Around line 773-786: selectionOverRangeTest currently exhausts the wallet
because createTestTransaction calls wallet.completeTx(req) repeatedly; change
the test so each amount is tested independently by either creating a fresh
Wallet for each iteration (e.g., instantiate a new wallet before calling
createTestTransaction inside selectionOverRangeTest) or avoid mutating the
wallet by calling the selection logic directly (invoke
CoinJoinCoinSelector.select(...) instead of
wallet.completeTx(req)/createTestTransaction), ensuring you update references to
createTestTransaction/wallet.completeTx(req) accordingly.
🧹 Nitpick comments (7)
core/src/main/java/org/bitcoinj/coinjoin/CoinJoinCoinSelector.java (2)
70-71: Remove commented-out debug code.These commented-out
System.out/log.infolines are leftover debug artifacts and add noise.🧹 Proposed cleanup
- // log.info("--------------------------"); - // selected.forEach(System.out::println);
168-195: Fee estimation hard-codes P2PKH input sizes — document the assumption.
148bytes per input and34bytes per output is correct for P2PKH, which is the standard for CoinJoin. Consider adding a brief constant/comment noting this is P2PKH-specific, so future maintainers adapting for P2SH or other script types know to revisit.📝 Extract constants for clarity
+ // P2PKH transaction size constants (bytes) + private static final int TX_HEADER_SIZE = 10; // version(4) + varint(1+1) + locktime(4) + private static final int P2PKH_INPUT_SIZE = 148; // outpoint(36) + scriptLen(1) + sig(~107) + seq(4) + private static final int P2PKH_OUTPUT_SIZE = 34; // value(8) + scriptLen(1) + pubkeyHash(25)core/src/test/java/org/bitcoinj/coinjoin/CoinJoinGreedyCustomTest.java (1)
97-110: Use the existingWalletEx.markAsFullyMixed()test helper instead of reflection.
WalletExalready exposes a@VisibleForTesting public void markAsFullyMixed(TransactionOutPoint)method (seeWalletEx.javasnippets). Using it avoids fragile reflection that will silently break if the field is renamed.♻️ Replace reflection with public API
- private void setOutputRounds(TransactionOutPoint outPoint, int rounds) { - try { - java.lang.reflect.Field field = WalletEx.class.getDeclaredField("mapOutpointRoundsCache"); - field.setAccessible(true); - `@SuppressWarnings`("unchecked") - java.util.Map<TransactionOutPoint, Integer> cache = - (java.util.Map<TransactionOutPoint, Integer>) field.get(wallet); - cache.put(outPoint, rounds); - System.out.println("Set rounds for " + outPoint + " to " + rounds); - } catch (Exception e) { - throw new RuntimeException("Failed to set output rounds", e); - } - } + private void setOutputRounds(TransactionOutPoint outPoint, int rounds) { + wallet.markAsFullyMixed(outPoint); + System.out.println("Set rounds for " + outPoint + " to " + rounds); + }Note:
markAsFullyMixedhardcodes round count to 16. If testing with a specific non-16 round count is needed, you could still keep a reflection-based path, but for "fully mixed" simulation this is sufficient and matches the test's intent (round 10 vs 16 both exceed the mixing threshold).core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (4)
583-651:greedyAlgorithmTestexercises the greedy path and validates single-output (no change) behavior — looks good.The diagnostic output is thorough. One nit: the
hasLargerInputcheck (lines 626-632) is informational only (no assertion) — consider promoting it to anassertFalseif the constraint is a requirement of the greedy algorithm.
587-588: Remove commented-out code.Lines 587 and 598 contain commented-out alternatives that are no longer relevant.
🧹 Cleanup
- //Coin sendAmount = Coin.valueOf(1050000000L); Coin sendAmount = Coin.COIN.div(2);req.returnChange = false; - //req.greedyAlgorithm = true;Also applies to: 598-598
653-719:greedyAlgorithmRecipientPaysFeeTest— functional and covers the recipient-pays-fees scenario.The test correctly configures
recipientsPayFees = trueand validates the no-change contract. The diagnostic logging mirrorsgreedyAlgorithmTestclosely — consider extracting the shared input-iteration / logging block into a helper to reduce duplication.
767-771:selectionTestis a quick smoke test — acceptable.Tests both greedy and non-greedy paths with the same amount. Minimal assertions (relies on
completeTxnot throwing). Consider adding at least an output-count assertion.
core/src/main/java/org/bitcoinj/coinjoin/CoinJoinCoinSelector.java
Outdated
Show resolved
Hide resolved
Comment on lines
262
to
336
| // Phase 4: Optimize by consolidating 10x smaller denominations with 1x larger denomination | ||
| log.info("Phase 4 - Optimizing denomination consolidation"); | ||
|
|
||
| // Create a copy of selection to test optimizations | ||
| ArrayList<TransactionOutput> optimizedSelection = new ArrayList<>(selection); | ||
| HashMap<Coin, ArrayList<TransactionOutput>> optimizedWorkingMap = new HashMap<>(); | ||
| workingDenomMap.forEach((denom, outputs) -> optimizedWorkingMap.put(denom, new ArrayList<>(outputs))); | ||
|
|
||
| // Count outputs by denomination and calculate total in one pass | ||
| HashMap<Coin, Integer> selectionCounts = new HashMap<>(); | ||
| long optimizedTotal = 0; | ||
| for (TransactionOutput output : optimizedSelection) { | ||
| selectionCounts.merge(output.getValue(), 1, Integer::sum); | ||
| optimizedTotal += output.getValue().value; | ||
| } | ||
|
|
||
| // Use ascending order for consolidation (smallest to largest) | ||
| for (int i = 0; i < denomsAscending.size() - 1; i++) { | ||
| Coin smallDenom = denomsAscending.get(i); | ||
| Coin largeDenom = denomsAscending.get(i + 1); | ||
|
|
||
| // Check if we have 10+ of the small denomination and the large denom is exactly 10x | ||
| if (largeDenom.value == smallDenom.value * 10) { | ||
| int smallCount = selectionCounts.getOrDefault(smallDenom, 0); | ||
| ArrayList<TransactionOutput> availableLarge = optimizedWorkingMap.get(largeDenom); | ||
|
|
||
| if (smallCount >= 10 && availableLarge != null && !availableLarge.isEmpty()) { | ||
| log.info("Consolidating 10x" + smallDenom.toFriendlyString() + | ||
| " with 1x" + largeDenom.toFriendlyString()); | ||
|
|
||
| // Remove 10 small denominations from optimized selection | ||
| int removed = 0; | ||
| for (int j = optimizedSelection.size() - 1; j >= 0 && removed < 10; j--) { | ||
| if (optimizedSelection.get(j).getValue().equals(smallDenom)) { | ||
| optimizedSelection.remove(j); | ||
| removed++; | ||
| } | ||
| } | ||
|
|
||
| // Add 1 large denomination (total value unchanged: 10 * small = 1 * large) | ||
| optimizedSelection.add(availableLarge.remove(availableLarge.size() - 1)); | ||
|
|
||
| // Update counts | ||
| selectionCounts.put(smallDenom, selectionCounts.get(smallDenom) - 10); | ||
| selectionCounts.put(largeDenom, selectionCounts.getOrDefault(largeDenom, 0) + 1); | ||
|
|
||
| // Recalculate fee with new input count (fewer inputs = lower fee) | ||
| int newNumInputs = optimizedSelection.size(); | ||
| int newTxSize = 10 + (newNumInputs * 148) + 34; | ||
| long newCalculatedFee = (feePerKb.value * newTxSize) / 1000; | ||
|
|
||
| // Total value is unchanged, but fee is reduced | ||
| long optimizedRemaining = target + newCalculatedFee - optimizedTotal; | ||
|
|
||
| log.info("After consolidation: " + newNumInputs + " inputs, fee: " + | ||
| Coin.valueOf(newCalculatedFee).toFriendlyString() + | ||
| ", remaining: " + Coin.valueOf(optimizedRemaining).toFriendlyString()); | ||
|
|
||
| // If optimization is valid (we still have enough value), use it | ||
| if (optimizedRemaining <= 0) { | ||
| selection = optimizedSelection; | ||
| workingDenomMap.clear(); | ||
| workingDenomMap.putAll(optimizedWorkingMap); | ||
| calculatedFee = newCalculatedFee; | ||
| log.info("Optimization successful - using consolidated selection"); | ||
| } else { | ||
| log.info("Optimization failed - insufficient value, reverting"); | ||
| // Revert the changes | ||
| selectionCounts.put(smallDenom, selectionCounts.get(smallDenom) + 10); | ||
| selectionCounts.put(largeDenom, selectionCounts.get(largeDenom) - 1); | ||
| break; // Stop trying further consolidations for this iteration | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Phase 4: aliased selection is mutated on a failed consolidation after a prior success.
After a successful consolidation at line 322, selection and optimizedSelection become the same reference. If the next loop iteration's consolidation fails (line 327), optimizedSelection has already been irreversibly modified (lines 294-302 removed/added elements) but only selectionCounts is reverted — the list itself is not restored.
With standard CoinJoin denominations (each 10× the previous) this failure path is practically unreachable (value is preserved, fee decreases), but any future denomination set that breaks the 10× invariant would silently corrupt the selection.
🛡️ Defensive fix: re-copy before each consolidation attempt
for (int i = 0; i < denomsAscending.size() - 1; i++) {
+ // Re-snapshot so a failed attempt doesn't corrupt the committed selection
+ optimizedSelection = new ArrayList<>(selection);
+ optimizedWorkingMap = new HashMap<>();
+ workingDenomMap.forEach((d, o) -> optimizedWorkingMap.put(d, new ArrayList<>(o)));
+ selectionCounts.clear();
+ optimizedTotal = 0;
+ for (TransactionOutput output : optimizedSelection) {
+ selectionCounts.merge(output.getValue(), 1, Integer::sum);
+ optimizedTotal += output.getValue().value;
+ }
+
Coin smallDenom = denomsAscending.get(i);
Coin largeDenom = denomsAscending.get(i + 1);🤖 Prompt for AI Agents
In `@core/src/main/java/org/bitcoinj/coinjoin/CoinJoinCoinSelector.java` around
lines 262 - 336, The bug is that after a successful consolidation you assign
selection = optimizedSelection (making them the same reference), so a subsequent
failed consolidation mutates the already-adopted selection; to fix, perform each
consolidation on a fresh trial copy instead of mutating optimizedSelection in
place—i.e., before removing 10 small outputs and adding a large output, create a
tempList = new ArrayList<>(optimizedSelection) and tempWorkingMap = deep copy of
optimizedWorkingMap, apply the removals/additions to those temps, compute
newCalculatedFee and optimizedRemaining from the temps, and only if
optimizedRemaining <= 0 copy tempList into selection and tempWorkingMap into
workingDenomMap and set calculatedFee; do not assign selection =
optimizedSelection directly and ensure selectionCounts updates are done against
the trial copy or reverted from the trial, referencing symbols: selection,
optimizedSelection, optimizedWorkingMap, workingDenomMap, selectionCounts,
calculatedFee in CoinJoinCoinSelector.
core/src/test/java/org/bitcoinj/coinjoin/CoinJoinGreedyCustomTest.java
Outdated
Show resolved
Hide resolved
Comment on lines
+773
to
+786
| @Test | ||
| public void selectionOverRangeTest() throws InsufficientMoneyException { | ||
| ArrayList<Transaction> list = Lists.newArrayList(); | ||
| for (int i = 0; i < 100; i++) { | ||
| for (int j = 0; j < 10; ++j) { | ||
| if (i == 0 && j == 0) j = 1; | ||
| Transaction tx = createTestTransaction(Coin.valueOf(i, j * 10), true); | ||
| list.add(tx); | ||
| } | ||
| } | ||
| list.forEach(tx -> { | ||
| System.out.println("" + tx.getValue(wallet).toFriendlyString() + " fee: " + tx.getFee().toFriendlyString()); | ||
| }); | ||
| } |
There was a problem hiding this comment.
selectionOverRangeTest will exhaust wallet UTXOs and throw InsufficientMoneyException partway through.
This test calls createTestTransaction 999 times on the same wallet. Each call runs wallet.completeTx(req), which marks inputs as spent in the wallet's internal state. With ~169 DASH available, sending amounts up to 99.90 DASH will deplete the wallet well before all iterations complete, causing the test to fail with InsufficientMoneyException.
If the intent is to test selection over a range of amounts independently, you need to either:
- Reset the wallet state before each iteration, or
- Create a fresh wallet for each iteration, or
- Only test selection (via
CoinJoinCoinSelector.select()directly) withoutcompleteTx.
🤖 Prompt for AI Agents
In `@core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java` around
lines 773 - 786, selectionOverRangeTest currently exhausts the wallet because
createTestTransaction calls wallet.completeTx(req) repeatedly; change the test
so each amount is tested independently by either creating a fresh Wallet for
each iteration (e.g., instantiate a new wallet before calling
createTestTransaction inside selectionOverRangeTest) or avoid mutating the
wallet by calling the selection logic directly (invoke
CoinJoinCoinSelector.select(...) instead of
wallet.completeTx(req)/createTestTransaction), ensuring you update references to
createTestTransaction/wallet.completeTx(req) accordingly.
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
Bug Fixes
Tests
Style
Chores