fix(keymap): use correct derivation_path for keys with origin - #863
Merged
apoelstra merged 1 commit intoOct 10, 2025
Merged
Conversation
- Fixes the implementation of `GetKey` for `KeyRequest::Bip32` when there's `Xpriv` has an origin, and it matches with the given `KeyRequest::Bip32` derivation_path. It should strip the matching part, to correctly derive the key at the correct child number. - Updates the existing test to the correct and expected behavior.
oleonardolima
force-pushed
the
fix/getkey-bip32-key-origin-deriv
branch
from
October 10, 2025 01:23
b8f4d24 to
2f72b27
Compare
| if let Some(matched_path) = descriptor_xkey.matches(key_source, secp) { | ||
| let (_, full_path) = key_source; | ||
|
|
||
| let derivation_path = &full_path[matched_path.len()..]; |
Contributor
There was a problem hiding this comment.
The matched path includes the xkey.derivation_path, therefore we need to derive the xkey.derivation_path together with the remaining un-matched path. That's my hunch.
Member
There was a problem hiding this comment.
This was never resolved, did you investigate this @oleonardolima?
Is this what you were talking about at the Summit @ValuedMammal or was that a different bug you think you found?
Contributor
Author
There was a problem hiding this comment.
Yes, VM explained this issue to me at the summit.
I'm planning on working on this issue and adding more integration tests for different signing scenarios this week.
@ValuedMammal let me know if you found any other issues with it.
Contributor
There was a problem hiding this comment.
I think this will work, following the same implementation in bdk.
if let Some(..) = descriptor_xkey.matches(key_source, secp) {
let (_, full_path) = key_source;
let derived = match &descriptor_xkey.origin {
Some((_, origin_path)) => {
let derivation_path = &full_path[origin_path.len()..];
descriptor_xkey.xkey.derive_priv(secp, &derivation_path)?
}
None => descriptor_xkey.xkey.derive_priv(secp, &full_path)?,
};
return Ok(Some(derived.to_priv()));
}
Contributor
Author
There was a problem hiding this comment.
I've opened a new PR with the fix: #872 and I'll work on adding some more testing for different scenarios, specifically the multipath one.
Member
|
Yep, I think this is correct. |
3 tasks
3 tasks
apoelstra
added a commit
that referenced
this pull request
Jun 27, 2026
…info a2f4497 test(key_map): improve `Xprv` key origin tests (Leonardo Lima) 20ae466 fix(key_map): derivation_path for `Xprv` with key origin info (Leonardo Lima) Pull request description: ### Description The `GetKey` for `DescriptorSecretKey` (and thus `KeyMap`) returned the wrong key when an extended private key with key origin info was queried via `KeyRequest::Bip32`. The key request path is master-relative, but the `xkey` is anchored at its origin (e.g. `[d34db33f/84h/1h/0h]`). The old code stripped the path returned by `matches()`; the fix strips the origin prefix instead (and uses the full path when there is no origin) before deriving from the `xkey`. -- Also, this PR introduces the test `get_key_xpriv_with_key_origin` covering the scenarios: bare wildcard; single fixed step; fixed step then wildcard, and the matching/non-matching cases build requests from explicit `(fingerprint, path)` key sources. ### Notes If you'd like an overview on the issue, see: #863 (comment) ### Changelog notice ``` ### Fixed - keymap: fix `GetKey` derivation path for `Xprv` with key origin info [#872](#872) ``` ### Checklists #### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [ ] I'm linking the issue being fixed by this PR ACKs for top commit: notmandatory: ACK a2f4497 noahjoeris: tACK a2f4497 apoelstra: ACK a2f4497; successfully ran local tests Tree-SHA512: 7980e4cfccc8b0b53a054350dd7870122192407c4e17c36f75350093d7be63d4a5e75be40f4e4583c1a1cf0641a469d55842b7108e8bce6e8aa44f519e51b963
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GetKeyforKeyRequest::Bip32when there'sXprivhas an origin, and it matches with the givenKeyRequest::Bip32derivation_path. It should strip the matching part, to correctly derive the key at the correct child number.