Continue with BTC address derivation using account config + derivation info #3335
Conversation
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
benma
force-pushed
the
split-verif
branch
2 times, most recently
from
e35862b to
332e692
Compare
BitBox01 support was removed (other than device management so users can check and create backups, etc) in 3d70ed2
- Type-matching is not nice - BTC address verification input params are inherently different to - ETH. ETH is basically showing the address at one keypath. BTC however is about deriving an address based on (isChange, addressIndex) from an account descriptor. So far it worked because we only use simple Bitcoin accounts and not descriptors, but it's good to be a bit more general to allow more flexible descriptor accounts in the future.
A BTC address is derived from an account using change/addressIndex. There is no "address-level" signing configuration. `signingConfig.Derive()` assumes there is one base level keypath and xpub, which is not true for a descriptor in general. For example, a multisig descriptor has multiple xpubs, each with their own keypath.
bznein
reviewed
May 19, 2025
| s.Require().Equal(uint32(index), address.Configuration.AbsoluteKeypath().ToUInt32()[1]) | ||
| s.Require().Equal(getPubKey(index), address.Configuration.PublicKey()) | ||
| s.Require().Equal(uint32(index), address.AbsoluteKeypath().ToUInt32()[1]) | ||
| _ = getPubKey |
There was a problem hiding this comment.
Thanks, yeah. Removed
| VerifyAddressBTC( | ||
| accountConfiguration *signing.Configuration, | ||
| derivation btctypes.Derivation, | ||
| coin coin.Coin) error |
There was a problem hiding this comment.
is there a specific reason we don't directly use btc.Coin and eth.Coin (below) so we don't have to cast it inside the method?
There was a problem hiding this comment.
Because of a circular import between the packages 😅 Maybe we can think another time of if and how to break the cycle.
The previous commits moved towards using an account config plus derivation info to get to the address. This commit removes the address-level signing config for BTC. This is a simplification, as it does not make sense to derive a full "signing config" when all that is derived is the keypath, nothing else changes. This is part of moving to general descriptor support. There are still remaining issues where code assumes it's a single-sig (calls to address.AbsoluteKeypath(), and NewAddress(), etc.), but we can solve that later step by step.
bznein
approved these changes
May 20, 2025
| VerifyAddressBTC( | ||
| accountConfiguration *signing.Configuration, | ||
| derivation btctypes.Derivation, | ||
| coin coin.Coin) error |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.