backport: bitcoin#14707, #23495, #23639, #23652, #23672, #23682, #23687, #23714, #23715, #23740, #24068 #6601
Conversation
c771ee8 doc: use BIP125-replaceable (fanquake) 36dc5bb doc: Extract CreateTxDoc in rawtransaction (fanquake) Pull request description: For the fields: inputs, outputs, locktime, replaceable. Similar to bitcoin#23172. Can be reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`. ACKs for top commit: MarcoFalke: ACK c771ee8 😸 Tree-SHA512: e6e4211b89bedec472f8381b3cbea5670f82b728955888c794f694164b8d8bdd51a99c64762b625357ac2171005712b82f81ee7c1b8f5c5620bdedeeefa2b9da
WalkthroughThis pull request makes several updates across the codebase. The CI configuration is modified to use a new task targeting macOS with a specific focus on SQLite, and the Homebrew install script has been adjusted accordingly. The Fontconfig dependency is upgraded from version 2.12.1 to 2.12.6, with corresponding SHA256 and patch list changes, including updates to identifier names in patch files. Several wallet RPC commands are modified to support an optional parameter to include immature coinbase transactions, alongside related documentation updates. Additionally, the SpanReader class constructors are simplified by removing a redundant positional parameter, with corresponding changes in PSBT handling, blockfilter processing, and various tests. New testing data and methods are also introduced, and outdated methods have been removed from the testing framework and CLI tests. ✨ 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: 1
🧹 Nitpick comments (7)
src/streams.h (1)
152-153: API simplification by removing position parameter.The constructor has been simplified by removing the position parameter and associated functionality. This makes the API cleaner but requires consumers to create appropriate subspans before passing data to SpanReader if they need to start reading from a specific position.
Consider adding a comment in the constructor documentation mentioning this change for developers who might be used to the previous behavior:
/** * @param[in] type Serialization Type * @param[in] version Serialization Version (including any flags) * @param[in] data Referenced byte vector to overwrite/append + * Note: To read from a specific position, create a subspan of the data before passing it to this constructor. */ SpanReader(int type, int version, Span<const unsigned char> data) : m_type(type), m_version(version), m_data(data) {}test/functional/wallet_listreceivedby.py (1)
5-5: Minor typos in docstring.The line mentions “getreceivedybaddress” which should be “getreceivedbyaddress.” Consider fixing it for clarity.
- """Test the listreceivedbyaddress, listreceivedbylabel, getreceivedybaddress, ... + """Test the listreceivedbyaddress, listreceivedbylabel, getreceivedbyaddress, ...src/wallet/rpcwallet.cpp (5)
568-577: Clarify deprecated vs. new coinbase handling logic.
The introduction of theinclude_immature_coinbaseparameter and the newinclude_coinbaseboolean for detecting theexclude_coinbasedeprecation is logically sound. However, repeating the same check and error message in different parts of the code might become cumbersome. Consider encapsulating this conflict check into a small utility function to avoid duplication.
953-953: Update function signature documentation.
The new parameterinclude_immature_coinbaseadded toListReceivedis not documented in the function header comment. Include usage details and any implications for external callers to keep the signature usage clear.
984-991: Avoid repeating identical depreciation blocks.
These lines duplicate the same “exclude_coinbase” check as before. Extracting them into a helper method (e.g.,VerifyCoinbaseFlags(include_immature_coinbase)) would reduce repetition and keep logic in sync.
1166-1167: New parameter introduction for getreceivedbyaddress.
The addition ofinclude_immature_coinbaseat parameter index 5 is consistent with earlier logic. Consider adding explicit mention of the new parameter to the RPC help text block for clarity.Also applies to: 1170-1170
1213-1214: Parameter usage consistency for getreceivedbylabel.
Similar to getreceivedbyaddress, theinclude_immature_coinbaseparameter is inserted at index 4. Ensure that test coverage verifies the interplay with theminconfandaddlockedparameters.Also applies to: 1217-1217
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.cirrus.yml(1 hunks)depends/packages/fontconfig.mk(1 hunks)depends/patches/fontconfig/gperf_header_regen.patch(1 hunks)depends/patches/fontconfig/remove_char_width_usage.patch(0 hunks)doc/dependencies.md(1 hunks)doc/release-notes-14707.md(1 hunks)src/blockfilter.cpp(2 hunks)src/psbt.h(3 hunks)src/rpc/client.cpp(1 hunks)src/rpc/misc.cpp(1 hunks)src/rpc/net.cpp(4 hunks)src/rpc/rawtransaction.cpp(3 hunks)src/rpc/signmessage.cpp(1 hunks)src/rpc/util.cpp(1 hunks)src/streams.h(1 hunks)src/test/fuzz/golomb_rice.cpp(2 hunks)src/test/streams_tests.cpp(3 hunks)src/wallet/rpcwallet.cpp(13 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)test/functional/data/rpc_decodescript.json(1 hunks)test/functional/interface_bitcoin_cli.py(0 hunks)test/functional/p2p_timeouts.py(1 hunks)test/functional/rpc_decodescript.py(2 hunks)test/functional/rpc_generate.py(1 hunks)test/functional/test_framework/test_framework.py(1 hunks)test/functional/wallet_listreceivedby.py(3 hunks)
💤 Files with no reviewable changes (2)
- depends/patches/fontconfig/remove_char_width_usage.patch
- test/functional/interface_bitcoin_cli.py
✅ Files skipped from review due to trivial changes (5)
- doc/dependencies.md
- test/functional/rpc_generate.py
- src/rpc/signmessage.cpp
- src/test/fuzz/golomb_rice.cpp
- src/rpc/misc.cpp
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (47)
test/functional/test_framework/test_framework.py (1)
1107-1114: Well-implemented addition of theis_specified_wallet_compiledmethod.This new method provides a clear way to check whether wallet support for the specified type (legacy or descriptor wallet) has been compiled. It correctly delegates to either
is_sqlite_compiled()oris_bdb_compiled()based on theself.options.descriptorssetting, which is consistent with how wallet types are determined elsewhere in the codebase.test/functional/p2p_timeouts.py (1)
99-99: Increased time advancement improves test robustness.The change from 3 to 5 seconds gives more margin to ensure the timeout events are properly detected, making the test more reliable. This is a good improvement since the peer timeout is set to 3 seconds (line 41), and advancing time by exactly the timeout value could lead to race conditions.
test/functional/data/rpc_decodescript.json (1)
1-18: Well-structured test data for script decoding.This JSON file provides valuable test cases for the decodescript RPC, covering both scripthash and nulldata script types with comprehensive expected outputs. This will help ensure script decoding functionality remains correct through future code changes.
src/wallet/test/wallet_tests.cpp (1)
707-707: Updated SpanReader constructor aligns with API changes.The removal of the fourth parameter from the SpanReader constructor is part of a broader simplification of the SpanReader API across the codebase. This change maintains test functionality while updating to the new constructor signature.
doc/release-notes-14707.md (1)
1-19: Clear and comprehensive release notes for the RPC changes.The release notes effectively document the changes to wallet RPC behavior regarding coinbase transactions. They provide all necessary information:
- Which RPCs are affected
- What changed in their behavior
- The new parameter and its default value
- A clear definition of immature coinbase transactions
- How to maintain the old behavior if needed
This documentation will be valuable for users upgrading to the new version.
src/blockfilter.cpp (2)
53-53: The initial position parameter has been removed from SpanReader initialization.The fourth parameter (initial position
0) has been removed from theSpanReaderconstructor. This appears to be part of a refactoring of theSpanReaderclass across multiple files, making the API more concise by removing redundant parameters.
107-107: The initial position parameter has been removed from SpanReader initialization.Similar to the previous change, the fourth parameter (initial position
0) has been removed from theSpanReaderconstructor in theMatchInternalmethod. This maintains consistency with the updatedSpanReaderAPI across the codebase.src/rpc/util.cpp (1)
587-587: Improved formatting of help text descriptions.The formatting of the description in the
ToStringmethod has been improved:
- Added an extra newline before the description for better visual separation
- Now uses
TrimStringto remove leading and trailing whitespace from the descriptionThis change enhances readability of RPC help text, particularly for the wallet RPC commands that now include the new
include_immature_coinbaseparameter mentioned in the PR description.src/test/streams_tests.cpp (3)
77-77: The initial position parameter has been removed from SpanReader initialization.The fourth parameter (initial position
0) has been removed from theSpanReaderconstructor. This is part of a consistent refactoring of theSpanReaderAPI across the codebase.
107-107: The initial position parameter has been removed from SpanReader initialization.Similar to the previous change, the fourth parameter has been removed from the
SpanReaderconstructor in the test case that reads from the beginning of the buffer.
121-121: The initial position parameter has been removed from SpanReader initialization.The fourth parameter has been removed from the
SpanReaderconstructor in thestreams_vector_reader_rvaluetest case, consistent with the other changes.src/psbt.h (3)
282-283: The initial position parameter has been removed from SpanReader initialization in PSBTInput.The fourth parameter (initial position
0) has been removed from theSpanReaderconstructor in thePSBTInput::Unserializemethod. This is consistent with the updatedSpanReaderAPI being applied across the codebase.
536-537: The initial position parameter has been removed from SpanReader initialization in PSBTOutput.The fourth parameter has been removed from the
SpanReaderconstructor in thePSBTOutput::Unserializemethod, consistent with the changes in other parts of the code.
714-715: The initial position parameter has been removed from SpanReader initialization in PartiallySignedTransaction.The fourth parameter has been removed from the
SpanReaderconstructor in thePartiallySignedTransaction::Unserializemethod, completing the consistent application of this change across all PSBT-related structures.test/functional/rpc_decodescript.py (3)
7-8: Good addition of necessary imports.These imports enable JSON parsing and file path management needed for the new data-driven testing approach.
179-185: Well-structured data-driven test implementation.The data-driven approach improves maintainability by separating test data from test logic. This makes it easier to add new test cases without modifying the test code.
191-192: Good integration with existing test structure.The new test method is properly integrated into the test flow with appropriate logging.
.cirrus.yml (2)
171-171: Appropriate task name update.The task name now correctly reflects that this build uses SQLite only, which is consistent with the PR objective of backporting changes from Bitcoin Core v23.
173-173: Properly removed Berkeley DB dependency.Removing berkeley-db@4 from the build dependencies ensures the macOS build will use SQLite for wallet storage, aligning with the SQLite-only approach indicated in the task name.
depends/patches/fontconfig/gperf_header_regen.patch (2)
18-18: Properly fixed file timestamp update.Adding
touch $@ensures the target file's timestamp is updated correctly after moving the temporary file, preventing unnecessary rebuilds.
20-22: Fixed dependency rule to prevent inadvertent header regeneration.Removing the dependency on
fcobjshash.gperfand explicitly using the filename in the command properly addresses the issue mentioned in the commit message about preventing unwanted header regeneration.src/rpc/client.cpp (4)
53-53: Properly added parameter conversion for getreceivedbyaddress.Added support for the new
include_immature_coinbaseparameter, which aligns with the PR objective to include coinbase transactions in wallet RPC results.
56-56: Properly added parameter conversion for getreceivedbylabel.Added support for the new
include_immature_coinbaseparameter, which aligns with the PR objective to include coinbase transactions in wallet RPC results.
62-62: Properly added parameter conversion for listreceivedbyaddress.Added support for the new
include_immature_coinbaseparameter, which aligns with the PR objective to include coinbase transactions in wallet RPC results.
67-67: Properly added parameter conversion for listreceivedbylabel.Added support for the new
include_immature_coinbaseparameter, which aligns with the PR objective to include coinbase transactions in wallet RPC results.src/rpc/rawtransaction.cpp (3)
125-158: Great refactoring to centralize transaction documentation.Extracting the common transaction documentation parameters into a reusable function improves maintainability and ensures consistency between the
createrawtransactionandcreatepsbtRPC calls.
700-700: Well-implemented reference to the centralized documentation.The code now uses the
CreateTxDoc()function instead of duplicating parameter definitions, which is a good application of the DRY principle.
1754-1754: Consistent implementation with createrawtransaction.Good to see the same pattern being applied here as well, maintaining consistency across similar RPC methods.
src/rpc/net.cpp (6)
98-98: No functional issues identified.This return statement for the
getpeerinfoRPC has been refactored to a more structuredRPCHelpManobject. The approach is consistent with other RPC definitions and improves clarity.
113-114: Correct switch of "mapped_as" type to numeric.Changing "mapped_as" from string to numeric is appropriate for representing the AS number. Ensure it supports large AS numbers (64-bit) as intended.
145-148: Marking fields optional is appropriate."startingheight", "synced_headers", "synced_blocks", and "inflight" now have
optional=true. This change aligns with the possibility that these values may be unavailable in some scenarios.
152-154: Optional address relay fields properly documented.Marking "addr_relay_enabled", "addr_processed", and "addr_rate_limited" as optional makes sense if these fields are not always populated.
172-173: Documentation note on output stability is clear.The added note emphasizes potential upcoming changes to the RPC output. This transparency benefits API consumers.
181-181: Helpful example usage.The added
HelpExampleRpc("getpeerinfo", "")line makes the RPC usage clearer.depends/packages/fontconfig.mk (3)
2-2: Upgrade to version 2.12.6 is valid.The update from 2.12.1 to 2.12.6 appears consistent with standard release progression.
5-5: SHA256 hash update matches the new version.Ensure the provided hash exactly matches the 2.12.6 archive.
Would you like to run a script to confirm the integrity of the tarball with this hash?
7-7: Patch list maintenance is consistent.Removing the old patch and keeping
gperf_header_regen.patchaligns with the upstream changes.test/functional/wallet_listreceivedby.py (7)
8-8: Addition of COINBASE_MATURITY import is correct.This constant is needed for the block testing logic.
22-23: Deprecation behavior tested on second node.Using
-deprecatedrpc=exclude_coinbaseis aligned with transitional testing. This ensures backward compatibility is exercised.
170-195: Thorough testing of immature coinbase in “getreceivedbyaddress/label.”Ensures that by default, immature coinbase is excluded, and is correctly returned when
include_immature_coinbaseis true.
196-215: Verifying lists with immature coinbase.These checks confirm that addresses and labels reflect the reward only when specifically including immature coinbase.
216-239: Post-maturity checks are well-handled.After generating 100 blocks, the coinbase reward becomes spendable by default. The tests confirm normal
getreceivedbyandlistreceivedbybehaviors.
237-257: Invalidate block logic is correct.The test ensures that invalidating the block removes it from the “received” totals when coinbase is included with zero confirmations.
258-287: Excluding coinbase functionality properly tested.Verifies that coinbase-based amounts never appear for nodes with
-deprecatedrpc=exclude_coinbase, and tests the error condition when combining it withinclude_immature_coinbase.src/wallet/rpcwallet.cpp (3)
582-586: Ensure consistent transaction filtering.
This logic correctly filters out coinbase and immature coinbase transactions when desired. The multiple conditions appear to be correct, but be sure to verify that additional code paths (e.g., zero-confirmation coinbase or race conditions) remain consistent in different wallet states (like watch-only).Also applies to: 589-589
976-976: Parameter safety check.
This condition guards against a faulty empty string for the address filter. Ensure edge cases are tested (e.g., explicitly null or invalid string).
1184-1184: Document the newly introduced RPCArg.
When adding{"include_immature_coinbase", RPCArg::Type::BOOL, ...}, extend the method’s docstring so end users understand how immature coinbase outputs are included in the array.
| // Tally | ||
| std::map<CTxDestination, tallyitem> mapTally; | ||
| for (const std::pair<const uint256, CWalletTx>& pairWtx : wallet.mapWallet) { | ||
| const CWalletTx& wtx = pairWtx.second; | ||
|
|
||
| if (wtx.IsCoinBase() || !wallet.chain().checkFinalTx(*wtx.tx)) | ||
| continue; | ||
|
|
||
| int nDepth = wtx.GetDepthInMainChain(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate repeated maturity checks.
Much like the previous segments, the condition for skipping coinbase or immature coinbase transactions reappears here. Centralizing this filtering logic can help maintain consistent behavior everywhere immature coinbase is managed.
Also applies to: 1001-1007
knst
changed the title
src/rpc/net.cpp
Outdated
| @@ -109,7 +110,8 @@ static RPCHelpMan getpeerinfo() | |||
| {RPCResult::Type::STR, "addrbind", /* optional */ true, "(ip:port) Bind address of the connection to the peer"}, | |||
| {RPCResult::Type::STR, "addrlocal", /* optional */ true, "(ip:port) Local address as reported by the peer"}, | |||
| {RPCResult::Type::STR, "network", "Network (" + Join(GetNetworkNames(/* append_unroutable */ true), ", ") + ")"}, | |||
| {RPCResult::Type::STR, "mapped_as", /* optional */ true, "The AS in the BGP route to the peer used for diversifying peer selection"}, | |||
| {RPCResult::Type::NUM, "mapped_as", /* optional */ true, "The AS in the BGP route to the peer used for diversifying\n" | |||
| "peer selection (only available if the asmap config flag is set)"}, | |||
There was a problem hiding this comment.
23652: unrelated changes - "(only available if the asmap config flag is set)" belongs to some another backport
fab6c43 doc: Document optional result fields in validateaddress (MarcoFalke) faee265 doc: Document optional result fields in getpeerinfo (MarcoFalke) Pull request description: ACKs for top commit: shaavan: ACK fab6c43 Tree-SHA512: 78458d0c4deb9253fbfe37fa5736a7db14eb0478bcc4adeba10ba6945e83d8eac92048293f50c054ea612609939151b4a2e1226c06f6067901f3d58c127c7e18
…ript.py b35942e tests: Add data-driven testcases to rpc_decodescript.py (Dimitri) Pull request description: closes bitcoin#23641 Top commit has no ACKs. Tree-SHA512: 2f494c78ad085d523fae15befaadb9e0fc382b5310e3a95395ecf06a90968b15b6d232f7564098ed0a68419e27aa2e5260fe691cf2ce84af9fb6b65634e54d77
…Reader 31ba1af Remove unused (and broken) functionality in SpanReader (Pieter Wuille) Pull request description: This removes the ability to set an offset in the `SpanReader::SpanReader` constructor, as the current code is broken since bitcoin#23653. All call sites use `pos=0`, so it is actually unused. If future call sites need it, `SpanReader{a, b, c, d}` is equivalent to `SpanReader{a, b, c.subspan(d)}`. It also removes the ability to deserialize from `SpanReader` directly from the constructor. This too is unused, and can be more idiomatically simulated using `(SpanReader{a, b, c} >> x >> y >> z)` instead of `SpanReader{a, b, c, x, y, z}`. This was pointed out by achow101 in bitcoin#23653 (comment). ACKs for top commit: jb55: crACK 31ba1af achow101: ACK 31ba1af Tree-SHA512: 700ebcd74147628488c39168dbf3a00f8ed41709a26711695f4bf036250a9b115574923bbf96040ec7b7fee4132d6dbbcb5c6e5a2977c4beb521dc1500e6ed53
…y RPCs 1dcba99 Coinbase receivedby rpcs release notes (Andrew Toth) b569675 Test including coinbase transactions in receivedby wallet rpcs (Andrew Toth) bce20c3 Include coinbase transactions in receivedby wallet rpcs (Andrew Toth) Pull request description: The current `*receivedby*` RPCs filter out coinbase transactions. This doesn't seem correct since an output to your address in a coinbase transaction *is* receiving those coins. This PR corrects this behaviour. Also, a new option `include_immature_coinbase` is added (default=`false`) that includes immature coinbase transactions when set to true. However, since this is potentially a breaking change this PR introduces a hidden configuration option `-deprecatedrpc=exclude_coinbase`. This can be set to revert to previous behaviour. If no reports of broken workflow are received, then this option can be removed in a future release. Fixes bitcoin#14654. ACKs for top commit: jnewbery: reACK 1dcba99 Tree-SHA512: bfc43b81279fea5b6770a4620b196f6bc7c818d221b228623e9f535ec75a2406bc440e3df911608a3680f11ab64c5a4103917162114f5ff7c4ca8ab07bb9d3df
faa0833 doc: Normalize RPC description whitespace (MarcoFalke) Pull request description: It is tedious to manually add trailing newlines after the description so that there is an empty new line before the `Arguments` section. Fix that by adding it with C++ code. ACKs for top commit: fanquake: ACK faa0833 Tree-SHA512: 061786c7f19d767f2b7a0362b948e34d181f4cc740a60211756da29ece7554e95be39a9beec3e201eddc8da3ea7e22ac917479eae04b230bb7b0db7a9647af8c
…e availability 84bc35d test: feature_rbf.py: check specified wallet type availability (Sebastian Falbesoner) Pull request description: The test currently leads to a failure if in general wallet support is compiled, but the library for the specified type (BDB/SQLite) is not, i.e. if started with the `--legacy-wallet` parameter, but bitcoind is compiled without BDB support, see e.g. bitcoin#23682 (comment) Fix this by checking if the specified wallet type (BDB for legacy wallet, SQLite for descriptor wallet) is available. Also move the helper `is_specified_wallet_compiled()` to the test framework's class BitcoinTestFramework first, so it can be reused. Should further pave the way for bitcoin#23682. On my local instance without BDB compiled, all targets in test_runner pass now. ACKs for top commit: mzumsande: ACK 84bc35d, changes loook good for me and I confirmed that this fixes the error encountered on master when running the test `--without-bdb`. Tree-SHA512: 1575c03c793c8e0ac195d0914eff75d02604301c8fb77d0fdb7c0b245561569c0c7db387ef4de499044b68ab6e14b4b78b955f6e74c84197bcaed95f439c9824
6575d35 build: Bump Fonconfig version up to 2.12.6 (Hennadii Stepanov) Pull request description: This PR gets rid of `remove_char_width_usage.patch`. Some additional observations: 1. Newer Fontconfig versions (2.13.0 and 2.13.1) introduce a new dependency, `uuid`, in the [`7b48fd3dd406b926f0e5240b211f72197ed538a9`](https://gitlab.freedesktop.org/fontconfig/fontconfig/-/commit/7b48fd3dd406b926f0e5240b211f72197ed538a9) commit 2. In Fonconfig 2.13.1 (the current stable) excludes the `fcobjshash.h` from the distributive archive (see [`31269e3589e0e6432d12f55db316f4c720a090b5`](https://gitlab.freedesktop.org/fontconfig/fontconfig/-/commit/31269e3589e0e6432d12f55db316f4c720a090b5)), that makes our `gperf_header_regen.patch` unusable, and requires `gperf` as a dependency. ACKs for top commit: fanquake: ACK 6575d35 - from the best I can determine this doesn't have any versioning / ABI implications. The ABI difference between 2.12.1 and 2.12.6 is two symbol additions, neither of which are used by Qt. Fontconfig seems to be better at maintaining backwards compatibility compared to a library like Freetype. Tree-SHA512: 36780a0c5a658469697e524d682ebab56c320cb04f8297bc215f4552f183d4f560501fb0a869982fd9053d4a2d571c7fd971d8f5e96c9da9a9d142c485e3baa4
fad2e0a ci: Make macOS native task sqlite only (MarcoFalke) Pull request description: There are many sqlite-only test failures (bitcoin#23563, bitcoin#23562), so make one CI task sqlite-only. Obviously this removes bdb coverage from macOS, but I don't expect this to break very often. ACKs for top commit: fanquake: ACK fad2e0a - clearly worthwhile having a sqlite only CI given the amount of bugs this has turned up in the past week or so. Tested running the functional tests with a `--without-bdb` build on macOS and Linux. Tree-SHA512: d88ad576bbe974a51a2d115e4102fc1bd73772b7393dfa99700fbfd1bdf5deebbf247e35d09757639649a6ad5b193c0998c7db679301a7c012572dfd54a6a289
…imeouts.py 0bfb920 test: fix test failures in test/functional/p2p_timeouts.py (Jon Atack) Pull request description: Fixes bitcoin#23739. Top commit has no ACKs. Tree-SHA512: 0173be87d673dd34fe8ebe77789f85a469fde75a81d5d5bf09b5586c7a1092658bcb056ff5ea4b21ba22c99aba06592815734d68f928c131bfa824f3c1b3c2e6
fa4c72e doc: Rework 14707 release notes (MarcoFalke) fa9377c doc: move-only release note snippets (MarcoFalke) Pull request description: Requested by myself in https://github.com/bitcoin/bitcoin/pull/14707/files#r764313750 ACKs for top commit: andrewtoth: ACK fa4c72e Tree-SHA512: 091b9cb511ed3bef1e3d48ba5917ee23dd4bbb0f1daf999ca2b429fcff57b8876375b8579ac0a8ec438dc5c75d536d6d27074f325f51ffd38b0edc284d0c0155
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/functional/rpc_decodescript.py (1)
179-186: Well-implemented data-driven testing approach.Good implementation of data-driven tests that loads test cases from an external JSON file. This approach makes it easier to maintain and expand test cases without modifying the test code. The file handling using the
withstatement properly manages resources.Consider adding error handling for potential file access issues or invalid JSON format, though this is low priority for test code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.cirrus.yml(1 hunks)depends/packages/fontconfig.mk(1 hunks)depends/patches/fontconfig/gperf_header_regen.patch(1 hunks)depends/patches/fontconfig/remove_char_width_usage.patch(0 hunks)doc/dependencies.md(1 hunks)doc/release-notes-14707.md(1 hunks)src/blockfilter.cpp(2 hunks)src/psbt.h(3 hunks)src/rpc/client.cpp(1 hunks)src/rpc/misc.cpp(1 hunks)src/rpc/net.cpp(3 hunks)src/rpc/signmessage.cpp(1 hunks)src/rpc/util.cpp(1 hunks)src/streams.h(1 hunks)src/test/fuzz/golomb_rice.cpp(2 hunks)src/test/streams_tests.cpp(3 hunks)src/wallet/rpcwallet.cpp(13 hunks)src/wallet/test/wallet_tests.cpp(1 hunks)test/functional/data/rpc_decodescript.json(1 hunks)test/functional/interface_bitcoin_cli.py(0 hunks)test/functional/p2p_timeouts.py(1 hunks)test/functional/rpc_decodescript.py(2 hunks)test/functional/rpc_generate.py(1 hunks)test/functional/test_framework/test_framework.py(1 hunks)test/functional/wallet_listreceivedby.py(3 hunks)
💤 Files with no reviewable changes (2)
- test/functional/interface_bitcoin_cli.py
- depends/patches/fontconfig/remove_char_width_usage.patch
🚧 Files skipped from review as they are similar to previous changes (18)
- doc/dependencies.md
- test/functional/rpc_generate.py
- src/rpc/util.cpp
- test/functional/data/rpc_decodescript.json
- src/test/fuzz/golomb_rice.cpp
- doc/release-notes-14707.md
- .cirrus.yml
- src/rpc/misc.cpp
- test/functional/p2p_timeouts.py
- src/wallet/test/wallet_tests.cpp
- src/rpc/client.cpp
- src/test/streams_tests.cpp
- depends/patches/fontconfig/gperf_header_regen.patch
- src/rpc/signmessage.cpp
- src/blockfilter.cpp
- src/psbt.h
- src/rpc/net.cpp
- depends/packages/fontconfig.mk
🔇 Additional comments (22)
test/functional/test_framework/test_framework.py (1)
1107-1114: Well-structured method for wallet type compilation checksThis new method provides a clean way to verify if the wallet type specified in test options (descriptor or legacy) has been compiled with the appropriate database backend. It correctly routes descriptor wallets to check SQLite compilation and legacy wallets to check BDB compilation.
test/functional/rpc_decodescript.py (2)
7-8: LGTM: Appropriate imports added for the new data-driven testing feature.The added imports for
jsonandosmodules are necessary for handling the JSON test data file.
191-192: LGTM: Proper test execution flow.The test flow is correctly updated to include the new data-driven tests, with appropriate logging.
test/functional/wallet_listreceivedby.py (8)
5-5: Documentation update matches implementation.The docstring has been correctly updated to include all tested RPCs, which now includes
getreceivedbyaddressandgetreceivedbylabel.
8-8: LGTM: Appropriate constant import added.The import of
COINBASE_MATURITYmakes the test more maintainable by using a shared constant rather than hardcoding values.
22-23: Good backwards compatibility testing setup.Setting up a second node with
-deprecatedrpc=exclude_coinbaseallows for thorough testing of the new functionality alongside backward compatibility with the deprecated behavior.
170-218: Comprehensive testing of immature coinbase behavior.The added tests thoroughly verify that immature coinbase transactions are:
- Not included by default in result sets
- Included when
include_immature_coinbase=Trueis specified- Properly handled across all related wallet RPCs
The test generates realistic scenarios with proper assertions, covering all related RPCs.
219-236: Thorough verification of mature coinbase handling.This section properly tests that mature coinbase outputs (after
COINBASE_MATURITYconfirmations) are correctly included in the RPC results by default, which is essential to verify the new behavior works as expected.
237-257: Excellent edge case testing with block invalidation.Testing the behavior when a block is invalidated is an important edge case. This verifies that invalidated coinbase transactions are properly excluded from results even when
include_immature_coinbase=Trueandminconf=0, which helps prevent double-spending attacks.
258-279: Important backward compatibility testing.This section verifies that the deprecated behavior (
exclude_coinbase) still works correctly on the configured node, ensuring backward compatibility for users who rely on the old behavior.
280-286: Proper error handling for incompatible parameters.The tests verify that trying to use both the new
include_immature_coinbaseparameter with the deprecatedexclude_coinbaseflag properly raises an error with a clear message. This prevents confusing behavior from contradictory settings.src/wallet/rpcwallet.cpp (10)
568-576: Appropriate input validation.Good implementation of a compatibility check between the new
include_immature_coinbaseparameter and the deprecatedexclude_coinbaseoption. This prevents users from setting contradictory options.
583-589: Transaction filtering logic looks correct.The filtering logic has been appropriately updated to check both the coinbase status and the new immature coinbase parameter. Coinbase transactions are properly filtered based on the settings.
984-990: Similar compatibility check in ListReceived function.Consistent implementation of the same compatibility check between
include_immature_coinbaseandexclude_coinbaseas seen in the GetReceived function. This ensures consistent behavior across different RPC calls.
1001-1007: Consolidate repeated maturity checks.Much like the previous segments, the condition for skipping coinbase or immature coinbase transactions reappears here. Centralizing this filtering logic can help maintain consistent behavior everywhere immature coinbase is managed.
1166-1167: Parameter correctly passed to underlying implementation.The
include_immature_coinbaseparameter is correctly extracted from the RPC inputs and passed to theListReceivedimplementation.
1213-1214: Parameter consistently handled in listreceivedbylabel.The same pattern is correctly applied to the
listreceivedbylabelRPC method, ensuring consistent behavior between related RPC calls.
611-612: Documentation updated correctly.The RPC help has been properly updated to document the new parameter for
getreceivedbyaddress.
623-625: Examples updated with the new parameter.The example for
getreceivedbyaddressnow includes a demonstration of using the newinclude_immature_coinbaseparameter, which is helpful for users.
1127-1129: Documentation and parameter order consistency.The
listreceivedbyaddressRPC help correctly shows the new parameter, maintaining consistency with the other related RPC methods.
1152-1155: Complete examples in the help text.The examples for
listreceivedbyaddressare comprehensive and demonstrate the use of the new parameter in both CLI and RPC formats.src/streams.h (1)
152-153:✅ Verification successful
Constructor simplified - make sure all call sites are updated
The
SpanReaderconstructor has been simplified by removing the optionalsize_t posparameter that was previously used to specify an initial reading position. This is part of the Bitcoin Core v23 backport.
🏁 Script executed:
#!/bin/bash # Find and verify all SpanReader construction calls to ensure they've been updated to no longer use pos parameter echo "Looking for any remaining SpanReader constructor calls that might still be using the removed pos parameter..." # Search for SpanReader instantiations in the codebase rg -n "SpanReader\s*\([^)]*,[^)]*,[^)]*,[^)]*\)" --type cpp # Also look for specific uses in the files mentioned in the AI summary echo "Checking specific files mentioned in summary..." rg -n "SpanReader" --type cpp src/blockfilter.cpp src/psbt.h src/test/streams_tests.cpp src/test/fuzz/golomb_rice.cppLength of output: 1713
SpanReader constructor update verified
After running the search queries, all call sites in the codebase use the updated three-argument constructor with no remaining instances of the removed pos parameter:
- Valid usages in
src/blockfilter.cpp,src/psbt.h,src/test/streams_tests.cpp, andsrc/test/fuzz/golomb_rice.cppwere confirmed.- No instances of the outdated four-parameter constructor call were found.
This confirms that the constructor simplification has been applied consistently.
What was done?
Regular backports from Bitcoin Core v23
How Has This Been Tested?
Run unit / functional tests
Breaking Changes
Wallet
receivedbyRPCs now include coinbase transactionsPreviously, the following wallet RPCs excluded coinbase transactions:
getreceivedbyaddressgetreceivedbylabellistreceivedbyaddresslistreceivedbylabelThis release changes this behaviour and returns results accounting for received coins from coinbase outputs.
A new option,
include_immature_coinbase(default=false), determines whether to account for immature coinbase transactions.Immature coinbase transactions are coinbase transactions that have 100 or fewer confirmations, and are not spendable.
The previous behaviour can be restored using the configuration
-deprecatedrpc=exclude_coinbase, but may be removed in a future release.Checklist: