Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
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
143 changes: 118 additions & 25 deletions mm2src/mm2_bitcoin/keys/src/segwitaddress.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ pub enum SegwitAddrType {
P2wpkh,
/// pay-to-witness-script-hash
P2wsh,
/// pay-to-taproot
P2tr,
}

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand All @@ -70,7 +72,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 All @@ -84,6 +86,16 @@ impl SegwitAddress {
}
}

// TODO: This is test-only for now because we don't want to use a potentially panicing code in production.
#[cfg(test)]
pub fn new_with_version(hash: &AddressHashEnum, hrp: String, version: u8) -> SegwitAddress {
SegwitAddress {
hrp,
version: bech32::u5::try_from_u8(version).expect("0<32"),
program: hash.to_vec(),
}
}

/// Get the address type of the address.
/// None if unknown or non-standard.
pub fn address_type(&self) -> Option<SegwitAddrType> {
Expand All @@ -94,6 +106,10 @@ impl SegwitAddress {
32 => Some(SegwitAddrType::P2wsh),
_ => None,
},
1 => match self.program.len() {
32 => Some(SegwitAddrType::P2tr),
_ => None,
},
Comment on lines +109 to +112

Choose a reason for hiding this comment

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

Suggested change
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.

_ => None,
}
}
Expand Down Expand Up @@ -130,7 +146,14 @@ impl fmt::Display for SegwitAddress {
} else {
fmt as &mut dyn fmt::Write
};
let mut bech32_writer = bech32::Bech32Writer::new(self.hrp.as_str(), bech32::Variant::Bech32, writer)?;
let bech32_version = match self.version.to_u8() {
0 => bech32::Variant::Bech32,
1 => bech32::Variant::Bech32m,
// Ideally, all v1+ segwit addresses should be formatted using Bech32m.
// But let's error on such attempts unless we explicitly support higher versions.
_ => return Err(fmt::Error),
};
let mut bech32_writer = bech32::Bech32Writer::new(self.hrp.as_str(), bech32_version, writer)?;
bech32::WriteBase32::write_u5(&mut bech32_writer, self.version)?;
bech32::ToBase32::write_base32(&self.program, &mut bech32_writer)
}
Expand All @@ -147,9 +170,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,22 +188,38 @@ impl FromStr for SegwitAddress {
if version.to_u8() > 16 {
return Err(Error::InvalidWitnessVersion(version.to_u8()));
}

#[cfg(not(test))] // Relax this check in tests to be able to detect other errors (like invalid length, etc...)
// Only support segwit v0 and v1.
if ![0, 1].contains(&version.to_u8()) {
return Err(Error::UnsupportedWitnessVersion(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 {
return Err(Error::UnsupportedWitnessVersion(version.to_u8()));
if version.to_u8() == 0 {
// 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 {
return Err(Error::InvalidSegwitV0ProgramLength(program.len()));
}
if variant == bech32::Variant::Bech32m {
return Err(Error::UnsupportedAddressVariant(
"Bech32m is not supported for witness version 0. Bech32 should be used instead.".into(),
));
}
}

// Bech32 length check.
// 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 {
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 All @@ -190,6 +231,7 @@ mod tests {
use super::*;
use crypto::sha256;
use hex::ToHex;
use primitives::hash::H256;
use Public;

fn hex_to_bytes(s: &str) -> Option<Vec<u8>> {
Expand Down Expand Up @@ -230,36 +272,60 @@ mod tests {
assert_eq!(addr.address_type(), Some(SegwitAddrType::P2wsh));
}

#[test]
fn test_p2tr_address() {
let x_only_pub = "d5e89e0b73605abba690ba5e00484e279d006283bed0055a0530fb6a8c9adac7";
let bytes = hex_to_bytes(x_only_pub).unwrap();
let x_only_pub = H256::from_slice(&bytes).unwrap();
let hrp = "tb";
let addr = SegwitAddress::new_with_version(&AddressHashEnum::WitnessScriptHash(x_only_pub), hrp.to_string(), 1);
assert_eq!(
&addr.to_string(),
"tb1p6h5fuzmnvpdthf5shf0qqjzwy7wsqc5rhmgq2ks9xrak4ry6mtrscsqvzp"
);
assert_eq!(addr.address_type(), Some(SegwitAddrType::P2tr));
}

#[test]
// https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#test-vectors
fn test_valid_segwit() {
// p2wpkh
let addr = "BC1QW508D6QEJXTDG4Y5R3ZARVARY0C5XW7KV8F3T4";
let segwit_addr = SegwitAddress::from_str(addr).unwrap();
assert_eq!(0, segwit_addr.version.to_u8());
assert_eq!(
"751e76e8199196d454941c45d1b3a323f1433bd6",
segwit_addr.program.to_hex::<String>()
);

// p2wsh
let addr = "tb1qrp33g0q5c5txsp9arysrx4k6zdkfs4nce4xj0gdcccefvpysxf3q0sl5k7";
let segwit_addr = SegwitAddress::from_str(addr).unwrap();
assert_eq!(0, segwit_addr.version.to_u8());
assert_eq!(
"1863143c14c5166804bd19203356da136c985678cd4d27a1b8c6329604903262",
segwit_addr.program.to_hex::<String>()
);

// p2wsh
let addr = "tb1qqqqqp399et2xygdj5xreqhjjvcmzhxw4aywxecjdzew6hylgvsesrxh6hy";
let segwit_addr = SegwitAddress::from_str(addr).unwrap();
assert_eq!(0, segwit_addr.version.to_u8());
assert_eq!(
"000000c4a5cad46221b2a187905e5266362b99d5e91c6ce24d165dab93e86433",
segwit_addr.program.to_hex::<String>()
);
// p2tr
let addr = "tb1p6h5fuzmnvpdthf5shf0qqjzwy7wsqc5rhmgq2ks9xrak4ry6mtrscsqvzp";
let segwit_addr = SegwitAddress::from_str(addr).unwrap();
assert_eq!(1, segwit_addr.version.to_u8());
assert_eq!(
"d5e89e0b73605abba690ba5e00484e279d006283bed0055a0530fb6a8c9adac7",
segwit_addr.program.to_hex::<String>()
);
}

#[test]
// https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#test-vectors
// https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki#test-vectors
fn test_invalid_segwit_addresses() {
// Invalid checksum
let invalid_address = "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t5";
Expand All @@ -271,11 +337,16 @@ mod tests {
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
assert_eq!(err, Error::InvalidWitnessVersion(17));

// Invalid program length
// Invalid program length (bech32)
let invalid_address = "bc1rw5uspcuh";
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
assert_eq!(err, Error::InvalidWitnessProgramLength(1));

// Invalid program legnth (bech32m)
let invalid_address = "bc1pw5dgrnzv";
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
assert_eq!(err, Error::InvalidWitnessProgramLength(1));

// Invalid program length
let invalid_address = "bc10w508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7kw5rljs90";
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
Expand Down Expand Up @@ -308,21 +379,43 @@ mod tests {

// Version 1 shouldn't be used with bech32 variant although the below address is given as valid in BIP173
// https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki#abstract
// If the version byte is 1 to 16, no further interpretation of the witness program or witness stack happens,
// and there is no size restriction for the witness stack. These versions are reserved for future extensions
// https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#witness-program
let invalid_address = "bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx";
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
assert_eq!(err, Error::UnsupportedWitnessVersion(1));
assert_eq!(
err,
Error::UnsupportedAddressVariant(
"Bech32 is not supported for witness version 1. Bech32m should be used instead.".into()
)
);

// Invalid checksum for version 0 (bech32m instead of bech32)
let invalid_address = "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kemeawh";
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
assert_eq!(
err,
Error::UnsupportedAddressVariant(
"Bech32m is not supported for witness version 0. Bech32 should be used instead.".into()
)
);

// Version 16 shouldn't be used with bech32 variant although the below address is given as valid in BIP173
// Version 16 shouldn't be used with bech32
let invalid_address = "BC1SW50QA3JX3S";
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
assert_eq!(err, Error::UnsupportedWitnessVersion(16));
assert_eq!(
err,
Error::UnsupportedAddressVariant(
"Bech32 is not supported for witness version 16. Bech32m should be used instead.".into()
)
);

// Version 2 shouldn't be used with bech32 variant although the below address is given as valid in BIP173
// Version 2 shouldn't be used with bech32
let invalid_address = "bc1zw508d6qejxtdg4y5r3zarvaryvg6kdaj";
let err = SegwitAddress::from_str(invalid_address).unwrap_err();
assert_eq!(err, Error::UnsupportedWitnessVersion(2));
assert_eq!(
err,
Error::UnsupportedAddressVariant(
"Bech32 is not supported for witness version 2. Bech32m should be used instead.".into()
)
);
}
}
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 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