8000 Address validity invariant cleanups by sanket1729 · Pull Request #1564 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Address validity invariant cleanups #1564

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 24, 2023
Merged

Address validity invariant cleanups #1564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41652ca
Introduce is_spend_standard method
sanket1729 Jan 19, 2023
6ebc9de
Introduce WitnessProgram struct and cleanup Address validity invariants
sanket1729 Jan 19, 2023
a446df5
Make Payload non-exhaustive
sanket1729 Jan 20, 2023
44d3ec4
Rename Payload::as_bytes to inner_prog_as_bytes
sanket1729 Jan 20, 2023
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
161 changes: 101 additions & 60 deletions bitcoin/src/address.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,18 +386,48 @@ impl From<WitnessVersion> for opcodes::All {

/// The method used to produce an address.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[non_exhaustive]
pub enum Payload {
/// P2PKH address.
PubkeyHash(PubkeyHash),
/// P2SH address.
ScriptHash(ScriptHash),
/// Segwit address.
WitnessProgram {
/// The witness program version.
version: WitnessVersion,
/// The witness program.
program: Vec<u8>,
},
WitnessProgram(WitnessProgram),
}
8000 Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I think this enum should've been [non_exhaustive]. Anyway, I like this approach.


/// Witness program as defined in BIP141.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct WitnessProgram {
/// The witness program version.
version: WitnessVersion,
/// The witness program. (Between 2 and 40 bytes)
program: Vec<u8>,
}

impl WitnessProgram {
/// Creates a new witness program.
pub fn new(version: WitnessVersion, program: Vec<u8>) -> Result<Self, Error> {
if program.len() < 2 || program.len() > 40 {
return Err(Error::InvalidWitnessProgramLength(program.len()));
}

// Specific segwit v0 check. These addresses can never spend funds sent to them.
if version == WitnessVersion::V0 && (program.len() != 20 && program.len() != 32) {
return Err(Error::InvalidSegwitV0ProgramLength(program.len()));
}
Ok(WitnessProgram { version, program })
}

/// Returns the witness program version.
pub fn version(&self) -> WitnessVersion {
self.version
}

/// Returns the witness program.
pub fn program(&self) -> &[u8] {
&self.program
}
}

impl Payload {
Expand All @@ -412,18 +442,13 @@ impl Payload {
hash_inner.copy_from_slice(&script.as_bytes()[2..22]);
Payload::ScriptHash(ScriptHash::from_inner(hash_inner))
} else if script.is_witness_program() {
if script.witness_version() == Some(WitnessVersion::V0)
&& !(script.is_v0_p2wpkh() || script.is_v0_p2wsh())
{
return Err(Error::InvalidSegwitV0ProgramLength(script.len() - 2));
}

let opcode = script.first_opcode().expect("witness_version guarantees len() > 4");

Payload::WitnessProgram {
version: WitnessVersion::try_from(opcode)?,
program: script.as_bytes()[2..].to_vec(),
}
let witness_program = WitnessProgram::new(
WitnessVersion::try_from(opcode)?,
script.as_bytes()[2..].to_vec(),
)?;
Payload::WitnessProgram(witness_program)
} else {
return Err(Error::UnrecognizedScript);
})
Expand All @@ -434,8 +459,8 @@ impl Payload {
match *self {
Payload::PubkeyHash(ref hash) => script::ScriptBuf::new_p2pkh(hash),
Payload::ScriptHash(ref hash) => script::ScriptBuf::new_p2sh(hash),
Payload::WitnessProgram { version, program: ref prog } =>
script::ScriptBuf::new_witness_program(version, prog)
Payload::WitnessProgram(ref prog) =>
script::ScriptBuf::new_witness_program(prog)
}
}

