backport: bitcoin#25383, 25535, 25337 #6524
Conversation
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
Jan_2025_04
branch
from
d82b8b5 to
d82edfc
Compare
vijaydasmp
force-pushed
the
Jan_2025_04
branch
3 times, most recently
from
6d99da7 to
9afdab1
Compare
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
Jan_2025_04
branch
from
9afdab1 to
f1c649f
Compare
vijaydasmp
force-pushed
the
Jan_2025_04
branch
from
f1c649f to
3be5fd7
Compare
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
Jan_2025_04
branch
6 times, most recently
from
007a285 to
f246628
Compare
vijaydasmp
changed the title
…letBatch' is created c318211 walletdb: fix last client version update (furszy) bda8ebe wallet: don't read db every time that a new WalletBatch is created (furszy) Pull request description: Found it while was working on bitcoin#25297. We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not. As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached. ACKs for top commit: achow101: ACK c318211 w0xlt: reACK bitcoin@c318211 Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
WalkthroughThe changes update several aspects of wallet functionality and its testing. In wallet interfaces, the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 Cppcheck (2.10-2)src/wallet/interfaces.cpp[error] 225-225: Syntax Error (internalAstError) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (25)
✨ Finishing Touches
🪧 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 (
|
![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 (2)
src/wallet/wallet.cpp (2)
2903-2910: Add documentation about concurrencyThis method relies on the wallet lock being held but does not explicitly mention it in the function signature or comments. Consider clarifying that the caller must hold
cs_walletto avoid confusion.
2927-2940: Clarify handling of empty “purpose” argumentWhen the “purpose” parameter is empty, the logic adds all non-change labels to the returned set. Consider documenting this fallback scenario or validating that it matches the intended design for filtering address labels by purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/interfaces/wallet.h(1 hunks)src/wallet/bdb.cpp(0 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(6 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)src/wallet/walletdb.cpp(2 hunks)test/functional/feature_dbcrash.py(2 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/wallet/bdb.cpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
🔇 Additional comments (20)
src/interfaces/wallet.h (1)
137-137: Excellent improvement for const-correctness.Adding the
constqualifier to thegetAddresses()method properly indicates that it doesn't modify the wallet's state. This follows C++ best practices and enables calling the method on const instances of theWalletinterface.test/functional/wallet_listreceivedby.py (1)
65-68: Good test enhancement for address classification.This addition improves test coverage by validating that no addresses returned by
listreceivedbyaddress()are change addresses. This helps ensure that the address filtering logic properly distinguishes between receiving addresses and change addresses, which is crucial for accurate wallet display and accounting.test/functional/wallet_basic.py (1)
30-30: Improved test configuration with more specific parameter.Replacing
-acceptnonstdtxn=1with-dustrelayfee=0is a good refinement that:
- Uses a more targeted approach to handling small-value outputs
- Maintains other transaction standardness rules while specifically addressing dust handling
- Provides better alignment with how small outputs are handled in production environments
This change makes the tests more precise without broadly bypassing all standardness checks.
Also applies to: 34-34
src/wallet/walletdb.cpp (2)
883-885: Enhanced wallet version handling.The addition of the
has_last_clientvariable properly tracks whether the last client version was successfully read from the database. This improves logging clarity by showing both the wallet version and client version in a single message.
907-907: More robust version update logic.The revised condition
!has_last_client || last_client != CLIENT_VERSIONensures the version is properly updated both when it's missing from the database and when it differs from the current client version. This provides better handling for new wallets and migration scenarios.test/functional/feature_dbcrash.py (3)
65-66: Improved documentation clarity for node configurationThe comment has been updated to better clarify that node3 will handle "dust" outputs, which aligns with the parameter change below.
66-66: Configuration parameter update from-acceptnonstdtxnto-dustrelayfee=0The change replaces the older non-standard transaction acceptance flag with a more specific dust relay fee setting. This provides a more targeted approach for testing with small value outputs by explicitly setting a zero dust threshold rather than broadly accepting non-standard transactions.
220-224: Improved UTXO creation strategy and verificationThe UTXO creation process has been refactored from a single large batch of 5000 UTXOs to 5 smaller batches of 1000 each. This approach:
- Keeps the same total number of UTXOs (5000)
- Provides more granular control over the creation process
- Adds a verification step to ensure the mempool is empty after UTXO generation
This change improves test robustness by breaking the work into smaller chunks and adding explicit verification.
src/wallet/wallet.h (3)
797-802: Added a flexible filtering structure for address book operationsThe new
AddrBookFilterstructure provides a clean way to filter address book entries by label and whether to include change addresses. The default behavior (ignoring change addresses) is sensible as change addresses are typically not relevant for most user-facing operations.
804-813: Added flexible method for retrieving filtered address book dataThe
ListAddrBookAddressesmethod enables more flexible retrieval of address data by accepting an optional filter parameter. This is a good improvement over the removedGetLabelAddressesmethod as it provides more powerful filtering capabilities.
814-819: Added utility methods for address book operationsThe new methods
ListAddrBookLabelsandForEachAddrBookEntryprovide a clean interface for:
- Retrieving all labels for a specific purpose
- Iterating through address book entries with a callback function
These additions follow good programming practices by:
- Using a callback pattern for iteration that avoids exposing internal data structures
- Providing specific functionality for common address book operations
- Making the code more maintainable and easier to understand
src/wallet/interfaces.cpp (2)
208-219: Updated address lookup to use the new FindAddressBookEntry methodThe implementation of
getAddresshas been simplified by usingFindAddressBookEntryinstead of manually checking the address book. This change improves code maintainability by:
- Reducing duplicate code
- Centralizing the logic for finding address book entries
- Using a consistent approach for address book access across the codebase
221-230: Added const qualifier and improved address retrieval implementationTwo key improvements in this change:
- The
getAddressesmethod is now marked asconst, correctly indicating that it doesn't modify the wallet state- The implementation now uses the new
ForEachAddrBookEntrycallback method instead of direct iterationThis implementation:
- Is more concise and easier to understand
- Properly filters out change addresses
- Uses a consistent pattern for address book access
- Correctly constructs the result vector in a single operation
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.cpp (1)
2911-2926: Verify behavior for empty labelsThe filter logic correctly handles both ignore-change and optional label matching. However, if the label filter is set but the address has an empty label, the function will skip it. Verify if this is intended or if an empty label should be handled differently (e.g., treat empty label as a valid match).
src/wallet/rpcwallet.cpp (6)
543-543: Data structure change from set to vectorThis change from
std::set<CTxDestination>tostd::vector<CTxDestination>alters the fundamental container type for addresses. While vectors offer better cache locality, be aware that checking for address existence now has O(n) complexity instead of O(log n) that sets provide.
548-548: API migration to more flexible address filteringThe code now uses the new
ListAddrBookAddressesmethod withAddrBookFilterinstead of the olderGetLabelAddresses. This is part of a broader refactoring to provide more versatile address book querying capabilities.
559-559: Switched from set insertion to vector appendChanged from
address_set.insert(dest)toaddress_vector.emplace_back(dest), consistent with the container type change. Note thatemplace_backis generally more efficient thanpush_backfor complex objects as it constructs the element in-place.
579-579: Modified address existence checkThe address lookup has changed from the efficient
address_set.count(address)to the linear searchstd::find(address_vector.begin(), address_vector.end(), address) != address_vector.end(). This maintains the same functionality but may be less efficient for large address collections.
3949-3966: Refactored label-based address retrieval using ForEachAddrBookEntryThe implementation of
getaddressesbylabelhas been updated to use a lambda with the newForEachAddrBookEntrymethod, replacing a direct loop over address book entries. This approach is more maintainable as it centralizes address book iteration logic and simplifies the calling code.The lambda function now directly constructs and adds the value object to the results instead of using the removed
AddressBookDataToJSONfunction.
4013-4013: Updated label retrieval to use new APIThis line now uses
pwallet->ListAddrBookLabels(purpose)rather than directly iterating through the address book entries, which is consistent with the address book API modernization.
test/functional/feature_dbcrash.py
Outdated
| self.generate(self.nodes[3], 1, sync_fun=self.no_op) | ||
| assert_equal(len(self.nodes[3].getrawmempool()), 0) |
There was a problem hiding this comment.
22535: unrelated changes
| self.generate(self.nodes[3], 1, sync_fun=self.no_op) | |
| assert_equal(len(self.nodes[3].getrawmempool()), 0) |
| @@ -540,12 +540,12 @@ static RPCHelpMan listaddressbalances() | |||
|
|
|||
| static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) | |||
| { | |||
| std::set<CTxDestination> address_set; | |||
| std::vector<CTxDestination> address_vector; | |||
There was a problem hiding this comment.
25337: missing changes in UniValue ListReceived()
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/wallet/interfaces.cpp (1)
217-218: Direct access to entry fieldsWhile
GetLabel()is used for the label field, direct access is used for the purpose field. Consider creating a getter method for purpose as well for consistency.src/wallet/rpcwallet.cpp (4)
543-543: Consider potential performance implications of using a vector instead of a set.
If the size ofaddress_vectorgrows large, membership checks withstd::findoperate in O(n) time, which might degrade performance compared to a set-based approach.
579-579: Watch out for repeated linear searches onaddress_vector.
Usingstd::findevery time could introduce inefficiency if the vector grows large.
1000-1001: Lambda captures everything by reference; confirm no unintended captures.
Capturing[&]is convenient but requires careful maintenance if more external variables are used inside the lambda.
1041-1047: Consider extracting this filtered-address logic into its own function.
The nested if-else logic from lines 1041 to 1047 could be refactored for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/interfaces/wallet.h(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(9 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/feature_dbcrash.py(2 hunks)test/functional/wallet_basic.py(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/interfaces/wallet.h
- test/functional/wallet_basic.py
- test/functional/wallet_listreceivedby.py
- test/functional/feature_dbcrash.py
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (14)
src/wallet/interfaces.cpp (3)
208-209: Clean implementation of address lookupThe switch to using
FindAddressBookEntrywith explicitallow_change=falseparameter simplifies the code and makes the intent clearer.
211-212: Usage of accessor method for labelGood use of the
GetLabel()accessor method rather than directly accessing the label field.
221-229: Improved implementation using ForEachAddrBookEntryThe refactoring makes good use of the new
ForEachAddrBookEntrymethod to iterate over address book entries. The lambda function properly filters out change addresses and creates the result vector with all necessary information. The method is correctly marked asconstto indicate it doesn't modify the wallet state.The code properly declares exclusive lock requirements in the lambda to ensure proper concurrent access handling.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.h (3)
797-802: Well-designed filter structureThe
AddrBookFilterstruct is well-designed with appropriate use ofstd::optionalfor the label and sensible defaults. The comments clearly explain the purpose of each field.
804-807: Flexible address book filtering methodThe
ListAddrBookAddressesmethod provides a flexible way to filter address book entries with the optional filter parameter. It's correctly marked asconstand requires the wallet lock.
809-812: Clear method for retrieving address labelsThe
ListAddrBookLabelsmethod provides a clean way to retrieve unique labels associated with a specific purpose. Using astd::setfor the return type appropriately ensures uniqueness.src/wallet/wallet.cpp (3)
2903-2909: Well-structured iteration over address book entries.This helper method cleanly encapsulates the iteration logic, allowing the caller to easily process each address book entry. The read-only usage (function is const) is safe, and there are no obvious correctness or performance issues.
2911-2923: Parameter-driven address filtering is effective.The use of an optional filter object and a lambda to exclude change addresses or addresses not matching a specified label is straightforward and efficient. The
AssertLockHeld(cs_wallet)precondition ensures thread safety for reading the address book. No issues are evident.
2927-2939: Appropriate label retrieval functionality.This method correctly gathers distinct labels, ignoring change entries if specified, and matches a requested purpose if provided. Lock acquisition (
AssertLockHeld(cs_wallet)) is properly enforced. Implementation appears correct with no security or performance concerns.src/wallet/rpcwallet.cpp (5)
548-548: Verify correctness of the newListAddrBookAddressesusage.
Ensure all call sites that previously expected addresses fromGetLabelAddressescorrectly handle the list returned byListAddrBookAddresses.
559-559: Emplacing single destination into the vector looks correct.
No immediate issues spotted; consistent with the shift to a vector container.
954-954: Usage ofstd::optional<CTxDestination>is clear and self-explanatory.
This approach provides a robust way to handle the presence or absence of a filtered address.
1004-1004: Early return looks correct.
Prevents unnecessary processing when there is no matching tally entry andfIncludeEmptyis false.
1050-1051: Logic for label-based tally is straightforward.
This block nicely handles the aggregation of amounts by label.
| /** | ||
| * Walk-through the address book entries. | ||
| * Stops when the provided 'ListAddrBookFunc' returns false. | ||
| */ | ||
| using ListAddrBookFunc = std::function<void(const CTxDestination& dest, const std::string& label, const std::string& purpose, bool is_change)>; | ||
| void ForEachAddrBookEntry(const ListAddrBookFunc& func) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential inconsistency in ForEachAddrBookEntry documentation
The documentation comment states that iteration "Stops when the provided 'ListAddrBookFunc' returns false", but the function type is defined with a void return type, which can't return a boolean value to control iteration.
Verify that either:
- The callback function should return a boolean (in which case the function type should be changed), or
- The documentation should be updated to match the actual behavior
🏁 Script executed:
#!/bin/bash
# Check the implementation of ForEachAddrBookEntry to see if it expects a boolean return
grep -A 15 "void CWallet::ForEachAddrBookEntry" src/wallet/wallet.cppLength of output: 840
Update documentation for ForEachAddrBookEntry to correctly reflect its behavior
There is an inconsistency: the documentation claims that iteration stops when the callback returns false, yet the callback type is defined as void and the implementation iterates over all entries without checking any return value. Please update the comment to indicate that the address book is fully iterated without an early termination condition.
- Location:
src/wallet/wallet.h(Lines 814–819) - Action: Revise the documentation comment to remove any reference to stopping on a false return value.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/wallet/wallet.h (1)
814-819:⚠️ Potential issueUpdate documentation for ForEachAddrBookEntry
There is an inconsistency in the documentation comment. It states that iteration "Stops when the provided 'ListAddrBookFunc' returns false", but the function type is defined with a
voidreturn type, which can't return a boolean to control iteration.Please update the documentation to accurately reflect the behavior of the function. Either:
- Change the comment to indicate that all entries are always processed, or
- Modify the
ListAddrBookFunctype to return a boolean if early termination is desired
🧹 Nitpick comments (3)
src/wallet/wallet.cpp (1)
2903-2909: Consider asserting the wallet lock for consistency.
The function iterates overm_address_book, yet it does not explicitly assertcs_walletis held. Other similar methods (ListAddrBookAddresses,ListAddrBookLabels) do so. To maintain consistency and clarity about concurrency requirements, consider adding the lock assertion here as well.Here is a possible change:
void CWallet::ForEachAddrBookEntry(const ListAddrBookFunc& func) const +{ + AssertLockHeld(cs_wallet); for (const auto& item : m_address_book) { ... } }src/wallet/rpcwallet.cpp (2)
543-559: Consider potential performance trade-offs when using a vector instead of a set.These lines replace the previous set-based structure with a vector (
address_vector) for storing addresses. While vectors can be more cache-friendly and typically faster for small collections, lookups viastd::findare O(n), which may be a concern for larger address lists. If lookups are expected to happen frequently on a large collection, consider using a suitable set-like container or caching lookups to maintain efficiency.
1000-1008: Consider renaming the lambda for clarity.The introduced lambda function named
funcis correct, but giving it a more descriptive name (e.g.,processAddressBookEntry) would improve readability.- const auto& func = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) { + const auto& processAddressBookEntry = [&](const CTxDestination& address, const std::string& label, const std::string& purpose, bool is_change) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/interfaces/wallet.h(1 hunks)src/wallet/interfaces.cpp(1 hunks)src/wallet/rpcwallet.cpp(9 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(1 hunks)test/functional/wallet_listreceivedby.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/interfaces/wallet.h
- test/functional/wallet_listreceivedby.py
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/wallet/interfaces.cpp
[error] 225-225: Syntax Error
(internalAstError)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (14)
src/wallet/interfaces.cpp (3)
208-210: Improved address lookup withFindAddressBookEntryThe change from direct map access to the
FindAddressBookEntrymethod provides better encapsulation and explicit handling of change addresses. This approach is more robust and maintainable.
211-218: Updated accessor pattern for address book entriesThe switch from
it->second.nametoentry->GetLabel()aligns with the new address book structure. The method now properly handles the updated data structure while maintaining the same behavior.
221-228: Enhanced implementation withconstqualifier and callback patternThe addition of the
constqualifier correctly indicates this method doesn't modify wallet state. The implementation is improved by replacing direct iteration with theForEachAddrBookEntrycallback pattern, which provides better encapsulation and cleaner code.Note: The cppcheck error on line 225 appears to be a false positive related to the
EXCLUSIVE_LOCKS_REQUIREDthread safety annotation.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 225-225: Syntax Error
(internalAstError)
src/wallet/wallet.cpp (2)
2911-2925: Implementation looks correct.
This function properly locks the wallet (AssertLockHeld(cs_wallet)) and usesForEachAddrBookEntrywith a concise lambda to filter addresses by change status and optional label. The design is straightforward and appears robust.
2927-2939: Straightforward label filtering.
The method is clear, ignoring change addresses and collecting labels that match the specified purpose. This approach is consistent with the rest of the address-book-related methods.src/wallet/wallet.h (3)
796-802: Clean and well-structured AddrBookFilter structGood addition of a filtering structure with sensible defaults. The optional label filter and the change address exclusion flag provide flexible querying capabilities for address book entries.
804-808: Good API design for ListAddrBookAddressesThis method introduces a more flexible way of retrieving address book entries with filtering capabilities. This is a good enhancement over the removed
GetLabelAddressesmethod, allowing for more powerful queries.
809-813: Nicely implemented ListAddrBookLabels methodThis method is well-designed to retrieve a set of unique labels for a given purpose. Using a
std::setis the appropriate data structure for maintaining uniqueness of labels.src/wallet/rpcwallet.cpp (6)
954-959: LGTM: Address filtering logic is well-structured.The conditional checks (
!by_label && params.size() > 4) and the subsequent decoding ofparams[4]ensure safe usage. The new filtering approach for a singlefiltered_addressis straightforward and correct, and invalid addresses are robustly handled. No further changes required.
974-985: Clear conditional checks for filtered addresses and ownership.This loop properly extracts the destination, checks it against the optional
filtered_address, and verifies wallet ownership via(mine & filter). The flow guards against invalid or irrelevant outputs and should behave correctly in both the filtered and unfiltered scenarios.
1013-1013: Logic for tallying known addresses is clear.The early check on
it != mapTally.end()prevents erroneous lookups, and the subsequentelsebranch handles addresses not already added to the tally. The flow remains consistent, with no obvious logical or correctness issues.Also applies to: 1024-1024
1032-1034: Good addition of transaction IDs to the response.Collecting and pushing back the
txidsinto the JSON array provides clear traceability of transactions associated with each address. Implementation looks correct and should not introduce performance concerns.
1040-1040: No issues with lambda closure.This is simply the end brace for the lambda, and the closure is syntactically correct.
1042-1052: Well-handled branching for optional address filtering.By either processing a single
filtered_addressor iterating over the entire address book, the code comprehensively covers both scenarios. The subsequent logic referencinglabel_tallyandby_labelis straightforward and consistent with the rest of the function.
…ng dust (instead of `acceptnonstdtxn=1`) 1770be7 test: pass `dustrelayfee=0` option for tests using dust (instead of `acceptnonstdtxn=1`) (Sebastian Falbesoner) Pull request description: By specifying the `dustrelayfee=0` option instead of the more generic `acceptnonstdtxn=1`, we can be more specific about what part of the transaction is non-standard and can be sure that all other aspects follow the standard policy. Note that for the test `feature_dbcrash.py`, the UTXO creation at the start of the test has to be split up to several txs to not exceed the tx standard size limit of 100k vbytes https://github.com/bitcoin/bitcoin/blob/4129c1375430dbfe8dd414868c43fceb3d091fc3/src/policy/policy.h#L26-L27 ACKs for top commit: MarcoFalke: review ACK 1770be7 Tree-SHA512: 5cb852a92883a7443ab7dc15b48efa76b5d1424b6b0da1fa6b075fbe9a83522e3ff60382d36c08d4b07143ed898c115614582474e37837332caaee73b0db0e47
d69045e test: add coverage for 'listreceivedbyaddress' no change addrs return (furszy) 324f00a refactor: 'ListReceived' use optional for filtered address (furszy) b459fc1 refactor: RPC 'ListReceived', encapsulate m_address_book access (furszy) fa9f2ab refactor: RPC 'listlabels', encapsulate 'CWallet::ListAddrBookLabels' functionality (furszy) 83e42c4 refactor: use 'ForEachAddrBookEntry' in RPC 'getaddressesbylabel' (furszy) 2b48642 refactor: use ForEachAddrBookEntry in interfaces::getAddresses (furszy) 032842a wallet: implement ForEachAddrBookEntry method (furszy) 09649bc refactor: implement general 'ListAddrBookAddresses' for addressbook destinations lookup (furszy) 192eb1e refactor: getAddress don't access m_address_book, use FindAddressEntry function (furszy) Pull request description: ### Context The wallet's `m_address_book` field is being accessed directly from several places across the sources. ### Problem Code structure wise, we shouldn't be accessing it directly. It could end up being modified by mistake (from a place that has nothing to do with the wallet like an RPC command or the GUI) and cause a bigger issue: like an address book entry 'purpose' string change, which if done badly (from 'send' to 'receive'), could end up in a user sharing a "receive" address that he/she doesn't own. ### Solution Encapsulate `m_address_book` access inside the wallet. ------------------------------------------------------- Extra Note: This is the initial step towards decoupling the address book functionality from the wallet's sources. In other words, the creation of the `AddressBookManager` (which will be coming in a follow-up PR). ACKs for top commit: achow101: ACK d69045e theStack: ACK d69045e ✅ w0xlt: ACK bitcoin@d69045e Tree-SHA512: dba17acd86f171b4e9af0223bbbcad380048570f6a2f6a92732a51f01abe8806debaf65c9e9e5569fa76a541903cbb50adcb5f56ef77858151c698ae6b218e2a
bitcoin backports