ElementsProject / rust-elements

Rust support for Elements transaction/block deserialization
Creative Commons Zero v1.0 Universal
51 stars 32 forks source link

Update rust-bitcoin to 0.31.0, and associated dependencies #188

Closed apoelstra closed 8 months ago

apoelstra commented 9 months ago

Let me know if I should split this into two PRs; the blech32 changes should arguably be done separately.

apoelstra commented 9 months ago

Oops, CI failure is that rust-secp-zkp is broken with cfg=fuzzing. I need to do another point release.

apoelstra commented 9 months ago

Blocked on https://github.com/BlockstreamResearch/rust-secp256k1-zkp/pull/76 which will fix CI here.

RCasatta commented 9 months ago

In a downstream test, I've got an error on address generation which seems related only to the checksum.

thread 'tests::test_ks_flow' panicked at ks_bindings/src/lib.rs:312:9:          
assertion `left == right` failed                                                
  left: "tlq1qq2xvpcvfup5j8zscjq05u2wxxjcyewk7979f3mmz5l7uw5pqmx6xf5xy50hsn6vhkm5euwt72x878eq6zxx2z58hd7zrsg9qn"
 right: "tlg1qq2xvpcvfup5j8zscjq05u2wxxjcyewk7979f3mmz5l7uw5pqmx6xf5xy50hsn6vhkm5euwt72x878eq6zxx2z03a3gsu4psmv"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace 

I don't follow bech32 logic deeply, but I think the error is about the constant 0x455972a3350f7a1 which in this MR is used only to check (read-only) while on master is used also in creating the checksum https://github.com/ElementsProject/rust-elements/blob/613204f38eeb8f4fcc767042197fd763965f057a/src/blech32.rs#L195

apoelstra commented 9 months ago

Thanks! I will investigate the checksum bug next week. I'm not sure what you mean by "used only to check". You can see in the fmt::Display impl of my new code that I use Blech32/Blech32m for encoding as well.

My guess is that I messed up the logic for choosing between bech32 and blech32.

RCasatta commented 9 months ago

I'm not sure what you mean by "used only to check"

I was meaning that the constant is not used to write another value, like it's done in master in the line let plm: u64 = polymod(&values) ^ variant.constant(); but I realiazed this could happen in the bech32 crate which I didn't check. So disregard the comment.

jamesdorfman commented 9 months ago

ACK 009205c5c3f7b4a91d28d2715f0edb62f1b8fb30. I don't fully understand the bech32 commit -- someone more knowledgable than me should review that. But, the rust-bitcoin changes LGTM.

apoelstra commented 8 months ago

lol @RCasatta I'm an idiot. The issue is that I turned the prefix tlq into tlg. The checksum thing was a red herring; the checksum only changed because I changed one character. Lemme fix this and re-push.

apoelstra commented 8 months ago

Ok, should be good to go. Fixed the tlq/tlg thing, and CI should be fixed because we updated secp256k1-zkp to expose some trait impls even in fuzzing mode.

apoelstra commented 8 months ago

Oh, derp, I need to release a new version of rust-secp-zkp, not just rust-secp-zkp-sys.

apoelstra commented 8 months ago

cc https://github.com/BlockstreamResearch/rust-secp256k1-zkp/pull/78

RCasatta commented 8 months ago

The checksum thing was a red herring;

Doh! Sorry for being the first to fall into the trap and accusing the poor checksum to be at fault!

Going to check again downstream and report

RCasatta commented 8 months ago

No more errors found downstream, LGTM

@apoelstra did you forget this? https://github.com/ElementsProject/rust-elements/pull/188#discussion_r1441771204 doccomments of that line and the following needs some trim

apart from that

ACK 7fe86e544de8e3ce585aaa1337fde0cc8af0039c

apoelstra commented 8 months ago

Oops, I did forget. Fixed.