fix: follow-up for #7275: re-do Merge bitcoin/bitcoin#26076: Switch hardened derivation marker to h#7338
Conversation
|
This pull request has conflicts, please rebase. |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit fbffef9) |
WalkthroughThis PR migrates BIP32 hardened key path notation across the wallet and descriptor infrastructure from apostrophe-suffix format (e.g., 44'/0'/0') to h-suffix format (e.g., 44h/0h/0h). The core change updates the descriptor format string used during legacy wallet migration in LegacyScriptPubKeyMan::MigrateToDescriptor. Functional tests and test descriptors are updated to normalize and expect the new notation. Minor whitespace and comment adjustments are included. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/scriptpubkeyman.cpp`:
- Around line 2026-2027: The combo descriptor string literal still uses mixed
notation "0'"; update the account component to use the new 'h' suffix so the
descriptor is fully suffixed (change "0'" to "0h") where desc_str is constructed
using xpub_str, BIP32_PURPOSE_STANDARD, Params().ExtCoinType(), and i in
scriptpubkeyman.cpp; ensure the strprintf call that builds desc_str uses
"combo(%s/%dh/%dh/0h/%d/*)" instead of the current mixed-notation form.
🪄 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: a601391c-e2b5-4e8f-aa7f-7bd67b86c3b4
📒 Files selected for processing (5)
src/script/descriptor.cppsrc/wallet/scriptpubkeyman.cpptest/functional/wallet_descriptor.pytest/functional/wallet_migration.pytest/functional/wallet_signer.py
💤 Files with no reviewable changes (1)
- test/functional/wallet_descriptor.py
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small follow-up backport completing bitcoin#26076's apostrophe→h switch for the migratewallet path that PR #7275 missed. One real completeness issue: the inactive migration combo() descriptor in MigrateToDescriptor() still uses 0' for the third hardened step, which (because ParseKeyPath stores a single apostrophe-style flag set by the last hardened element) causes BIP32PubkeyProvider::ToPrivateString to re-emit the whole path in apostrophe form — leaving listdescriptors(true) for migrated wallets outside the intended bitcoin#26076 conversion. Other agent nits (descriptor.cpp blank line, commented-out wallet_signer.py descriptors) match the upstream snippet verbatim and are not actionable.
🟡 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/scriptpubkeyman.cpp`:
- [SUGGESTION] src/wallet/scriptpubkeyman.cpp:2026-2027: Migration combo() descriptor still emits apostrophes via the third hardened step
The new format string `combo(%s/%dh/%dh/0'/%d/*)` mixes `h` and `'` markers. This is not just cosmetic: `ParseKeyPath` (src/script/descriptor.cpp:830-841) updates a single `apostrophe` style flag on each hardened element, so the trailing `0'` makes `BIP32PubkeyProvider::m_apostrophe=true` for the entire descriptor. `BIP32PubkeyProvider::ToPrivateString` (src/script/descriptor.cpp:412-421) then reuses that single flag, so `listdescriptors(true)` on a migrated wallet will serialize this inactive combo back as `44'/1'/0'/i/*` — defeating this PR's stated goal of switching the migratewallet path to `h` markers. Public output via `ToNormalizedString` forces `normalized=true` and so still renders `h`, which is why `wallet_migration.py` (no longer asserting on private descriptor strings) doesn't catch the regression. Upstream bd13dc2f46 and Dash's earlier #26076 adaptation in `SetupDescriptorGeneration` both converted all three hardened markers; converting `0'`→`0h` here completes the switch and removes the internal inconsistency on a single line.
| const std::string desc_str = strprintf("combo(%s/%dh/%dh/0'/%d/*)", | ||
| xpub_str, BIP32_PURPOSE_STANDARD, Params().ExtCoinType(), i); |
There was a problem hiding this comment.
🟡 Suggestion: Migration combo() descriptor still emits apostrophes via the third hardened step
The new format string combo(%s/%dh/%dh/0'/%d/*) mixes h and ' markers. This is not just cosmetic: ParseKeyPath (src/script/descriptor.cpp:830-841) updates a single apostrophe style flag on each hardened element, so the trailing 0' makes BIP32PubkeyProvider::m_apostrophe=true for the entire descriptor. BIP32PubkeyProvider::ToPrivateString (src/script/descriptor.cpp:412-421) then reuses that single flag, so listdescriptors(true) on a migrated wallet will serialize this inactive combo back as 44'/1'/0'/i/* — defeating this PR's stated goal of switching the migratewallet path to h markers. Public output via ToNormalizedString forces normalized=true and so still renders h, which is why wallet_migration.py (no longer asserting on private descriptor strings) doesn't catch the regression. Upstream bd13dc2 and Dash's earlier bitcoin#26076 adaptation in SetupDescriptorGeneration both converted all three hardened markers; converting 0'→0h here completes the switch and removes the internal inconsistency on a single line.
| const std::string desc_str = strprintf("combo(%s/%dh/%dh/0'/%d/*)", | |
| xpub_str, BIP32_PURPOSE_STANDARD, Params().ExtCoinType(), i); | |
| const std::string desc_str = strprintf("combo(%s/%dh/%dh/0h/%d/*)", | |
| xpub_str, BIP32_PURPOSE_STANDARD, Params().ExtCoinType(), i); |
source: ['claude', 'codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding at src/wallet/scriptpubkeyman.cpp:2026 is FIXED — the migration combo() descriptor now emits combo(%s/%dh/%dh/0h/%d/*), so ParseKeyPath no longer flips BIP32PubkeyProvider::m_apostrophe and migrated private descriptors will not regress to ' markers. One in-scope suggestion remains: the migration functional test still does not assert the actual desc strings returned by listdescriptors(True), leaving the exact regression this PR fixes uncovered. No blockers.
🟡 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 `test/functional/wallet_migration.py`:
- [SUGGESTION] test/functional/wallet_migration.py:107-109: Migration test does not pin the regressed private descriptor output
This loop only verifies `mnemonic` / `mnemonicpassphrase` on each migrated descriptor, but never inspects the `desc` string itself. The bug this PR fixes is exactly that `ParseKeyPath()` flips `BIP32PubkeyProvider::m_apostrophe` based on the last hardened marker it sees, so a future accidental reintroduction of a mixed path like `.../0'...` in `MigrateToDescriptor()` would silently make migrated private descriptors serialize with apostrophes again without failing this test. Asserting that no `'` appears in the migrated `desc` strings directly pins the regression you're fixing.
|
LGTM overall but missing changes in diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py
index a50d69efb5..eb0610cb25 100755
--- a/test/functional/wallet_signer.py
+++ b/test/functional/wallet_signer.py
@@ -129,14 +129,14 @@ class WalletSignerTest(BitcoinTestFramework):
assert mock_wallet.getwalletinfo()['private_keys_enabled']
result = mock_wallet.importdescriptors([{
- "desc": "pkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#6k3x80k9",
+ "desc": "pkh([00000001/84h/1h/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#ta5h29a8",
"timestamp": 0,
"range": [0,1],
"internal": False,
"active": True
},
{
- "desc": "pkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/*)#tz5866xa",
+ "desc": "pkh([00000001/84h/1h/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/*)#6f3khsdl",
"timestamp": 0,
"range": [0, 0],
"internal": True, |
good catch, but it should be Force-pushed |
pls check original PR - it kept |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/functional/wallet_signer.py`:
- Line 138: Adjust the indentation of the continuation line that contains a lone
"{" so it aligns with the first descriptor object in the surrounding list
literal in test/functional/wallet_signer.py; locate the list of descriptor
objects (the multi-line list where the brace starts) and move the "{" to the
same column as the first descriptor entry to satisfy E122 (ensure opening brace
lines up with the first item in the list).
- Line 169: The derivation path string in wallet_signer.py is malformed
("84h/1h/h'")—update that part of the descriptor string (the "wpkh([...") to use
a proper hardened third index (replace "h'" with "0h'" so the path reads
"84h/1h/0h'") to match the other examples (lines referencing the same descriptor
pattern), and then recompute any checksum for the descriptor string if present.
🪄 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: ec21a816-28bc-487e-8784-22fda0c872b7
📒 Files selected for processing (5)
src/script/descriptor.cppsrc/wallet/scriptpubkeyman.cpptest/functional/wallet_descriptor.pytest/functional/wallet_migration.pytest/functional/wallet_signer.py
💤 Files with no reviewable changes (1)
- test/functional/wallet_descriptor.py
✅ Files skipped from review due to trivial changes (1)
- src/script/descriptor.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- test/functional/wallet_migration.py
- src/wallet/scriptpubkeyman.cpp
| "internal": False, | ||
| "active": True | ||
| }, | ||
| { |
There was a problem hiding this comment.
Verify indentation of the continuation line.
Flake8 flagged this line for missing or incorrect indentation (E122). Ensure the opening brace aligns properly with the first descriptor object in the list.
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 138-138: continuation line missing indentation or outdented
(E122)
🤖 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 `@test/functional/wallet_signer.py` at line 138, Adjust the indentation of the
continuation line that contains a lone "{" so it aligns with the first
descriptor object in the surrounding list literal in
test/functional/wallet_signer.py; locate the list of descriptor objects (the
multi-line list where the brace starts) and move the "{" to the same column as
the first descriptor entry to satisfy E122 (ensure opening brace lines up with
the first item in the list).
… to h Merged PR dashpay#7275 introduced migratewallet RPC and relevant changes from bitcoin#26076 has not been backported earlier This PR fixes functional test wallet_migrate.py and RPC output for migratewallet Co-authored-by: Andrew Chow <github@achow101.com>
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 `@test/functional/wallet_signer.py`:
- Line 132: The descriptor origin strings in test/functional/wallet_signer.py
use mixed hardened notation like "84h/1h/0'" and must be migrated to
fully-hardened notation "84h/1h/0h"; update each descriptor occurrence (the
shown descriptor value and the other instances noted at the same file) replacing
0' with 0h in both active test data and commented examples, then recompute and
replace the descriptor checksums for each edited descriptor so the tests remain
valid.
🪄 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: 32acb395-9926-47a0-ad79-50adcb64ee75
📒 Files selected for processing (5)
src/script/descriptor.cppsrc/wallet/scriptpubkeyman.cpptest/functional/wallet_descriptor.pytest/functional/wallet_migration.pytest/functional/wallet_signer.py
💤 Files with no reviewable changes (1)
- test/functional/wallet_descriptor.py
✅ Files skipped from review due to trivial changes (1)
- src/script/descriptor.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- test/functional/wallet_migration.py
- src/wallet/scriptpubkeyman.cpp
|
|
||
| result = mock_wallet.importdescriptors([{ | ||
| "desc": "pkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#6k3x80k9", | ||
| "desc": "pkh([00000001/84h/1h/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/0/*)#ta5h29a8", |
There was a problem hiding this comment.
Complete the hardened-marker migration in descriptor origins.
These descriptors still use mixed notation (84h/1h/0'). For this migration, the origin path should be fully h-suffixed (84h/1h/0h) in both active imports and commented examples, and descriptor checksums must be recalculated after the edit.
Proposed update pattern
- "desc": "pkh([00000001/84h/1h/0']... )#<old_checksum>",
+ "desc": "pkh([00000001/84h/1h/0h]... )#<recomputed_checksum>",
- # "desc": "wpkh([00000001/84h/1h/0']... )#<old_checksum>",
+ # "desc": "wpkh([00000001/84h/1h/0h]... )#<recomputed_checksum>",Also applies to: 139-139, 161-161, 169-169
🤖 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 `@test/functional/wallet_signer.py` at line 132, The descriptor origin strings
in test/functional/wallet_signer.py use mixed hardened notation like "84h/1h/0'"
and must be migrated to fully-hardened notation "84h/1h/0h"; update each
descriptor occurrence (the shown descriptor value and the other instances noted
at the same file) replacing 0' with 0h in both active test data and commented
examples, then recompute and replace the descriptor checksums for each edited
descriptor so the tests remain valid.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Verified at cfa0484. Latest delta (07979cd..cfa0484) is a small follow-up that converts apostrophe-style hardened markers to h in test/functional/wallet_signer.py. The active descriptors at lines 132/139 are correctly migrated. One transcription typo was introduced in a commented-out block at line 169 (84h/1h/h' instead of 84h/1h/0h) — no runtime effect, but it contradicts the PR's stated goal. The prior-review test-coverage gap in wallet_migration.py is still unaddressed at this head and is carried forward.
🟡 1 suggestion(s) | 💬 1 nitpick(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 `test/functional/wallet_migration.py`:
- [SUGGESTION] test/functional/wallet_migration.py:107-109: Carried forward: migration test doesn't pin the regressed private descriptor output
STILL VALID at cfa0484c — the latest delta only touched wallet_signer.py. This loop verifies only `mnemonic` / `mnemonicpassphrase` on each migrated descriptor returned by `listdescriptors(True)`, but never inspects the `desc` string itself. The exact bug this PR closes is that `ParseKeyPath()` flips a single hardened-marker style flag on `BIP32PubkeyProvider` based on the last hardened component it sees, so a future accidental reintroduction of a mixed path like `.../0'...` in `LegacyScriptPubKeyMan::MigrateToDescriptor()` would silently make migrated private descriptors serialize with apostrophes again without failing this test. Asserting that no `'` appears in the migrated `desc` strings here directly pins the regression #7338 is intended to fix.
| # }, | ||
| # { | ||
| # "desc": "wpkh([00000001/84'/1'/0']tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/*)#h92akzzg", | ||
| # "desc": "wpkh([00000001/84h/1h/h']tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/*)#h92akzzg", |
There was a problem hiding this comment.
💬 Nitpick: Latest-delta typo: 84h/1h/h' should be 84h/1h/0h
This delta replaces apostrophe-style hardened markers with h across this file. Three of the four conversions are correct, but on line 169 the literal 0 was replaced with h while the trailing apostrophe was left in place, producing the invalid key-origin component h' ([00000001/84h/1h/h']). The sibling descriptor on line 161 was correctly updated to [00000001/84h/1h/0h], so this is clearly an unintended slip in the same hunk. The block is fully commented out, so there is no runtime impact today, but if it is ever uncommented (as upstream does), the import will fail to parse, and the typo contradicts the PR's stated goal of completing the bitcoin#26076 marker switch. Cheap to fix here rather than carry forward.
| # "desc": "wpkh([00000001/84h/1h/h']tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/*)#h92akzzg", | |
| # "desc": "wpkh([00000001/84h/1h/0h]tpubD6NzVbkrYhZ4WaWSyoBvQwbpLkojyoTZPRsgXELWz3Popb3qkjcJyJUGLnL4qHHoQvao8ESaAstxYSnhyswJ76uZPStJRJCTKvosUCJZL5B/1/*)#h92akzzg", |
source: ['claude', 'codex']
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta (cfa0484..fbffef9) only adjusts 4 descriptor literals in test/functional/wallet_signer.py to match upstream bitcoin#26076 (reverting 0h→0' on lines 132/139/161/169). This both aligns with upstream and incidentally fixes the prior h' typo at line 169. The substantive PR fix at src/wallet/scriptpubkeyman.cpp:2026 (combo descriptor format string using %dh/%dh/0h) is preserved. One carried-forward suggestion remains: wallet_migration.py asserts only mnemonic/mnemonicpassphrase on migrated descriptors, never the desc string itself, so the regression this PR closes is not directly pinned.
🟡 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 `test/functional/wallet_migration.py`:
- [SUGGESTION] test/functional/wallet_migration.py:107-109: Migration test does not pin the regressed private descriptor serialization
This loop verifies only `mnemonic` / `mnemonicpassphrase` on each migrated descriptor returned by `listdescriptors(True)`, but never inspects the `desc` string. The bug this PR closes is that `ParseKeyPath()` stores a single hardened-marker style flag on `BIP32PubkeyProvider` based on the last hardened component, so a future accidental reintroduction of a mixed path like `.../0'...` in `LegacyScriptPubKeyMan::MigrateToDescriptor()` (currently fixed at src/wallet/scriptpubkeyman.cpp:2026) would silently make migrated private descriptors serialize with apostrophes again — without failing this test. Asserting that no `'` appears in each migrated private `desc` directly locks in the bitcoin#26076 behavior for the migratewallet path.
4 participants
Issue being fixed or feature implemented
PRs #7275 had a logical conflict with earlier merged #7237
PR #7275 introduces migratewallet RPC and relevant changes from bitcoin#26076 has never been backported because this code didn't exist.
What was done?
Added couple missing changes from bitcoin#26076 to fix RPC migratewallet, functional test wallet_migrate.py and couple other tiny other nits.
How Has This Been Tested?
Run wallet_migrate.py
Breaking Changes
N/A
Checklist: