IsStandardTx()
returns rejection reason “scriptsig-not-pushonly” if any one of the input’s scriptSig consists of any other ops than just PUSHs.
test: add unit test for non-standard “scriptsig-not-pushonly” txs #17720
pull theStack wants to merge 1 commits into bitcoin:master from theStack:20191211-test-check-for-non-standard-txs-with-non-push-scriptsig changing 1 files +34 −0-
theStack commented at 9:00 am on December 11, 2019: memberApproaches another missing unit test of issue #17394: Checks that the function
-
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: memberrather 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 newOP_N
s 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 approvedinstagibbs commented at 2:58 pm on December 16, 2019: membertheStack 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 withGetOp()
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, 2019DrahtBot commented at 6:50 pm on January 17, 2020: memberThe 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: memberCan you re-ack please @instagibbsDrahtBot added the label Needs rebase on Feb 10, 2020DrahtBot commented at 6:22 pm on February 10, 2020: membertheStack force-pushed on Feb 28, 2020theStack commented at 8:54 am on February 28, 2020: memberRebased.fanquake removed the label Needs rebase on Feb 28, 2020in 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 approvedtest: 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.
theStack force-pushed on Feb 28, 2020MarcoFalke commented at 8:28 pm on February 28, 2020: memberACK 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: contributorACK 5aab011805ceb12801644170700b1a62e0bf4a5d – patch looks correcttheStack commented at 6:03 am on March 21, 2020: memberSince 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, 2020MarcoFalke merged this on Mar 24, 2020MarcoFalke closed this on Mar 24, 2020
sidhujag referenced this in commit d6c5b753a2 on Mar 28, 2020theStack deleted the branch on Dec 1, 2020Fabcien referenced this in commit 58733a6c8b on Jan 12, 2021DrahtBot locked this on Feb 15, 2022
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 03:12 UTC
More mirrored repositories can be found on mirror.b10c.me