Move witness types to the script module #1846

tcharding · 2023-05-11T04:46:46Z

This is done as part of an ongoing effort to improve the address module and Address type.

Patch 1: Move WitnessVersion and WitnessProgram to their own modules within script - this is code move only except for the variant changes to the address::Error enum (see note below on rustfmt acting up).
Patch 2: Improves documentation on the WitnessProgram

Note on `rustfmt`

There are a bunch of formatting changes that shouldn't be in here, I stashed and re-ran the formatter a bunch of times but for some reason rustfmt wouldn't change address as it is but after I patched it rustfmt made a bunch of changes.

tcharding · 2023-05-11T04:50:43Z

The reason for creating two [trivial] modules is so that (with the re-exports in this PR) users who want to use the error will do so as:

use bitcoin::witness_version::{self, WitnessVersion};

...
enum Error {
    Witver(witness_version::Error),
    Other
}

And same for witness_program::Error.

Kixunil

Concept ACK. I did think it was odd that WitnessVersion is in address but wasn't thinking much about it. I'm not entirely sure about script but at least is sounds better than address.

bitcoin/src/address.rs

apoelstra · 2023-05-11T19:59:42Z

These are scriptPubKey concerns, rather than generic "script" concerns. In particular these values determine where the actual script to be interpreted should be found, but they have no meaning to the interpreter itself and are never treated as actual opcodes accessible from script. And a scriptPubKey is literally an address.

I don't feel strongly about this either way, so happy to go along with it, but IMHO these actually are address concerns..

apoelstra · 2023-05-11T20:00:43Z

Ah, I see, this creates a new witness_program module inside of script. Ok, agreed, that makes sense.

apoelstra

ACK dde7430f34669c936b4effddc5a25f9579b2f922

tcharding · 2023-05-11T21:38:05Z

8000

Should we put these two modules at the crate root instead? FTR this is only a code organisation issue, both modules and their types are re-exported at the crate root already.

apoelstra · 2023-05-11T22:05:16Z

No, I think witness_program is fine to live under script. It's a pretty obscure part of the library.

tcharding · 2023-05-11T22:27:59Z

Just realized that we actually have the WitnessProgram type slightly wrong

From the BIP:

The value of the first push is called the "version byte". The following byte vector pushed is called the "witness program".

But we have

/// The segregated witness program.
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct WitnessProgram {
    /// The witness program version number.
    version: WitnessVersion,
    /// The witness program (between 2 and 40 bytes).
    program: PushBytesBuf,
}

The version is not part of the witness program according to the wording of the BIP.

apoelstra · 2023-05-11T22:45:36Z

When "witness program" was a representation of an address, this made sense :)

apoelstra · 2023-05-11T22:46:52Z

In any case, a witness program without a version is a meaningless sequence of bytes. Unless the BIP gives us a different word to use in place of "witness version + witness program" I think we should continue to use the existing structure with the existing name.

tcharding · 2023-05-12T01:02:44Z

I read your comments @apoelstra but went ahead and did an exploratory patch that pulls the version byte out of WitnessProgram and puts it in the payload. On its own, admittedly, the change does not bring that much improvement but when viewed as part of a bigger effort to refactor/improve the address module's types I believe it is worth doing.

Its a separate patch so I can drop it if its disliked.

Kixunil · 2023-05-12T07:34:11Z

We could call it WitnessProgramWithVersion :)

WitnessProgram on its own makes sense once we use array + len instead of Vec/PushBytesBuf.

apoelstra · 2023-05-12T13:17:37Z

Neat. Ok, I like 97f531f19dbfa3e9eefa60d93fbcc2f1419dd576. I'm a tiny bit worried that it could confuse people who don't realize that Taproot outputs are also Segwit outputs, but it's more correct so I think it's the right move.

tcharding · 2023-05-14T00:43:24Z

I'm a tiny bit worried that it could confuse people who don't realize that Taproot outputs are also Segwit outputs

I think if someone does not realise that they are in for a world of pain using our library in general.

tcharding · 2023-05-14T00:48:44Z

