Add tests checking missing reject reasons for function IsStandardTx #17394

theStack · 2019-11-06T13:09:06Z

The function IsStandardTx can return a number of reasons for non-standard transactions. A few of those are currently tested on one hand by the unit test test_IsStandard (transaction_tests.cpp), directly checking the result of the function call, on the other hand by the functional test mempool_accept.py, indirectly testing by using the testmempoolaccept RPC and checking the reject-reason field in the result. On the current master branch (7967104), the test coverage looks like the following:

`IsStandardTx()` Reject reason	Functional test (`mempool_accept.py`)	Unit test (`transaction_tests.cpp`)
`"version"`	✔️
`"tx-size"`	✔️
`"scriptsig-size"`
`"scriptsig-not-pushonly"`	✔️
`"scriptpubkey"`	✔️	✔️
`"bare-multisig"`
`"dust"`	✔️	✔️
`"multi-op-return"`	✔️	✔️

I think it would make sense to have at least one test for each reason by either a unit test or functional test -- for "scriptsig-size" and "bare-multisig" we don't have any, so those are most needed as now. Given that, that would lead to the following list of seven small tasks (roughly ordered by descending priority):

add unit test for "scriptsig-size" reason (PR test: add unit test for non-standard txs with too large scriptSig #17480, commit 5e8a563, done by my humble self)
add unit test for "bare-multisig" reason (PR test: add unit test for non-standard bare multisig txs #17502, commit 1bb5d51, done by my humble self)
add functional test for "scriptsig-size" reason (PR test: add functional test for non-standard txs with too large scriptSig #17532, commit 8f2d773, done by my humble self)
add functional test for "bare-multisig" reason (PR test: add functional test for non-standard bare multisig txs #17541, commit 1be0b1f, done by my humble self)
add unit test for "scriptsig-not-pushonly" reason (PR test: add unit test for non-standard "scriptsig-not-pushonly" txs #17720, commit 5aab011, done by my humble self)
add unit test for "tx-size" reason (PR test: add unit test for non-standard txs with too large tx size #17947, commit 4537ba5, dony by humble self)
add unit test for "version" reason (PR test: add unit test for non-standard txs with wrong nVersion #17555, commit 76303f6, done by dspicher)

This would be a good "good first issue" candidate I guess -- I'm tempted to work on that myself, but to encourage new contributors (and knowing how helpful "good first issues" can be to get into) I'll not touch it for a while.

The text was updated successfully, but these errors were encountered:

… scriptSig 5e8a563 test: add unit test for non-standard txs with too large scriptSig (Sebastian Falbesoner) Pull request description: Approaches the first missing test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"scriptsig-size"` if any one the inputs' scriptSig is larger than 1650 bytes. ACKs for top commit: MarcoFalke: ACK 5e8a563 instagibbs: ACK 5e8a563 Tree-SHA512: 79977b12ddea9438a37cefdbb48cc551e4ad02a8ccfaa2d2837ced9f3a185e2e07cc366c243b9e3c7736245e90e315d7b4110efc6b440c63dbef7ee2c9d78a73

… large scriptSig 8f2d773 test: add functional test for non-standard txs with too large scriptSig (Sebastian Falbesoner) Pull request description: Approaches another missing functional test of issue #17394 (counterpart to unit test in PR #17480, Commit 5e8a563): A transaction is rejected by the mempool with reason `"scriptsig-size"` if any of the inputs' scriptSig is larger than 1650 bytes. ACKs for top commit: MarcoFalke: ACK 8f2d773 instagibbs: ACK 8f2d773 Tree-SHA512: 7a45b8a4181158be3e3b91756783ddf032f132ca8780dc35fac91b2df2149268f784d28ac56005135c4d86a357c57805c5a54b8155f0d049932844b18dc03992

…ith too large scriptSig 8f2d773 test: add functional test for non-standard txs with too large scriptSig (Sebastian Falbesoner) Pull request description: Approaches another missing functional test of issue bitcoin#17394 (counterpart to unit test in PR bitcoin#17480, Commit bitcoin@5e8a563): A transaction is rejected by the mempool with reason `"scriptsig-size"` if any of the inputs' scriptSig is larger than 1650 bytes. ACKs for top commit: MarcoFalke: ACK 8f2d773 instagibbs: ACK bitcoin@8f2d773 Tree-SHA512: 7a45b8a4181158be3e3b91756783ddf032f132ca8780dc35fac91b2df2149268f784d28ac56005135c4d86a357c57805c5a54b8155f0d049932844b18dc03992

dspicher · 2019-11-21T10:31:12Z

I would like to work on this.

@theStack did you already get started on the "bare-multisig" tests?

theStack · 2019-11-21T12:01:09Z

@dspicher: Glad to hear that you are interested! PRs for both the unit and functional test for "bare-multisig" have been already opened by me within the last days (see #17502 and #17541 -- both are not merged yet though), hence right now the following three tests are missing:

unit test for "scriptsig-not-pushonly" reason
unit test for "tx-size" reason
unit test for "version" reason

Feel free to work on those, and have fun! :)

…rsion 76303f6 test: add unit test for non-standard txs with wrong nVersion (Dominik Spicher) Pull request description: Takes care of one of the missing cases of #17394: nVersion must be within the allowed range. ACKs for top commit: instagibbs: ACK 76303f6 Tree-SHA512: 94464f781cf70a5616f7cea2014ae0a97a338c34411cc989c60389de2ce00368374811db78c919bda30e0ebf341fb830998a5e97c124dd8afc8feb726cedfd3a

…ong nVersion 76303f6 test: add unit test for non-standard txs with wrong nVersion (Dominik Spicher) Pull request description: Takes care of one of the missing cases of bitcoin#17394: nVersion must be within the allowed range. ACKs for top commit: instagibbs: ACK bitcoin@76303f6 Tree-SHA512: 94464f781cf70a5616f7cea2014ae0a97a338c34411cc989c60389de2ce00368374811db78c919bda30e0ebf341fb830998a5e97c124dd8afc8feb726cedfd3a

1bb5d51 test: add unit test for non-standard bare multisig txs (Sebastian Falbesoner) Pull request description: Approaches another missing unit test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"bare-multisig"` if any one of the outputs' scriptPubKey has bare multisignature format (i.e. `M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`, not P2SH!) and the policy flag `fIsBareMultisigStd` is set to false. ACKs for top commit: instagibbs: utACK 1bb5d51 Tree-SHA512: d7c95e35da16520d6dcd2b4278e2426fedd13f68d1f23c90e85e929774e123fbfcfbccc26df6ad1c0dd61780896fa4b4b3d4e8280c647bb06df2bfcf2ba572fb

…ig txs 1bb5d51 test: add unit test for non-standard bare multisig txs (Sebastian Falbesoner) Pull request description: Approaches another missing unit test of issue bitcoin#17394: Checks that the function `IsStandardTx()` returns rejection reason `"bare-multisig"` if any one of the outputs' scriptPubKey has bare multisignature format (i.e. `M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`, not P2SH!) and the policy flag `fIsBareMultisigStd` is set to false. ACKs for top commit: instagibbs: utACK bitcoin@1bb5d51 Tree-SHA512: d7c95e35da16520d6dcd2b4278e2426fedd13f68d1f23c90e85e929774e123fbfcfbccc26df6ad1c0dd61780896fa4b4b3d4e8280c647bb06df2bfcf2ba572fb

…g txs 1be0b1f test: add functional test for non-standard bare multisig txs (Sebastian Falbesoner) Pull request description: Approaches another missing functional test of issue #17394 (counterpart to unit test in PR #17502): A transaction is rejected by the mempool with reason `"bare-multisig"` if any of the outputs' scriptPubKey has bare multisig format (`M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`) and bitcoind is started with the argument `-permitbaremultisig=0`. ACKs for top commit: instagibbs: utACK 1be0b1f kristapsk: ACK 1be0b1f Tree-SHA512: 2cade68c4454029b62278b38d0f137c2605a0e4450c435cdda2833667234edd4406f017ed12fa8df9730618654acbaeb68b16dcabb9f5aa84bad9f1c76c6d476

…multisig txs 1be0b1f test: add functional test for non-standard bare multisig txs (Sebastian Falbesoner) Pull request description: Approaches another missing functional test of issue bitcoin#17394 (counterpart to unit test in PR bitcoin#17502): A transaction is rejected by the mempool with reason `"bare-multisig"` if any of the outputs' scriptPubKey has bare multisig format (`M <PubKey1> <PubKey2> ... <PubKeyN> N OP_CHECKSIG`) and bitcoind is started with the argument `-permitbaremultisig=0`. ACKs for top commit: instagibbs: utACK bitcoin@1be0b1f kristapsk: ACK 1be0b1f Tree-SHA512: 2cade68c4454029b62278b38d0f137c2605a0e4450c435cdda2833667234edd4406f017ed12fa8df9730618654acbaeb68b16dcabb9f5aa84bad9f1c76c6d476

… tx size 4537ba5 test: add unit test for non-standard txs with too large tx size (Sebastian Falbesoner) Pull request description: Approaches another missing unit test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason `"tx-size"` if the transaction weight is larger than `MAX_STANDARD_TX_WEIGHT` (=400000 vbytes). ACKs for top commit: Empact: Code Review ACK 4537ba5 instagibbs: ACK 4537ba5 Tree-SHA512: ab32e3e47e0b337253aef3da9b7c97d01f4130d00d5860588dfed02114eec3ba49473acc6419448affd63e883fd827bf308716965606eaddee242c4c5a4eb799

…o large tx size 4537ba5 test: add unit test for non-standard txs with too large tx size (Sebastian Falbesoner) Pull request description: Approaches another missing unit test of issue bitcoin#17394: Checks that the function `IsStandardTx()` returns rejection reason `"tx-size"` if the transaction weight is larger than `MAX_STANDARD_TX_WEIGHT` (=400000 vbytes). ACKs for top commit: Empact: Code Review ACK bitcoin@4537ba5 instagibbs: ACK bitcoin@4537ba5 Tree-SHA512: ab32e3e47e0b337253aef3da9b7c97d01f4130d00d5860588dfed02114eec3ba49473acc6419448affd63e883fd827bf308716965606eaddee242c4c5a4eb799

…honly" txs 5aab011 test: add unit test for non-standard "scriptsig-not-pushonly" txs (Sebastian Falbesoner) Pull request description: 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. ACKs for top commit: MarcoFalke: ACK 5aab011 🍟 practicalswift: ACK 5aab011 -- patch looks correct Tree-SHA512: fbe25bcf57e5f0c8d2397eb67e61fe8d9145ba83032789adb2b67d6fcbcd87e6427e9d965e8cd7bbaaea482e39ec2f110f71ef2de079c7d1fba2712848caa9ba

theStack · 2020-03-24T15:26:02Z

All missing unit tests and functional tests are implemented and merged in the master branch now 🎉 🍺 so I'm closing this issue.

…not-pushonly" txs 5aab011 test: add unit test for non-standard "scriptsig-not-pushonly" txs (Sebastian Falbesoner) Pull request description: Approaches another missing unit test of issue bitcoin#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. ACKs for top commit: MarcoFalke: ACK 5aab011 🍟 practicalswift: ACK 5aab011 -- patch looks correct Tree-SHA512: fbe25bcf57e5f0c8d2397eb67e61fe8d9145ba83032789adb2b67d6fcbcd87e6427e9d965e8cd7bbaaea482e39ec2f110f71ef2de079c7d1fba2712848caa9ba

Summary: > Takes care of one of the missing cases of [[bitcoin/bitcoin#17394]]: nVersion must be within the allowed range. This is a backport of Core [[bitcoin/bitcoin#17555 | PR17555]] Test Plan: `ninja && ninja check` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8286