backport: Merge bitcoin#29144, 28946, 28980, 28554, (partial) 28414 - #7354
backport: Merge bitcoin#29144, 28946, 28980, 28554, (partial) 28414#7354vijaydasmp wants to merge 5 commits into
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
vijaydasmp
changed the title
c3073b7 to
e5faf0f
Compare
e901404 settings: add auto-generated warning msg for editing the file manually (furszy) 966f5de init: improve corrupted/empty settings file error msg (furszy) Pull request description: Small and simple issue reported [here](https://community.umbrel.com/t/bitcoin-docker-container-keeps-restarting/2144). Improving a confusing situation reported by users who did not understand why a settings parsing error occurred when the file was empty and did not know how to solve it. Empty setting file could be due (1) corruption or (2) an user manually cleaning up the file content. In both scenarios, the 'Unable to parse settings file' error does not help the user move forward. ACKs for top commit: achow101: ACK e901404 hebasto: re-ACK e901404. ryanofsky: Code review ACK e901404. Just whitespace formatting changes and shortening a test string literal since last review shaavan: Code review ACK e901404 Tree-SHA512: 2910654c6b9e9112de391eedb8e46980280f822fa3059724dd278db7436804dd27fae628d2003f2c6ac1599b07ac5c589af016be693486e949f558515e662bec
8f6ab31 init: don't delete PID file if it was not generated (willcl-ark) Pull request description: In a similar vein to bitcoin#28784, if a second `bitcoind` is started using the same datadir it will fail to start up, but during shutdown remove the PID file from the first `bitcoind` instance. ACKs for top commit: achow101: ACK 8f6ab31 andrewtoth: ACK 8f6ab31 romanz: ACK bitcoin@8f6ab31 Tree-SHA512: c9af703cbfa179d33ef9580a51e86c1b0acbd28daa18c8d2e5e5ff796ab4d3e2009a962a47e6046a0e5ece936f8a06ee8af5fdf8ff4ae1e52cbcdbec4b942271
fd42f3d to
baf55e1
Compare
![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: baf55e1735
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "For security reasons, the encryption process will generate a new HD seed, resulting\n" | ||
| "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n" | ||
| "securely back up the newly generated wallet file using the backupwallet RPC.\n", |
There was a problem hiding this comment.
For every nonblank HD wallet, this help text claims encryption generates a new seed and descriptors, but CWallet::EncryptWallet() explicitly keeps the existing seed and mnemonic for descriptor wallets and only tops up legacy HD wallets (src/wallet/wallet.cpp:872-883). This can mislead users about which backup material controls their funds; document Dash's actual behavior instead of the Bitcoin Core seed-rotation behavior.
Useful? React with 👍 / 👎.
| if (!values.contains("_warning_")) { | ||
| out.pushKV("_warning_", strprintf("This file is automatically generated and updated by %s. Please do not edit this file while the node " | ||
| "is running, as any changes might be ignored or overwritten.", | ||
| PACKAGE_NAME)); |
There was a problem hiding this comment.
After the first settings write, the next startup reads _warning_ into m_settings.rw_settings because ReadSettings() imports every JSON key. ArgsManager::ReadSettingsFile() then treats it as an unknown setting and logs Ignoring unknown rw_settings value _warning_, followed by logging it as a setting, on every normal startup. Filter this metadata key while reading (or before settings validation) so the generated warning is not interpreted as configuration.
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughThis PR adds an optional serialized transaction Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WalletTest
participant walletprocesspsbt
participant sendrawtransaction
WalletTest->>walletprocesspsbt: Process and finalize PSBT
walletprocesspsbt-->>WalletTest: Return complete PSBT and hex
WalletTest->>sendrawtransaction: Submit returned hex
sendrawtransaction-->>WalletTest: Return transaction id
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/rpc/encrypt.cpp`:
- Around line 223-227: The help text in the encryptwallet RPC command (around
lines 223-227) claims encryption always generates a fresh HD seed and
descriptors, but the actual returned message (around lines 270-273)
distinguishes between HD-enabled and non-HD wallets with different backup
guidance. Update the help text to conditionally describe the backup behavior
based on wallet type: for HD-enabled wallets, mention the fresh HD
seed/descriptors generation and backupwallet requirement; for non-HD wallets,
provide the appropriate alternative guidance. This aligns the help documentation
with the actual runtime behavior and prevents conflicting instructions to users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ae84c2f5-b22f-4a95-95bc-2abe0a3435e6
📥 Commits
Reviewing files that changed from the base of the PR and between 7fcc54a and baf55e17359556ada273517c463ee137f129efab.
📒 Files selected for processing (16)
doc/release-notes-28414.mdsrc/init.cppsrc/qt/test/optiontests.cppsrc/rpc/mining.cppsrc/test/settings_tests.cppsrc/util/settings.cppsrc/wallet/rpc/encrypt.cppsrc/wallet/rpc/spend.cpptest/functional/feature_filelock.pytest/functional/feature_settings.pytest/functional/rpc_blockchain.pytest/functional/rpc_psbt.pytest/functional/wallet_fundrawtransaction.pytest/functional/wallet_multisig_descriptor_psbt.pytest/functional/wallet_send.pytest/functional/wallet_signer.py
| "If the wallet is already encrypted, use the walletpassphrasechange call.\n" | ||
| "** IMPORTANT **\n" | ||
| "For security reasons, the encryption process will generate a new HD seed, resulting\n" | ||
| "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n" | ||
| "securely back up the newly generated wallet file using the backupwallet RPC.\n", |
There was a problem hiding this comment.
Keep the backup guidance consistent with the actual wallet state.
The new help text says encryption always generates a fresh HD seed/descriptors, but the returned message still distinguishes HD-enabled wallets from non-HD wallets, and the provided GUI/docs snippets show those cases need different backup guidance. As written, users can get conflicting instructions about whether to preserve seed-based backups or make a new backupwallet backup.
Based on the provided GUI/docs snippets, the RPC text should match the same HD/non-HD behavior.
🛠 Suggested wording adjustment
- "For security reasons, the encryption process will generate a new HD seed, resulting\n"
- "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n"
- "securely back up the newly generated wallet file using the backupwallet RPC.\n",
+ "For security reasons, encrypting the wallet changes the backup procedure.\n"
+ "If the wallet is HD-enabled, preserve the existing seed-based backup guidance; otherwise,\n"
+ "create a new backup with the backupwallet RPC.\n",
...
- return "wallet encrypted; The keypool has been flushed. You need to make a new backup with the backupwallet RPC.";
+ return "wallet encrypted; The keypool has been flushed. Make sure you create a new backup with the backupwallet RPC.";Also applies to: 270-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/rpc/encrypt.cpp` around lines 223 - 227, The help text in the
encryptwallet RPC command (around lines 223-227) claims encryption always
generates a fresh HD seed and descriptors, but the actual returned message
(around lines 270-273) distinguishes between HD-enabled and non-HD wallets with
different backup guidance. Update the help text to conditionally describe the
backup behavior based on wallet type: for HD-enabled wallets, mention the fresh
HD seed/descriptors generation and backupwallet requirement; for non-HD wallets,
provide the appropriate alternative guidance. This aligns the help documentation
with the actual runtime behavior and prevents conflicting instructions to users.
|
✅ Review complete (commit 2c6ca84) |
vijaydasmp
changed the title
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Backport PR cherry-picking five small Bitcoin Core PRs (29144, 28946, 28980, 28554, 28414). All five adapt cleanly to Dash with appropriate symbol/branding substitutions, tests updated correctly, and no consensus-affecting changes. No blocking issues. One nitpick on PR title scope mismatch. Most agent findings critique upstream design choices on a backport, which is out of scope per the backport review process.
💬 1 nitpick(s)
|
This pull request has conflicts, please rebase. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative review of a clean five-commit Bitcoin backport stack (bitcoin#29144, bitcoin#28946, bitcoin#28980, bitcoin#28554, bitcoin#28414). The prior nitpick about a missing bitcoin#27302 cherry-pick is resolved (PR title now matches the stack). One in-scope suggestion remains: the encryptwallet help text imported verbatim from bitcoin#28980 advertises a new-HD-seed-on-encryption behavior that Dash deliberately does not perform for descriptor wallets, making the help misleading for Dash users.
🟡 1 suggestion(s)
🤖 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/wallet/rpc/encrypt.cpp`:
- [SUGGESTION] src/wallet/rpc/encrypt.cpp:224-227: encryptwallet help text describes upstream-only HD-seed rotation that Dash does not perform
The IMPORTANT block backported from bitcoin#28980 tells users that `encryptwallet` will "generate a new HD seed, resulting in the creation of a fresh set of active descriptors," but `CWallet::EncryptWallet` in src/wallet/wallet.cpp:872-874 explicitly keeps the existing seed for descriptor wallets (`// Do nothing for descriptor wallets (keep old seed / mnemonic)`). The success string at src/wallet/rpc/encrypt.cpp:271 corroborates this — it tells HD users to back up their existing seed/mnemonic, not a new one. Only the legacy non-HD branch at src/wallet/wallet.cpp:887 flushes the keypool. As written, this help text will mislead Dash users into believing their pre-encryption seed backup is now stale, or push them to perform an unnecessary fresh backup. Either drop the IMPORTANT block or rephrase it to reflect Dash's actual behavior (only the legacy non-HD path needs a new backup; HD/descriptor wallets retain their existing mnemonic).
| "** IMPORTANT **\n" | ||
| "For security reasons, the encryption process will generate a new HD seed, resulting\n" | ||
| "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n" | ||
| "securely back up the newly generated wallet file using the backupwallet RPC.\n", |
There was a problem hiding this comment.
🟡 Suggestion: encryptwallet help text describes upstream-only HD-seed rotation that Dash does not perform
The IMPORTANT block backported from bitcoin#28980 tells users that encryptwallet will "generate a new HD seed, resulting in the creation of a fresh set of active descriptors," but CWallet::EncryptWallet in src/wallet/wallet.cpp:872-874 explicitly keeps the existing seed for descriptor wallets (// Do nothing for descriptor wallets (keep old seed / mnemonic)). The success string at src/wallet/rpc/encrypt.cpp:271 corroborates this — it tells HD users to back up their existing seed/mnemonic, not a new one. Only the legacy non-HD branch at src/wallet/wallet.cpp:887 flushes the keypool. As written, this help text will mislead Dash users into believing their pre-encryption seed backup is now stale, or push them to perform an unnecessary fresh backup. Either drop the IMPORTANT block or rephrase it to reflect Dash's actual behavior (only the legacy non-HD path needs a new backup; HD/descriptor wallets retain their existing mnemonic).
source: ['claude']
There was a problem hiding this comment.
28980: thepastaclaw's, coderabbitai's and codex's warnings are valid.
if (pwallet->IsHDEnabled()) {
return "wallet encrypted; If you forget the passphrase, you will lose access to your funds. Make sure that you have backup of your seed or mnemonic.";
}
and
// If we are using descriptors, make new descriptors with a new seed
if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
// Do nothing for descriptor wallets (keep old seed / mnemonic)
} else if (auto spk_man = GetLegacyScriptPubKeyMan()) {
// if we are not using HD, generate new keypool
actually, this comment is misleading in develop / master branches and should be fixed:
- // If we are using descriptors, make new descriptors with a new seed@vijaydasmp please keep changes for `return "wallet encrypted...." (line 273) but remove changes in RPC help with this warning
- "** IMPORTANT **\n"
- "For security reasons, the encryption process will generate a new HD seed, resulting\n"
- "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n"
- "securely back up the newly generated wallet file using the backupwallet RPC.\n",There was a problem hiding this comment.
Resolved in this update — encryptwallet help text describes upstream-only HD-seed rotation that Dash does not perform no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
29144: ok
28946: ok
28980: requires dashification
28554: ok
28814: make partial
| "** IMPORTANT **\n" | ||
| "For security reasons, the encryption process will generate a new HD seed, resulting\n" | ||
| "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n" | ||
| "securely back up the newly generated wallet file using the backupwallet RPC.\n", |
There was a problem hiding this comment.
28980: thepastaclaw's, coderabbitai's and codex's warnings are valid.
if (pwallet->IsHDEnabled()) {
return "wallet encrypted; If you forget the passphrase, you will lose access to your funds. Make sure that you have backup of your seed or mnemonic.";
}
and
// If we are using descriptors, make new descriptors with a new seed
if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) {
// Do nothing for descriptor wallets (keep old seed / mnemonic)
} else if (auto spk_man = GetLegacyScriptPubKeyMan()) {
// if we are not using HD, generate new keypool
actually, this comment is misleading in develop / master branches and should be fixed:
- // If we are using descriptors, make new descriptors with a new seed@vijaydasmp please keep changes for `return "wallet encrypted...." (line 273) but remove changes in RPC help with this warning
- "** IMPORTANT **\n"
- "For security reasons, the encryption process will generate a new HD seed, resulting\n"
- "in the creation of a fresh set of active descriptors. Therefore, it is crucial to\n"
- "securely back up the newly generated wallet file using the backupwallet RPC.\n",| final = self.nodes[0].finalizepsbt(signed["psbt"]) | ||
| assert self.nodes[0].testmempoolaccept([final["hex"]])[0]["allowed"] | ||
| final = signed["hex"] | ||
| assert self.nodes[0].testmempoolaccept([final])[0]["allowed"] |
There was a problem hiding this comment.
28414: mark as partial due to missing changes for taproot.
… errors Backport of bitcoin#27708 Automated backport — cherry-pick of `c92fd63886`. Prerequisite: - bitcoin#27150 is backported in the immediately preceding commit, so ParseArgs() now uses common::InitConfig() exactly as upstream intends. The AbortError-to-InitError conversion remains in that prerequisite with its upstream owner, d172b5c. Dash adaptations: - Preserve Dash-only -printcrashinfo as an early successful-return command: skip config/datadir/loose-token handling after command-line syntax parses, then print it from ProcessInitCommands(). Focused regression coverage is retained in feature_help.py. - Retain Dash CoreContext context{node}; in AppInit() for Dash subsystem lifetime management after upstream splits argument handling from startup. - Adapt the fatal-index change to Dash current BaseIndex::FatalErrorImpl(). - Keep Dash interfaces::Node and AppInitParameterInteraction API shapes while threading the new exit status through them. Excluded / deferred (intentional): - No invented src/bitcoin-chainstate.cpp InitShutdownState/exit_status wiring; that has no upstream counterpart and is not part of this backport. - No bitcoin#28946 PID-file ownership fix (g_generated_pid / feature_filelock coverage); that already belongs to separate dashpay#7354. - src/qt/bitcoin.cpp does not add node/context.h because Dash reads the status through interfaces::Node::getExitStatus(), so the header would be unused.
… errors Backport of bitcoin#27708 Automated backport — cherry-pick of `c92fd63886`. Prerequisite: - bitcoin#27150 is backported in the immediately preceding commit, so ParseArgs() now uses common::InitConfig() exactly as upstream intends. The AbortError-to-InitError conversion remains in that prerequisite with its upstream owner, d172b5c. Dash adaptations: - Preserve Dash-only -printcrashinfo as an early successful-return command: skip config/datadir/loose-token handling after command-line syntax parses, then print it from ProcessInitCommands(). Focused regression coverage is retained in feature_help.py. - Retain Dash CoreContext context{node}; in AppInit() for Dash subsystem lifetime management after upstream splits argument handling from startup. - Adapt the fatal-index change to Dash current BaseIndex::FatalErrorImpl(). - Keep Dash interfaces::Node and AppInitParameterInteraction API shapes while threading the new exit status through them. Excluded / deferred (intentional): - No invented src/bitcoin-chainstate.cpp InitShutdownState/exit_status wiring; that has no upstream counterpart and is not part of this backport. - No bitcoin#28946 PID-file ownership fix (g_generated_pid / feature_filelock coverage); that already belongs to separate dashpay#7354. - src/qt/bitcoin.cpp does not add node/context.h because Dash reads the status through interfaces::Node::getExitStatus(), so the header would be unused.
5b499d2 backport: Merge bitcoin#27810: fuzz: Partially revert bitcoin#27780 (Lil Claw) 1e4bea7 backport: Merge bitcoin#27780: fuzz: Avoid timeout in utxo_total_supply (Lil Claw) be0337e backport: Merge bitcoin#27708: Return EXIT_FAILURE on post-init fatal errors (Lil Claw) a69ad0c backport: Merge bitcoin#27150: Deduplicate init code (Lil Claw) f7999f7 backport: Merge bitcoin#27170: refactor: Stop using gArgs global in system.cpp (Lil Claw) 0663ec9 backport: Merge bitcoin#27778: ci: Enable float-divide-by-zero check (Lil Claw) 4c8f13c build: adapt dash-chainstate verification call (PastaClaw) Pull request description: Batch 3 of Bitcoin Core v0.26 backports. **Dash support commit:** - build: adapt dash-chainstate verification call This keeps the experimental `dash-chainstate` verification call aligned with Dash's current `VerifyLoadedChainstate(ChainstateManager&, ...)` signature. **6 Bitcoin backports:** - bitcoin#27778 — ci: Enable float-divide-by-zero check - bitcoin#27170 — refactor: Stop using gArgs global in system.cpp - bitcoin#27150 — Deduplicate bitcoind and bitcoin-qt init code - bitcoin#27708 — Return EXIT_FAILURE on post-init fatal errors - bitcoin#27780 — fuzz: Avoid timeout in utxo_total_supply - bitcoin#27810 — fuzz: Partially revert bitcoin#27780 **Removed / deferred from this batch:** - bitcoin#27695 — deferred; not retained in this PR. - bitcoin#27625 — deferred; backported fully only after #5943 rather than kept as a heavily adapted non-partial backport. - bitcoin#27783 — removed after review because Dash has not yet backported the txrequest prerequisite (bitcoin#19988 / dash#5943). **Intentional exclusions / Dash adaptations:** - bitcoin#27170: Dash's existing empty `dash.conf` creation path now passes the current `ArgsManager` to `GetConfigFile()` after the upstream API is made explicit. No upstream hunks are excluded. - bitcoin#27150: Dash retains CoreContext construction, `-printcrashinfo`, and Dash-specific binary/config names. Its shared `common::InitConfig()` uses `CheckDataDirOption(args)` from the immediately preceding bitcoin#27170 prerequisite. The fatal-index change is adapted to Dash's current `BaseIndex::FatalErrorImpl()`. Nested `tfm::format()` calls are qualified for Dash's C++20 build, matching the later upstream ADL fix. `src/common/init.cpp` is registered in Dash's IWYU script because upstream's CI script no longer exists here. - bitcoin#27708: bitcoin#27150 is backported immediately before it, so `ParseArgs()` uses `common::InitConfig()` as upstream intends. Dash-only `-printcrashinfo` remains an early successful-return command (skip config/datadir/loose-token handling after command-line syntax parses, then print from `ProcessInitCommands()`) with focused regression coverage in `feature_help.py`. Dash's CoreContext lifetime handling is retained. Interfaces/Node and AppInitParameterInteraction shapes keep the new exit-status threading. No invented `src/bitcoin-chainstate.cpp` InitShutdownState/exit_status wiring (no upstream counterpart). No bitcoin#28946 PID-file ownership fix (`g_generated_pid` / `feature_filelock` coverage); that already belongs to separate #7354. `src/qt/bitcoin.cpp` does not add `node/context.h` because Dash reads status through `interfaces::Node::getExitStatus()`. - bitcoin#27810: retained directly after bitcoin#27780 because it fixes the duplicate-coinbase assertion being evaluated for rejected blocks. Automated backport via PastaClaw backport pipeline. Each Bitcoin backport was individually cherry-picked and reviewer-verified before batching. Top commit has no ACKs. Tree-SHA512: 8c8b7901d068fcb502b399b326a75c90e52d3741ea8c14f200240f4d75ecf8369d8c0aa4a111d691034765a9d27a937aed94b1df8dcc047e1ffc375de8fb318c
vijaydasmp
changed the title
…n and backup requirement ca09415 rpc, doc: encryptwallet, mention HD seed rotation and new backup (furszy) Pull request description: Small and simple PR, updating the `encryptwallet` help message. Better to notify users about the HD seed rotation and the new backup requirement before executing the encryption process. Ensuring they are prepared to update previous backups and securely safeguard the updated wallet file. ACKs for top commit: S3RK: ACK ca09415 achow101: ACK ca09415 Tree-SHA512: f0ee65f5cea66450566e3a85e066d4c06b3293dd0e0b2ed5fafdb7fb11da0a2cd94407299a3c57a0706c2ed782f8eabb73443e85d8099a62a3fb10a02636ab46
…s passed to getnetworkhashps RPC 9ac114e Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp) Pull request description: When writing some scripts that iterated over many blocks to generate hashrate estimates I realized that my script was going out of range of the current chain tip height but was not encountering any errors. I believe that passing an invalid block height to this function but receiving the hashrate estimate for the chain tip instead should be considered unexpected behavior. ACKs for top commit: Sjors: re-utACK 9ac114e kevkevinpal: reACK [9ac114e](bitcoin@9ac114e) achow101: ACK 9ac114e Tree-SHA512: eefb465c2dd654fc48267f444e1809597ec5363cdd131ea9ec812458fed1e4bffbbbb0617d74687c9f7bb16274b598d8292f5eeb7953421e5d2a8dc2cc081f2b
…alletprocesspsbt if complete 2e249b9 doc: add release note for PR bitcoin#28414 (Matthew Zipkin) 4614332 test: remove unnecessary finalizepsbt rpc calls (ismaelsadeeq) e3d484b wallet rpc: return final tx hex from walletprocesspsbt if complete (Matthew Zipkin) Pull request description: See bitcoin#28363 (comment) `walletprocesspsbt` currently returns a base64-encoded PSBT and a boolean indicating if the tx is "complete". If it is complete, the base64 PSBT can be finalized with `finalizepsbt` which returns the hex-encoded transaction suitable for `sendrawtransaction`. With this patch, `walletprocesspsbt` return object will ALSO include the broadcast-able hex string if the tx is already final. This saves users the extra step of calling `finalizepsbt` assuming they have already inspected and approve the transaction from earlier steps. ACKs for top commit: ismaelsadeeq: re ACK 2e249b9 BrandonOdiwuor: re ACK 2e249b9 Randy808: Tested ACK 2e249b9 achow101: ACK 2e249b9 ishaanam: ACK 2e249b9 Tree-SHA512: 229c1103265a9b4248f080935a7ad5607c3be3f9a096a9ab6554093b2cd8aa8b4d1fa55b1b97d3925ba208dbc3ccba4e4d37c40e1491db0d27ba3d9fe98f931e
There was a problem hiding this comment.
Preliminary review — Codex only
The misleading HD-seed-rotation warning from the prior review is absent at the exact head, so that finding is resolved. However, removing the warning also deleted the pre-existing final help sentence and its terminating comma, leaving the RPCHelpMan initializer syntactically invalid and preventing wallet-enabled builds from compiling. The bitcoin#28980 commit message also still claims that HD-seed-rotation guidance was backported even though Dash intentionally omitted it.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Source: reviewers gpt-5.6-sol (general, dash-core-commit-history, backport-reviewer); final verifier gpt-5.6-sol — attempts 1 and 2 returned parseable JSON but failed the deterministic prerequisite-adjudication schema gate, and fresh attempt 3 completed; Sonnet was not run because the blocker gate stopped at preliminary.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/wallet/rpc/encrypt.cpp`:
- [BLOCKING] src/wallet/rpc/encrypt.cpp:222-223: Incorrect bitcoin#28980 adaptation removes a required initializer separator
The Dash parent and the upstream pre-PR code both ended the `RPCHelpMan` description with `"If the wallet is already encrypted, use the walletpassphrasechange call.\n",`. Upstream bitcoin#28980 retained that sentence while moving the terminating comma after its new warning. Dash correctly needs to omit the upstream warning because its HD wallets retain their existing seed, but commit 921eef18c28 deleted the pre-existing sentence and comma as well. The string literal on line 222 is now followed directly by the argument-list braced initializer on line 223, so the second and third `RPCHelpMan` constructor arguments are not separated and this translation unit cannot compile. Restore the pre-existing sentence and comma while continuing to omit the inaccurate warning.
In `<commit:921eef1>`:
- [SUGGESTION] <commit:921eef1>:1: Reword the now-partial bitcoin#28980 backport
Commit 921eef18c28 is titled `Merge bitcoin/bitcoin#28980: rpc: encryptwallet help, mention HD seed rotation and backup requirement`, and its body says encryption rotates the HD seed and requires an updated backup. The Dash adaptation intentionally omits that behavior-specific warning because Dash retains the existing seed; after restoring the accidentally deleted separator, the actual backported change only clarifies use of the `backupwallet` RPC in the non-HD result. Reword this as a partial backport that describes the retained change and records why the upstream HD-seed warning was excluded, while preserving the upstream commit SHA and provenance.
| "Use the walletpassphrase call for this, and then walletlock call.\n" | ||
| "If the wallet is already encrypted, use the walletpassphrasechange call.\n", | ||
| { |
There was a problem hiding this comment.
🔴 Blocking: Incorrect bitcoin#28980 adaptation removes a required initializer separator
The Dash parent and the upstream pre-PR code both ended the RPCHelpMan description with "If the wallet is already encrypted, use the walletpassphrasechange call.\n",. Upstream bitcoin#28980 retained that sentence while moving the terminating comma after its new warning. Dash correctly needs to omit the upstream warning because its HD wallets retain their existing seed, but commit 921eef1 deleted the pre-existing sentence and comma as well. The string literal on line 222 is now followed directly by the argument-list braced initializer on line 223, so the second and third RPCHelpMan constructor arguments are not separated and this translation unit cannot compile. Restore the pre-existing sentence and comma while continuing to omit the inaccurate warning.
| "Use the walletpassphrase call for this, and then walletlock call.\n" | |
| "If the wallet is already encrypted, use the walletpassphrasechange call.\n", | |
| { | |
| "Use the walletpassphrase call for this, and then walletlock call.\n" | |
| "If the wallet is already encrypted, use the walletpassphrasechange call.\n", | |
| { |
source: ['codex']
4 participants
bitcoin back ports