test: add unit test for non-standard "scriptsig-not-pushonly" txs

theStack commented at 9:00 am on December 11, 2019: member

Approaches another missing unit test of issue #17394: Checks that the function IsStandardTx() returns rejection reason “scriptsig-not-pushonly” if any one of the input’s scriptSig consists of any other ops than just PUSHs.

theStack force-pushed on Dec 11, 2019

DrahtBot added the label Tests on Dec 11, 2019

instagibbs commented at 3:06 pm on December 12, 2019: member

rather than ad-hoc replacing front and back elements, you could loop through each position and replace it with each non-push op? concept ACK anyways.

practicalswift commented at 3:23 pm on December 12, 2019: contributor

Concept ACK

Thanks a lot for adding tests!

theStack force-pushed on Dec 16, 2019

in src/test/transaction_tests.cpp:828 in 1b38fa1105 outdated

820@@ -821,6 +821,41 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
821     BOOST_CHECK(!IsStandardTx(CTransaction(t), reason));
822     BOOST_CHECK_EQUAL(reason, "scriptsig-size");
823 
824+    // Check scriptSig format (non-standard if there are any other ops than just PUSHs)
825+    t.vin[0].scriptSig = CScript()
826+        << OP_TRUE << OP_0 << OP_1NEGATE << OP_16 // OP_n (single byte pushes: n = 1, 0, -1, 16)
827+        << std::vector<unsigned char>(75, 0)      // OP_PUSHx [...x bytes...]
828+        << OP_7 << OP_1

instagibbs commented at 2:58 pm on December 16, 2019:

you interleaved these new OP_Ns without comments. Could you group them all at the top?

theStack commented at 3:03 pm on December 16, 2019:

Since in the current approach push-ops needing more than total one byte (see comment below) are not getting substituted, I wanted that the non-push-operations get substituted also in the middle of the script (in-between OP_PUSHx pushes).

instagibbs approved

instagibbs commented at 2:58 pm on December 16, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/17720/commits/1b38fa11051d9a796a5092b3b5caca107a10e35a

theStack commented at 3:00 pm on December 16, 2019: member

@instagibbs @practicalswift: Thanks for reviewing!

rather than ad-hoc replacing front and back elements, you could loop through each position and replace it with each non-push op? concept ACK anyways.

The reason that I took front and back of the scriptSig for substitution was that I intentionally set those to single-byte push operations, so it would be trivial to replace them. Unfortunately, most push operations (like OP_PUSHDATAx as well as opcodes 1-75) also have a concrete payload and length, so just replacing the opcode without deleting/modifying the additional length/payload would result in an invalid script. The idea is now to get through the script with GetOp() and apply your idea for just single-byte pushes (see https://github.com/bitcoin/bitcoin/commit/1b38fa11051d9a796a5092b3b5caca107a10e35a). Still feels a bit overloaded and complicated, maybe someone has a better idea to test this in an elegant way.

theStack force-pushed on Dec 17, 2019

DrahtBot commented at 6:50 pm on January 17, 2020: member

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

Conflicts

No conflicts as of last run.

laanwj commented at 5:43 pm on February 10, 2020: member

Can you re-ack please @instagibbs

DrahtBot added the label Needs rebase on Feb 10, 2020

DrahtBot commented at 6:22 pm on February 10, 2020: member

theStack force-pushed on Feb 28, 2020

theStack commented at 8:54 am on February 28, 2020: member

Rebased.

fanquake removed the label Needs rebase on Feb 28, 2020

in src/test/transaction_tests.cpp:852 in 60cd4c83ea outdated

847+        for (auto op : non_push_ops) {
848+            t.vin[0].scriptSig[index] = op;
849+            BOOST_CHECK(!IsStandardTx(CTransaction(t), reason));
850+            BOOST_CHECK_EQUAL(reason, "scriptsig-not-pushonly");
851+        }
852+        t.vin[0].scriptSig[index] = orig_op; // restore op

MarcoFalke commented at 3:31 pm on February 28, 2020:

Could assert this is standard again?

0        t.vin[0].scriptSig[index] = orig_op; // restore op
1        BOOST_CHECK(IsStandardTx(CTransaction(t), reason));

theStack commented at 8:21 pm on February 28, 2020:

Good idea, done.

MarcoFalke approved

test: add unit test for non-standard "scriptsig-not-pushonly" txs

The function IsStandardTx() returns rejection reason "scriptsig-not-pushonly"
if the transaction has at least one input for which the scriptSig consists of
any other ops than just PUSHs.

5aab011805

theStack force-pushed on Feb 28, 2020

MarcoFalke commented at 8:28 pm on February 28, 2020: member

ACK 5aab011805ceb12801644170700b1a62e0bf4a5d 🍟

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 5aab011805ceb12801644170700b1a62e0bf4a5d 🍟
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgoYgv+MYxwzja9dZJt3ypVTWiDaK8SefeVF11rY5W7sIXTvl6B1uhDMV80r2ZR
 8NFd5XVpbB6apMDKWE7hQuaFOKcG9EL+YOgrhW4auE1/RE8WG2qUtLNqWMF1xK6k2
 9uzjtNc8a6KFJqdVa9xpfBbF0EkTmgwPbWhMv/BIh47d+ails7tZb3fSQxsj1+T6s
10GfkZ5WvT3djISMeM58+p0QyXIiQNNteC4sBsRkmmWW6H/JeKDAfK1DWcu4KWnFPb
11XGCgL0hUr73Z6J8nUxgKeVtnr94U0kXkmDJaQ2j0wZ0mv0WO9ms9WXEBfIrR+M2y
12sm7NPDXRhqMUi4uPE+kMwZyH9TnSZVPYENABiu8qCji+y+7R/k8o60WJKOYxJvO9
13k4zfSBO66UZa6jhIRZrYmzqOhrAxpZv/1Oe0HOHVryoJErZynOuacVOYcHUVhKln
14a599PcUVl7ZXCqv8JIUSIlaCAjYtsPy0cEN8tz5qKtonsB7OkGAsqbdcTFGBRxMN
15WSN3etVi
16=1Fj3
17-----END PGP SIGNATURE-----

Timestamp of file with hash a55e1e4aea7c2acb33ef56ed7197dabadf848ebb48dbcc6c89f25c702c9c9d89 -

practicalswift commented at 7:51 pm on February 29, 2020: contributor

ACK 5aab011805ceb12801644170700b1a62e0bf4a5d – patch looks correct

theStack commented at 6:03 am on March 21, 2020: member

Since there has been no activity for three weeks: is there anything else I can do for this PR?

fanquake requested review from instagibbs on Mar 21, 2020

MarcoFalke merged this on Mar 24, 2020

MarcoFalke closed this on Mar 24, 2020

sidhujag referenced this in commit d6c5b753a2 on Mar 28, 2020

theStack deleted the branch on Dec 1, 2020

Fabcien referenced this in commit 58733a6c8b on Jan 12, 2021

DrahtBot locked this on Feb 15, 2022

test: add unit test for non-standard “scriptsig-not-pushonly” txs #17720

Conflicts