ci(fuzz): replace toolchain and runner by realeinherjar · Pull Request #2116 · rust-bitcoin/rust-bitcoin

realeinherjar · 2023-10-08T09:19:32Z

Changes the toolchain fom actions-rs/toolchain to dtolnay/rust-toolchain.
Closes ci: change from actions-rs/toolchain to dtolnay/rust-toolchain #2113.
Changes the CI runner from ubuntu-20.04 to ubuntu-latest.
Related Updating CI to run on `ubuntu-22.04` #2114.
Updates the fuzz/generate-files.sh to match .github/workflows/fuzz.yml.
Closes fuzz/generate-files.sh goes stale #2115.

apoelstra · 2023-10-08T14:15:09Z

@realeinherjar if you change fuzz.yml you need to change generate-files.sh which generates this file. See #2115.

If you're feeling really motivated, you could add a CI check that these files are in sync, which would fix #2115 :) but @tcharding and I can handle it if you don't want to.

realeinherjar · 2023-10-08T15:06:32Z

I'll dive into that and let you both know...

realeinherjar · 2023-10-08T21:47:59Z

Fuzz bitcoin_deserialize_address is failing. Any idea why?

tcharding · 2023-10-08T22:07:07Z

The fuzz fail is unrelated to this PR, its because of a bug in rust-bech32 that we fixed recently, we need to get a new release out to fix this.

tcharding · 2023-10-08T22:13:59Z

ref: rust-bitcoin/rust-bech32#135

vincenzopalazzo

Concept ACK eb85212

The generate-files.sh script currently just echoes content in the fuzz. I had examined it in relation to this pull request, but it appears to be outdated. Running it on the master branch produces a diff concerning the fuzz_target.

This raises the question: Does it require updates, or should we consider removing it if it's no longer in use?

update

ops 😸 #2116

realeinherjar · 2023-10-09T14:13:35Z

Now with rust-bitcoin/rust-bech32#135 merged tagged and release.
Do I need to update Cargo.toml to use bech32 = "0.10.0-beta"?

apoelstra · 2023-10-09T17:15:30Z

@realeinherjar yes please! That should fix the final fuzztest.

apoelstra · 2023-10-09T17:16:17Z

Remember that Cargo-recent.lock and Cargo-minimal.lock also need to be changed.

tcharding · 2023-10-09T19:48:42Z

Oh I did not read this before doing #2117, hope I didn't step on your toes @realeinherjar.

realeinherjar · 2023-10-10T08:52:38Z

Rebased to fix the bug in rust-bech32 that was fixed in #2117.
All green, should be ready to review and merge.

vincenzopalazzo

OK the CI yaml change are good but the script fix is not enough

if you run it you find the following change

diff --git a/.github/workflows/fuzz.yml b/.github/workflows/fuzz.yml
index 0d0904b3..1c1b03ea 100644
--- a/.github/workflows/fuzz.yml
+++ b/.github/workflows/fuzz.yml
@@ -16,30 +16,29 @@ jobs:
       fail-fast: false
       matrix:
         fuzz_target: [
-          bitcoin_outpoint_string,
-          bitcoin_deserialize_amount,
-          bitcoin_deserialize_transaction,
-          bitcoin_deser_net_msg,
-          bitcoin_deserialize_address,
-          bitcoin_script_bytes_to_asm_fmt,
-          bitcoin_deserialize_prefilled_transaction,
-          bitcoin_deserialize_witness,
-          bitcoin_deserialize_psbt,
-          bitcoin_deserialize_block,
-          bitcoin_deserialize_script,
-          hashes_json,
-          hashes_cbor,
-          hashes_sha256,
-          hashes_ripemd160,
-          hashes_sha512_256,
-          hashes_sha512,
-          hashes_sha1,
+bitcoin_deserialize_witness,
+bitcoin_script_bytes_to_asm_fmt,
+bitcoin_deserialize_block,
+bitcoin_deserialize_script,
+bitcoin_deserialize_prefilled_transaction,
+bitcoin_deserialize_address,
+bitcoin_deser_net_msg,
+bitcoin_deserialize_psbt,
+bitcoin_deserialize_amount,
+bitcoin_deserialize_transaction,
+bitcoin_outpoint_string,
+hashes_ripemd160,
+hashes_sha256,
+hashes_cbor,
+hashes_sha512,
+hashes_sha1,
+hashes_sha512_256,
+hashes_json,
         ]
     steps:
       - name: Install test dependencies
         run: sudo apt-get update -y && sudo apt-get install -y binutils-dev libunwind8-dev libcurl4-openssl-dev libelf-dev libdw-dev cmake gcc libiberty-dev
-      - name: Checkout Crate
-        uses: actions/checkout@v4
+      - uses: actions/checkout@v4
       - uses: actions/cache@v3
         id: cache-fuzz
         with:
@@ -48,10 +47,9 @@ jobs:
             fuzz/target
             target
           key: cache-${{ matrix.target }}-${{ hashFiles('**/Cargo.toml','**/Cargo.lock') }}
-      - name: Checkout Toolchain
-        uses: dtolnay/rust-toolchain@stable
+      - uses: dtolnay/rust-toolchain@stable
         with:
