-
Notifications
You must be signed in to change notification settings - Fork 113
feat(taproot): add full taproot support #2630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
we don't need to gaurd a segwit address, segwit addresses will be properly detected inside with_key_pair::sign_tx() using the prev_script
filler code for signing and recognition of taproot addresses etc...
his commit refactors the UTXO address handling system to support versioned segwit addresses, laying the groundwork for taproot (segwit v1) implementation:
- **AddressFormat refactor**: Convert `AddressFormat::Segwit` to `AddressFormat::Segwit { version: u8 }` to support multiple segwit versions
- **Method updates**: Replace `is_segwit()` calls with `is_segwit_v0()` throughout the codebase to maintain existing segwit v0 behavior
- **Bech32m support**: Add bech32m encoding support for segwit v1+ addresses in SegwitAddress display formatting
- **Address construction**: Update SegwitAddress constructor to accept version parameter
- **Script type mapping**: Add P2TR script type detection and mapping for taproot addresses
- Segwit v0 addresses continue to use bech32 encoding
- Segwit v1+ addresses use bech32m encoding as per BIP-350
- Added version validation and proper script type detection
- Maintains backward compatibility with existing segwit v0 functionality
- Includes FIXME comments marking areas requiring taproot-specific implementation
- Taproot signing logic in transaction generation
- P2TR output script handling in spending operations
- Hardware wallet support for taproot addresses
- Complete taproot validation and witness handling
but not yet for wasm! since taproot signing uses rust-bitcoin (to no re-invent the wheel), adding rust-bitcoin dependency on wasm will break linking just like what happened in #2566 since #262 already fixes this issue by converging all versions of secp (and thus rust-bitcoin) and enabling them on wasm environment, let's defer wasm support till that PR is merged
as it could be used now to create taproot addresses
don't involve fork_id logic in Sighash. also remove the abstracting function signature_hash_fork_id and replace it by signature_hash_witness0
based on our coins config, there exists no fork_id chain that doens't have the fork_id bit set. thus we don't really need to handle a fork_id chain that doesn't have the fork_id bit set and is required to be signed as signature_hash_original (huh?) cause this DOESN'T EXIST
since it's already detectable from within KDF. the variant is kept though to be used internally as a flag on how to sign the (witness) transaction
that's a stray witnessscripthash that i forgot to change
this resolves a couple of fixmes as well
…rrect error if it's not correct
this commit doesn't bring any logical change, don't worry ;D
haven't tested withdrawing yet
|
Fmt & linting pipelines are failing. |
| let (script_type, mut locking_destination) = match (address.version_as_u8(), address.program.len()) { | ||
| (0, 20) => (AddressScriptType::P2WPKH, LockingDestination::default_address_hash()), | ||
| (0, 32) => ( | ||
| AddressScriptType::P2WSH, | ||
| LockingDestination::default_witness_script_hash(), | ||
| ), | ||
| (0, _) => return Err("Expect either 20 or 32 bytes long hash for witness v0 address".into()), | ||
| (1, 32) => ( | ||
| AddressScriptType::P2TR, | ||
| LockingDestination::default_tweaked_xonly_pubkey(), | ||
| ), | ||
| (1, _) => return Err("Expect 32 bytes long public key for witness v1 address".into()), | ||
| (v, _) => return Err(format!("Unsupported segwit version: {v}")), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #2625 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i moved all the wild cards to the end (namely, (0, _) to the end).
do u mean something like having a wild card at the end for all the wild cards and match which error message we wanna return in it?
something like:
match (version, length) {
(0, 20) => ...,
(0, 32) => ...,
(1, 32) => ...,
(v, _l) => match v {
0 => Err("Expect either 20 or 32 bytes long hash for witness v0 address"),
1 => Err("Expect 32 bytes long public key for witness v1 address"),
v => Err(format!("Unsupported segwit version: {v}")),
}
}in such matching, as long as the only wild card arm (i.e. (v, _l) arm) is at the end, we won't face any bug.
change the conidition so to allow cashaddress and legacy address types interchanging between the conf and request
leaving the expect() as is. since we already have a lot of expects that enforce our invariants. if we somehow fail to catch an invalid segwit version while constructing, then let's just panic here since this is a bug
|
@shamardy |
|
@shamardy |
5 similar comments
|
@shamardy |
|
@shamardy |
|
@shamardy |
|
@shamardy |
|
@shamardy |
This PR adds full taproot support (address parsing, address generation, withdrawls, signing, trezor, etc...).
For now, this adds support for native only (no wasm) until #2623 is merged. This is due to the same issue related to multiple secp dependencies co-existing at the same time.
Tests to be added.
Closes #2625 (since it didn't get merged early enough)