backport: bitcoin#19909, #21576, #23319, #24865, #26213, #26265, #26289, #26598, #26734, #26750 - #7666
Conversation
|
✅ Final review complete — no blockers (commit aff57c7) · triage: normal · Phase 2 only (queue backlog) |
![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 👍 / 👎.
There was a problem hiding this comment.
This is unchanged from upstream: Bitcoin Core v25.0 through v29.0 call EnsureBlockDataFromTime at the same spot after the progress dialog is opened. Keeping the backport 1:1; a fix belongs in a separate follow-up, preferably upstream first. importwallet is legacy-wallet only.
| @@ -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. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
Verified the supplied findings against head 50d9348 and locally available upstream history. Two newly introduced runtime defects and an undeclared incomplete builder-key backport require changes; two documentation observations remain nonblocking, and the overlapping commit-message finding is consolidated. The worktree is unchanged and the range passes git diff --check; this verification did not run builds or runtime tests.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — Despite extensive copyright-only churn, the backports change transaction standardness policy, mempool ancestor handling, and wallet/RPC parameter validation, where Dash-specific integration errors could disrupt transaction acceptance, relay, or funding. - Phase 1 reviewers: not run (skipped for throughput: 24 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— backport-reviewer (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
🔴 3 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(s)
5 finding(s) omitted from inline comments because GitHub refused the PR diff as too large; listed below.
5 unmapped finding(s)
1. [blocking] Check blockindex before querying pruning state
src/rpc/rawtransaction.cpp:486
The newly added verbosity-2 path can reach this call with a null blockindex: GetTransaction returns a mempool transaction without setting hash_block, and the subsequent block-index lookup does not find a block. BlockManager::IsBlockPruned dereferences its argument when m_have_pruned is true, before the later !blockindex condition can select the no-undo fallback. Guard the pruning query with the null check so this supported lookup cannot terminate a pruned node.
2. [blocking] Close import progress when the pruning precheck throws
src/wallet/rpc/backup.cpp:590
EnsureBlockDataFromTime throws when the required history has been pruned, but importwallet has already emitted progress 0 and subsequent updates. Unwinding skips both completion notifications. The chain progress signal reaches BitcoinGUI::showProgress, which creates an application-modal dialog with automatic closing disabled and explicitly closes it only at progress 100; RPCConsole's exception handler only reports the error. This PR moves pruning rejection from before the dialog opens to this unprotected path, leaving stale modal progress after an expected import error. Emit completion before rethrowing or provide scoped progress cleanup.
3. [blocking] Complete bitcoin#26598's builder-key inventory removal
test/lint/lint-spelling.py:15
The PR advertises bitcoin#26598 without a partial-scope qualification, but its central inventory-removal transformation is missing. Upstream removes contrib/builder-keys/keys.txt together with the README and links; Dash's earlier bitcoin#11909 adaptation changed only the README, so Dash still stores the inventory as ten .pgp files. All ten remain unchanged here while their documentation and spelling exclusion are removed. Adapt the removal to Dash's retained inventory, or explicitly declare and justify retaining it and preserve appropriate documentation and lint handling. If retention is intentional, qualify commit 352699e as well: its current message describes key removal and migration that its documentation/lint-only diff does not perform. The blocker is the undeclared incomplete advertised backport, not a claim that retaining public keys is itself a runtime vulnerability.
4. [suggestion] Document the declared omission of bitcoin#21576's signer regression test
test/functional/wallet_signer.py:188-191
Commit ab8e163 explicitly limits this backport to refactorings and excludes bumpfee, so the missing external-signer fee-bump implementation and regression test are intentional, not a runtime prerequisite blocker. However, the PR title lists bitcoin#21576 without qualification and the description only says regular V25 backports. Mark bitcoin#21576 as partial in the PR metadata and state that signer fee-bump support and its regression test are excluded. This documents the existing scope rather than requesting RBF support in Dash.
5. [nitpick] Preserve bitcoin#26734's complete script-type help
src/rpc/rawtransaction.cpp:176
Upstream bitcoin#26734 makes the shared ScriptPubKeyDoc helper enumerate output types with GetAllOutputTypes(), but this adaptation retains the old single pubkeyhash example. GetAllOutputTypes() is already declared and implemented at the Dash review base, including Dash's output-type range, so no missing prerequisite prevents carrying this documentation change. Use the upstream expression to preserve the complete help for decoded outputs and verbosity-2 prevouts.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/rpc/rawtransaction.cpp`:
- [BLOCKING] src/rpc/rawtransaction.cpp:486: Check blockindex before querying pruning state
The newly added verbosity-2 path can reach this call with a null blockindex: GetTransaction returns a mempool transaction without setting hash_block, and the subsequent block-index lookup does not find a block. BlockManager::IsBlockPruned dereferences its argument when m_have_pruned is true, before the later !blockindex condition can select the no-undo fallback. Guard the pruning query with the null check so this supported lookup cannot terminate a pruned node.
- [NITPICK] src/rpc/rawtransaction.cpp:176: Preserve bitcoin#26734's complete script-type help
Upstream #26734 makes the shared ScriptPubKeyDoc helper enumerate output types with GetAllOutputTypes(), but this adaptation retains the old single pubkeyhash example. GetAllOutputTypes() is already declared and implemented at the Dash review base, including Dash's output-type range, so no missing prerequisite prevents carrying this documentation change. Use the upstream expression to preserve the complete help for decoded outputs and verbosity-2 prevouts.
In `src/wallet/rpc/backup.cpp`:
- [BLOCKING] src/wallet/rpc/backup.cpp:590: Close import progress when the pruning precheck throws
EnsureBlockDataFromTime throws when the required history has been pruned, but importwallet has already emitted progress 0 and subsequent updates. Unwinding skips both completion notifications. The chain progress signal reaches BitcoinGUI::showProgress, which creates an application-modal dialog with automatic closing disabled and explicitly closes it only at progress 100; RPCConsole's exception handler only reports the error. This PR moves pruning rejection from before the dialog opens to this unprotected path, leaving stale modal progress after an expected import error. Emit completion before rethrowing or provide scoped progress cleanup.
In `test/lint/lint-spelling.py`:
- [BLOCKING] test/lint/lint-spelling.py:15: Complete bitcoin#26598's builder-key inventory removal
The PR advertises bitcoin#26598 without a partial-scope qualification, but its central inventory-removal transformation is missing. Upstream removes contrib/builder-keys/keys.txt together with the README and links; Dash's earlier #11909 adaptation changed only the README, so Dash still stores the inventory as ten .pgp files. All ten remain unchanged here while their documentation and spelling exclusion are removed. Adapt the removal to Dash's retained inventory, or explicitly declare and justify retaining it and preserve appropriate documentation and lint handling. If retention is intentional, qualify commit 352699e161 as well: its current message describes key removal and migration that its documentation/lint-only diff does not perform. The blocker is the undeclared incomplete advertised backport, not a claim that retaining public keys is itself a runtime vulnerability.
In `test/functional/wallet_signer.py`:
- [SUGGESTION] test/functional/wallet_signer.py:188-191: Document the declared omission of bitcoin#21576's signer regression test
Commit ab8e1636c9 explicitly limits this backport to refactorings and excludes bumpfee, so the missing external-signer fee-bump implementation and regression test are intentional, not a runtime prerequisite blocker. However, the PR title lists #21576 without qualification and the description only says regular V25 backports. Mark #21576 as partial in the PR metadata and state that signer fee-bump support and its regression test are excluded. This documents the existing scope rather than requesting RBF support in Dash.
thepastaclaw
added
the
pastaclaw:changes-requested
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d420426961
ℹ️ 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".
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The pruning-enabled raw-transaction test has a confirmed shutdown failure because it does not account for Dash's governance warning. The script-type help backport also omits upstream's complete enumeration; the null-blockindex crash and builder-key removal are fixed, while the signer-test omission is explicitly documented and the import-progress issue remains intentionally deferred. Validation included source and upstream-diff comparisons plus a focused reproduction using TestNode.stop_node; no end-to-end functional test pass is claimed.
🔴 1 blocking | 💬 1 nitpick(s)
2 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Handle the pruning warning when shutting down node 2
test/functional/rpc_rawtransaction.py:65
The new -prune=1 argument automatically disables governance and makes startup emit EXPECTED_STDERR_NO_GOV_PRUNE. This test never supplies that expected stderr when stopping node 2, so final framework shutdown calls TestNode.stop_node() with an empty expectation. Its stderr comparison raises AssertionError, and BitcoinTestFramework.shutdown() marks the test FAILED even if every RPC assertion succeeds. A focused reproduction using the actual stop_node method confirmed both the failure with the default expectation and success with the warning supplied. Import EXPECTED_STDERR_NO_GOV_PRUNE and explicitly stop node 2 with that expectation after getrawtransaction_verbosity_tests(), following wallet_pruning.py.
source: gpt-6-astra (phase2-reviewer: general)
💬 Nitpick: Preserve bitcoin#26734's complete script-type help
src/rpc/rawtransaction.cpp:176
Upstream commit 97115de introduces ScriptPubKeyDoc() with "The type (one of: " + GetAllOutputTypes() + ")", sharing the complete enumeration between output and prevout documentation. This backport instead shares the older single-example description, also replacing the complete enumeration previously present in the prevout documentation. GetAllOutputTypes() is already available, so retain the upstream description to complete this documentation change. This is a documentation-completeness issue, not a runtime defect or missing-subsystem blocker.
{RPCResult::Type::STR, "type", "The type (one of: " + GetAllOutputTypes() + ")"},
source: gpt-6-astra (phase2-reviewer: backport-reviewer)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This multi-PR backport includes RPC boolean-validation changes, mempool refactoring, and transaction relay-policy changes requiring substantive review, but the extensive mechanical churn does not establish a large or intricate change to a qualifying critical surface. - Phase 1 reviewers: not run (skipped for throughput: 12 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/functional/rpc_rawtransaction.py`:
- [BLOCKING] test/functional/rpc_rawtransaction.py:65: Handle the pruning warning when shutting down node 2
The new `-prune=1` argument automatically disables governance and makes startup emit `EXPECTED_STDERR_NO_GOV_PRUNE`. This test never supplies that expected stderr when stopping node 2, so final framework shutdown calls `TestNode.stop_node()` with an empty expectation. Its stderr comparison raises `AssertionError`, and `BitcoinTestFramework.shutdown()` marks the test FAILED even if every RPC assertion succeeds. A focused reproduction using the actual stop_node method confirmed both the failure with the default expectation and success with the warning supplied. Import `EXPECTED_STDERR_NO_GOV_PRUNE` and explicitly stop node 2 with that expectation after `getrawtransaction_verbosity_tests()`, following `wallet_pruning.py`.
In `src/rpc/rawtransaction.cpp`:
- [NITPICK] src/rpc/rawtransaction.cpp:176: Preserve bitcoin#26734's complete script-type help
Upstream commit 97115de1839c69d63cae6ea772c17905739bf0ae introduces ScriptPubKeyDoc() with `"The type (one of: " + GetAllOutputTypes() + ")"`, sharing the complete enumeration between output and prevout documentation. This backport instead shares the older single-example description, also replacing the complete enumeration previously present in the prevout documentation. GetAllOutputTypes() is already available, so retain the upstream description to complete this documentation change. This is a documentation-completeness issue, not a runtime defect or missing-subsystem blocker.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Legacy wallet import can leave the GUI progress dialog open — EnsureBlockDataFromTime() can still throw after importwallet opens its application-modal progress dialog and before a completion notification closes it. The placement matches upstream commit e6906fc, and the author's reply explicitly defers this inherited behavior to a separate upstream-first follow-up.
- Follow-up: Track exception-safe progress-dialog cleanup separately, as requested by the author.
thepastaclaw
added
the
pastaclaw:changes-requested
|
This pull request has conflicts, please rebase. |
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The backport series is generally consistent with the advertised Bitcoin Core changes, and the previously reported null dereference, builder-key cleanup, and import-progress concerns are either fixed or intentionally deferred. The wallet-pruning functional test still constructs legacy coinbases across Dash DIP3 activation, so its 600-block mining loop can submit invalid blocks and fail to exercise pruning. The reported signer-test and script-documentation omissions are not actionable: the former is explicitly documented in the backport commit, while the latter matches the upstream change as applied here.
🔴 1 blocking
1 finding(s) not shown inline (GitHub refused the PR diff as too large)
🔴 Blocking: Delay DIP3 when mining legacy test blocks
test/functional/wallet_pruning.py:61
This loop mines 600 blocks starting from the current height and calls create_coinbase without dip4_activated=True. On Dash regtest, the batch crosses DIP3 activation at height 432, after which the submitted coinbases lack the required special transaction payload and are rejected with bad-cbtx-version. The test then cannot reliably reach the pruning assertions. Split the pre- and post-DIP3 ranges or construct valid post-activation coinbases, as the corresponding pruning tests do.
source: muse-spark-1.3-contributor (phase1-reviewer: general, backport-reviewer, dash-core-commit-history)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: backport-reviewer); reviewer 3: muse-spark-1.3-contributor (agent: phase1-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — This is a broad multi-PR backport spanning hundreds of source, test, CI, tooling, and documentation files, including behavior changes across RPC parsing, mempool policy, networking, and other core logic, but the diff does not clearly demonstrate a large intricate change directly altering consensus, funds movement, cryptography, peer-facing deserialization, or storage migration. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— backport-reviewer (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— dash-core-commit-history (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 11% left, 5h 100% left),glm-5.3-flash(zai below 15% reserve: 5h 100% left, weekly 13% left) - Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/functional/wallet_pruning.py`:
- [BLOCKING] test/functional/wallet_pruning.py:61: Delay DIP3 when mining legacy test blocks
This loop mines 600 blocks starting from the current height and calls `create_coinbase` without `dip4_activated=True`. On Dash regtest, the batch crosses DIP3 activation at height 432, after which the submitted coinbases lack the required special transaction payload and are rejected with `bad-cbtx-version`. The test then cannot reliably reach the pruning assertions. Split the pre- and post-DIP3 ranges or construct valid post-activation coinbases, as the corresponding pruning tests do.
@thepastaclaw it's not true, becase Please retract your blocking comment |
|
Retracting my blocking comment on : I missed this test's , which set DIP3 and all relevant deployments to height 2000. The 600-block mining loop therefore remains before DIP3 activation and does not hit the reported path. No code change is needed for this finding. |
|
Correction to my retraction: I missed this test\x27s DEPLOYMENT_ARGS, which set DIP3 and all relevant deployments to height 2000. The 600-block mining loop therefore remains before DIP3 activation and does not hit the reported bad-cbtx-version path. I retract the blocking finding on test/functional/wallet_pruning.py; no code change is needed for it. |
…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>
9075a44 test: add regression test for the getrawtransaction segfault (Martin Zumsande) 494a926 rpc: fix getrawtransaction segfault (Martin Zumsande) Pull request description: The crash, reported in bitcoin#28986, happens when calling `getrawtransaction` for any mempool transaction with `verbosity=2`, while pruning, because the rpc calls `IsBlockPruned(const CBlockIndex* pblockindex)`, which dereferences `pblockindex` without a check. For ease of backporting this PR fixes it just locally in `rpc/rawtransaction.cpp` by moving the check for`!blockindex` up so that `IsBlockPruned()` will not be called with a `nullptr`. We might also want to change `IsBlockPruned()` so it doesn't crash when called with a `nullptr`, but I didn't do that here. Fixes bitcoin#28986 ACKs for top commit: maflcko: lgtm test-was-added ACK 9075a44 theStack: Tested ACK 9075a44 Tree-SHA512: 0f7ed52579487196c206e16b45582b64e4b02ecf2a2eb0a31d2f3b52415bc9c64278cb94259314ef14ab7fb393c6195f79b3027d6de471d67614e51474498b11 Co-authored-by: fanquake <fanquake@gmail.com>
…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>
thepastaclaw
removed
the
pastaclaw:changes-requested
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 2 only (queue backlog)
The current head contains no in-scope blocking defects or actionable suggestions. The prior findings are either fixed, withdrawn after verification, or intentionally deferred; the two remaining backport-completeness claims are not actionable because the signer scope is explicitly excluded and the documentation matches the upstream backport diff.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 3: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: backport-reviewer); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
- Triage:
normalbygpt-6-astra(effort low) — The batch combines mostly mechanical copyright updates with RPC boolean validation, transaction standardness policy changes, and mempool ancestor refactoring, warranting ordinary substantive review without clearly introducing large or intricate changes to a qualifying critical surface. - Phase 1 reviewers: not run (skipped for throughput: 28 PRs queued, above the 10 limit)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6-astra— general (completed, effort high); agentphase2-reviewer,gpt-6-astra— backport-reviewer (completed, effort high); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
3 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: