fix: improve getTransactions in Wallet #276
Conversation
WalkthroughThe changes introduce enhancements to wallet transaction retrieval in the core library, add a new large-scale CoinJoin wallet test, and update example and run configuration files to use new modules and parameters. Notable additions include new methods for transaction counting and listing, a performance-focused test class, and updated run parameters for various WalletTool configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as LargeCoinJoinWalletTest
participant Wallet as WalletEx
participant File as coinjoin-large.wallet
Test->>File: Load wallet data
Test->>Wallet: Deserialize wallet
Test->>Wallet: Retrieve CoinJoin rounds, key usage
Test->>Wallet: Refresh unused keys
Test->>Wallet: Get balances and mixing progress
Test->>Wallet: Generate transaction reports
Test->>Wallet: getTransactionCount(includeDead)
Test->>Wallet: getTransactionList(includeDead)
Wallet-->>Test: Return results for assertions and timing
Poem
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.31.1)core/src/main/java/org/bitcoinj/wallet/Wallet.java✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…allet-improvements
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
.run/WalletTool CJ address (devnet).run.xml (1)
4-5: Configuration updated to use test network and standardized module path.The changes standardize this configuration to use the same module name and wallet file as other configurations, and switch from DEVNET to TEST network.
Consider renaming the file to reflect that it now uses the TEST network instead of DEVNET to avoid confusion.
core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (5)
1-15: Copyright year needs to be corrected.The copyright year is set to 2025, which is in the future. Please update it to the current year.
- * Copyright 2025 Dash Core Group + * Copyright 2023 Dash Core Group
43-46: Add proper resource handling in the setup method.The try-with-resources statement is correctly used for the InputStream, but there's a potential issue with how the wallet is loaded that could lead to resource leaks if an exception occurs during serialization.
Consider adding a more robust error handling mechanism to ensure all resources are properly closed:
try (InputStream is = getClass().getResourceAsStream("coinjoin-large.wallet")) { Stopwatch watch = Stopwatch.createStarted(); + if (is == null) { + throw new RuntimeException("Test wallet file 'coinjoin-large.wallet' not found in resources"); + } wallet = (WalletEx) new WalletProtobufSerializer().readWallet(is); info("loading wallet: {}; {} transactions", watch, wallet.getTransactionCount(true)); }
129-148: Consider using SLF4J directly for logging.The test implements a custom logging system that prints directly to standard output, despite importing SLF4J. Consider using the imported SLF4J logger directly for better integration with the project's logging configuration.
public static void info(String format, Object... args) { - log("INFO", format, args); + log.info(format, args); } public static void error(String format, Object... args) { - log("ERROR", format, args); + log.error(format, args); } - private static void log(String level, String format, Object... args) { - String timestamp = java.time.LocalDateTime.now().toString(); - String message = formatMessage(format, args); - System.out.printf("%s [%s] %s%n", timestamp, level, message); - } - - private static String formatMessage(String format, Object... args) { - for (Object arg : args) { - format = format.replaceFirst("\\{\\}", arg == null ? "null" : arg.toString()); - } - return format; - }
143-148: Improve string formatting efficiency.The current implementation of
formatMessage()uses regex replacement for each argument, which can be inefficient for multiple replacements. Consider using a more efficient approach or leveraging existing libraries.If you decide to keep the custom logging:
private static String formatMessage(String format, Object... args) { - for (Object arg : args) { - format = format.replaceFirst("\\{\\}", arg == null ? "null" : arg.toString()); - } - return format; + StringBuilder result = new StringBuilder(format); + for (Object arg : args) { + int index = result.indexOf("{}"); + if (index != -1) { + result.replace(index, index + 2, arg == null ? "null" : arg.toString()); + } + } + return result.toString(); }
34-149: Missing @after method for proper cleanup.The test class is missing an
@Aftermethod to properly clean up resources after tests. Consider adding one to release resources and reset the state.+ import org.junit.After; + + ... + + @After + public void tearDown() { + // Clear any caches or connections that might be held by the wallet + if (wallet != null) { + wallet.shutdownAutosaveAndWait(); + // Add any other necessary cleanup + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.run/WalletTool CJ Mix.run.xml(1 hunks).run/WalletTool CJ Sync.run.xml(1 hunks).run/WalletTool CJ address (devnet).run.xml(1 hunks)core/src/main/java/org/bitcoinj/wallet/Wallet.java(1 hunks)core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java(1 hunks)examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: JAVA 11 OS macOS-latest Gradle
- GitHub Check: Analyze (java)
🔇 Additional comments (10)
core/src/main/java/org/bitcoinj/wallet/Wallet.java (3)
3267-3277: Good performance optimization in getTransactions method!Pre-allocating the HashSet capacity based on the combined size of all transaction pools is a nice performance improvement. This avoids expensive HashSet resizing operations when dealing with wallets that have many transactions.
3284-3291: Excellent addition of a lightweight transaction count method.This new method provides an efficient way to get just the count of transactions without creating a collection. This is more performant than calling
getTransactions().size()when only the count is needed.
3297-3311: Well-implemented transaction list retrieval method.Adding a method that returns a List instead of a Set is a useful addition, especially when order matters or when working with APIs that expect lists. The pre-allocation of the ArrayList size is a good performance practice, consistent with the improvements to
getTransactions()..run/WalletTool CJ Sync.run.xml (1)
5-5: Added debug logging capability to wallet synchronization.The addition of the
--debuglogflag will provide more detailed logging during wallet synchronization, which is helpful for troubleshooting transaction retrieval issues..run/WalletTool CJ Mix.run.xml (1)
4-5: Updated module path and optimized CoinJoin mixing parameters.The configuration has been standardized to use the common module path and the CoinJoin mixing parameters have been adjusted:
- Increased mix amount from 0.15 to 0.50
- Reduced sessions from 8 to 6
- Removed the "--multi-session" flag
- Added debug logging
These changes likely represent optimizations to the mixing process based on testing.
examples/src/main/java/org/bitcoinj/examples/RestoreFromSeedThenDump.java (3)
32-32: Added import for WalletEx to support CoinJoin functionality.This import is necessary for using the extended wallet implementation with CoinJoin support.
88-88: Updated to use WalletEx instead of Wallet.Switching to WalletEx enables enhanced wallet capabilities, particularly for CoinJoin operations, while maintaining the same initialization parameters.
94-94: Added CoinJoin initialization.The new
initializeCoinJoin(0)call properly sets up the wallet for CoinJoin functionality with account index 0, which is essential for the improved transaction handling mentioned in the PR objectives.core/src/test/java/org/bitcoinj/wallet/LargeCoinJoinWalletTest.java (2)
34-38: The test class setup looks good.The class correctly defines the necessary fields for testing a large CoinJoin wallet including the wallet instance, test parameters, and logging.
40-52:✅ Verification successful
Verify the test wallet file availability.
The test depends on a resource file
coinjoin-large.walletwhich might not be included in the repository. Ensure this file is available or provide clear documentation on how to generate it.
🏁 Script executed:
#!/bin/bash # Check if the test wallet file exists in resources echo "Checking for test wallet file..." WALLET_PATH=$(find . -name "coinjoin-large.wallet" -type f) if [ -z "$WALLET_PATH" ]; then echo "WARNING: Test wallet file 'coinjoin-large.wallet' not found in resources." echo "This test will fail without this file." else echo "Found test wallet file at: $WALLET_PATH" ls -la "$WALLET_PATH" fiLength of output: 679
Test Wallet Resource Verified
Thecoinjoin-large.walletfile is present atcore/src/test/resources/org/bitcoinj/wallet/coinjoin-large.wallet, so the setup inLargeCoinJoinWalletTestwill succeed as expected.
| @Test | ||
| public void coinJoinInfoTest() { | ||
| info("rounds: {}", wallet.coinjoin.rounds); | ||
|
|
||
| Stopwatch watch0 = Stopwatch.createStarted(); | ||
| info("key usage: {}; time: {}", wallet.coinjoin.getKeyUsage(), watch0); | ||
| Stopwatch watch1 = Stopwatch.createStarted(); | ||
| wallet.coinjoin.refreshUnusedKeys(); | ||
| info("unused key count: {}; time: {}", wallet.coinjoin.getUnusedKeyCount(), watch1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to validate CoinJoin information.
This test method executes operations but doesn't assert that the results meet any expected criteria. Without assertions, it functions more as a performance benchmark than a validation test.
@Test
public void coinJoinInfoTest() {
info("rounds: {}", wallet.coinjoin.rounds);
Stopwatch watch0 = Stopwatch.createStarted();
- info("key usage: {}; time: {}", wallet.coinjoin.getKeyUsage(), watch0);
+ int keyUsage = wallet.coinjoin.getKeyUsage();
+ info("key usage: {}; time: {}", keyUsage, watch0);
+ // Assert expected key usage values
+ assertTrue("Key usage should be positive", keyUsage > 0);
Stopwatch watch1 = Stopwatch.createStarted();
wallet.coinjoin.refreshUnusedKeys();
- info("unused key count: {}; time: {}", wallet.coinjoin.getUnusedKeyCount(), watch1);
+ int unusedKeyCount = wallet.coinjoin.getUnusedKeyCount();
+ info("unused key count: {}; time: {}", unusedKeyCount, watch1);
+ // Assert expected unused key count
+ assertTrue("Unused key count should be positive after refresh", unusedKeyCount > 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void coinJoinInfoTest() { | |
| info("rounds: {}", wallet.coinjoin.rounds); | |
| Stopwatch watch0 = Stopwatch.createStarted(); | |
| info("key usage: {}; time: {}", wallet.coinjoin.getKeyUsage(), watch0); | |
| Stopwatch watch1 = Stopwatch.createStarted(); | |
| wallet.coinjoin.refreshUnusedKeys(); | |
| info("unused key count: {}; time: {}", wallet.coinjoin.getUnusedKeyCount(), watch1); | |
| } | |
| @Test | |
| public void coinJoinInfoTest() { | |
| info("rounds: {}", wallet.coinjoin.rounds); | |
| Stopwatch watch0 = Stopwatch.createStarted(); | |
| int keyUsage = wallet.coinjoin.getKeyUsage(); | |
| info("key usage: {}; time: {}", keyUsage, watch0); | |
| // Assert expected key usage values | |
| assertTrue("Key usage should be positive", keyUsage > 0); | |
| Stopwatch watch1 = Stopwatch.createStarted(); | |
| wallet.coinjoin.refreshUnusedKeys(); | |
| int unusedKeyCount = wallet.coinjoin.getUnusedKeyCount(); | |
| info("unused key count: {}; time: {}", unusedKeyCount, watch1); | |
| // Assert expected unused key count | |
| assertTrue("Unused key count should be positive after refresh", unusedKeyCount > 0); | |
| } |
| @Test | ||
| public void transactionReportTest() { | ||
| Stopwatch watch0 = Stopwatch.createStarted(); | ||
| wallet.getTransactionReport(); | ||
| info("getTransactionReport: {}", watch0); | ||
|
|
||
| Stopwatch watch1 = Stopwatch.createStarted(); | ||
| wallet.getTransactionReport(); | ||
| info("getTransactionReport: {}", watch1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for transaction report test.
This test runs the transaction report twice but doesn't validate the output. Consider adding assertions to verify the report's correctness or at least checking for non-null results.
@Test
public void transactionReportTest() {
Stopwatch watch0 = Stopwatch.createStarted();
- wallet.getTransactionReport();
+ String report1 = wallet.getTransactionReport();
info("getTransactionReport: {}", watch0);
+
+ // Validate that the report is properly generated
+ assertNotNull("Transaction report should not be null", report1);
+ assertFalse("Transaction report should not be empty", report1.isEmpty());
Stopwatch watch1 = Stopwatch.createStarted();
- wallet.getTransactionReport();
+ String report2 = wallet.getTransactionReport();
info("getTransactionReport: {}", watch1);
+
+ // Ensure the reports are consistent
+ assertEquals("Transaction reports should be consistent", report1, report2);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void transactionReportTest() { | |
| Stopwatch watch0 = Stopwatch.createStarted(); | |
| wallet.getTransactionReport(); | |
| info("getTransactionReport: {}", watch0); | |
| Stopwatch watch1 = Stopwatch.createStarted(); | |
| wallet.getTransactionReport(); | |
| info("getTransactionReport: {}", watch1); | |
| } | |
| @Test | |
| public void transactionReportTest() { | |
| Stopwatch watch0 = Stopwatch.createStarted(); | |
| String report1 = wallet.getTransactionReport(); | |
| info("getTransactionReport: {}", watch0); | |
| // Validate that the report is properly generated | |
| assertNotNull("Transaction report should not be null", report1); | |
| assertFalse("Transaction report should not be empty", report1.isEmpty()); | |
| Stopwatch watch1 = Stopwatch.createStarted(); | |
| String report2 = wallet.getTransactionReport(); | |
| info("getTransactionReport: {}", watch1); | |
| // Ensure the reports are consistent | |
| assertEquals("Transaction reports should be consistent", report1, report2); | |
| } |
| @Test | ||
| public void getTransactionsTest() { | ||
| Stopwatch watch0 = Stopwatch.createStarted(); | ||
| wallet.getTransactionCount(true); | ||
| info("wallet.getTransactionCount: {}", watch0); | ||
|
|
||
| Stopwatch watch1 = Stopwatch.createStarted(); | ||
| wallet.getTransactions(true); | ||
| info("wallet.getTransactions: {}", watch1); | ||
|
|
||
| Stopwatch watch2 = Stopwatch.createStarted(); | ||
| wallet.getTransactionList(true); | ||
| info("wallet.getTransactionList: {}", watch2); | ||
|
|
||
| Stopwatch watch3 = Stopwatch.createStarted(); | ||
| wallet.getWalletTransactions(); | ||
| info("wallet.getWalletTransactions(): {}", watch3); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions to validate transaction retrieval methods.
This method tests the new transaction retrieval methods but doesn't verify their correctness. Add assertions to ensure the methods return the expected results.
@Test
public void getTransactionsTest() {
Stopwatch watch0 = Stopwatch.createStarted();
- wallet.getTransactionCount(true);
+ int transactionCount = wallet.getTransactionCount(true);
info("wallet.getTransactionCount: {}", watch0);
+ assertTrue("Transaction count should be positive", transactionCount > 0);
Stopwatch watch1 = Stopwatch.createStarted();
- wallet.getTransactions(true);
+ Set<Transaction> transactions = wallet.getTransactions(true);
info("wallet.getTransactions: {}", watch1);
+ assertNotNull("Transactions set should not be null", transactions);
+ assertEquals("Transaction count should match set size", transactionCount, transactions.size());
Stopwatch watch2 = Stopwatch.createStarted();
- wallet.getTransactionList(true);
+ List<Transaction> transactionList = wallet.getTransactionList(true);
info("wallet.getTransactionList: {}", watch2);
+ assertNotNull("Transaction list should not be null", transactionList);
+ assertEquals("Transaction list size should match count", transactionCount, transactionList.size());
Stopwatch watch3 = Stopwatch.createStarted();
- wallet.getWalletTransactions();
+ Iterable<WalletTransaction> walletTransactions = wallet.getWalletTransactions();
info("wallet.getWalletTransactions(): {}", watch3);
+ assertNotNull("Wallet transactions should not be null", walletTransactions);
+
+ // Count the wallet transactions
+ int walletTransactionCount = 0;
+ for (WalletTransaction ignored : walletTransactions) {
+ walletTransactionCount++;
+ }
+ assertTrue("Wallet should contain transactions", walletTransactionCount > 0);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| public void getTransactionsTest() { | |
| Stopwatch watch0 = Stopwatch.createStarted(); | |
| wallet.getTransactionCount(true); | |
| info("wallet.getTransactionCount: {}", watch0); | |
| Stopwatch watch1 = Stopwatch.createStarted(); | |
| wallet.getTransactions(true); | |
| info("wallet.getTransactions: {}", watch1); | |
| Stopwatch watch2 = Stopwatch.createStarted(); | |
| wallet.getTransactionList(true); | |
| info("wallet.getTransactionList: {}", watch2); | |
| Stopwatch watch3 = Stopwatch.createStarted(); | |
| wallet.getWalletTransactions(); | |
| info("wallet.getWalletTransactions(): {}", watch3); | |
| } | |
| @Test | |
| public void getTransactionsTest() { | |
| Stopwatch watch0 = Stopwatch.createStarted(); | |
| int transactionCount = wallet.getTransactionCount(true); | |
| info("wallet.getTransactionCount: {}", watch0); | |
| assertTrue("Transaction count should be positive", transactionCount > 0); | |
| Stopwatch watch1 = Stopwatch.createStarted(); | |
| Set<Transaction> transactions = wallet.getTransactions(true); | |
| info("wallet.getTransactions: {}", watch1); | |
| assertNotNull("Transactions set should not be null", transactions); | |
| assertEquals("Transaction count should match set size", transactionCount, transactions.size()); | |
| Stopwatch watch2 = Stopwatch.createStarted(); | |
| List<Transaction> transactionList = wallet.getTransactionList(true); | |
| info("wallet.getTransactionList: {}", watch2); | |
| assertNotNull("Transaction list should not be null", transactionList); | |
| assertEquals("Transaction list size should match count", transactionCount, transactionList.size()); | |
| Stopwatch watch3 = Stopwatch.createStarted(); | |
| Iterable<WalletTransaction> walletTransactions = wallet.getWalletTransactions(); | |
| info("wallet.getWalletTransactions(): {}", watch3); | |
| assertNotNull("Wallet transactions should not be null", walletTransactions); | |
| // Count the wallet transactions | |
| int walletTransactionCount = 0; | |
| for (WalletTransaction ignored : walletTransactions) { | |
| walletTransactionCount++; | |
| } | |
| assertTrue("Wallet should contain transactions", walletTransactionCount > 0); | |
| } |
| @Test | ||
| Stopwatch watch0 = Stopwatch.createStarted(); | ||
| wallet.toString(true, false, true, null); | ||
| info("wallet.toString: {}", watch0); | ||
|
|
||
| Stopwatch watch1 = Stopwatch.createStarted(); | ||
| wallet.toString(true, false, true, null); | ||
| info("wallet.toString: {}", watch1); | ||
|
|
||
| Stopwatch watch2 = Stopwatch.createStarted(); | ||
| wallet.coinjoin.toString(); | ||
| info("wallet.coinjoin.toString: {}", watch2); | ||
|
|
||
| Stopwatch watch3 = Stopwatch.createStarted(); | ||
| wallet.coinjoin.toString(true, false, null); | ||
| info("wallet.coinjoin.toString(true, false, null): {}", watch3); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation to the toString test method.
Similar to the transaction report test, this method exercises various toString methods but doesn't validate their output. Consider adding basic assertions to ensure the methods return meaningful results.
@Test
public void walletToStringTest() {
Stopwatch watch0 = Stopwatch.createStarted();
- wallet.toString(true, false, true, null);
+ String walletStr1 = wallet.toString(true, false, true, null);
info("wallet.toString: {}", watch0);
+ assertNotNull("Wallet toString should not be null", walletStr1);
+ assertFalse("Wallet toString should not be empty", walletStr1.isEmpty());
Stopwatch watch1 = Stopwatch.createStarted();
- wallet.toString(true, false, true, null);
+ String walletStr2 = wallet.toString(true, false, true, null);
info("wallet.toString: {}", watch1);
+ assertEquals("Wallet toString should be consistent", walletStr1, walletStr2);
Stopwatch watch2 = Stopwatch.createStarted();
- wallet.coinjoin.toString();
+ String coinjoinStr = wallet.coinjoin.toString();
info("wallet.coinjoin.toString: {}", watch2);
+ assertNotNull("CoinJoin toString should not be null", coinjoinStr);
+ assertFalse("CoinJoin toString should not be empty", coinjoinStr.isEmpty());
Stopwatch watch3 = Stopwatch.createStarted();
- wallet.coinjoin.toString(true, false, null);
+ String coinjoinDetailedStr = wallet.coinjoin.toString(true, false, null);
info("wallet.coinjoin.toString(true, false, null): {}", watch3);
+ assertNotNull("CoinJoin detailed toString should not be null", coinjoinDetailedStr);
+ assertFalse("CoinJoin detailed toString should not be empty", coinjoinDetailedStr.isEmpty());
}
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
Improvements
Configuration
Examples