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

simplicity: add fuzz target #1332

Closed
wants to merge 165 commits into from
Closed

Conversation

delta1
Copy link
Member

@delta1 delta1 commented May 6, 2024

adds a new fuzz target for elements_simplicity_execSimplicity

compile with instructions in fuzzing.md

run with: FUZZ=simplicity src/test/fuzz/fuzz

laanwj and others added 30 commits August 1, 2022 15:35
Tree-SHA512: 1bc98f4a6d15d8f810fa6d2ee26c495a8da08dff3efd3d2ec6bbc6bd8091751a21436efc39f04fb68c5279312644ff9a63bcbc3c4df02e0bf89cd1cd8473bac5
Github-Pull: #22511
Rebased-From: 3c4d2c4
Github-Pull: #22511
Rebased-From: a884a1e
Github-Pull: #22533
Rebased-From: 9f01fed
Github-Pull: #22538
Rebased-From: d7b7f61
Github-Pull: #22538
Rebased-From: 198ceb8
When verifying guix attestations, it is useful to set a particular
signer's manifest as the base to compare against.

Github-Pull: #22531
Rebased-From: 4a46638
One of the issues observed during the 22.0rc1 release process was that a
codesigner's attestation mismatched non-codesigner attestations because
the guix-codesign step was performed prior to tagging the version in
bitcoin-detached-sigs.

Github-Pull: #22531
Rebased-From: d080c27
guix-attest mistakenly added an extra \r to the line endings in
all.SHA256SUMS, causing guix-verify to erroneously fail.

Co-Authored-By: Carl Dong <[email protected]>

Github-Pull: #22531
Rebased-From: 43225f0
If the user has set log.showSignature=true in their git config, then the
git log will always output GPG signature information. Since git log is
used to set EPOCH_SOURCE_DATE, this will mistakenly have GPG signature
information in it which causes issues for the build. To avoid this
issue, we override the config and force log.showSignature=false.

Github-Pull: #22531
Rebased-From: 9b313df
Github-Pull: #22589
Rebased-From: 2962640
Tree-SHA512: 2e9db22da514b0ef4456f46524762420730cdd3c160a4012ca9d736d18d01abd2403b0ce800743b1f92212937436fe849b1c1b07bed0e3ad9f61165be55b7958
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
When no external signers are available, the option to enable external
signers should always be disabled. However the encrypt wallet checkbox
can erroneously re-enable the external signer checkbox. To avoid this,
CreateWalletDialog now stores whether signers were available during
setSigners so that future calls to external_signer_checkbox->setEnabled
can account for whether signers are available.

Github-Pull: bitcoin-core/gui#396
Rebased-From: a9b9ca8
Previously, if verification fails, the correct message will be printed,
but the exit code would still be 0.

Github-Pull: bitcoin/bitcoin#22643
Rebased-From: d451b60
This allows us to remove the rfc4880 EOL hacks and release with a
SHA256SUMS.asc file that's a combination of all signer signatures.

Github-Pull: bitcoin/bitcoin#22642
Rebased-From: 90b3e48
For target value calculations, GetSelectionAmount should be used, not
m_effective_value or m_value.

Specifically, ApproximateBestSubset mistakenly uses m_value when
calculating whether the target value has been met. This has been changed
to use GetSelectionAmount.

Github-Pull: bitcoin/bitcoin#22686
Rebased-From: 2de222c
When the fee is not subtracted from the outputs, the amount that has
been reserved for the fee (change_and_fee - change_amount) must be
enough to cover the fee that is needed. It would be a bug to not do so,
so use an assert to make this obvious if such a situation were to occur.

Github-Pull: bitcoin/bitcoin#22686
Rebased-From: d926232
ApproximateBestSubset had an edge case (due to not using
GetSelectionAmount) where it was possible for it to return success but
fail to select enough to cover transaction fees. A test is added that
could trigger this failure prior to the fix being implemented.

Github-Pull: bitcoin/bitcoin#22686
Rebased-From: 92885c4
Previously, building from a release source tarball would result in a
version string like v22.0.0-<commithash>, but we expect just v22.0.0.
This commit solves this problem.

Also use PACKAGE_VERSION instead of reconstructing it.

Github-Pull: bitcoin/bitcoin#22685
Rebased-From: 5100dee
The SHA256SUMS file can be used in a sha256sum -c command to verify
downloaded binaries. However users are likely to download just a single
file and not place this file in the correct directory relative to the
SHA256SUMS file for the simple verification command to work. By not
including the directory name in the SHA256SUMS file, it will be easier
for users to verify downloaded binaries.

Co-authored-by: Carl Dong <[email protected]>

Github-Pull: bitcoin/bitcoin#22654
Rebased-From: fb17c99
The uploaded binaries need to match the same flat directory structure of
the SHA256SUMS file in order for torrent downloaders to be able to
verify the download without moving files. Mention this in the release
process doc.

Github-Pull: bitcoin/bitcoin#22654
Rebased-From: 132cae4
This step was missed. See translation_process.md

Github-Pull: bitcoin-core/gui#406
Rebased-From: 2b3d8f3
BOOST_FILESYSTEM_C_STR changed to accept the path as an argument

Github-Pull: bitcoin/bitcoin#22713
Rebased-From: acb7aad
Github-Pull: bitcoin/bitcoin#22648
Rebased-From: b87a9c4

Tree-SHA512: 3567e6dbfb7c3f410ea84d3f005c86be0283f92e1be1bc2a7ce93240a0b8460b985b3cec5873a19eade52dc1d78ca383c99e023d0bb4247048ae7e43b47522a5
src/test/fuzz/simplicity.cpp Outdated Show resolved Hide resolved
@delta1
Copy link
Member Author

delta1 commented May 8, 2024

