Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vendor Bitcoin Core v26.0 #93

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Feb 9, 2024

  • Patch 1 is the usual vendoring of Core v26.0
  • Patch 2 modifies the API. I elected to not deprecate - we want to force users to understand this I believe
  • Patch 3 fixes typo in the date of the last two releases
  • Patch 4 bumps the version

Tested in: rust-bitcoin/rust-bitcoin#2461

Close: #66

Run the vendoring script:

   `./contrib/vendor-bitcoin-core.sh v26.0`
@tcharding
Copy link
Member Author

Gentle bump @apoelstra

@tcharding tcharding mentioned this pull request Feb 11, 2024
@tcharding tcharding force-pushed the 02-09-vendor-26.0 branch 3 times, most recently from a0e9fbc to 188d51e Compare February 22, 2024 00:55
@tcharding
Copy link
Member Author

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

@tcharding
Copy link
Member Author

Status update: unit test as mentioned in PR description.

@tcharding tcharding force-pushed the 02-09-vendor-26.0 branch 2 times, most recently from 3c74fa0 to c0ed53a Compare February 29, 2024 19:48
@tcharding tcharding marked this pull request as ready for review February 29, 2024 19:50
@tcharding
Copy link
Member Author

Massive props to @Davidson-Souza for debugging this.

src/lib.rs Outdated
/// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 33022e2:

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.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 26f488a except the comment about the Utxo struct

@tcharding
Copy link
Member Author

tcharding commented Feb 29, 2024 via email

Now that we have upgraded to Bitcoin Core v26 we can support
verification of taproot scripts.

Add an initial API to do so - needs iterating upon.
We used the wrong month for the last two release dates, fix them.
Add a changelog entry, bump the version, and update the lock files.
@tcharding
Copy link
Member Author

Should be right to go.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 81515e2

@apoelstra apoelstra merged commit ca6b531 into rust-bitcoin:master Mar 1, 2024
5 checks passed
@apoelstra
Copy link
Member

Tagged and published.

@tcharding
Copy link
Member Author

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
Copy link

panicfarm commented Mar 16, 2024

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
Copy link
Member

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Bitcoin core?
3 participants