Introduce and use `test_hex_unwrap` macro in `internals` #4250

tcharding · 2025-03-16T23:31:35Z

We have the hex_lit dependency for converting a hex string literal to an array.

Currently we have a test_hex_unwrap macro in the hex v0.3.0 release but not on either master
or the upcoming v1.0.0-alpha.0 release. This is making PRs around releasing and depending on the
release more noisy than required.

Introduce a test_hex_unwrap macro in internals for usage when the input is not a string literal.

Use hex_lit::hex where possible (often needing an additional call to

tcharding · 2025-03-16T23:32:02Z

This will make #4249 less noisy.

Kixunil

Unless I'm missing something, all of these can, and probably should, use hex_lit.

apoelstra · 2025-03-17T14:50:25Z

Yeah, would prefer to use hex_lit for uniformity.

tcharding · 2025-03-17T22:14:12Z

Not all call sites use string literals. Now uses hex_lit::hex where possible. I've updated the PR and description.

tcharding · 2025-03-17T22:17:42Z

What is going on here, the dev-dependency seems not to be available in bench code?

Kixunil · 2025-03-18T07:50:03Z

base58/Cargo.toml

@@ -23,6 +23,7 @@ internals = { package = "bitcoin-internals", version = "0.4.0" }

 [dev-dependencies]
 hex = { package = "hex-conservative", version = "0.3.0", default-features = false, features = ["alloc"] }
+internals = { package = "bitcoin-internals", version = "0.4.0", features = ["hex"] }


Looks like this needs hex_lit dependency instead.

Woops, I tried that before I worked out to throw .to_vec() everywhere. Done as suggested.

Kixunil · 2025-03-18T07:50:58Z

bitcoin/src/blockdata/witness.rs

        // annex starting with 0x50 causes the branching logic.
-        let annex = hex!("50");
+        let annex = hex!("50").to_vec();


Why are these needed?

I think the root cause is that we don't implement Encodable for slices.

Now that I looked below, it only needs slices.

Kixunil · 2025-03-18T07:51:45Z

internals/src/macros.rs

+#[cfg(feature = "hex")]
+mod tests {
+    #[test]
+    fn parse_hex_into_vector() {


Is this still needed?

The test you mean or the macro itself? The macro is used and I'd say if we use the macro then we keep the test as well.

I meant the macro, yes. Interesting that it's used - we have dynamic test data somewhere.

apoelstra · 2025-03-18T13:12:21Z

CI failure is real.

apoelstra · 2025-03-18T13:17:50Z

In fc334a1:

:/ there are a few points of messiness here:

One is the need for .to_vec for things that we're Decodeing. Ok, I think we can live with that since we decided elsewhere not to support Decodeing from byteslices directly.
Another is the need for .to_vec in Witness::from_slice. This maybe should be promoted to an API issue on Witness.
In one place you have use internals::test_hex_unwrap as hex;, and to support this you modify internals and also add an internals dep. Since it's only one location can you just write let hex = |b| <alloc::vec::Vec<u8> as hex::FromHex>::from_hex(b).unwrap());?

Only the third point is something that I'd like changed about this PR.

Kixunil · 2025-03-18T17:48:23Z

One is the need for .to_vec for things that we're Decodeing. Ok, I think we can live with that since we decided elsewhere not to support Decodeing from byteslices directly.

Do you mean Encodableing? ( 😉 )

Honestly, I think Encodable for Vec and no equivalent thing for slices is beyond weird. We already have a case that's not encoded with length (TaprootMerkleBranch). Instead it should be something like SliceExt with encode_len_prefixed method. But please, nobody bother with it now.

Another is the need for .to_vec in Witness::from_slice. This maybe should be promoted to an API issue on Witness.

That's not Witness issue but "nobody thought to make compiler suggest array of slices rather than array of differently-sized arrays" so Tobin didn't notice he can just cast the first array to a slice. :) And since this is easy to change and the PR is going to change anyway I'd like this to be changed.

tcharding · 2025-03-19T04:16:08Z

Only the third point is something that I'd like changed about this PR.

Mainly I just called Vec::from_hex and imported the trait.

Kixunil

IDK, so many call to to_vec where slices can be used is somewhat confusing.

Kixunil · 2025-03-19T20:23:08Z

bitcoin/src/blockdata/transaction.rs


    const SOME_TX: &str = "0100000001a15d57094aa7a21a28cb20b59aab8fc7d1149a3bdbcddba9c622e4f5f6a99ece010000006c493046022100f93bb0e7d8db7bd46e40132d1f8242026e045f03a0efe71bbb8e3f475e970d790221009337cd7f1f929f00cc6ff01f03729b069a7c21b59b1736ddfee5db5946c5da8c0121033b9b137ee87d5a812d6f506efdd37f0affa7ffc310711c06c7f3e097c9447c52ffffffff0100e1f505000000001976a9140389035a9225b3839e2bbf32d826a1e222031fd888ac00000000";

    #[bench]
    pub fn bench_transaction_size(bh: &mut Bencher) {
-        let raw_tx = hex!(SOME_TX);
+        let raw_tx = Vec::from_hex(SOME_TX).unwrap();


This also shouldn't need Vec, might need cast to &[u8] below.

Added a patch up front that cleans up the bench code in transaction.

Kixunil · 2025-03-19T20:24:03Z

bitcoin/src/blockdata/witness.rs

-        let el_0 = hex!("03d2e15674941bad4a996372cb87e1856d3652606d98562fe39c5e9e7e413f2105");
-        let el_1 = hex!("000000");
+        let el_0 = hex!("03d2e15674941bad4a996372cb87e1856d3652606d98562fe39c5e9e7e413f2105").to_vec();
+        let el_1 = hex!("000000").to_vec();


These could've been just slices.

I now see that for some reason this tests that Vec<Vec<u8>> serialization is the same as Witness. Do we actually need this? @apoelstra I think since we added Witness Vec<Vec<u8>> serialization is no longer relevant.

I now see that for some reason this tests that Vec<Vec<u8>> serialization is the same as Witness.

Well, because that's how the serialization of Witness is defined.

Maybe we can drop the encoding impls for Vec<Vec<u8>>, though it's a single line of code and isn't hurting anything and makes this test easy to write -- although it should probably be an arbitrary/kani test rather than a fixed-vector one.

Well, because that's how the serialization of Witness is defined.

My idea is to move the entire thing to Witness - indeed, removing Vec<Vec<u8>>.

isn't hurting anything

If any other future soft fork adds a structure that's similar to witness but encoded differently it'll be a mess. I would even argue it already has something similar - taproot merkle branch. Though we have a type that's so much more convenient nobody will ever convert it so it's not a problem now. Note that I don't object to a function capable of serializing Vec<Vec<u8>> existing, just to it being the trait impl.

And yes, I think we should fuzz it.

I don't think that taproot merkle branches are similar at all to a Vec<Vec>. They are similar to a Vec<[u8; 32]>, and without looking I can't say whether they are serialized like that or like a Vec.

But regardless, it's a distinct object which has its own distinct type, and the "raw type" that shares its serialization means little more than a mnemonic for remembering/implementing/testing the serialization.

I don't really mind removing Vec<Vec>, but I really don't think it hurts anything.

Kixunil · 2025-03-19T20:24:35Z

bitcoin/src/blockdata/witness.rs

        // annex starting with 0x50 causes the branching logic.
-        let annex = hex!("50");
+        let annex = hex!("50").to_vec();


Again, could've been slices.

Kixunil · 2025-03-19T20:26:06Z

bitcoin/src/blockdata/witness.rs

        // annex starting with 0x50 causes the branching logic.
-        let annex = hex!("50");
+        let annex = hex!("50").to_vec();


Now that I looked below, it only needs slices.

Kixunil · 2025-03-19T20:26:28Z

bitcoin/src/blockdata/witness.rs

        // annex starting with 0x50 causes the branching logic.
-        let annex = hex!("50");
+        let annex = hex!("50").to_vec();


Slices again.

Kixunil · 2025-03-19T20:29:16Z

bitcoin/src/psbt/mod.rs

-                                    hex!("3045022100d12b852d85dcd961d2f5f4ab660654df6eedcc794c0c33ce5cc309ffb5fce58d022067338a8e0e1725c197fb1a88af59f51e44e4255b20167c8684031c05d1f2592a01"),
-                                    hex!("0223b72beef0965d10be0778efecd61fcac6f79a4ea169393380734464f84f2ab3"),
+                                    hex!("3045022100d12b852d85dcd961d2f5f4ab660654df6eedcc794c0c33ce5cc309ffb5fce58d022067338a8e0e1725c197fb1a88af59f51e44e4255b20167c8684031c05d1f2592a01").to_vec(),
+                                    hex!("0223b72beef0965d10be0778efecd61fcac6f79a4ea169393380734464f84f2ab3").to_vec(),


Again, slices on all Witness constructions above.

tcharding · 2025-03-20T22:45:38Z

I'm leaving all the witness stuff as is. The macro being replaced produced a Vec, I don't see any reason why this PR should do otherwise. Feel free to follow up and remove the calls to .to_vec().

Kixunil · 2025-03-21T05:41:56Z

The reason is that the test is not supposed to test if vec works in particular (quite the opposite, a test should make sure non-vecs are usable). I know that historically we had Vec<Vec<u8>> but given the current code &[impl AsRef<[u8]>] better models the input to the Witness.

I feel like the cleanup should go before this PR, not after. I can send it if you're fine with waiting.

tcharding · 2025-03-21T20:30:04Z

Thanks, go for it.

Kixunil

Looks much better now.

Kixunil · 2025-03-24T05:05:34Z

bitcoin/src/blockdata/transaction.rs

@@ -2013,10 +2007,18 @@ mod benches {

    #[bench]
    pub fn bench_transaction_deserialize(bh: &mut Bencher) {
-        let raw_tx = hex!(SOME_TX);
+        let raw_tx = <Vec<u8> as hex::FromHex>::from_hex(SOME_TX).unwrap();


I don't understand this change. I think it's equivalent to the macro and should work equally well with hex_lit.

hex_lit doesn't work in bench code for some reason. Seems the dev-dependecies aren't available.

Oh, whaat?! OK then, let's keep it this way. A comment if you can be bothered would be nice.

I suspect this would be fixed with criterion. We should probably have a "switch to criterion" tracking issue so that we notice when we hit a critical mass of pain with the built-in benchmarks and feel like doing the work to switch.

I suspect this would be fixed with criterion. We should probably have a "switch to criterion" tracking issue so that we notice when we hit a critical mass of pain with the built-in benchmarks and feel like doing the work to switch.

Added to #3333

84bee2f Simplify `Witness` construction in tests (Martin Habovstiak) 3551ec2 Don't access internalls of `Witness` in tests (Martin Habovstiak) c807836 Impl `PartialEq` between `Witness` and containers (Martin Habovstiak) 587a66d Add a bunch of missing conversions for `Witness` (Martin Habovstiak) Pull request description: This is supposed to go in front of #4250 `Witness` lacked a bunch of APIs that were making it harder to use and test, so this also adds them in addition to cleaning up tests. (I only realized they are missing when I tried to clean up tests and got a bunch of errors.) ACKs for top commit: tcharding: ACK 84bee2f apoelstra: ACK 84bee2f; successfully ran local tests Tree-SHA512: 7973f2a56b070babba7b4c632f45858154ccd00f8e77956ad2d28cb66e1fd18ff60d92c031ba3b76d0958e4acd34adfca10607fa26ec569dfd52ba1c1e2c79eb

We can use `deserialize_hex` when outside of the actual benchmark code to simplify the functions. Also add an additional test that benchmarks `deserialize_hex`.

We have the `hex_lit` dependency for converting a hex string literal to an array. Currently we have a `test_hex_unwrap` macro in the `hex v0.3.0` release but not on either `master` or the upcoming `v1.0.0-alpha.0` release. This is making PRs around releasing and depending on the release more noisy than required. Use `hex_lit::hex` where possible (often needing an additional call to `to_vec()`) and where not possible use `Vec::from_hex`.

tcharding · 2025-04-10T20:49:33Z

Rebased and added a comment to the bench usage of FromHex.

apoelstra

ACK d6296cd; successfully ran local tests

yancyribbens · 2025-04-13T12:03:27Z

bitcoin/src/blockdata/witness.rs


        let mut want_witness = Witness::default();
        want_witness.push(&el_0);
        want_witness.push(&el_1);

-        let vec = vec![el_0.clone(), el_1.clone()];
+        let vec = vec![el_0, el_1];


This is later used in a function let got_witness = Witness::from_slice(&vec);. Can't this just be a slice the begin with?

If we need to rebase this PR we could fix this. I'm a little surprised the clippy "unnecesary vec" lint didn't notice it.

Kixunil

ACK d6296cd

github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-internals PRs modifying the internals crate C-base58 labels Mar 16, 2025

tcharding force-pushed the 03-17-hex-macro branch 3 times, most recently from 813a170 to c3b0e79 Compare March 17, 2025 00:16

Kixunil reviewed Mar 17, 2025

View reviewed changes

tcharding force-pushed the 03-17-hex-macro branch from c3b0e79 to 9fdb191 Compare March 17, 2025 22:13

Kixunil reviewed Mar 18, 2025

View reviewed changes

tcharding force-pushed the 03-17-hex-macro branch from 9fdb191 to fc334a1 Compare March 18, 2025 08:03

tcharding force-pushed the 03-17-hex-macro branch from fc334a1 to 382ed03 Compare March 19, 2025 03:24

github-actions bot removed the C-internals PRs modifying the internals crate label Mar 19, 2025

tcharding force-pushed the 03-17-hex-macro branch 3 times, most recently from 89a6596 to fa8e588 Compare March 19, 2025 03:59

Kixunil reviewed Mar 19, 2025

View reviewed changes

tcharding marked this pull request as draft March 21, 2025 20:30

Kixunil mentioned this pull request Mar 23, 2025

Witness api improvements and test cleanups #4279

Merged

tcharding force-pushed the 03-17-hex-macro branch from fa8e588 to 7560f0f Compare March 23, 2025 22:20

github-actions bot added C-internals PRs modifying the internals crate C-primitives labels Mar 23, 2025

Kixunil reviewed Mar 24, 2025

View reviewed changes

tcharding added 2 commits April 11, 2025 06:48

Simplify and improve transaction benchmarks

37035e2

We can use `deserialize_hex` when outside of the actual benchmark code to simplify the functions. Also add an additional test that benchmarks `deserialize_hex`.

tcharding force-pushed the 03-17-hex-macro branch from 7560f0f to d6296cd Compare April 10, 2025 20:49

tcharding marked this pull request as ready for review April 10, 2025 20:49

github-actions bot removed C-internals PRs modifying the internals crate C-primitives labels Apr 10, 2025

apoelstra approved these changes Apr 11, 2025

View reviewed changes

yancyribbens mentioned this pull request Apr 13, 2025

Consider using criterion for benches #3333

Open

yancyribbens reviewed Apr 13, 2025

View reviewed changes

Kixunil approved these changes Apr 23, 2025

View reviewed changes

apoelstra merged commit 5f4075a into rust-bitcoin:master Apr 24, 2025
24 checks passed

tcharding deleted the 03-17-hex-macro branch April 28, 2025 03:37

Introduce and use test_hex_unwrap macro in internals #4250

Introduce and use test_hex_unwrap macro in internals #4250

Uh oh!

Conversation

tcharding commented Mar 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 16, 2025

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Mar 17, 2025

Uh oh!

tcharding commented Mar 17, 2025

Uh oh!

tcharding commented Mar 17, 2025

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!

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!

apoelstra commented Mar 18, 2025

Uh oh!

apoelstra commented Mar 18, 2025

Uh oh!

Kixunil commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Mar 19, 2025

Uh oh!

Kixunil left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Mar 20, 2025

Uh oh!

Kixunil commented Mar 21, 2025

Uh oh!

tcharding commented Mar 21, 2025

Uh oh!

Kixunil left a comment

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

Introduce and use `test_hex_unwrap` macro in `internals` #4250

Introduce and use `test_hex_unwrap` macro in `internals` #4250

tcharding commented Mar 16, 2025 •

edited

Loading

Kixunil commented Mar 18, 2025 •

edited

Loading