ee30eaa81b fuzz: add more coverage for `deserialize_block` (Bruno Garcia)
Pull request description:
This PR adds more coverage for the `deserialize_block` target. First, it serializes the block and compares with the data provided and then call some block functions to ensure they run without any issue.
ACKs for top commit:
apoelstra:
ACK ee30eaa81b Thanks!
Tree-SHA512: db6c7befa7b429267e9b28fb7910d2b9e8d462e22c791b544b1aa528c7ba4f8e1cd32e1276c67d3da1f97ce484e7ed19ec20b10d2030f2977fd8f855cf510ffe
We were using an outdated CBOR crate for MSRV reasons. But this old
crate is causing suprious test failures. So delete it. (Sadly, updating
the crate doesn't fix the issue, replacing it with ciborium breaks our
MSRV tests because it needs a more recent `half` dependency, and
replacing it with `minicbor` doesn't work because minicbor is not based
on serde. So we don't really have any options.)
In general, I am suspicious of this decode-then-reencode test. CBOR has
some ambiguity in integer encoding. Empirically it has seemed to
work for a long time, but this seems more like an indictment of our test
than a positive result.
Also, round-trip testing serde encoding of a byte vector is probably not
a great use of our fuzz resources. I don't believe we have ever had a
problem with this.
Fixes#2801
Recently we modified the fuzz job manually and forgot about the
`generate-files.sh` file.
Update the script to match the current CI job, running it now produces
the same file `cron-daily-fuzz.sh`.
3fa3d37c21 Add set -euo pipefail (Tobin C. Harding)
Pull request description:
Add `euo pipefail` to all non-trial shell scripts, note if `x` is already set we maintain it.
ACKs for top commit:
sanket1729:
utACK 3fa3d37c21
apoelstra:
ACK 3fa3d37c21
Tree-SHA512: 24c0da96bea44ea4f550847c8f73bbe1d9ccd1bfee1714ef8a0c23075e3a6d438b6006e351cce0df7a1cb0b9d1b50096270f65a62d0def05c84c9573ffefacba
Add `euo pipefail` to all non-trial shell scripts, note if `x` is
already set we maintain it.
Note we have a pipe in `run_task.sh` that relies on grep not finding
anything i.e., failing, so we cannot use pipefail there. Disable it and
re-enable it after the pipe.
Waiting for the fuzzer slows down the dev feedback loop because it makes
CI slow. We still want fuzz coverage but not in a way that slows down
devs.
Instead of running the fuzzer on every PR just run it once a nightly.
As we do for other daily jobs, rename the yaml file to make explicit
what it does.
Open questions:
- This should run for 30 minutes but I can't work out why the current
set up only runs for a shorter time than that?
- Is this less fuzzing i.e., is 30 minutes once a day better or worse that 1
minute 30 times?
These are not run in CI since #2353 and are likely to go out of date. If
we want a script that users can run locally then we should create a new
script that wraps our current CI.
Use `bash` instead of `sh` to run shell scripts.
We would like to support Nix users who do not typically have any shell
other than `sh` at a known path, therefore use `/usr/bin/env bash`.
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
Recently we introduced a bug in the weight/size code, while
investigating I found that our `Transaction`/`Block` weight/size APIs
were in a total mess because:
- The docs were stale
- The concept of weight (weight units) and size (bytes) were mixed up
I audited all the API functions, read some bips (141, 144) and re-wrote
the API with the following goals:
- Use terminology from the bips
- Use abstractions that mirror the bips where possible
The `network` module deals with data types and logic related to
internetworking bitcoind nodes, this is commonly referred to as the p2p
layer.
Rename the `network` module to `p2p` and fix all the paths.
Last release we added a type alias for `Psbt`, now lets just rename the
type and be done with it.
Includes re-export at the crate root because `bitcoin::Psbt` is clear
and obvious.
We should probably restore this in the future, but we need to rethink
how we fuzz hashes -- right now when cfg(fuzzing) is set, we break all
the hash functions in a way that won't match any other library.
We should probably make this breakage opt-in but this will require
buy-in from rust-lightning and maybe others.
AFAICT we literally never used this; it was available only on the
bitcoin targets and not the honggfuzz ones; AFL has a broken dep
tree (or at least, requires some more MSRV pins that I did not care
to investigate).
Just remove it entirely.
We would like to bring the `bitcoin_hashes` crate into the
`rust-bitcoin` repository.
Import `bitcoin_hashes` into `rust-bitocin/hashes`, doing so looses all
the commit history from the original crate but if we archive the
original repository then the history will be preserved. We maintain the
same version number obviously and in the changelog we note the change of
repository.
Commit hash that was tip of `bitcoin_hashes` at time of import:
commit 54c16249e06cc6b7870c7fc07d90f489d82647c7
Includes making `embedded` and `fuzzing` per-crate i.e., move them into
`bitcoin` as hashes includes these also.
NOTE: Does _not_ enable fuzzing for `hashes` in CI.
Notes on CI:
Attempts to merge in the github actions from the hashes crate however reduces
coverage by not running hashes tests for beta toolchain. Some additional
work could be done to improve the CI to increase efficiency without
reducing coverage. Leaving for another day.
Done as part of flattening the `util` module. Simply move the `amount`
module out of the `util` module and to the crate root. Justified by the
fact that the `Amount` type is more-or-less a "primitive" bitcoin type.
We are attempting to flatten the `util` module; move the `bip152` module
to the crate root out of `util`.
Currently `src/util/` is ignored by the formatter so this move causes
the `bip152` module to be formatted.
02a2b43b2b Remove Default impl for Target and Work (Tobin C. Harding)
cb9893c4a9 Add Target and Difficulty types (Tobin C. Harding)
Pull request description:
Ugh! 1600 lines of green and 1100 of red - my apologies.
Currently we use the `Uint256` type for proof-of-work calculations. It was observed in #1181 that providing a public 256 bit integer type like this implies that it is a general purpose integer type. We do not want to provide a general purpose integer type (see the 1000 arithmetic functions on stdlib integer types for why not :)
Add two new opaque integer types `Target` and `Work`. These are the inverse of each other, both conceptually and mathematically.
There is a lot of code in this PR, sorry about that. At a high level the PR does:
- Add a `pow` module.
- Put a modified version of `Uint256` in `pow`, making it private.
- Add two new wrapper types `Target` and `Work` that provide a very limited API specific to their use case. In particular there are methods on each to convert to the other.
- Only implement methods that we use on each type.
### Note
During development I got mixed up with the word "difficulty", I have discovered this has a very specific meaning in Bitcoin. Please see rustdocs on the `Target::difficulty` function for explanation of this term. For this reason we use the type `Work` defined as the inverse of target, and reserve "difficulty" for the Bitcoin concept.
ACKs for top commit:
apoelstra:
ACK 02a2b43b2b
Kixunil:
ACK 02a2b43b2b
Tree-SHA512: 4d701756a42b832f03b8d542f3a5278b4ca1d5983ffd7d4630577ebd4cc8f47029719f9018185e01fa459d8fb32426b5cb4d6b8d8b588ebbb7b65e4aeee94412
Andrew Poelstra
Bruno Garcia
Divyansh Gupta
Jose Storopoli
Tobin C. Harding
shuoer86
Einherjar
Riccardo Casatta