Expand All @@ -454,10 +479,11 @@ impl Payload {

/// Create a witness pay to public key payload from a public key
pub fn p2wpkh(pk: &PublicKey) -> Result<Payload, Error> {
Ok(Payload::WitnessProgram {
version: WitnessVersion::V0,
program: pk.wpubkey_hash().ok_or(Error::UncompressedPubkey)?.as_ref().to_vec(),
})
let prog = WitnessProgram::new(
WitnessVersion::V0,
pk.wpubkey_hash().ok_or(Error::UncompressedPubkey)?.as_ref().to_vec()
)?;
Ok(Payload::WitnessProgram(prog))
}

/// Create a pay to script payload that embeds a witness pay to public key
Expand All @@ -471,10 +497,11 @@ impl Payload {

/// Create a witness pay to script hash payload.
pub fn p2wsh(script: &script::Script) -> Payload {
Payload::WitnessProgram {
version: WitnessVersion::V0,
program: script.wscript_hash().as_ref().to_vec(),
}
let prog = WitnessProgram::new(
WitnessVersion::V0,
script.wscript_hash().as_ref().to_vec()
).expect("wscript_hash has len 32 compatible with segwitv0");
Payload::WitnessProgram(prog)
}

/// Create a pay to script payload that embeds a witness pay to script hash address
Expand All @@ -492,28 +519,31 @@ impl Payload {
merkle_root: Option<TapNodeHash>,
) -> Payload {
let (output_key, _parity) = internal_key.tap_tweak(secp, merkle_root);
Payload::WitnessProgram {
version: WitnessVersion::V1,
program: output_key.to_inner().serialize().to_vec(),
}
let prog = WitnessProgram::new(
WitnessVersion::V1,
output_key.to_inner().serialize().to_vec()
).expect("taproot output key has len 32 <= 40");
Payload::WitnessProgram(prog)
}

/// Create a pay to taproot payload from a pre-tweaked output key.
///
/// This method is not recommended for use and [Payload::p2tr()] should be used where possible.
pub fn p2tr_tweaked(output_key: TweakedPublicKey) -> Payload {
Payload::WitnessProgram {
version: WitnessVersion::V1,
program: output_key.to_inner().serialize().to_vec(),
}
let prog = WitnessProgram::new(
WitnessVersion::V1,
output_key.to_inner().serialize().to_vec()
).expect("taproot output key has len 32 <= 40");
Payload::WitnessProgram(prog)
}

/// Returns a byte slice of the payload
pub fn as_bytes(&self) -> &[u8] {
/// Returns a byte slice of the inner program of the payload. If the payload
/// is a script hash or pubkey hash, a reference to the hash is returned.
fn inner_prog_as_bytes(&self) -> &[u8] {
match self {
Payload::ScriptHash(hash) => hash.as_ref(),
Payload::PubkeyHash(hash) => hash.as_ref(),
Payload::WitnessProgram { program, .. } => program,
Payload::WitnessProgram(prog) => prog.program(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated: But this API is sketchy in how it is used. If anything I would expect the witness_version to get serialized in as_bytes method of the PayLoad.

Should open an issue about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I think I complained about this, IIRC this was introduced when implementing is_related_to_pubkey. The function shouldn't be pub.

}
}
}
Expand Down Expand Up @@ -546,7 +576,8 @@ impl<'a> fmt::Display for AddressEncoding<'a> {
prefixed[1..].copy_from_slice(&hash[..]);
base58::encode_check_to_fmt(fmt, &prefixed[..])
}
Payload::WitnessProgram { version, program: prog } => {
Payload::WitnessProgram(witness_prog) => {
let (version, prog) = (witness_prog.version(), witness_prog.program());
let mut upper_writer;
let writer = if fmt.alternate() {
upper_writer = UpperWriter(fmt);
Expand All @@ -556,7 +587,7 @@ impl<'a> fmt::Display for AddressEncoding<'a> {
};
let mut bech32_writer =
bech32::Bech32Writer::new(self.bech32_hrp, version.bech32_variant(), writer)?;
bech32::WriteBase32::write_u5(&mut bech32_writer, (*version).into())?;
bech32::WriteBase32::write_u5(&mut bech32_writer, version.into())?;
bech32::ToBase32::write_base32(&prog, &mut bech32_writer)
}
}
Expand Down Expand Up @@ -725,15 +756,15 @@ impl<V: NetworkValidation> Address<V> {
match self.payload {
Payload::PubkeyHash(_) => Some(AddressType::P2pkh),
Payload::ScriptHash(_) => Some(AddressType::P2sh),
Payload::WitnessProgram { version, program: ref prog } => {
Payload::WitnessProgram(ref prog) => {
// BIP-141 p2wpkh or p2wsh addresses.
match version {
WitnessVersion::V0 => match prog.len() {
match prog.version() {
WitnessVersion::V0 => match prog.program().len() {
20 => Some(AddressType::P2wpkh),
32 => Some(AddressType::P2wsh),
_ => None,
_ => unreachable!("Address creation invariant violation: invalid program length")
},
WitnessVersion::V1 if prog.len() == 32 => Some(AddressType::P2tr),
WitnessVersion::V1 if prog.program().len() == 32 => Some(AddressType::P2tr),
_ => None,
}
}
Expand Down Expand Up @@ -848,10 +879,26 @@ impl Address {
self.address_type_internal()
}

/// Checks whether or not the address is following Bitcoin standardness rules when
/// *spending* from this address. *NOT* to be called by senders.
///
/// <details>
/// <summary>Spending Standardness</summary>
///
/// For forward compatibility, the senders must send to any [`Address`]. Receivers
/// can use this method to check whether or not they can spend from this address.
///
/// SegWit addresses with unassigned witness versions or non-standard program sizes are
/// considered non-standard.
/// </details>
///
pub fn is_spend_standard(&self) -> bool { self.address_type().is_some() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm correct that receiver statically knows the address is standard then I think the method is useless and we should just remove it. If there is any use then I want a warning in doc that this should not be called by the sender.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I'm correct that receiver statically knows the address is standard

This method is preciously how the receiver would know that the address they are receiving the money to is standard.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The receiver is the one who constructs the address so I would expect this to be handled by construction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you suggesting two structures? SendingAddress and RecievingAddress. Or perhaps yet another phantom data type in Address itself. We want to support sending to unknown addresses, but don't want to receive it ourselves. I don't see how we can handle this under the current Address construction.

That being said, I don't see any utility for wallet developers who want to use this method. It maybe useful enthusiasts who scan the blockchain for non-standard spends or for block explorers. I recall somebody did a taproot spend before taproot was activated to fork off UASF nodes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you suggesting two structures?

Not really. If I write an application that uses some of the common ways of receiving then I write the code such that Address is always valid and don't need to check it. Would would I even do if it's wrong?

That being said, I don't see any utility for wallet developers who want to use this method.

Yeah, I believe we both don't for the same reason.

It maybe useful enthusiasts who scan the blockchain for non-standard spends or for block explorers.

Shouldn't the scripts be checked instead of addresses?

I recall somebody did a taproot spend before taproot was activated to fork off UASF nodes.

OT but sounds like that person was confused? I don't see how it could fork off UASF nodes.

Copy link
Member

Choose a reason for hiding this comment

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

Not really. If I write an application that uses some of the common ways of receiving then I write the code such that Address is always valid and don't need to check it.

This is why it's a utility method and not something that we're gating constructors on or enforcing in the type system.

Would would I even do if it's wrong?

Lol assert probably. But I think it's most common to hit cases like this during transition periods...like, maybe your wallet would let you generate a Taproot address before Taproot was active, but would put a bunch of warnings on it or otherwise try to prevent you from using it until it became standard.

Shouldn't the scripts be checked instead of addresses?

This is an interesting thought. Maybe users who want to do this should need to first convert the address to a scriptpubkey, and then do a standardness check on that. But I think such an API change should wait for script tagging.


/// Checks whether or not the address is following Bitcoin standardness rules.
///
/// SegWit addresses with unassigned witness versions or non-standard program sizes are
/// considered non-standard.
#[deprecated(since = "0.30.0", note = "Use Address::is_spend_standard instead")]
pub fn is_standard(&self) -> bool { self.address_type().is_some() }

/// Constructs an [`Address`] from an output script (`scriptPubkey`).
Expand Down Expand Up @@ -884,7 +931,7 @@ impl Address {
/// given key. For taproot addresses, the supplied key is assumed to be tweaked
pub fn is_related_to_pubkey(&self, pubkey: &PublicKey) -> bool {
let pubkey_hash = pubkey.pubkey_hash();
let payload = self.payload.as_bytes();
let payload = self.payload.inner_prog_as_bytes();
let xonly_pubkey = XOnlyPublicKey::from(pubkey.inner);

(*pubkey_hash.as_ref() == *payload)
Expand All @@ -897,7 +944,7 @@ impl Address {
/// This will only work for Taproot addresses. The Public Key is
/// assumed to have already been tweaked.
pub fn is_related_to_xonly_pubkey(&self, xonly_pubkey: &XOnlyPublicKey) -> bool {
let payload = self.payload.as_bytes();
let payload = self.payload.inner_prog_as_bytes();
payload == xonly_pubkey.serialize()
}
}
Expand Down Expand Up @@ -1031,22 +1078,15 @@ impl FromStr for Address<NetworkUnchecked> {
(WitnessVersion::try_from(v[0])?, bech32::FromBase32::from_base32(p5)?)
};

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

// Specific segwit v0 check.
if version == WitnessVersion::V0 && (program.len() != 20 && program.len() != 32) {
return Err(Error::InvalidSegwitV0ProgramLength(program.len()));
}
let witness_program = WitnessProgram::new(version, program)?;

// Encoding check
let expected = version.bech32_variant();
if expected != variant {
return Err(Error::InvalidBech32Variant { expected, found: variant });
}

return Ok(Address::new(network, Payload::WitnessProgram { version, program }));
return Ok(Address::new(network, Payload::WitnessProgram(witness_program)));
}

// Base58
Expand Down Expand Up @@ -1228,7 +1268,8 @@ mod tests {
let program = hex!(
"654f6ea368e0acdfd92976b7c2103a1b26313f430654f6ea368e0acdfd92976b7c2103a1b26313f4"
);
let addr = Address::new(Bitcoin, Payload::WitnessProgram { version: WitnessVersion::V13, program });
let witness_prog = WitnessProgram::new(WitnessVersion::V13, program).unwrap();
let addr = Address::new(Bitcoin, Payload::WitnessProgram(witness_prog));
roundtrips(&addr);
}

Expand Down Expand Up @@ -1439,10 +1480,10 @@ mod tests {
Payload::ScriptHash(ScriptHash::all_zeros()),
];
let segwit_payload = (0..=16)
.map(|version| Payload::WitnessProgram {
version: WitnessVersion::try_from(version).unwrap(),
program: vec![],
})
.map(|version| Payload::WitnessProgram(WitnessProgram::new(
WitnessVersion::try_from(version).unwrap(),
vec![0xab; 32], // Choose 32 to make test case valid for all witness versions(including v0)
).unwrap()))
.collect::<Vec<_>>();

const LEGACY_EQUIVALENCE_CLASSES: &[&[Network]] =
Expand Down
33 changes: 26 additions & 7 deletions bitcoin/src/blockdata/script.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ use crate::policy::DUST_RELAY_TX_FEE;
use crate::OutPoint;

use crate::key::PublicKey;
use crate::address::WitnessVersion;
use crate::address::{WitnessVersion, WitnessProgram};
use crate::taproot::{LeafVersion, TapNodeHash, TapLeafHash};
use secp256k1::{Secp256k1, Verification, XOnlyPublicKey};
use crate::schnorr::{TapTweak, TweakedPublicKey, UntweakedPublicKey};
Expand Down Expand Up @@ -1122,28 +1122,47 @@ impl ScriptBuf {

/// Generates P2WPKH-type of scriptPubkey.
pub fn new_v0_p2wpkh(pubkey_hash: &WPubkeyHash) -> Self {
ScriptBuf::new_witness_program(WitnessVersion::V0, &pubkey_hash[..])
// pubkey hash is 20 bytes long, so it's safe to use `new_witness_program_unchecked` (Segwitv0)
ScriptBuf::new_witness_program_unchecked(WitnessVersion::V0, &pubkey_hash[..])
}

/// Generates P2WSH-type of scriptPubkey with a given hash of the redeem script.
pub fn new_v0_p2wsh(script_hash: &WScriptHash) -> Self {
ScriptBuf::new_witness_program(WitnessVersion::V0, &script_hash[..])
// script hash is 32 bytes long, so it's safe to use `new_witness_program_unchecked` (Segwitv0)
ScriptBuf::new_witness_program_unchecked(WitnessVersion::V0, &script_hash[..])
}

/// Generates P2TR for script spending path using an internal public key and some optional
/// script tree merkle root.
pub fn new_v1_p2tr<C: Verification>(secp: &Secp256k1<C>, internal_key: UntweakedPublicKey, merkle_root: Option<TapNodeHash>) -> Self {
let (output_key, _) = internal_key.tap_tweak(secp, merkle_root);
ScriptBuf::new_witness_program(WitnessVersion::V1, &output_key.serialize())
// output key is 32 bytes long, so it's safe to use `new_witness_program_unchecked` (Segwitv1)
ScriptBuf::new_witness_program_unchecked(WitnessVersion::V1, &output_key.serialize())
}

/// Generates P2TR for key spending path for a known [`TweakedPublicKey`].
pub fn new_v1_p2tr_tweaked(output_key: TweakedPublicKey) -> Self {
ScriptBuf::new_witness_program(WitnessVersion::V1, &output_key.serialize())
// output key is 32 bytes long, so it's safe to use `new_witness_program_unchecked` (Segwitv1)
ScriptBuf::new_witness_program_unchecked(WitnessVersion::V1, &output_key.serialize())
}

/// Generates P2WSH-type of scriptPubkey with a given hash of the redeem script.
pub fn new_witness_program(version: WitnessVersion, program: &[u8]) -> Self {
/// Generates P2WSH-type of scriptPubkey with a given [`WitnessProgram`].
pub fn new_witness_program(witness_program: &WitnessProgram) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn new_witness_program(witness_program: &WitnessProgram) -> Self {
/// Generates P2WSH-type of scriptPubkey with a given [`WitnessProgram`].
pub fn new_witness_program(witness_program: &WitnessProgram) -> Self {

Builder::new()
.push_opcode(witness_program.version().into())
.push_slice(witness_program.program())
.into_script()
}

/// Generates P2WSH-type of scriptPubkey with a given [`WitnessVersion`] and the program bytes.
/// Does not do any checks on version or program length.
///
/// Convenience method used by `new_v0_p2wpkh`, `new_v0_p2wsh`, `new_v1_p2tr`, and
/// `new_v1_p2tr_tweaked`.
fn new_witness_program_unchecked(version: WitnessVersion, program: &[u8]) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a debug_assert!?

debug_assert!(program.len() >= 2 && program.len() <= 40);
// In segwit v0, the program must be 20 or 32 bytes long.
debug_assert!(version != WitnessVersion::V0 || program.len() == 20 || program.len() == 32);
Builder::new()
.push_opcode(version.into())
.push_slice(program)
Expand Down
6 changes: 5 additions & 1 deletion bitcoin/src/psbt/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1742,6 +1742,7 @@ mod tests {
#[cfg(feature = "rand-std")]
fn sign_psbt() {
use crate::WPubkeyHash;
use crate::address::WitnessProgram;
use crate::bip32::{Fingerprint, DerivationPath};

let unsigned_tx = Transaction {
Expand Down Expand Up @@ -1771,9 +1772,12 @@ mod tests {
psbt.inputs[0].bip32_derivation = map;

// Second input is unspendable by us e.g., from another wallet that supports future upgrades.
let unknown_prog = WitnessProgram::new(
crate::address::WitnessVersion::V4, vec![0xaa; 34]
).unwrap();
let txout_unknown_future = TxOut{
value: 10,
script_pubkey: ScriptBuf::new_witness_program(crate::address::WitnessVersion::V4, &[0xaa; 34]),
script_pubkey: ScriptBuf::new_witness_program(&unknown_prog),
};
psbt.inputs[1].witness_utxo = Some(txout_unknown_future);

Expand Down
0