tests: Unit tests for IsPayToWitnessScriptHash and IsWitnessProgram #14752

domob1812 commented at 2:59 PM on November 18, 2018: contributor

This adds basic unit tests for CScript::IsPayToWitnessScriptHash and CScript::IsWitnessProgram, similar to the existing tests for CScript::IsPayToScriptHash. These tests are probably not super important given the other existing tests for segwit related code, but may be useful in catching some errors early.

This implements #14737.

fanquake added the label Tests on Nov 18, 2018

in src/test/script_segwit_tests.cpp:29 in 6473702ebc outdated

  24 | +BOOST_AUTO_TEST_CASE(IsPayToWitnessScriptHash_Invalid_NotOp0)
  25 | +{
  26 | +    uint256 dummy;
  27 | +    CScript notp2wsh;
  28 | +    notp2wsh << OP_1 << ToByteVector(dummy);
  29 | +    BOOST_CHECK(!notp2wsh.IsPayToWitnessScriptHash ());

practicalswift commented at 7:48 AM on November 19, 2018:

Nit: Remove space before () :-)

Applies throughout this PR.

domob1812 commented at 10:51 AM on November 19, 2018:

Good catch (that's left over from me coding GNU style most of the time outside of Bitcoin), fixed.

domob1812 force-pushed on Nov 19, 2018

in src/test/script_segwit_tests.cpp:14 in 54c648d327 outdated

   9 | +
  10 | +BOOST_FIXTURE_TEST_SUITE(script_segwit_tests, BasicTestingSetup)
  11 | +
  12 | +BOOST_AUTO_TEST_CASE(IsPayToWitnessScriptHash_Valid)
  13 | +{
  14 | +    uint256 dummy;

dongcarl commented at 8:31 AM on November 28, 2018:

I might be missing something but the two BOOST_CHECKs here seem to be testing IsPayToWitnessScriptHash() on identical CScripts. If the intention is to test that the two ways of constructing a CScript yield the same CScript, perhaps that should be in another test case in another file.

domob1812 commented at 1:14 PM on November 28, 2018:

The main reason why I added the second version here is because that code is used later for constructing the "invalid" ones as well (basically so that we would likely notice if the construction itself is wrong and the other tests invalid). But if you thank that it is clear enough that those scripts are constructed in the right way, then I'm happy to drop either of the two tests in this test case.

in src/test/script_segwit_tests.cpp:107 in 54c648d327 outdated

 100 | +    wit.clear();
 101 | +    program.resize(40);
 102 | +    wit << OP_16 << program;
 103 | +    BOOST_CHECK(IsExpectedWitnessProgram(wit, 16, program));
 104 | +
 105 | +    program.resize(32);

dongcarl commented at 8:37 AM on November 28, 2018:

It's a little unclear why the construction of the CScript here is different from the other two CHECKs in the same test, does this change in construction test the correctness of IsExpectedWitnessProgram in any way? As in, what advantages does this have over:

wit.clear();
program.resize(30);
wit << OP_5 << program;
BOOST_CHECK(IsExpectedWitnessProgram(wit, 5, program));

domob1812 commented at 1:16 PM on November 28, 2018:

I added this for the same reason as with the other comment above - to make sure that code constructing a script "this way" yields a valid witness program as well, while very similar code in later tests with slight modifications yields an invalid program. Again I'm happy to remove that one if everyone else thinks this is clear anyway.

in src/test/script_segwit_tests.cpp:96 in 54c648d327 outdated

  90 | +
  91 | +} // anonymous namespace
  92 | +
  93 | +BOOST_AUTO_TEST_CASE(IsWitnessProgram_Valid)
  94 | +{
  95 | +    std::vector<unsigned char> program = {42, 18};

dongcarl commented at 9:21 AM on November 28, 2018:

    // Witness programs have a minimum data push of 2 bytes
    std::vector<unsigned char> program = {42, 18};

domob1812 commented at 1:19 PM on November 28, 2018:

Added to the commit, thanks for the suggestion!

in src/test/script_segwit_tests.cpp:103 in 54c648d327 outdated

  96 | +    CScript wit;
  97 | +    wit << OP_0 << program;
  98 | +    BOOST_CHECK(IsExpectedWitnessProgram(wit, 0, program));
  99 | +
 100 | +    wit.clear();
 101 | +    program.resize(40);

dongcarl commented at 9:22 AM on November 28, 2018:

    // Witness programs have a maximum data push of 40 bytes
    program.resize(40);

domob1812 commented at 1:19 PM on November 28, 2018:

Added.

dongcarl changes_requested

domob1812 force-pushed on Nov 28, 2018

in src/test/script_p2sh_tests.cpp:231 in 5072c40b29 outdated

 234 | +    pushdata2.push_back(OP_EQUAL);
 235 | +    BOOST_CHECK(!CScript(pushdata2.begin(), pushdata2.end()).IsPayToScriptHash());
 236 | +    std::vector<unsigned char> pushdata4 = {OP_HASH160, 20, 0, 0, 0};
 237 | +    pushdata4.insert(pushdata4.end(), 20, 0);
 238 | +    pushdata4.push_back(OP_EQUAL);
 239 | +    BOOST_CHECK(!CScript(pushdata4.begin(), pushdata4.end()).IsPayToScriptHash());

MarcoFalke commented at 7:57 PM on December 12, 2018:

Meh, I'd prefer if this was dropped from this pull request. Could create a separate pull request later if you feel strongly.

domob1812 commented at 7:43 AM on December 13, 2018:

I do not feel particularly strongly, but I think it is clearly an improvement in readability. Since it is mostly unrelated to this change (except that it is the same kind of test), I put it into a separate commit. But I'm happy to remove it from here and perhaps submit separately if the community thinks that's what we should do.

domob1812 commented at 9:06 AM on January 4, 2019:

Split out into #15099.

MarcoFalke commented at 7:58 PM on December 12, 2018: member

Concept ACK on adding tests for those uncovered branches.

MarcoFalke commented at 8:03 PM on December 12, 2018: member

Could also add coverage to IsPayToScriptHash?

domob1812 commented at 7:44 AM on December 13, 2018: contributor

Could also add coverage to IsPayToScriptHash?

That test already exists, in script_p2sh_tests.cpp (in fact, that's where the "unrelated" change from above is).

domob1812 referenced this in commit bcfa192771 on Dec 17, 2018

domob1812 force-pushed on Jan 4, 2019

MarcoFalke referenced this in commit fe5a70b9fe on Jan 4, 2019

domob1812 force-pushed on Apr 23, 2019

domob1812 commented at 3:31 PM on April 23, 2019: contributor

Rebased and updated the code for the recent refactoring of the testing setup.

in src/test/script_segwit_tests.cpp:6 in a1cb474203 outdated

   0 | @@ -0,0 +1,164 @@
   1 | +// Copyright (c) 2012-2019 The Bitcoin Core developers
   2 | +// Distributed under the MIT software license, see the accompanying
   3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +#include <script/script.h>
   6 | +#include <test/setup_common.h>

leonardojobim commented at 11:23 AM on March 5, 2021:

#include <test/util/setup_common.h>

This change makes the test works with current master branch.

domob1812 commented at 12:57 PM on March 5, 2021:

Good catch (I already had mostly forgotten about this PR). I've rebased it to latest master now + added your fix, which indeed makes it work also for me.

leonardojobim approved

leonardojobim commented at 11:28 AM on March 5, 2021: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/14752/commits/a1cb474203b583df140005cc1aab80310e55ac9e on Ubuntu 20.04. Takes 0.4s to run.

domob1812 force-pushed on Mar 5, 2021

PastaPastaPasta referenced this in commit 330f293e96 on Jun 26, 2021

PastaPastaPasta referenced this in commit 5d36cdfb5a on Jun 28, 2021

MarcoFalke requested review from dongcarl on Oct 6, 2021

DrahtBot added the label Needs rebase on Oct 8, 2021

in src/Makefile.test.include:123 in b6b64d467d outdated

 116 | @@ -117,6 +117,7 @@ BITCOIN_TESTS =\
 117 |    test/scheduler_tests.cpp \
 118 |    test/script_p2sh_tests.cpp \
 119 |    test/script_tests.cpp \
 120 | +  test/script_segwit_tests.cpp \

MarcoFalke commented at 12:35 PM on October 8, 2021:

Could sort the lines here?

DrahtBot commented at 3:43 PM on October 8, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Unit tests for IsWitnessProgram and IsP2WSH.

The new unit test file script_segwit_tests.cpp contains some basic
unit tests for CScript::IsPayToWitnessScriptHash and
CScript::IsWitnessProgram.

bce9aaf31e

domob1812 force-pushed on Oct 15, 2021

domob1812 commented at 4:47 AM on October 15, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Oct 15, 2021

MarcoFalke commented at 12:49 PM on October 15, 2021: member

cc @dongcarl Do you still have concerns about this?

aureleoules commented at 1:29 AM on March 11, 2022: member

tACK bce9aaf31e2b0428e686e151324f8561ad71f11f (make check). Tested on NixOS 22.05 64 bits.

I have look thoroughly at every unit test of bce9aaf31e2b0428e686e151324f8561ad71f11f.
Took the time to read and understand each function being tested:
https://github.com/bitcoin/bitcoin/blob/bce9aaf31e2b0428e686e151324f8561ad71f11f/src/script/script.cpp#L210-L216
and
https://github.com/bitcoin/bitcoin/blob/bce9aaf31e2b0428e686e151324f8561ad71f11f/src/script/script.cpp#L218-L234

I've tweaked values locally of both functions. For CScript::IsPayToWitnessScriptHash:

pay-to-witness-hash expected size modified
First expected OP code modified
Second byte expected modified

For CScript::IsWitnessProgram:

Min and max witness program size modified
First expected OP code modified

Tests fail accordingly. I have not found any untested edge-case, excellent coverage!

MarcoFalke merged this on Mar 16, 2022

MarcoFalke closed this on Mar 16, 2022

in src/test/script_segwit_tests.cpp:10 in bce9aaf31e

   5 | +#include <script/script.h>
   6 | +#include <test/util/setup_common.h>
   7 | +
   8 | +#include <boost/test/unit_test.hpp>
   9 | +
  10 | +BOOST_FIXTURE_TEST_SUITE(script_segwit_tests, BasicTestingSetup)

MarcoFalke commented at 5:12 PM on March 16, 2022:

nit: I don't think you need any setup. BOOST_AUTO_TEST_SUITE(script_segwit_tests) should work fine for pure utility tests.

sidhujag referenced this in commit 934db4033b on Mar 16, 2022

domob1812 deleted the branch on Mar 17, 2022

DrahtBot locked this on Mar 17, 2023