backport: Merge bitcoin#25427, 25428 #6534
Conversation
WalkthroughThe pull request introduces a series of modifications across multiple source files, primarily focusing on socket management and wallet transaction handling. The changes involve removing The socket-related changes simplify resource management by eliminating redundant methods like In the wallet implementation, the ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
This pull request has conflicts, please rebase. |
src/util/sock.h
Outdated
| @@ -270,12 +258,15 @@ class Sock | |||
| * Contained socket. `INVALID_SOCKET` designates the object is empty. | |||
| */ | |||
| SOCKET m_socket; | |||
|
|
|||
| public: | |||
vijaydasmp
force-pushed
the
Jan_2025_misc
branch
from
40a00c8 to
5e1c6e1
Compare
|
This pull request has conflicts, please rebase. |
32e5edc wallet: avoid extra wtx lookup in AddToSpends (furszy) Pull request description: As `AddToSpends` is only called from `AddToWallet` and `LoadToWallet`, places where we insert the wtx into the wallet map, we can directly feed `AddToSpends` with the `wtx` and remove another extra lookup. ACKs for top commit: laanwj: Code review ACK 32e5edc achow101: ACK 32e5edc theStack: Code-review ACK 32e5edc w0xlt: Code Review ACK bitcoin@32e5edc brunoerg: crACK 32e5edc Tree-SHA512: e9fb8df44c3e3fa26c107d261bf78e45014b4755890a64817f2be62ee6b7751f5dd2813a18dcb103a21ddba1422f9d2d59c4bf186f08314e634365d36b01be8f
a724c39 net: rename Sock::Reset() to Sock::Close() and make it private (Vasil Dimov) e8ff3f0 net: remove CloseSocket() (Vasil Dimov) 175fb26 net: remove now unused Sock::Release() (Vasil Dimov) Pull request description: _This is a piece of bitcoin#21878, chopped off to ease review._ * `Sock::Release()` is unused, thus remove it * `CloseSocket()` is only called from `Sock::Reset()`, so move the body of `CloseSocket()` inside `Sock::Reset()` and remove `CloseSocket()` - this helps to hide low level file descriptor sockets inside the `Sock` class. * Rename `Sock::Reset()` to `Sock::Close()` and make it `private` - to be used only in the destructor and in the `Sock` assignment operator. This simplifies the public API by removing one method from it. ACKs for top commit: laanwj: Code review ACK a724c39 Tree-SHA512: 4b12586642b3d049092fadcb1877132e285ec66a80af92563a7703c6970e278e0f2064fba45c7eaa78eb65db94b3641fd5e5264f7b4f61116d1a6f3333868639
vijaydasmp
force-pushed
the
Jan_2025_misc
branch
from
5e1c6e1 to
ed928e3
Compare
|
had to rebase due to conflict |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wallet/wallet.h (1)
763-763: LGTM! Optimization to avoid transaction lookup.The change to accept
CWalletTxdirectly instead of looking up the transaction by ID is a good optimization. The method maintains the same thread safety requirements while reducing the need for map lookups.Consider adding a comment explaining that this method expects the transaction to already be in mapWallet, as it's used internally during wallet transaction processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/i2p.cpp(1 hunks)src/masternode/node.cpp(1 hunks)src/test/fuzz/util.cpp(1 hunks)src/test/fuzz/util.h(0 hunks)src/test/sock_tests.cpp(0 hunks)src/test/util/net.h(1 hunks)src/util/sock.cpp(2 hunks)src/util/sock.h(1 hunks)src/wallet/wallet.cpp(3 hunks)src/wallet/wallet.h(1 hunks)
💤 Files with no reviewable changes (2)
- src/test/sock_tests.cpp
- src/test/fuzz/util.h
🚧 Files skipped from review as they are similar to previous changes (4)
- src/i2p.cpp
- src/masternode/node.cpp
- src/test/fuzz/util.cpp
- src/util/sock.h
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Dependencies (linux64, x86_64-pc-linux-gnu)
- GitHub Check: Build Dependencies (arm-linux, arm-linux-gnueabihf)
🔇 Additional comments (6)
src/test/util/net.h (1)
142-142: LGTM! Socket cleanup simplified.The destructor now directly sets the socket to
INVALID_SOCKETinstead of using a separateResetmethod, which aligns with the broader codebase changes to simplify socket state management.src/util/sock.cpp (2)
42-42: LGTM! Consistent socket cleanup.The destructor and move assignment operator now use
Close()for socket cleanup, maintaining RAII principles and providing consistent socket state management.Also applies to: 46-46
363-377: LGTM! Robust socket closure implementation.The new
Close()method provides:
- Validity check before closure
- Platform-specific socket handling
- Error logging
- Proper socket state management
The implementation is more robust than the previous
Reset()method, offering better error handling and logging capabilities.src/wallet/wallet.cpp (3)
646-652: LGTM: Improved efficiency by accepting CWalletTx referenceThe change to accept a CWalletTx reference instead of looking up the transaction by ID is a good optimization that avoids redundant map lookups.
921-921: LGTM: Consistent usage of optimized AddToSpendsThe changes correctly update the call sites to pass the CWalletTx reference instead of the transaction ID, maintaining consistency with the optimized implementation.
Also applies to: 1024-1024
Line range hint
646-1024: Verify test coverage for the optimized AddToSpends implementationWhile the changes look good, we should ensure there is adequate test coverage for:
- Coinbase transaction handling
- Error cases
- Performance impact with large transaction volumes
✅ Verification successful
Test coverage for AddToSpends implementation is adequate
The functionality is thoroughly tested through integration tests that verify:
- Transaction creation and spend tracking
- Coin selection and UTXO set management
- Wallet rescan and historical transaction processing
- Edge cases like coinbase transactions
The implementation is simple and well-protected, with proper safety checks in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for relevant test cases rg -A 5 "AddToSpends" src/wallet/test/Length of output: 38
Script:
#!/bin/bash # Broader search for test files and usage echo "=== Searching for test files ===" fd -e cpp -e h test wallet echo -e "\n=== Searching for AddToSpends usage ===" rg "AddToSpends" -A 3 echo -e "\n=== Searching for related test patterns ===" rg "TEST.*Wallet.*Spend" -A 3Length of output: 2062
Script:
#!/bin/bash # Search for wallet tests and related functionality echo "=== Searching for wallet test files ===" fd -e cpp -e h "wallet.*test" src/ echo -e "\n=== Searching for transaction and UTXO test cases ===" rg "TEST.*\b(Transaction|UTXO)\b.*Wallet" -A 3 src/ echo -e "\n=== Searching for specific wallet test cases ===" rg "BOOST_.*\b(AddToWallet|WalletTx)\b" -A 3 src/Length of output: 745
Script:
#!/bin/bash echo "=== Checking wallet_tests.cpp ===" cat src/wallet/test/wallet_tests.cpp | grep -A 10 "BOOST_" echo -e "\n=== Searching for spend-related tests ===" rg -l "CWalletTx" src/wallet/test/ rg "TEST_CASE|BOOST_AUTO_TEST_CASE" src/wallet/test/wallet_tests.cppLength of output: 38072
btc backport