tests: Unit tests for IsPayToWitnessScriptHash and IsWitnessProgram #14752

domob1812 · 2018-11-18T14:59:13Z

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.

src/test/script_segwit_tests.cpp

dongcarl · 2018-11-28T08:31:10Z

src/test/script_segwit_tests.cpp

+
+BOOST_AUTO_TEST_CASE(IsPayToWitnessScriptHash_Valid)
+{
+    uint256 dummy;


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.

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.

dongcarl · 2018-11-28T08:37:47Z

src/test/script_segwit_tests.cpp

+    wit << OP_16 << program;
+    BOOST_CHECK(IsExpectedWitnessProgram(wit, 16, program));
+
+    program.resize(32);


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));

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.

src/test/script_segwit_tests.cpp

maflcko

Concept ACK on adding tests for those uncovered branches.

src/test/script_p2sh_tests.cpp

maflcko · 2018-12-12T20:03:00Z

Could also add coverage to IsPayToScriptHash?

domob1812 · 2018-12-13T07:44:39Z

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).

3c8e3a6 Strip name prefix for IsWitnessProgram. (Daniel Kraft) 729d587 Unit tests for IsWitnessProgram and IsP2WSH. (Daniel Kraft) Pull request description: This fixes an issue with names and native segwit scripts (bech32 addresses). `CScript::IsWitnessProgram` did not strip name prefixes, which meant that names at bech32 addresses were anyone-can-spend even after segwit activation - but the transaction had to be included in a block, since the transaction would be rejected by a non-mandatory rule from the mempool. P2SH-Segwit is already working as expected. With this fix, names should work well together with segwit in all variants. This also includes tests to verify that, including (but not limited to) the situations described in #258. Note that this is in theory a consensus change, but it is safe to commit because segwit is not yet active for Namecoin. This includes a cherry-pick from bitcoin/bitcoin#14752, to create a place for the new name/segwit unit tests. Tree-SHA512: 01ec39dfcb2b167d3393dfa403c924ba9b71052eb5c93ff817f71b11ecad1ff3f78fefe6d97fa5bf7b53b16a905008707662ca35e5f80d88e9056ffedf23af36

…est data 6b25f29 Use std::vector API for construction of test data. (Daniel Kraft) Pull request description: For constructing test scripts, use `std::vector` and, in particular, `std::vector::insert` to insert 20 zero bytes rather than listing the full array of bytes explicitly. This makes the code easier to read and makes it immediately obvious what the structure of the data is, without having to count the zeros to understand it. Of course, that is a matter of taste - so if you disagree that the change makes the code easier to read, let me know. This has been split out of bitcoin#14752. Tree-SHA512: af82d447f0077259049f1da2d6f86a6c29723c6e17bd342e9a9ecf37b13bddff40643af95c8b3a3260765a5591713d31ca8a45a5a0c20a12c139aee53ea150da

domob1812 · 2019-04-23T15:31:48Z

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

leonardojobim

Tested ACK a1cb474 on Ubuntu 20.04. Takes 0.4s to run.

src/test/script_segwit_tests.cpp

…est data 6b25f29 Use std::vector API for construction of test data. (Daniel Kraft) Pull request description: For constructing test scripts, use `std::vector` and, in particular, `std::vector::insert` to insert 20 zero bytes rather than listing the full array of bytes explicitly. This makes the code easier to read and makes it immediately obvious what the structure of the data is, without having to count the zeros to understand it. Of course, that is a matter of taste - so if you disagree that the change makes the code easier to read, let me know. This has been split out of bitcoin#14752. Tree-SHA512: af82d447f0077259049f1da2d6f86a6c29723c6e17bd342e9a9ecf37b13bddff40643af95c8b3a3260765a5591713d31ca8a45a5a0c20a12c139aee53ea150da

src/Makefile.test.include

DrahtBot · 2021-10-08T15:43:08Z

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

Conflicts

No conflicts as of last run.

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

domob1812 · 2021-10-15T04:47:14Z

Rebased

maflcko · 2021-10-15T12:49:46Z

cc @dongcarl Do you still have concerns about this?

aureleoules · 2022-03-11T01:29:41Z

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

I have look thoroughly at every unit test of bce9aaf.
Took the time to read and understand each function being tested:

bitcoin/src/script/script.cpp

Lines 210 to 216 in bce9aaf

    
           bool CScript::IsPayToWitnessScriptHash() const 
        
           { 
        
               // Extra-fast test for pay-to-witness-script-hash CScripts: 
        
               return (this->size() == 34 && 
        
                       (*this)[0] == OP_0 && 
        
                       (*this)[1] == 0x20); 
        
           }

and

bitcoin/src/script/script.cpp

