Bitcoin: Add tests checking missing reject reasons for function IsStandardTx

Created on 6 Nov 2019  路  3Comments  路  Source: bitcoin/bitcoin

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 (7967104aee055476107dc17265cefc4ae4e75378), the test coverage looks like the following:

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

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

  • [x] add unit test for "scriptsig-size" reason (PR #17480, commit 5e8a56348b5e1026e9ddcae0b2fa2a68faf4439e, done by my humble self)
  • [x] add unit test for "bare-multisig" reason (PR #17502, commit 1bb5d517aa616c1d5b5801d2ea36a2de5fb61eba, done by my humble self)
  • [x] add functional test for "scriptsig-size" reason (PR #17532, commit 8f2d7737cc236b6122f30e31856eb3181960fba1, done by my humble self)
  • [x] add functional test for "bare-multisig" reason (PR #17541, commit 1be0b1fb2adcf95d76f879195564c0bf84162e31, done by my humble self)
  • [x] add unit test for "scriptsig-not-pushonly" reason (PR #17720, commit 5aab011805ceb12801644170700b1a62e0bf4a5d, done by my humble self)
  • [x] add unit test for "tx-size" reason (PR #17947, commit 4537ba5f21ad8afb705325cd8e15dd43877eb28f, dony by humble self)
  • [x] add unit test for "version" reason (PR #17555, commit https://github.com/bitcoin/bitcoin/pull/17555/commits/76303f65f92a0fbe9a90c0e807554a6daa860636, 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.

Tests good first issue

Most helpful comment

@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! :)

All 3 comments

I would like to work on this.

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

@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! :)

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

Was this page helpful?
0 / 5 - 0 ratings