Vendor 0.21.2 #79

tcharding · 2023-12-17T20:35:00Z

This is #77 without the version bump patch.

Upgrade to a new way of vendoring Core and vendor version 0.21.2 - note please this will be directly followed by an upgrade to 0.21-final. Done separately to proof out the workflow of such an upgrade.

apoelstra · 2023-12-18T19:27:36Z

I'm gonna go ahead and review/ACK this although strictly speaking the intermediate commits don't compile. I think this crate has historically had a lot of vendoring-related grief and bisection is probably already pretty busted, and this puts us on the start to fixing that.

tcharding · 2023-12-18T20:18:47Z

Yes, my bad I could have mentioned that - they explicitly do not compile, and I had exactly the same thoughts as you mention. It would be nice if we could start getting them all to compile from now. The do in secp so I think we should, I"ll try and keep that as a goal.

apoelstra · 2023-12-18T20:32:15Z

contrib/vendor-bitcoin-core.sh

+# not specified a revision, just copy the directory rather than using 'git clone'.
+# This lets us use non-git repos or dirty source trees as secp sources.
+if [ "$CORE_VENDOR_CP_NOT_CLONE" == "yes" ]; then
+    cp -r "$CORE_VENDOR_REPO" .


In b83e1de:

In the rust-secp version of this it's a cp "$VENDOR_REPO" "$DIR". Why did you change $DIR to .? It causes the following chmod to fail.

At the time I couldn't work out a sane way to call the script, I was using

CORE_VENDOR_CP_NOT_CLONE=yes CORE_VENDOR_REPO=/home/tobin/build/github.com/tcharding/bitcoin contrib/vendor-bitcoin-core.sh v0.21-final

But that results in bitcoin/bitcoin. How do you call it?

Are you using CORE_VENDOR_REPO=/path/to/bitcoin/*

No, just CORE_VENDOR_REPO=/path/to/bitcoin. As in

CORE_VENDOR_CP_NOT_CLONE=yes \ CORE_VENDOR_GIT_ROOT=.. \ CORE_VENDOR_REPO=/nix/store/77qhrhjq230jh6zswzg2nfxirpw4zdd3-source \ CORE_VENDOR_DEPEND_DIR=./depend2/ \ ./contrib/vendor-bitcoin-core.sh -f

If you are not specifying CORE_VENDOR_DEPEND_DIR then it will default to depend/. And you'll get a directory depend/bitcoin as expected. I'm not sure how you would get bitcoin/bitcoin.

ooooo, TIL, if the destination directory exists already then the behaviour of cp is different to if it does not exist - I have never realized this, wow, mind blown. If the directory does not exist it is as you say, it creates the directory correctly, but if it exists then it gets copied into the existing one.

I still don't know how this was happening though because we explicitly rm -rf $DIR right above. Anyways, onwards and upwards!

I have never realized this, wow, mind blown

FWIW I always use rsync and never use cp -r, its still surprising that I did not know the nuances of cp.

I wouldn't be opposed to using rsync here, FWIW, though it would be an extra dependency I'd have to pull into my nix derivation and it's probably overkill here.

Agreed that cp is nuts :)

Oh I didn't mean to use it here, I just meant on my machine I never use cp for directories.

tcharding · 2023-12-21T21:06:55Z

Force pushed the change discussed above, and tested with CORE_VENDOR_CP_NOT_CLONE=yes CORE_VENDOR_REPO=/home/tobin/build/github.com/tcharding/bitcoin contrib/vendor-bitcoin-core.sh v0.21.2

Thanks for you patience :)

apoelstra · 2023-12-22T14:31:14Z

contrib/vendor-bitcoin-core.sh

+    shift
+done
+
+echo "Vendoring Bitcoin Core version: $CORE_VENDOR_VERSION"


In 902bc9c:

The rust-secp version of this allows you to leave the revision unset and just use whatever code is present in the directory you're copying. We should preserve this behavior.

Cracking the whip on me, I was too lazy to parse out the version number using regex - I'll do it now :)

Done, that wasn't so painful.

apoelstra · 2023-12-22T14:31:46Z

contrib/vendor-bitcoin-core.sh

+
+# Check out specified revision
+pushd "$DIR" > /dev/null
+git checkout "$CORE_VENDOR_VERSION"


In 902bc9c:

In particular, here you unconditionally do a git checkout. This will not work if you are copying a source tree that is not in git.

Now mimic's the secp script.

Instead of using a subtree add a script to vendor Bitcoin Core. Based on the one in `rust-secp256k1`.

Run the vendoring script: `./vendor-bitcoin-core.sh v0.21.2`

Fix the build after vendoring new version of Core. Includes changes to the build script by Jake and update to the C++ toolchain suggested by Richard. Co-developed-by: Jake Rawsthorne <jake@jakerawsthorne.co.uk> Co-developed-by: Richard Ulrich <richard.ulrich@seba.swiss>

tcharding · 2023-12-23T21:41:11Z

Now more like the secp script and includes -h option.

$ contrib/vendor-bitcoin-core.sh -h

Usage: script OPTIONS [bitcoin-core-version]

OPTIONS:

 -f    Vendor even if there are local changes to the rust-bitcoinconsensus git index
 -h    Print this help an exit

Example:

    vendor-bitcoin-core v0.21.2

apoelstra · 2023-12-25T16:29:53Z

Awesome :) we are super close. The vendoring script works now, but when I check the vendored code against the source I see

diff -r depend/bitcoin/src/clientversion.cpp depend2/bitcoin/src/clientversion.cpp
28c28
< #define GIT_COMMIT_ID "12d7ec38e2c69539d6d9d7e99c4da8e41b7824d6"
---
> #define GIT_COMMIT_ID "af591f2068d0363c92d9756ca39c43db85e5804c"

These two commit IDs are the tip of this PR in this repo and the commit from Bitcoin Core that we're vendoring.

This is suuper weird. If you look at bitcoin/src/clientversion.cpp you will find neither line. Instead you will see

  //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives. $Format:%n#define GIT_COMMIT_ID "%H"$

How does git do this and under what circumstances? I don't know. But I worry that it is putting our commit ID into this line rather than the Core commit ID. OTOH I don't think any of these constants are used by libbitcoinconsensus so maybe it doesn't matter..

Anyway I think I should just whitelist this specific file and forget about it. But let me know what you think.

tcharding · 2023-12-27T21:33:28Z

But I worry that it is putting our commit ID into this line rather than the Core commit ID.

I don't really like the idea of having this change in there, could bite us down the track. A simple hack could be, at the end of the vendoring script, to just grep for GIT_COMMIT_ID and echo a message "manually remove the change to GIT_COMMIT_ID" - we could spit out the old/new lines and the file.

tcharding · 2023-12-27T21:52:43Z

#define GIT_COMMIT_ID "12d7ec38e2c69539d6d9d7e99c4da8e41b7824d6"

I had a bit of a play and we are still doing something different, I don't get these lines in clientversion.cpp, and they don't exist on this branch?

I tried using clone (and last time I pushed I was using copy)?

apoelstra · 2024-01-02T16:40:59Z

I think you need to use git archive or something to populate these lines. I am getting it by using Nix to source the git repo.

I think this is a non-issue because:

The offending line doesn't actually appear in the repo; it is already dependent on some git magic that users are unlikely to trigger.
The offending #define is not actually used in bitcoinconsensus anywhere.

tcharding · 2024-01-07T21:17:47Z

Cool, so this is good to go in then, right?

apoelstra · 2024-01-08T15:40:21Z

Yep. I'll tweak my scripts to ignore this line and ACK/merge it.

apoelstra

ACK 12d7ec3

tcharding mentioned this pull request Dec 17, 2023

Release tracking PR: v0.101.0+0.21.2 #77

Merged

apoelstra reviewed Dec 18, 2023

View reviewed changes

tcharding mentioned this pull request Dec 19, 2023

Add CI script and minimal/recent lock files #81

Merged

tcharding force-pushed the vendor-0.21.2 branch from a8f4627 to 1419657 Compare December 21, 2023 21:05

apoelstra reviewed Dec 22, 2023

View reviewed changes

tcharding force-pushed the vendor-0.21.2 branch from 1419657 to ab80dbb Compare December 23, 2023 21:38

tcharding added 5 commits December 24, 2023 08:38

Add Bitcoin Core vendor script

619611e

Instead of using a subtree add a script to vendor Bitcoin Core. Based on the one in `rust-secp256k1`.

Remove the depend directory

3ca86b9

Update README about vendoring

8b3d23f

Vendor Bitcoin Core v0.21.2

a4f30ef

Run the vendoring script: `./vendor-bitcoin-core.sh v0.21.2`

tcharding force-pushed the vendor-0.21.2 branch from ab80dbb to 12d7ec3 Compare December 23, 2023 21:38

apoelstra approved these changes Jan 8, 2024

View reviewed changes

apoelstra merged commit 875511c into rust-bitcoin:master Jan 8, 2024
9 checks passed

tcharding deleted the vendor-0.21.2 branch January 9, 2024 02:30

apoelstra mentioned this pull request Feb 7, 2024

Update Bitcoin core? #66

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vendor 0.21.2 #79

Vendor 0.21.2 #79

tcharding commented Dec 17, 2023

apoelstra commented Dec 18, 2023

tcharding commented Dec 18, 2023

apoelstra Dec 18, 2023

tcharding Dec 19, 2023 •

edited

Loading

tcharding Dec 19, 2023

apoelstra Dec 19, 2023

apoelstra Dec 19, 2023

tcharding Dec 21, 2023

tcharding Dec 21, 2023

tcharding Dec 21, 2023

apoelstra Dec 22, 2023

tcharding Dec 23, 2023

tcharding commented Dec 21, 2023

apoelstra Dec 22, 2023

tcharding Dec 23, 2023 •

edited

Loading

tcharding Dec 23, 2023

apoelstra Dec 22, 2023

tcharding Dec 23, 2023

tcharding commented Dec 23, 2023

apoelstra commented Dec 25, 2023

tcharding commented Dec 27, 2023 •

edited

Loading

tcharding commented Dec 27, 2023

apoelstra commented Jan 2, 2024

tcharding commented Jan 7, 2024

apoelstra commented Jan 8, 2024

apoelstra left a comment

Vendor 0.21.2 #79

Vendor 0.21.2 #79

Conversation

tcharding commented Dec 17, 2023

apoelstra commented Dec 18, 2023

tcharding commented Dec 18, 2023

Choose a reason for hiding this comment

tcharding Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Dec 21, 2023

Choose a reason for hiding this comment

tcharding Dec 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Dec 23, 2023

apoelstra commented Dec 25, 2023

tcharding commented Dec 27, 2023 • edited Loading

tcharding commented Dec 27, 2023

apoelstra commented Jan 2, 2024

tcharding commented Jan 7, 2024

apoelstra commented Jan 8, 2024

apoelstra left a comment

Choose a reason for hiding this comment

tcharding Dec 19, 2023 •

edited

Loading

tcharding Dec 23, 2023 •

edited

Loading

tcharding commented Dec 27, 2023 •

edited

Loading