-          toolchain: "1.65.0"
+          toolchain: '1.65.0'
       - name: fuzz
         run: |
           if [[ "${{ matrix.fuzz_target }}" =~ ^bitcoin ]]; then

To really fix the script you should generate the ci from the script and not change the CI and then update the script, otherwise, there is not reason to have the script.

realeinherjar · 2023-10-10T10:22:42Z

fuzz/generate-files.sh

For optimal indenting in YAML file

fuzz/fuzz-util.sh

apoelstra · 2023-10-10T14:10:20Z

Ok, I still have a diff in two places:

The sort order doesn't quite match. It looks like my sort treats _ as greater than alphanumerics but less than end-of-word whereas yours is the opposite.
In fuzz/Cargo.toml you have double //s and I have only one /.

realeinherjar · 2023-10-10T14:17:48Z

I think that these things are because of different bash versions?

Both of these bash versions (MacOs default and Nix unstable) give me the same files:

GNU bash, version 5.2.15(1)-release (aarch64-apple-darwin22.6.0)
GNU bash, version 3.2.57(1)-release (arm64-apple-darwin23)

Do you mind committing on top of this PR your local changes after running fuzz/generate-files.sh?

apoelstra · 2023-10-10T14:29:42Z

I have GNU bash, version 5.1.16(1)-release (x86_64-pc-linux-gnu).

And sure, one sec.

realeinherjar · 2023-10-10T16:54:55Z

Ok ready to merge

vincenzopalazzo

we clean up a little bit the commits? looks like we can create a better git history for this change

old: `actions-rs/toolchain` new: `dtolnay/rust-toolchain` fix ci(fuzz): change runner to ubuntu-latest ci: update run syntax in fuzz job ci: update and run fuzz/generate-files.sh

apoelstra

ACK 539f4ba

vincenzopalazzo

ACK 539f4ba

Good job! this PR was a Pandora's box eh? 😸

realeinherjar · 2023-10-10T20:53:53Z

ACK 539f4ba

Good job! this PR was a Pandora's box eh? 😸

Yes but now I know how the fuzz/ folder works, if there are any issues with it feel free to tag me.

apoelstra · 2023-10-10T21:02:04Z

Even though this is a CI fix I'm going to wait for a second ACK from @tcharding because we had a bunch of shell-script inconsistency.

tcharding

ACK 539f4ba

realeinherjar changed the title ~~ci: replace fuzz toolchain~~ ci(fuzz): replace toolchain and runner Oct 8, 2023

vincenzopalazzo reviewed Oct 9, 2023

View reviewed changes

vincenzopalazzo suggested changes Oct 10, 2023

View reviewed changes

realeinherjar requested a review from vincenzopalazzo October 10, 2023 10:21

realeinherjar commented Oct 10, 2023

View reviewed changes

fuzz/generate-files.sh Outdated

Copy link
Copy Markdown

Contributor Author

realeinherjar Oct 10, 2023

Choose a reason for hiding this comment

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

For optimal indenting in YAML file

realeinherjar commented Oct 10, 2023

View reviewed changes

fuzz/fuzz-util.sh Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Oct 10, 2023

View reviewed changes

realeinherjar and others added 2 commits October 10, 2023 14:39

ci: replace fuzz toolchain

20f3a05

old: `actions-rs/toolchain` new: `dtolnay/rust-toolchain` fix ci(fuzz): change runner to ubuntu-latest ci: update run syntax in fuzz job ci: update and run fuzz/generate-files.sh

apoelstra run of generate-files.sh

539f4ba

apoelstra approved these changes Oct 10, 2023

View reviewed changes

vincenzopalazzo approved these changes Oct 10, 2023

View reviewed changes

tcharding approved these changes Oct 10, 2023

View reviewed changes

apoelstra merged commit 90a95f6 into rust-bitcoin:master Oct 11, 2023

realeinherjar deleted the einherjar/rust-toolchain branch October 11, 2023 19:57

Conversation

realeinherjar commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Oct 8, 2023

Uh oh!

realeinherjar commented Oct 8, 2023

Uh oh!

realeinherjar commented Oct 8, 2023

Uh oh!

tcharding commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Oct 8, 2023

Uh oh!

vincenzopalazzo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

update

Uh oh!

realeinherjar commented Oct 9, 2023

Uh oh!

apoelstra commented Oct 9, 2023

Uh oh!

apoelstra commented Oct 9, 2023

Uh oh!

tcharding commented Oct 9, 2023

Uh oh!

realeinherjar commented Oct 10, 2023

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

realeinherjar Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apoelstra commented Oct 10, 2023

Uh oh!

realeinherjar commented Oct 10, 2023

Uh oh!

apoelstra commented Oct 10, 2023

Uh oh!

realeinherjar commented Oct 10, 2023

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

realeinherjar commented Oct 10, 2023

Uh oh!

apoelstra commented Oct 10, 2023

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

realeinherjar commented Oct 8, 2023 •

edited

Loading

tcharding commented Oct 8, 2023 •

edited

Loading

vincenzopalazzo left a comment •

edited

Loading