Update what deps to pin

[yancyribbens]

Update the deps to pin so that they match contrib/test.sh: https://github.com/rust-bitcoin/rust-bitcoin/blob/master/bitcoin/contrib/test.sh#L27

[tcharding]

We still pin half in the fuzz tests.

[yancyribbens]

This was a mistake on my part for not confirming ahead of time, but I'm not sure why these deps need to be pinned. @apoelstra mentioned they where out of sync now with contrib/test.sh #1969.

[apoelstra]

I think it's probably fine not to mention half since downstream users won't need to pin it.

Re "not sure why", try compiling with 1.48.0 without the pin, and see what happens.

[yancyribbens]

I tested building rust-bitcoin as well as a downstream crate that depends on rust-bitcoin without the pinning and both built without a problem using cargo +1.48 build. To build rust-bitcoin, I had to copy Cargo-recent.lock -> Cargo.lock. I tested the builds inside a clean bsd jail to be sure there wasn't any cargo cache or something like that.

[yancyribbens]

also note the downstream crate is pointing at the master branch on git that I tested.

[apoelstra]

If you are using Cargo-recent.lock then you're not testing the pins.

For my part I can't build with 1.48.0 with the current set of pins, because proc-macros2 doesn't build with the latest MSRV. When I add that pin, I then can't build because of some incompatibility with hashes macros.

[yancyribbens]

After a clean install the pins in ./bitcoin/contrib/test.sh without changing Cargo.lock:

cargo update -p serde_json --precise 1.0.99
cargo update -p serde --precise 1.0.156

however this does not build for me.

[yancyribbens]

If you are using Cargo-recent.lock then you're not testing the pins.

If that's the case, then do the pins actually need to be run in ./bitcoin/contrib/test.sh?

[yancyribbens]

I tested taking the pins out of test.sh and the test suits passes.

[apoelstra]

ACK 634f873

[apoelstra]

Ok, the following series of actions doesn't work:

cargo +1.48.0 update
#cargo +1.48.0 build ## will fail, needs serde pins

# serde pins copied from contrib/test.sh
cargo update -p serde_json --precise 1.0.99
cargo update -p serde --precise 1.0.156
#cargo +1.48.0 build ## will fail, needs proc-macro2 pin

cargo update -p quote --precise 1.0.30 # needed to pin proc-macro2
cargo update -p proc-macro2 --precise 1.0.63
#cargo +1.48.0 build ## will fail, gives me macro errors

# Tell cargo to actually build the code in the repo, don't download other random code and build that instead.
# This is so surprising and weird that it probably should get a CVE..
cargo +1.48.0 update -p bitcoin:0.30.1 --precise 0.30.0

cargo +1.48.0 build # Ok now this will build.

# Let's try with all features now..
#cargo +1.48.0 build --all-features --all-targets ## will fail, needs serde_test pin

cargo update -p serde_test --precise 1.0.175
cargo +1.48.0 build --all-features --all-targets ## Ok, good now

While the macro errors appear to originate inside the repo, they don't occur when I use Cargo-recent.lock. Investigating further..

Edit: Ok, the issue seems to be that after running cargo update, it starts trying to build a random cached version of rust-bitcoin with the version of bitcoin_hashes from the repo, which is clearly a cargo bug.

Edit 2: Got it working. Updated code. I think that @yancyribbons is right that the half pin isn't needed.v

[tcharding]

Tell cargo to actually build the code in the repo, don't download other random code and build that instead.
This is so surprising and weird that it probably should get a CVE..
cargo +1.48.0 update -p bitcoin:0.30.1 --precise 0.30.0

Holy Moly, I did not get this step any time I tried to workout the pins.

[tcharding]

Confirming sequence of command posted above. So the readme needs to be something like:

# To build without running tests, pin these four:
cargo update -p serde_json --precise 1.0.99
cargo update -p serde --precise 1.0.156
cargo update -p quote --precise 1.0.30
cargo update -p proc-macro2 --precise 1.0.63

# To build and run tests, also pin:
cargo update -p serde_test --precise 1.0.175

[yancyribbens]

Both cargo build and cargo test worked for me with the four pins:

cargo update -p serde_json --precise 1.0.99
cargo update -p serde --precise 1.0.156
cargo update -p quote --precise 1.0.30
cargo update -p proc-macro2 --precise 1.0.63

I didn't need to run the following to run test:

cargo update -p serde_test --precise 1.0.175

note I tested this on a clean install.

[yancyribbens]

The README has been updated to include the four pins.

[apoelstra]

This PR now has a stray "add max standard tx weight to transaction" commit in it which does not match any other PR.

[yancyribbens]

@apoelstra thanks. Rebase gone bad I guess :)

[apoelstra]

Looks good! As a separate PR we should add a test that directly tries to use the latest deps from cargo (and that will require the pins).

[tcharding]

ACK 8ac3eb2

[apoelstra]

ACK 8ac3eb2

[tcharding]

Strange that you didn't need to pin serde_test, I rechecked on my system and this script does not run if I comment out the serde_test pin

#!/bin/sh

rm Cargo.lock
cargo +1.48.0 update
#cargo +1.48.0 build ## will fail, needs serde pins

# serde pins copied from contrib/test.sh
cargo update -p serde_json --precise 1.0.99
cargo update -p serde --precise 1.0.156
#cargo +1.48.0 build ## will fail, needs proc-macro2 pin

cargo update -p quote --precise 1.0.30 # needed to pin proc-macro2
cargo update -p proc-macro2 --precise 1.0.63
#cargo +1.48.0 build ## will fail, gives me macro errors

cargo +1.48.0 update -p bitcoin:0.30.1 --precise 0.30.0
# cargo +1.48.0 build ## will fail, needs serde_test pin

cargo update -p serde_test --precise 1.0.175

cargo +1.48.0 test

[apoelstra]

@tcharding oh, oops. So, CI currently does not test pinning at all, because it exclusively tests with the fixed lockfiles. I think we do need the serde_test pin.

[tcharding]

So, CI currently does not test pinning at all

Yeah, sadly I can feel a multi hour bash hacking session coming on ...

[sanket1729]

The following works for rust-minsicript
https://github.com/rust-bitcoin/rust-miniscript/blob/d1c61e49316f00835dea13a7ec81eb85f13e3742/contrib/test.sh#L24-L31

Note that not all of these would be required as rust-miniscript also has a dev dependency on bitcoind crate.

[yancyribbens]

Strange that you didn't need to pin serde_test, I rechecked on my system and this script does not run if I comment out the serde_test pin

Oops yeah you're right, serde_test pin is required. I think maybe I accidently ran cargo test without adding 1.48 yesterday.

[yancyribbens]

Added cargo update -p serde_test --precise 1.0.175 to the README.md

[apoelstra]

ACK 443a4c5

[tcharding]

ACK 443a4c5