fuzz: Fix DecodeHexTx fuzzing harness issue #20290

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:fuzzers-decode_tx-fixup changing 1 files +1 −2

practicalswift commented at 10:24 PM on November 2, 2020: contributor

Fix DecodeHexTx fuzzing harness issue.

Before this patch:

$ src/test/fuzz/decode_tx
decode_tx: test/fuzz/decode_tx.cpp:29: 
    void test_one_input(const std::vector<uint8_t> &):
    Assertion `result_try_witness_and_maybe_no_witness' failed.
…

After this patch:

$ src/test/fuzz/decode_tx
…

fuzz: Fix DecodeHexTx fuzzing harness issue 28f8cb13d4
fanquake added the label Tests on Nov 2, 2020

in src/test/fuzz/decode_tx.cpp:29 in 28f8cb13d4

  23 | @@ -24,8 +24,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
  24 |      if (result_try_witness_and_maybe_no_witness) {
  25 |          assert(result_try_no_witness || result_try_witness);
  26 |      }
  27 | -    // if (result_try_no_witness) { // Uncomment when https://github.com/bitcoin/bitcoin/pull/17775 is merged
  28 | -    if (result_try_witness) { // Remove stop-gap when https://github.com/bitcoin/bitcoin/pull/17775 is merged
  29 | +    if (result_try_no_witness) {
  30 |          assert(result_try_witness_and_maybe_no_witness);
  31 |      }

mjdietzx commented at 1:14 AM on November 3, 2020:

nit: to me it made more sense what this was doing when stepping back and rewriting it like this:

assert(!result_none);
if (result_try_witness) {
    assert(result_try_witness_and_maybe_no_witness);
} if (result_try_no_witness) {
    assert(result_try_witness_and_maybe_no_witness);
}

idk if it would be more logical to use else if here

mjdietzx commented at 1:18 AM on November 3, 2020:

or maybe even cleaner:

assert(!result_none);
if (result_try_witness || result_try_no_witness) {
    assert(result_try_witness_and_maybe_no_witness);
}

mjdietzx commented at 1:20 AM on November 3, 2020:

maybe I'm not thinking about this right though.. I do see how this can be different. Fuzzing is new to me, interested to see what you think (or if I'm wrong here)

MarcoFalke commented at 7:36 AM on November 3, 2020: member

review ACK 28f8cb13d4d5a3d6aefa6e192e55f9aa87579e52
lehuuhieu7777 approved
MarcoFalke merged this on Nov 3, 2020
MarcoFalke closed this on Nov 3, 2020

MarcoFalke commented at 9:02 AM on November 3, 2020: member

Could make sense to check that witness isn't created with the legacy decoding?

diff --git a/src/test/fuzz/decode_tx.cpp b/src/test/fuzz/decode_tx.cpp
index a29686cbdd..6bbebc1478 100644
--- a/src/test/fuzz/decode_tx.cpp
+++ b/src/test/fuzz/decode_tx.cpp
@@ -20,6 +20,9 @@ void test_one_input(const std::vector<uint8_t>& buffer)
     const bool result_try_witness = DecodeHexTx(mtx, tx_hex, false, true);
     const bool result_try_witness_and_maybe_no_witness = DecodeHexTx(mtx, tx_hex, true, true);
     const bool result_try_no_witness = DecodeHexTx(mtx, tx_hex, true, false);
+    if (result_try_no_witness) {
+        assert(!mtx.HasWitness());
+    }
     assert(!result_none);
     if (result_try_witness_and_maybe_no_witness) {
         assert(result_try_no_witness || result_try_witness);

sidhujag referenced this in commit 0a73ac712d on Nov 3, 2020
MarcoFalke referenced this in commit f33e332541 on Nov 5, 2020
sidhujag referenced this in commit 24e3db9816 on Nov 5, 2020
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#20303 fuzz: Assert expected DecodeHexTx behaviour when using legacy decoding