For good measure I added:

    /// Checks whether a script is a segwit scriptPubkey.
    ///
    /// Remember, includes segwit v0 and taproot (v1) and any version to come.
    #[inline]
    pub fn is_segwit_script_pubkey(&self) -> bool {

tcharding · 2023-05-14T00:50:59Z

Changes in force push:

Added rustdoc posted above
Removed a line of whitespace

apoelstra

ACK cc5b6498b0d6de518d44ea4d0f522c855de5165f

tcharding · 2023-05-25T03:54:04Z

Gentle bump.

Kixunil

Doesn't look terrible but I don't like losing the protection and I'm too tired to confidently ACK now, so leaving this for reconsideration.

Kixunil · 2023-05-28T19:05:40Z

bitcoin/src/address.rs

+        /// Witness version (version byte of the scriptPubkey).
+        version: WitnessVersion,
+        /// Witness program (as defined by BIP141).
+        program: WitnessProgram,


So this no longer protects the invariant. I think a struct would be better. I guess calling it Segwit would be fine but I'm too tired to say for sure.

I believe this is resolved but the original code that this talks about is so long ago I don't really remember it. We do have a struct variant now.

I think Kix is right here. We can create the following invalid value for the Payload type.

let prog = WitnessProgram::new(WitnessVersion::V2, &[0x02; 31]).unwrap(); //31 bytes v2 progs are okay let payload = Payload::Segwit {version: WitnessVersion::V0, program: prog}; // 31 bytes v0 are incorrect.

I was happy to find that I did actually notice this when I coded it up, its in the patch description

This change makes the code more semantically correct. Note that it does introduce a non-explicit assumption that the same version be passed to `WitnessProgram::new` as is used in the `Payload` - is this acceptable engineering?

After attempting to fix this I ended up creating a struct that was exactly the same as the original WitnessProgram but with a different name. I've elected to drop the second patch and just add the following comment to the WitnessProgram

/// The segregated witness program is technically only the program bytes _excluding_ the witness /// version, however we maintain length invariants on the `program` that are governed by the version /// number, therefore we carry the version number around along with the program bytes.

bitcoin/src/blockdata/script/witness_program.rs

bitcoin/src/blockdata/script/owned.rs

tcharding · 2023-05-30T05:59:00Z

Rebased on master and removed the doc(hidden) usage.

042dcaa Remove doc(hidden) from error conversion functions (Tobin C. Harding) Pull request description: Give people access to the error type conversion docs, its no harm and it may be useful when the compiler does not give enough information. Done based on discussion here: #1846 (comment) ACKs for top commit: Kixunil: ACK 042dcaa apoelstra: ACK 042dcaa Tree-SHA512: 9d975845ba84213b203062282b68f06f6790d03dbc88d66dce82e9bedff4696fc01da6216920de9e9e4130f6469b32ff9c168d0800d057ec0bae51702d4a139e

apoelstra

ACK fc42616f4a37d820aa45165a9e7c778c88324d66

tcharding · 2023-06-06T07:20:31Z

Converting to draft until I resolve review comments.

tcharding · 2023-07-10T02:01:43Z

Brought this back to life because I started working on #1809, please see re-written PR description.

apoelstra · 2023-07-12T13:32:22Z

concept ACK. I think this is an improvement. I don't mind slipping it into this release since while it's API breaking, few users are poking around inside of Address.

apoelstra

ACK ab0a7f3

sanket1729

I think the second commit needs another struct SegwitProgram with private fields instead of a public enum

sanket1729 · 2023-07-12T19:36:22Z

bitcoin/src/address.rs

+        /// Witness version (version byte of the scriptPubkey).
+        version: WitnessVersion,
+        /// Witness program (as defined by BIP141).
+        program: WitnessProgram,


I think Kix is right here. We can create the following invalid value for the Payload type.

let prog = WitnessProgram::new(WitnessVersion::V2, &[0x02; 31]).unwrap(); //31 bytes v2 progs are okay let payload = Payload::Segwit {version: WitnessVersion::V0, program: prog}; // 31 bytes v0 are incorrect.

apoelstra · 2023-07-12T20:01:29Z

Ah, good point. I rescind my ACK.

tcharding · 2023-07-12T22:40:04Z

Cool, will fix. Thanks

From BIP 141: > A scriptPubKey (or redeemScript as defined in BIP16/P2SH) that > consists of a 1-byte push opcode (for 0 to 16) followed by a data push > between 2 and 40 bytes gets a new special meaning. The value of the > first push is called the "version byte". The following byte vector > pushed is called the "witness program". `WitnessVersion` and `WitnessProgram` are scriptPubkey concerns and scriptPubkey is basically synonymous with address so in one way it makes sense that these types are in `address` however we are in the process of overhauling the `Address` (and `AddressInner`) types so lets move the witness stuff to `script` and put it in individual sub-modules. This move helps simplify the address error type also. Note please, there are a bunch of formatting changes in here in the error type that I cannot explain and could not remove.

Add rustdocs to `WitnessProgram` commenting on why we carry the witness version number around with the witness program. This is mainly a dev comment but it helps document the invariants so make it a rustdoc comment.

tcharding · 2023-07-12T23:59:50Z

Changes in force push:

rebase on master
remove original patch 2 (changes to WitnessProgram)
add rustdoc to original WitnessProgram as a new patch 2
Re-write PR description

This PR is still an API breaking change because WitnessVersion and WitnessProgram are no longer publicly available at the path bitcoin::address::Witness*, instead they are available from the crate root and also from bitcoin::witness_program::WitnessProgram (and bitcoin::script::witness_program::WitnessProgram), similarly for version.

sanket1729

ACK 552f19a

apoelstra

ACK 552f19a

tcharding force-pushed the 05-11-move-types branch 2 times, most recently from 618c835 to dde7430 Compare May 11, 2023 07:59

Kixunil reviewed May 11, 2023

View reviewed changes

bitcoin/src/address.rs Outdated Show resolved Hide resolved

apoelstra previously approved these changes May 11, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via 97f531f May 12, 2023 01:00

tcharding mentioned this pull request May 12, 2023

Remove Network from AddressInner #1832

Merged

tcharding force-pushed 8000 the 05-11-move-types branch from 97f531f to cc5b649 Compare May 14, 2023 00:50

apoelstra previously approved these changes May 14, 2023

View reviewed changes

Kixunil reviewed May 28, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via c9c4bc7 May 30, 2023 05:49

tcharding force-pushed the 05-11-move-types branch 3 times, most recently from 0dde64a to fc42616 Compare May 30, 2023 05:58

tcharding mentioned this pull request May 30, 2023

Remove doc(hidden) from error conversion functions #1887

Merged

apoelstra previously approved these changes May 30, 2023

View reviewed changes

tcharding marked this pull request as draft June 6, 2023 07:20

tcharding dismissed apoelstra’s stale review via ab0a7f3 July 10, 2023 01:56

tcharding force-pushed the 05-11-move-types branch from fc42616 to ab0a7f3 Compare July 10, 2023 01:56

tcharding marked this pull request as ready for review July 10, 2023 01:58

apoelstra previously approved these changes Jul 12, 2023

View reviewed changes

sanket1729 reviewed Jul 12, 2023

View reviewed changes

tcharding added 2 commits July 13, 2023 09:51

Add more rustdocs to WitnessProgram

552f19a

Add rustdocs to `WitnessProgram` commenting on why we carry the witness version number around with the witness program. This is mainly a dev comment but it helps document the invariants so make it a rustdoc comment.

tcharding dismissed apoelstra’s stale review via 552f19a July 12, 2023 23:55

tcharding force-pushed the 05-11-move-types branch from ab0a7f3 to 552f19a Compare July 12, 2023 23:55

tcharding added the API break This PR requires a version bump for the next release label Jul 13, 2023

sanket1729 approved these changes Jul 13, 2023

View reviewed changes

apoelstra approved these changes Jul 13, 2023

View reviewed changes

apoelstra merged commit c1efb20 into rust-bitcoin:master Jul 13, 2023

tcharding deleted the 05-11-move-types branch July 14, 2023 03:24

Move witness types to the script module #1846

Move witness types to the script module #1846

Uh oh!

Conversation

Uh oh!

Note on rustfmt

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Note on `rustfmt`