Updated to include the expected invariants for control block and script_bytes.

Haven't found any crashes now, and submitted some seed data in ElementsProject/qa-assets#2

Run with: FUZZ=simplicity src/test/fuzz/fuzz /path/to/qa-assets/fuzz_seed_corpus/simplicity

apoelstra added a commit to ElementsProject/qa-assets that referenced this pull request May 8, 2024
fc127cc fuzz: add simplicity seed data (Byron Hambly)

Pull request description:

  seed data for simplicity fuzzing in ElementsProject/elements#1332

ACKs for top commit:
  apoelstra:
    ACK fc127cc -- don't really have a way to review this, but checked commit 38f0a49

Tree-SHA512: e514243e9441c0a196a281f2afab34b67aff79c088d657b847b4c7cdb0e4678639bd8231c18db2adf316ec6bd715dc8661021d5315b7587f37c4b8708f296a54
const std::vector<unsigned char> program = ConsumeRandomLengthByteVector(fuzzed_data_provider);

simplicity_err error;
elements_simplicity_execSimplicity(&error, imr.data(), tx, ix, taproot, genesisBlockHash.data(), budget, amr.data(), program.data(), program.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

We will probably need to do something about this IMR field. I don't think the fuzzer is going to figure out the hash of the program randomly by itself. I'll look into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, actually it is the scriptCMR that is the problem. This imr is actually an output field (and thus you don't need to fill it with random data).

Copy link
Contributor

Choose a reason for hiding this comment

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

One idea is to exclude this check when compiling with FUZZER defined mode.

https://github.com/BlockstreamResearch/simplicity/blob/195c5152683330cc34fa4b6d39008a1f6cf09ecb/C/primitive/elements/exec.c#L71-L75

Though the idea of disabling important checks doesn't thrill me. There might be some safer variations of this idea.

@roconnor-blockstream
Copy link
Contributor

One thing we should verify is that the PRODUCTION flag is not enable during fuzzing to ensure all assertions are being run:

https://github.com/BlockstreamResearch/simplicity/blob/master/C/simplicity_assert.h#L12

If there is some sort of FUZZ define, we can add a check/warning to the simplicity code that FUZZ and PRODUCTION are not both set.

@roconnor-blockstream
Copy link
Contributor

roconnor-blockstream commented May 25, 2024

@delta1 I don't know why, but running the command

$ PRINT_ALL_FUZZ_TARGETS_AND_ABORT= src/test/fuzz/fuzz

doesn't list "simplicity" for some reason.

@delta1
Copy link
Member Author

delta1 commented May 27, 2024

@roconnor-blockstream I saw the same issue, made a change in fuzz.cpp and recompiled, and now it seems to appear? Maybe ccache related, are you using that too?

Here's my output with some lines removed for clarity

> PRINT_ALL_FUZZ_TARGETS_AND_ABORT= src/test/fuzz/fuzz
addition_overflow
...
signet
simplicity
snapshotmetadata_deserialize
...
witness_program
fuzz: test/fuzz/fuzz.cpp:66: auto initialize()::(anonymous class)::operator()() const: Assertion `"!should_abort" && check' failed.
zsh: IOT instruction (core dumped)  PRINT_ALL_FUZZ_TARGETS_AND_ABORT= src/test/fuzz/fuzz

The change I made before recompiling was:

diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index a33297e0ed..800bcd0354 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -27,6 +27,7 @@ std::map<std::string_view, std::tuple<TypeTestOneInput, TypeInitialize, TypeHidd
 
 void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, TypeInitialize init, TypeHidden hidden)
 {
+    std::cout << "register: " << name << std::endl;
     const auto it_ins = FuzzTargets().try_emplace(name, std::move(target), std::move(init), hidden);
     Assert(it_ins.second);
 }

Which I then confirmed printed register: simplicity and then saw simplicity in the target list...

@roconnor-blockstream
Copy link
Contributor

Oops I was building #1326 instead of this PR. My bad.

if (mtx_precomputed) {
const CTransaction tx_precomputed{*mtx_precomputed};
const PrecomputedTransactionData precomputed_transaction_data{tx_precomputed};
const transaction* tx = precomputed_transaction_data.m_simplicity_tx_data;
Copy link
Contributor

@roconnor-blockstream roconnor-blockstream May 28, 2024

Choose a reason for hiding this comment

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

I've finally got the coverage analysis of fuzzing working, and I will share how to do that with you and everyone else shortly.

However, in the meantime, the results I have suggest to me that tx is always NULL, and thus the fuzzer doesn't even make it past the first line of elements_simplicity_execSimplicity.

I'm not 100% sure of this, so you should double check this.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh interesting. maybe if we provide a well formed transaction to the fuzzer via the corpus it will have a better starting point

Copy link
Contributor

@roconnor-blockstream roconnor-blockstream May 28, 2024

Choose a reason for hiding this comment

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

You can probably go ahead and check the return value of elements_simplicity_execSimplicity and assert that it is true. elements_simplicity_execSimplicity only returns false during catastrophic failure, such as malloc failing, or, as in this case, if you have called it with (detectable) incorrect parameters.

The implementation actually does this assertion:

if (!elements_simplicity_execSimplicity(&error, 0, txdata->m_simplicity_tx_data, nIn, simplicityTapEnv, txdata->m_hash_genesis_block.data(), budget, 0, witness.data(), witness.size())) {
assert(!"elements_simplicity_execSimplicity internal error");
}

The result of evaluation, (success or what sort of failure) is really returned in the &error argument.

@roconnor-blockstream
Copy link
Contributor

Please rebase this PR. Thanks.

@delta1
Copy link
Member Author

delta1 commented Jun 7, 2024

@roconnor-blockstream rebased

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.