backport: bitcoin#19909, #21576, #23319, #24865, #26213, #26265, #26289, #26598, #26734, #26750 - #7666
backport: bitcoin#19909, #21576, #23319, #24865, #26213, #26265, #26289, #26598, #26734, #26750#7666knst wants to merge 10 commits into
Conversation
|
🕓 Queued for automated review — 61st in line, estimated start in ~57 h (commit 50d9348)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11e0b79006
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| CBlockUndo blockUndo; | ||
| CBlock block; | ||
| const bool is_block_pruned{WITH_LOCK(cs_main, return chainman.m_blockman.IsBlockPruned(blockindex))}; |
There was a problem hiding this comment.
On a pruned node, getrawtransaction with verbosity 2 for an unconfirmed mempool transaction leaves blockindex null, but this call passes it to BlockManager::IsBlockPruned, which dereferences pblockindex when m_have_pruned is true. The later !blockindex condition is therefore too late, and a valid RPC request can crash the node; check for null before calling IsBlockPruned.
AGENTS.md reference: AGENTS.md:L200-L202
Useful? React with 👍 / 👎.
| previousblockhash = int(best_block["hash"], 16) | ||
| big_script = CScript([OP_RETURN] + [OP_TRUE] * 950000) | ||
| for _ in range(n): | ||
| block = create_block(hashprev=previousblockhash, ntime=self.nTime, coinbase=create_coinbase(height, script_pubkey=big_script)) |
There was a problem hiding this comment.
During the 600-block batch, this loop reaches regtest DIP3 activation at height 432, but create_coinbase is called without dip4_activated=True, so the submitted block lacks Dash's required special coinbase payload and is rejected with bad-cbtx-version. The test consequently never builds enough valid data to prune and fails its later has_block(0) assertion; delay Dash deployments as feature_pruning.py does or construct valid post-DIP3 coinbases.
AGENTS.md reference: AGENTS.md:L197-L202
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73ac9f365f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| scripts.emplace_back(script, birth_time); | ||
| } | ||
| } | ||
| file.close(); | ||
| EnsureBlockDataFromTime(*pwallet, nTimeBegin); |
There was a problem hiding this comment.
When a Qt user imports a dump whose birth block has already been pruned, this call throws after the import progress dialog was opened and advanced to 1–50%. The subsequent showProgress("", 100, ...) calls are skipped, while BitcoinGUI::showProgress only closes its application-modal dialog upon receiving 100, so the GUI remains blocked after this expected RPC error; close the progress display before throwing or use scoped cleanup.
Useful? React with 👍 / 👎.
| @@ -1,32 +0,0 @@ | |||
| ## PGP keys of builders and Developers | |||
There was a problem hiding this comment.
This backport removes the builder-key README and every live documentation link, but a repository-wide search shows that all ten Dash .pgp key files remain under contrib/builder-keys/; it also removes that directory from the spelling-lint exclusion. If these keys are superseded they should be removed as the upstream change intended, while if Dash still needs them the documentation and exclusion must remain—leaving undocumented key material makes this a silent partial backport.
AGENTS.md reference: AGENTS.md:L187-L194
Useful? React with 👍 / 👎.
|
This pull request has conflicts, please rebase. |
…ters fa0153e refactor: Replace isTrue with get_bool (MarcoFalke) fa2cc5d bugfix: Strict type checking for RPC boolean parameters (MarcoFalke) Pull request description: ACKs for top commit: ryanofsky: Code review ACK fa0153e furszy: Code ACK fa0153e Tree-SHA512: b221f823c69d90c94447fd491071ff3659cfd512872b495ebc3e711f50633351974102c9ef7e50fa4a393c4131d349adea8fd41cc9d66f1f31e1f5e7a5f78757 Co-authored-by: fanquake <fanquake@gmail.com>
…nsaction f866971 rpc: Return fee and prevout(s) to getrawtransaction (Douglas Chimento) Pull request description: Add fee response in BTC to getrawtransaction bitcoin#23264 ### For Reviewers * Verbose arg is now an int * Verbose = 2 includes a `fee` field and `prevout` * [./test/functional/rpc_rawtransaction.py](./test/functional/rpc_rawtransaction.py) contains a new test to validate fields of new verbosity 2 (not the values) ``` bitcoin-cli -chain=test getrawtransaction 9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4 2 000000000000001d442e556146d5f2841d85150c200e8d8b8a4b5005b13878f6 ``` ``` "in_active_chain": true, "txid": "9ae533f7da9be4a34997db78343a8d8d6d6186b6bba3959e56f416a5c70e7de4", "hash": "7f23e3f3a0a256ddea1d35ffd43e9afdd67cc68389ef1a804bb20c76abd6863e", .... "vin": [ { "txid": "23fc75d6d74f6f97e225839af69ff36a612fe04db58a4414ec4828d1749a05a0", "vout": 0, "scriptSig": { "asm": "", "hex": "" }, "prevout": { "generated": false, "height": 2099486, "value": 0.00017764, "scriptPubKey": { "asm": "0 7846ce1ced3253d8bd43008db2ca364cc722f5a2", "hex": "00147846ce1ced3253d8bd43008db2ca364cc722f5a2", "address": "tb1q0prvu88dxffa302rqzxm9j3kfnrj9adzk49mlp", "type": "witness_v0_keyhash" } }, "sequence": 4294967295 }, ... "fee": 0.00000762 } ``` ACKs for top commit: achow101: ACK f866971 aureleoules: ACK f866971 hernanmarino: re ACK f866971 pablomartin4btc: re-tACK f866971 Tree-SHA512: 591fdc285d74fa7803e04ad01c7b70bc20fac6b1369e7bd5b8e2cde9b750ea52d6c70d79225b74bef4f4bbc0fb960877778017184e146119da4a55f9593d1224 Co-authored-by: Andrew Chow <github@achow101.com>
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
…d test 564b580 test: Introduce MIN_BLOCKS_TO_KEEP constant (Aurèle Oulès) 71d9a7c test: Wallet imports on pruned nodes (Aurèle Oulès) e6906fc rpc: Enable wallet import on pruned nodes (Aurèle Oulès) Pull request description: Reopens bitcoin#16037 I have rebased the PR, addressed the comments of the original PR and added a functional test. > Before this change importwallet fails if any block is pruned. This PR makes it possible to importwallet if all required blocks aren't pruned. This is possible because the dump format includes key timestamps. For reviewers: `python test/functional/wallet_pruning.py --nocleanup` will generate a large blockchain (~700MB) that can be used to manually test wallet imports on a pruned node. Node0 is not pruned, while node1 is. ACKs for top commit: kouloumos: ACK 564b580 achow101: reACK 564b580 furszy: ACK 564b580 w0xlt: ACK bitcoin@564b580 Tree-SHA512: b345a6c455fcb6581cdaa5f7a55d79e763a55cb08c81d66be5b12794985d79cd51b9b39bdcd0f7ba0a2a2643e9b2ddc49310ff03d16b430df2f74e990800eabf Co-authored-by: Andrew Chow <github@achow101.com>
BACKPORT NOTE: only refactorings from this PR is backported ; no any bumpfee code is touched 2c07cfa gui: bumpfee signer support (Sjors Provoost) 7e02a33 rpc: bumpfee signer support (Sjors Provoost) 304ece9 rpc: document bools in FillPSBT() calls (Sjors Provoost) Pull request description: The `bumpfee` RPC call and GUI fee bump interface now work with an external signer. ACKs for top commit: achow101: ACK 2c07cfa furszy: code review ACK 2c07cfa jarolrod: tACK 2c07cfa Tree-SHA512: 0c7b931f76fac67c9e33b9b935f29af6f69ac67a5ffcc586ed2f1676feac427735b1d971723b29ef332bb6fb5762949598ebbf728587e8f0ded95a9bfbb3e7a4 Co-authored-by: Andrew Chow <github@achow101.com>
97115de doc: Refactor/Format getrawtransaction RPC docs and add ScriptPubKeyDoc function (Douglas Chimento) Pull request description: Added `ScriptPubKeyDoc` function ACKs for top commit: MarcoFalke: ACK 97115de kristapsk: cr utACK 97115de Tree-SHA512: 1371375986177862e8c99923eb7f1800fef8da7a7ac9f0ec9037bf5b23681c3348d5afe913aab7457f029ee1774d160ac10d7f57238500a03c6385cc0c7013fc Co-authored-by: fanquake <fanquake@gmail.com>
3ae76ea scripted-diff: Insert missed copyright header (Hennadii Stepanov) 306ccd4 scripted-diff: Bump copyright headers (Hennadii Stepanov) Pull request description: This PR bumps the existing copyright headers, as we did every year, and adds a missed one. Top commit has no ACKs. Tree-SHA512: 5f6b02e2baad21750e3dd8f0612bb6e7e2cfa6a743c669f26baf5a39c168b2d3a92afae1ce2dad59b70492175186c38f172c4ee68fc7ac87a4d85330429ca054 Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
…stors 47c4b1f mempool: log/halt when CalculateMemPoolAncestors fails unexpectedly (stickies-v) 5481f65 mempool: add AssumeCalculateMemPoolAncestors helper function (stickies-v) f911bdf mempool: use util::Result for CalculateMemPoolAncestors (stickies-v) 66e028f mempool: use util::Result for CalculateAncestorsAndCheckLimits (stickies-v) Pull request description: Upon reviewing the documentation for `CTxMemPool::CalculateMemPoolAncestors`, I noticed `setAncestors` was meant to be an `out` parameter but actually is an `in,out` parameter, as can be observed by adding `assert(setAncestors.empty());` as the first line in the function and running `make check`. This PR fixes this unexpected behaviour and introduces refactoring improvements to make intents and effects of the code more clear. ## Unexpected behaviour This behaviour occurs only in the package acceptance path, currently only triggered by `testmempoolaccept` and `submitpackage` RPCs. In `MemPoolAccept::AcceptMultipleTransactions()`, we first call `PreChecks()` and then `SubmitPackage()` with the same `Workspace ws` reference. `PreChecks` leaves `ws.m_ancestors` in a potentially non-empty state, before it is passed on to `MemPoolAccept::SubmitPackage`. `SubmitPackage` is the only place where `setAncestors` isn't guaranteed to be empty before calling `CalculateMemPoolAncestors`. The most straightforward fix is to just forcefully clear `setAncestors` at the beginning of CalculateMemPoolAncestors, which is done in the first bugfix commit. ## Improvements ### Return value instead of out-parameters This PR updates the function signatures for `CTxMemPool::CalculateMemPoolAncestors` and `CTxMemPool::CalculateAncestorsAndCheckLimits` to use a `util::Result` return type and eliminate both the `setAncestors` `in,out`-parameter as well as the error string. It simplifies the code and makes the intent and effects more explicit. ### Observability There are 7 instances where we currently call `CalculateMemPoolAncestors` without actually checking if the function succeeded because we assume that it can't fail, such as in [miner.cpp](https://github.com/bitcoin/bitcoin/blob/69b10212ea5370606c7a5aa500a70c36b4cbb58f/src/node/miner.cpp#L399). This PR adds a new wrapper `AssumeCalculateMemPoolAncestors` function that logs such unexpected failures, or in case of debug builds even halts the program. It's not crucial to the objective, more of an observability improvement that seems sensible to add on here. ACKs for top commit: achow101: ACK 47c4b1f w0xlt: ACK bitcoin@47c4b1f glozow: ACK 47c4b1f furszy: light code review ACK 47c4b1f aureleoules: ACK 47c4b1f Tree-SHA512: d908dad00d1a5645eb865c4877cc0bae74b9cd3332a3641eb4a285431aef119f9fc78172d38b55c592168a73dae83242e6af3348815f7b37cbe2d448a3a58648 Co-authored-by: Andrew Chow <github@achow101.com>
fa818e1 txmempool: Remove unused clear() member function (MarcoFalke) Pull request description: Seems odd to have code in Bitcoin Core that is unused. Moreover the function was broken (see bitcoin#24145) and is brittle, as there is nothing that prevents similar bugs from re-appearing. Fix both issues by replacing it with C++11 member initializers. ACKs for top commit: glozow: ACK fa818e1 Tree-SHA512: e79e44cac7d5a84d9ecc8e3f3b0b9a50e1e3ebec358b20ba5dac175ef07d1fbe338a20f83ee80f746f7c726c79e77f8be49e14bca57a41063da8a5302123c3a9 Co-authored-by: glozow <gloriajzhao@gmail.com>
e6864fa contrib: remove builder keys (fanquake) Pull request description: This has been superseded by adding a builder-keys/ directory in guix.sigs, where the presence of keys, and validity of signatures is checked. Preventing issues like missing keys or invalid signatures. New (or exisiting) Guix builders can add their key in the next PR they open adding attestations. Related to issues like bitcoin#26566, bitcoin#26563. Also follows up with the comment here: bitcoin#26565 (comment). ACKs for top commit: hebasto: ACK e6864fa, modulo s/update/remove/ in the PR tittle. Tree-SHA512: 095b4cf12ed0baeaf0ee7b8edcb3e2647e9c0f812e8fd63915ddb454f81dacc9c2d2b409de2773b7adb5ff643893d614d8aad1bc44c26da648e1bbbe19e11e05 Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
… 65 non-witness bytes b2aa9e8 Add release note for MIN_STANDARD_TX_NONWITNESS_SIZE relaxation (Greg Sanders) 8c5b364 Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes (Greg Sanders) Pull request description: Since the original fix was set to be a "reasonable" transaction to reduce allocations and the true motivation later revealed, it makes sense to relax this check to something more principled. There are more exotic transaction patterns that could take advantage of a relaxed requirement, such as 1 input, 1 output OP_RETURN to burn a utxo to fees for CPFP purposes when change isn't practical. Two changes could be accomplished: 1) Anything not 64 bytes could be allowed 2) Anything above 64 bytes could be allowed In the Great Consensus Cleanup, suggestion (2) was proposed as a consensus change, and is the simpler of the two suggestions. It would not allow an "empty" OP_RETURN but would reduce the required padding from 22 bytes to 5. The functional test is also modified to test the actual case we care about: 64 bytes Related mailing list discussions here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/020995.html And a couple years earlier: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-May/017883.html ACKs for top commit: achow101: reACK b2aa9e8 glozow: reACK b2aa9e8 pablomartin4btc: re-ACK bitcoin@b2aa9e8 jonatack: ACK b2aa9e8 with some suggestions Tree-SHA512: c1ec1af9ddcf31b2272209a4f1ee0c5607399f8172e5a1dfd4604cf98bfb933810dd9369a5917ad122add003327c9fcf6ee26995de3aca41d5c42dba527991ad Co-authored-by: Andrew Chow <github@achow101.com>
2 participants
What was done?
Regular backports from Bitcoin Core V25
How Has This Been Tested?
Run unit & functional tests
Breaking Changes
RPC
setban,addpeeraddress,walletcreatefundedpsbt, methodsallowed non-boolean and non-null values to be passed as boolean parameters.
Any string, number, array, or object value that was passed would be treated
as false. After this change, passing any value except
true,false, ornullnow triggers a JSON value is not of expected type error.Checklist: