rust-bitcoin / rust-bitcoinconsensus

Bitcoin's libbitcoinconsenus.a with Rust binding. Built from Bitcoin sources with cargo.
Apache License 2.0
46 stars 34 forks source link

Vendor Bitcoin Core `v26.0` #93

Closed tcharding closed 8 months ago

tcharding commented 9 months ago

Tested in: https://github.com/rust-bitcoin/rust-bitcoin/pull/2461

Close: #66

tcharding commented 9 months ago

Gentle bump @apoelstra

tcharding commented 9 months ago

Current status: some linking error that is likely just brain dead but I can't figure it out right now.

tcharding commented 8 months ago

Status update: unit test as mentioned in PR description.

tcharding commented 8 months ago

Massive props to @Davidson-Souza for debugging this.

tcharding commented 8 months ago

Let's fix it before merge, I'll do it today.

On Fri, Mar 1, 2024, at 08:22, Andrew Poelstra wrote:

@.**** commented on this pull request.

In src/lib.rs https://github.com/rust-bitcoin/rust-bitcoinconsensus/pull/93#discussion_r1508208505:

 }

}

+/// Mimics the Bitcoin Core UTXO typedef (bitcoinconsenus.h) +// This is the TxOut data for utxos being spent, i.e., previous outputs. +#[repr(C)] +pub struct Utxo {

  • /// WARNING: This should be the consensus encoded script without the leading var int!
  • ///
  • /// ScriptPubkey's in rust-bitcoin are consensus encoded with a variable byte length value
  • /// prepended to the front - remove the varint then take a pointer to the encoded vector.

In33022e2:

I'm ok merging this for now and doing a followup PR (or another release) to update the comment. But this comment seems like it's way overcomplicated things. It looks like you just need a raw pointer to the underlying bytes of the Script as well as its length. You should be able to just use [..] to get a byteslice from a Script (or whatever our accessor is for this) then extract the pointer/length from there.

I don't see why you would need to encode and tweak the encoding.

— Reply to this email directly, view it on GitHub https://github.com/rust-bitcoin/rust-bitcoinconsensus/pull/93#pullrequestreview-1909897187, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADAKQ5NUNFYE65BJSES6XG3YV6NZZAVCNFSM6AAAAABDAW67DOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBZHA4TOMJYG4. You are receiving this because you authored the thread.Message ID: @.***>

-- Tobin http://tobin.cc

tcharding commented 8 months ago

Should be right to go.

apoelstra commented 8 months ago

Tagged and published.

tcharding commented 8 months ago

Legend, thanks man. I realised late last night that I'd put the version bump in here instead of doing a separate PR, and I'm staying in the middle of nowhere with no mobile/internet coverage. Thanks for handling it.

panicfarm commented 8 months ago

Thank you, guys! I wanted to thank you earlier, but after running verify() on ~100k inputs, we intermittently encountered Invalid Schnorr signature hash type error here. The input is valid and reliably verifiable by btcdeb. It's an intermittent error, that sometimes happens and sometimes does not, on exactly same transaction data, for example on txid cd4a6bb55046e96c25d453f6e0a889c4cb75a6be86c86e67a689d8be7edb9318 inp 0 (and on many others). I will try to make something more reproducible and update, unless you have an idea of why this might be happening intermittently?

apoelstra commented 8 months ago

I wonder if this could be related to our msan issues. Possibly bitcoinconsensus' vendored libsecp and rust-secp's libsecp are somehow incompatible? Just speculating.

panicfarm commented 7 months ago

I wonder if this could be related to our msan issues. Possibly bitcoinconsensus' vendored libsecp and rust-secp's libsecp are somehow incompatible? Just speculating.

It was our own problem: In the UTXO struct the pub script_pubkey: *const c_uchar got corrupted, because it was poiting to a slice that had a shorter lifetime than the UTXO. The compiler did not catch this because UTXO does not use unsafe.