key_map.rs: propagate "invalid key" errors from multipath keys#861
key_map.rs: propagate "invalid key" errors from multipath keys#861apoelstra merged 2 commits intorust-bitcoin:masterfrom
Conversation
|
cc @oleonardolima can you review whether this PR looks correct, and if so, ACK it (then I will merge it) and also backport it into #860? |
apoelstra
force-pushed
the
2025-09/fix-keymap
branch
2 times, most recently
from
fe3342d to
df8d860
Compare
oleonardolima
left a comment
oleonardolima
left a comment
There was a problem hiding this comment.
Thanks! Sorry for the oversight on the multipath descriptors.
Overall it looks good to me, just a minor comment on the test. I'll apply the tests to the backport PR.
src/descriptor/key_map.rs
Outdated
| assert!(matches!(result, Err(GetKeyError::NotSupported))); | ||
|
|
||
| // Also test with KeyMap; as in the previous test, the error turns to None. | ||
| let descriptor_s = "wpkh([d34db33f/84h/1h/0h]tprv8ZgxMBicQKsPd3EupYiPRhaMooHKUHJxNsTfYuScep13go8QFfHdtkG9nRkFGb7busX4isf6X9dURGCoKgitaApQ6MupRhZMcELAxTBRJgS/*)"; |
There was a problem hiding this comment.
question: any reason not to test with the multipath here too?
There was a problem hiding this comment.
Oops, no, just an oversight. This test has been pretty frustrating to write because both the keys and the descriptors have paths in them, and you can't directly construct a multi-path DerivationPath so you need to do everything by parsing descriptor strings..
There was a problem hiding this comment.
Although I suggested the changes, I noticed that I wont' be able to backport this to the release/12.x as it didn't converted Xpriv to Xpub properly for multipath: #757, and get the old error.
Claude 4 wrote the original tests; I then replaced its fix (which incorrectly also propagated errors in GetKey for KeyMap), updated the tests, and removed some extra tests which seemed uninformative and just noisy. To backport this to 12.x, just do s/KeyMap/KeyMapWrapper/ on the new tests. Oh, and add some `let keymap = KeyMapWrapper::from(keymap)` lines. Co-authored-by: aider (anthropic/claude-sonnet-4-20250514) <aider@aider.chat>
apoelstra
force-pushed
the
2025-09/fix-keymap
branch
from
df8d860 to
89b5048
Compare
3 participants
Since #851 we return a
NotSupportederror when attempting to request an xpriv from an x-only key. I'm not sure if this is the correct semantics, but it was deliberately written, so we'll keep it. However the behavior was different for multipath xprivs. There we returnedNone, i.e. "key not found/mismatch" rather than an error.This PR returns an error in both cases.