Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions mm2src/coins/utxo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1921,6 +1921,7 @@ pub fn output_script(address: &Address) -> Result<Script, keys::Error> {
AddressScriptType::P2SH => Ok(Builder::build_p2sh(address.hash())),
AddressScriptType::P2WPKH => Builder::build_p2wpkh(address.hash()),
AddressScriptType::P2WSH => Builder::build_p2wsh(address.hash()),
AddressScriptType::P2TR => Builder::build_p2tr(address.hash()),
}
}

Expand Down
3 changes: 3 additions & 0 deletions mm2src/coins/utxo/utxo_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,9 @@ pub fn addresses_from_script<T: UtxoCommonOps>(coin: &T, script: &Script) -> Res
),
AddressScriptType::P2WPKH => (UtxoAddressFormat::Segwit, AddressBuilderOption::PubkeyHash(dst.hash)),
AddressScriptType::P2WSH => (UtxoAddressFormat::Segwit, AddressBuilderOption::ScriptHash(dst.hash)),
AddressScriptType::P2TR => {
unreachable!("Currently we don't parse taproot addresses from scripts in `extract_destinations`.")
},
};

AddressBuilder::new(
Expand Down
18 changes: 12 additions & 6 deletions mm2src/mm2_bitcoin/keys/src/address.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ pub enum AddressScriptType {
/// as the scripthash, eg: bc1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3qccfmv3.
/// https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki
P2WSH,
/// Pay to Taproot
/// Segwit v1 P2TR which begins with the human readable part followed by 1 followed by 59 base32 characters
/// as the scripthash, eg: bc1p6gps4j04duwphrhkwx0vhl6r9kkq8m8n7r9r02rvwzrekjt0f4pskz8zas.
/// https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki
P2TR,
}

#[derive(Clone, Debug, Default, Display, Deserialize, Eq, Hash, PartialEq, Serialize)]
Expand Down Expand Up @@ -287,12 +292,13 @@ impl Address {
pub fn from_segwitaddress(segaddr: &str, checksum_type: ChecksumType) -> Result<Address, String> {
let address = SegwitAddress::from_str(segaddr).map_err(|e| e.to_string())?;

let (script_type, mut hash) = if address.program.len() == 20 {
(AddressScriptType::P2WPKH, AddressHashEnum::default_address_hash())
} else if address.program.len() == 32 {
(AddressScriptType::P2WSH, AddressHashEnum::default_witness_script_hash())
} else {
return Err("Expect either 20 or 32 bytes long hash".into());
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}")),
};
Comment on lines +295 to 302

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

hash.copy_from_slice(address.program.as_slice());

Expand Down
30 changes: 20 additions & 10 deletions mm2src/mm2_bitcoin/keys/src/segwitaddress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ pub struct SegwitAddress {
/// The human-readable part
pub hrp: String,
/// The witness program version
version: bech32::u5,
pub version: bech32::u5,
/// The witness program
pub program: Vec<u8>,
}
Expand Down Expand Up @@ -147,9 +147,11 @@ impl FromStr for SegwitAddress {
if payload.is_empty() {
return Err(Error::EmptyBech32Payload);
}

let mut is_bech32_non_modified = false;
match variant {
bech32::Variant::Bech32 => (),
bech32::Variant::Bech32m => return Err(Error::UnsupportedAddressVariant("Bech32m".into())),
bech32::Variant::Bech32 => is_bech32_non_modified = true,
bech32::Variant::Bech32m => (),
// Important: If a new variant is added we should return an error until we support the new variant
}

Expand All @@ -163,24 +165,32 @@ impl FromStr for SegwitAddress {
if version.to_u8() > 16 {
return Err(Error::InvalidWitnessVersion(version.to_u8()));
}
if program.len() < 2 || program.len() > 40 {
return Err(Error::InvalidWitnessProgramLength(program.len()));
}

// Specific segwit v0 check.
if version.to_u8() != 0 {
// Only support segwit v0 and v1.
if ![0, 1].contains(&version.to_u8()) {
return Err(Error::UnsupportedWitnessVersion(version.to_u8()));
}

// Bech32 length check.
if program.len() < 2 || program.len() > 40 {
return Err(Error::InvalidWitnessProgramLength(program.len()));
}

// Bech32 length check for segwit v0 (later versions use bech32m which isn't vulnerable to this problem).
// Important: we should be careful when using new program lengths since a valid Bech32 string can be modified according to
// the below 2 links while still having a valid checksum.
// https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki#motivation
// https://github.com/sipa/bech32/issues/51
if program.len() != 20 && program.len() != 32 {
if version.to_u8() == 0 && program.len() != 20 && program.len() != 32 {
return Err(Error::InvalidSegwitV0ProgramLength(program.len()));
}

if version.to_u8() != 0 && is_bech32_non_modified {
return Err(Error::UnsupportedAddressVariant(format!(
"Bech32 is not supported for witness version {}. Bech32m should be used instead.",
version.to_u8()
)));
}

Ok(SegwitAddress { hrp, version, program })
}
}
Expand Down
12 changes: 12 additions & 0 deletions mm2src/mm2_bitcoin/script/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,18 @@ impl Builder {
}
}

/// Builds p2tr script pubkey
pub fn build_p2tr(address_hash: &AddressHashEnum) -> Result<Script, Error> {
match address_hash {
// TODO: Don't use AddressHashEnum::WitnessScriptHash variant.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth to add the reason here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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),
Comment on lines +67 to +72
Copy link

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// 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),

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Collaborator

@dimxy dimxy Sep 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

/// Builds op_return script
pub fn build_nulldata(bytes: &[u8]) -> Script {
Builder::default()
Expand Down
11 changes: 3 additions & 8 deletions mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1227,7 +1227,7 @@ fn test_withdraw_segwit() {
assert!(withdraw_error.get("error_type").is_none());
assert!(withdraw_error.get("error_data").is_none());

// Withdraw to taproot addresses should fail
// Withdraw to taproot addresses should should also work
let withdraw = block_on(mm_alice.rpc(&json!({
"userpass": mm_alice.userpass,
"method": "withdraw",
Expand All @@ -1237,13 +1237,8 @@ fn test_withdraw_segwit() {
})))
.unwrap();

assert!(withdraw.0.is_server_error(), "tBTC withdraw: {}", withdraw.1);
log!("{:?}", withdraw.1);
let withdraw_error: Json = json::from_str(&withdraw.1).unwrap();
assert!(withdraw_error["error"]
.as_str()
.expect("Expected 'error' field")
.contains("address variant/format Bech32m is not supported yet"));
assert!(withdraw.0.is_success(), "tBTC withdraw: {}", withdraw.1);
let _: TransactionDetails = json::from_str(&withdraw.1).expect("Expected 'TransactionDetails'");

block_on(mm_alice.stop()).unwrap();
}
Expand Down
Loading