Lines 218 to 234 in bce9aaf

    
           // A witness program is any valid CScript that consists of a 1-byte push opcode 
        
           // followed by a data push between 2 and 40 bytes. 
        
           bool CScript::IsWitnessProgram(int& version, std::vector<unsigned char>& program) const 
        
           { 
        
               if (this->size() < 4 || this->size() > 42) { 
        
                   return false; 
        
               } 
        
               if ((*this)[0] != OP_0 && ((*this)[0] < OP_1 || (*this)[0] > OP_16)) { 
        
                   return false; 
        
               } 
        
               if ((size_t)((*this)[1] + 2) == this->size()) { 
        
                   version = DecodeOP_N((opcodetype)(*this)[0]); 
        
                   program = std::vector<unsigned char>(this->begin() + 2, this->end()); 
        
                   return true; 
        
               } 
        
               return false; 
        
           }

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!

maflcko · 2022-03-16T17:12:52Z

src/test/script_segwit_tests.cpp

+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(script_segwit_tests, BasicTestingSetup)


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

…nd IsWitnessProgram bce9aaf Unit tests for IsWitnessProgram and IsP2WSH. (Daniel Kraft) Pull request description: 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 bitcoin#14737. ACKs for top commit: aureleoules: tACK bce9aaf (`make check)`. Tree-SHA512: 3cff5efc4ac53079289c72bfba8b1937bc103baadd32bb1fba41e78017f65f9cca17678c3202ad0711eae42b351d4132d9ed9b4e2dc07d138298691a09c4e822

This was referenced Nov 18, 2018

Unit tests for IsWitnessProgram #14737

Closed

Fix for names and native segwit + tests namecoin/namecoin-core#274

Merged

fanquake added the Tests label Nov 18, 2018

practicalswift reviewed Nov 19, 2018

View reviewed changes

src/test/script_segwit_tests.cpp Outdated Show resolved Hide resolved

domob1812 force-pushed the script-segwit-tests branch from 6473702 to 54c648d Compare November 19, 2018 10:50

dongcarl suggested changes Nov 28, 2018

View reviewed changes

domob1812 force-pushed the script-segwit-tests branch from 54c648d to 5072c40 Compare November 28, 2018 13:19

maflcko reviewed Dec 12, 2018

View reviewed changes

src/test/script_p2sh_tests.cpp Outdated Show resolved Hide resolved

domob1812 force-pushed the script-segwit-tests branch 2 times, most recently from 3c36a8e to 4cf6e7c Compare January 4, 2019 08:59

domob1812 mentioned this pull request Jan 4, 2019

tests: Use std::vector API for construction of test data #15099

Merged

domob1812 force-pushed the script-segwit-tests branch from 4cf6e7c to a1cb474 Compare April 23, 2019 15:30

leonardojobim approved these changes Mar 5, 2021

View reviewed changes

src/test/script_segwit_tests.cpp Outdated Show resolved Hide resolved

domob1812 force-pushed the script-segwit-tests branch from a1cb474 to b6b64d4 Compare March 5, 2021 12:56

DrahtBot mentioned this pull request Oct 6, 2021

test: Add ParseMoney and ParseScript tests #23185

Merged

maflcko requested a review from dongcarl October 6, 2021 12:00

DrahtBot added the Needs rebase label Oct 8, 2021

maflcko reviewed Oct 8, 2021

View reviewed changes

src/Makefile.test.include Show resolved Hide resolved

Unit tests for IsWitnessProgram and IsP2WSH.

bce9aaf

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

domob1812 force-pushed the script-segwit-tests branch from b6b64d4 to bce9aaf Compare October 15, 2021 04:46

DrahtBot removed the Needs rebase label Oct 15, 2021

maflcko merged commit 3617d22 into bitcoin:master Mar 16, 2022

maflcko reviewed Mar 16, 2022

View reviewed changes

domob1812 deleted the script-segwit-tests branch March 17, 2022 12:37

bitcoin locked and limited conversation to collaborators Mar 17, 2023


		#include <boost/test/unit_test.hpp>

		BOOST_FIXTURE_TEST_SUITE(script_segwit_tests, BasicTestingSetup)

tests: Unit tests for IsPayToWitnessScriptHash and IsWitnessProgram #14752

tests: Unit tests for IsPayToWitnessScriptHash and IsWitnessProgram #14752

Uh oh!

Conversation

domob1812 commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dongcarl Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

domob1812 Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

dongcarl Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

domob1812 Nov 28, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 12, 2018

Uh oh!

domob1812 commented Dec 13, 2018

Uh oh!

domob1812 commented Apr 23, 2019

Uh oh!

leonardojobim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

domob1812 commented Oct 15, 2021

Uh oh!

maflcko commented Oct 15, 2021

Uh oh!

aureleoules commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

domob1812 commented Nov 18, 2018 •

edited

Loading

DrahtBot commented Oct 8, 2021 •

edited

Loading

aureleoules commented Mar 11, 2022 •

edited

Loading