backport: Merge bitcoin#29144, 28946, 28980, 28554, 28414#7354
backport: Merge bitcoin#29144, 28946, 28980, 28554, 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
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 Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between baf55e17359556ada273517c463ee137f129efab and d2ce666. 📒 Files selected for processing (16)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (13)
WalkthroughThis PR merges four independent improvements. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
|
✅ Final review complete — no blockers (commit d2ce666) |
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)
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
…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
…sspsbt 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
|
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",
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.
4 participants
bitcoin back ports