Address validity invariant cleanups #1564

sanket1729 · 2023-01-19T23:38:03Z

Highlights:

Segwitv0 programs with lengths apart from 20 or 32 are invalid Address struct. Such Addresses are useless and we should not parse/create them.
Renamed is_standard to is_spend_standard.

sanket1729 · 2023-01-19T23:53:12Z

bitcoin/src/address.rs

    }

    /// Returns a byte slice of the payload
    pub fn 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(),


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.

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

Kixunil

Looks good, I only want a reason to keep is_spend_standard and if yes, then better doc.

Kixunil · 2023-01-20T11:59:19Z

bitcoin/src/address.rs

-        program: Vec<u8>,
-    },
+    WitnessProgram(WitnessProgram),
+}


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

Kixunil · 2023-01-20T12:04:45Z

bitcoin/src/address.rs

    }

    /// Returns a byte slice of the payload
    pub fn 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(),


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

Kixunil · 2023-01-20T12:07:21Z

bitcoin/src/blockdata/script.rs

+    ///
+    // Used internally by `new_v0_p2wpkh`, `new_v0_p2wsh`, `new_v1_p2tr`, and `new_v1_p2tr_tweaked`
+    // for convenience.
+    fn new_witness_program_unchecked(version: WitnessVersion, program: &[u8]) -> Self {


Maybe add a debug_assert!?

Kixunil · 2023-01-20T12:18:37Z

bitcoin/src/address.rs

+    ///
+    /// SegWit addresses with unassigned witness versions or non-standard program sizes are
+    /// considered non-standard.
+    pub fn is_spend_standard(&self) -> bool { self.address_type().is_some() }


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.

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.

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

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.

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.

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.

Kixunil

ACK 30d251e

Kixunil · 2023-01-20T19:24:11Z

bitcoin/src/address.rs

-    /// Returns a byte slice of the payload
-    pub fn as_bytes(&self) -> &[u8] {
+    /// Returns a byte slice of the inner program of the payload. Incase the payload
+    /// ia a script hash or pubkey hash, the a reference to the hash is returned.


Nits: "is", "a reference"

Kixunil · 2023-01-20T19:26:37Z

bitcoin/src/address.rs

                        20 => Some(AddressType::P2wpkh),
                        32 => Some(AddressType::P2wsh),
-                        _ => None,
+                        _ => None, // Technically unreachable, but we don't want to panic when not required.


Silently ignoring bugs is not great either, debug_assert would be nice.

Agreed. TBH I'd even prefer an unconditional unreachable! panic.

Kixunil · 2023-01-20T19:31:12Z

bitcoin/src/address.rs

+    /// Checks whether or not the address is following Bitcoin standardness rules when
+    /// *spending* from this address. This method should *NOT* called by senders. 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.


I like having a warning in summary but not sure if this long is appropriate. Maybe something like "Checks whether the address obeys Bitcoin standardness rules when spending from this address. *NOT to be called by senders!" And full explanation in description. Anyway, I won't block the PR because of it.

tcharding

Just a few rustdoc comment fixes.

tcharding · 2023-01-20T23:44:46Z

bitcoin/src/address.rs

+    /// Returns a byte slice of the inner program of the payload. Incase the payload
+    /// ia a script hash or pubkey hash, the a reference to the hash is returned.


Suggested change

/// Returns a byte slice of the inner program of the payload. Incase the payload

/// ia a script hash or pubkey hash, the a reference to the hash is returned.

/// 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.

tcharding · 2023-01-20T23:49:17Z

bitcoin/src/blockdata/script.rs

+    /// Internal method to a witness scriptPubkey with a given version and program.
+    /// Does not do any checks on version or program.
+    ///
+    // Used internally by `new_v0_p2wpkh`, `new_v0_p2wsh`, `new_v1_p2tr`, and `new_v1_p2tr_tweaked`
+    // for convenience.
+    fn new_witness_program_unchecked(version: WitnessVersion, program: &[u8]) -> Self {


Suggested change

/// Internal method to a witness scriptPubkey with a given version and program.

/// Does not do any checks on version or program.

///

// Used internally by `new_v0_p2wpkh`, `new_v0_p2wsh`, `new_v1_p2tr`, and `new_v1_p2tr_tweaked`

// for convenience.

fn new_witness_program_unchecked(version: WitnessVersion, program: &[u8]) -> Self {

/// Generates P2WSH-type of scriptPubkey with a given [`WitnessVersion`] and the program bytes.

///

/// 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 {

Addressed. But also added the Does not do any checks on version or program which I feel is relevant to the method.

tcharding · 2023-01-20T23:49:31Z

bitcoin/src/blockdata/script.rs

    }

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


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 {

apoelstra

ACK 30d251e

apoelstra · 2023-01-23T21:22:20Z

@sanket1729 should I merge this or do you want to address @tcharding's nits?

sanket1729 · 2023-01-23T21:30:35Z

@apoelstra, force-pushing in a few mins

sanket1729 · 2023-01-23T21:36:31Z

Ready for review again.(after CI clears)

Addresses with Segwitv0 not having len 20/32 are invalid and cannot be constructed. Also cleans up a API bug in ScriptBuf::new_witness_prog(ver, prog) allowing prog of invalid lenghts.

It is unlikely that we will see another segwit type fork. But we never know, best to mark Payload as [non-exhaustive]

Also make it private

sanket1729 · 2023-01-23T21:46:42Z

I have made another force push that adds details in the summary section of the doc. Looks really neat, I will use it more often in the future.

apoelstra

ACK 44d3ec4

tcharding

ACK 44d3ec4

sanket1729 force-pushed the addr_fixes branch 3 times, most recently from 1cad205 to cc5ee00 Compare January 19, 2023 23:50

sanket1729 commented Jan 19, 2023

View reviewed changes

sanket1729 mentioned this pull request Jan 19, 2023

Should is_standard match sending-standardness or spending-standardness #1558

Closed

Kixunil requested changes Jan 20, 2023

View reviewed changes

Introduce is_spend_standard method

41652ca

sanket1729 force-pushed the addr_fixes branch from 727456c to 30d251e Compare January 20, 2023 15:31

Kixunil previously approved these changes Jan 20, 2023

View reviewed changes

tcharding reviewed Jan 20, 2023

View reviewed changes

apoelstra previously approved these changes Jan 23, 2023

View reviewed changes

sanket1729 dismissed stale reviews from apoelstra and Kixunil via b2f4559 January 23, 2023 21:34

sanket1729 force-pushed the addr_fixes branch from 30d251e to b2f4559 Compare January 23, 2023 21:34

sanket1729 force-pushed the addr_fixes branch from b2f4559 to f6f304e Compare January 23, 2023 21:39

sanket1729 added 3 commits January 23, 2023 13:45

Introduce WitnessProgram struct and cleanup Address validity invariants

6ebc9de

Addresses with Segwitv0 not having len 20/32 are invalid and cannot be constructed. Also cleans up a API bug in ScriptBuf::new_witness_prog(ver, prog) allowing prog of invalid lenghts.

Make Payload non-exhaustive

a446df5

It is unlikely that we will see another segwit type fork. But we never know, best to mark Payload as [non-exhaustive]

Rename Payload::as_bytes to inner_prog_as_bytes

44d3ec4

Also make it private

sanket1729 force-pushed the addr_fixes branch from f6f304e to 44d3ec4 Compare January 23, 2023 21:45

apoelstra approved these changes Jan 23, 2023

View reviewed changes

tcharding approved these changes Jan 23, 2023

View reviewed changes

TheBlueMatt mentioned this pull request Jan 23, 2023

Use rust-bitcoin address valididty checks for bolt12 fallback address verification lightningdevkit/rust-lightning#1982

Open

apoelstra merged commit d8d3411 into rust-bitcoin:master Jan 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address validity invariant cleanups #1564

Address validity invariant cleanups #1564

sanket1729 commented Jan 19, 2023

sanket1729 Jan 19, 2023

Kixunil Jan 20, 2023

Kixunil left a comment

Kixunil Jan 20, 2023

Kixunil Jan 20, 2023

Kixunil Jan 20, 2023

Kixunil Jan 20, 2023

sanket1729 Jan 20, 2023

Kixunil Jan 20, 2023

sanket1729 Jan 20, 2023

Kixunil Jan 22, 2023

apoelstra Jan 23, 2023

Kixunil left a comment

Kixunil Jan 20, 2023

Kixunil Jan 20, 2023

apoelstra Jan 23, 2023

Kixunil Jan 20, 2023

sanket1729 Jan 23, 2023

tcharding left a comment

tcharding Jan 20, 2023

tcharding Jan 20, 2023

sanket1729 Jan 23, 2023

tcharding Jan 20, 2023

apoelstra left a comment

apoelstra commented Jan 23, 2023

sanket1729 commented Jan 23, 2023

sanket1729 commented Jan 23, 2023 •

edited

sanket1729 commented Jan 23, 2023

apoelstra left a comment

tcharding left a comment

		/// Returns a byte slice of the inner program of the payload. Incase the payload
		/// ia a script hash or pubkey hash, the a reference to the hash is returned.

	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 {

Address validity invariant cleanups #1564

Address validity invariant cleanups #1564

Conversation

sanket1729 commented Jan 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

apoelstra commented Jan 23, 2023

sanket1729 commented Jan 23, 2023

sanket1729 commented Jan 23, 2023 • edited

sanket1729 commented Jan 23, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding left a comment

Choose a reason for hiding this comment

sanket1729 commented Jan 23, 2023 •

edited