feat(taproot): support withdrawing to taproot addresses#2625
feat(taproot): support withdrawing to taproot addresses#2625mariocynicys wants to merge 5 commits intodevfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for withdrawing to taproot (P2TR) addresses by implementing taproot address parsing and script generation. The implementation supports segwit v1 addresses using bech32m encoding while maintaining backward compatibility with existing segwit v0 addresses.
- Add P2TR script type and taproot address support in the address parsing logic
- Implement P2TR script building functionality for taproot outputs
- Update segwit address parsing to handle both bech32 and bech32m variants
- Modify test expectations to verify taproot withdrawal functionality works
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mm2_tests_inner.rs | Updates test to expect successful taproot withdrawals instead of failures |
| builder.rs | Adds build_p2tr method for generating taproot output scripts |
| segwitaddress.rs | Enhances segwit address parsing to support both v0 (bech32) and v1 (bech32m) addresses |
| address.rs | Adds P2TR script type and updates address parsing logic for taproot support |
| utxo_common.rs | Adds unreachable case for P2TR in script-to-address conversion |
| utxo.rs | Integrates P2TR script building into the output script generation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // TODO: Don't use AddressHashEnum::WitnessScriptHash variant. | ||
| AddressHashEnum::WitnessScriptHash(x_only_pubkey) => Ok(Builder::default() | ||
| .push_opcode(Opcode::OP_1) | ||
| .push_data(x_only_pubkey.as_ref()) | ||
| .into_script()), | ||
| AddressHashEnum::AddressHash(_) => Err(Error::WitnessHashMismatched), |
There was a problem hiding this comment.
Using WitnessScriptHash variant for taproot x-only public keys is confusing and error-prone. Consider adding a dedicated XOnlyPubkey variant to AddressHashEnum or using a more appropriate type to clearly distinguish between script hashes and x-only public keys.
| // TODO: Don't use AddressHashEnum::WitnessScriptHash variant. | |
| AddressHashEnum::WitnessScriptHash(x_only_pubkey) => Ok(Builder::default() | |
| .push_opcode(Opcode::OP_1) | |
| .push_data(x_only_pubkey.as_ref()) | |
| .into_script()), | |
| AddressHashEnum::AddressHash(_) => Err(Error::WitnessHashMismatched), | |
| AddressHashEnum::XOnlyPubkey(x_only_pubkey) => Ok(Builder::default() | |
| .push_opcode(Opcode::OP_1) | |
| .push_data(x_only_pubkey.as_ref()) | |
| .into_script()), | |
| _ => Err(Error::WitnessHashMismatched), |
There was a problem hiding this comment.
@shamardy here i didn't add a new variant as i still didn't fix this yet in my working branch.
this works fine though for now.
when i push the other complete PR, this will be fixed (either by adding another variant or renaming/generalizing the witnesscript hash variant).
There was a problem hiding this comment.
I would also suggest renaming AddressHashEnum as it's not quite accurate and now there is even more to it as it now contains not only hashes.
Naming to consider (from chatgpt): scriptPubKey payload or locking script data.
KDF WASM Playground Previews
|
| /// Builds p2tr script pubkey | ||
| pub fn build_p2tr(address_hash: &AddressHashEnum) -> Result<Script, Error> { | ||
| match address_hash { | ||
| // TODO: Don't use AddressHashEnum::WitnessScriptHash variant. |
There was a problem hiding this comment.
sorry i should have converted this to draft earlier.
there is a newer PR that's complete and fixes the problems found here (#2630).
i only pushed this PR early to support withdrawal thinking it was gonna be merged quickly. but since it doesnt share a linear history with the full taproot support PR, we better just ignore this one and merge the full support one.
this PR will be auto closed with the merger of the full support one.
| 1 => match self.program.len() { | ||
| 32 => Some(SegwitAddrType::P2tr), | ||
| _ => None, | ||
| }, |
There was a problem hiding this comment.
| 1 => match self.program.len() { | |
| 32 => Some(SegwitAddrType::P2tr), | |
| _ => None, | |
| }, | |
| 1 if self.program.len() == 32 => Some(SegwitAddrType::P2tr), |
Can also be applied to above case as well.
| let (script_type, mut hash) = match (address.version.to_u8(), address.program.len()) { | ||
| (0, 20) => (AddressScriptType::P2WPKH, AddressHashEnum::default_address_hash()), | ||
| (0, 32) => (AddressScriptType::P2WSH, AddressHashEnum::default_witness_script_hash()), | ||
| (0, _) => return Err("Expect either 20 or 32 bytes long hash".into()), | ||
| (1, 32) => (AddressScriptType::P2TR, AddressHashEnum::default_witness_script_hash()), | ||
| (1, _) => return Err("Expect 32 bytes long public key".into()), | ||
| (v, _) => return Err(format!("Unsupported segwit version: {v}")), | ||
| }; |
There was a problem hiding this comment.
I think this code is very dangerous to maintain. Any mistake on the order would easily cause a bug which would be pretty annoying to figure/find out.
|
Why is it still Draft PR while having pending review label? Which one is not correct? Also, various formatting pipelines are failing. |
|
closing in favor of #2630 |
3 participants
This PR adds support for withdrawing coins to a taproot addresses.
This doesn't handle parsing taproot output scripts supplied from the blockchain, but only taproot addresses (string format) and converting them to output scripts.
The later PR that brings full taproot support will clean up the TODOs and unreachables left here. They are perfectly safe for the time being though.
This is just pushed earlry so it could be integrated in the GUI.