Add tests checking missing reject reasons for function IsStandardTx #17394

issue theStack opened this issue on November 6, 2019
  1. theStack commented at 1:09 PM on November 6, 2019: contributor

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

    • add unit test for "scriptsig-size" reason (PR #17480, commit 5e8a56348b5e1026e9ddcae0b2fa2a68faf4439e, done by my humble self)
    • add unit test for "bare-multisig" reason (PR #17502, commit 1bb5d517aa616c1d5b5801d2ea36a2de5fb61eba, done by my humble self)
    • add functional test for "scriptsig-size" reason (PR #17532, commit 8f2d7737cc236b6122f30e31856eb3181960fba1, done by my humble self)
    • add functional test for "bare-multisig" reason (PR #17541, commit 1be0b1fb2adcf95d76f879195564c0bf84162e31, done by my humble self)
    • add unit test for "scriptsig-not-pushonly" reason (PR #17720, commit 5aab011805ceb12801644170700b1a62e0bf4a5d, done by my humble self)
    • add unit test for "tx-size" reason (PR #17947, commit 4537ba5f21ad8afb705325cd8e15dd43877eb28f, dony by humble self)
    • 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.

  2. fanquake added the label Tests on Nov 6, 2019
  3. MarcoFalke added the label good first issue on Nov 6, 2019
  4. theStack cross-referenced this on Nov 14, 2019 from issue test: add unit test for non-standard txs with too large scriptSig by theStack
  5. MarcoFalke referenced this in commit f92e750eb4 on Nov 15, 2019
  6. theStack cross-referenced this on Nov 18, 2019 from issue test: add unit test for non-standard bare multisig txs by theStack
  7. theStack cross-referenced this on Nov 20, 2019 from issue test: add functional test for non-standard txs with too large scriptSig by theStack
  8. fanquake referenced this in commit 3671c5721d on Nov 20, 2019
  9. sidhujag referenced this in commit f6fb810bdc on Nov 20, 2019
  10. theStack cross-referenced this on Nov 20, 2019 from issue test: add functional test for non-standard bare multisig txs by theStack
  11. dspicher commented at 10:31 AM on November 21, 2019: contributor

    I would like to work on this. @theStack did you already get started on the "bare-multisig" tests?

  12. theStack commented at 12:01 PM on November 21, 2019: contributor

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

  13. dspicher cross-referenced this on Nov 21, 2019 from issue test: add unit test for non-standard txs with wrong nVersion by dspicher
  14. KaanKC cross-referenced this on Nov 23, 2019 from issue test: add unit test for non-standard txs w/ too large tx size by KaanKC
  15. MarcoFalke referenced this in commit 54b12e425b on Dec 3, 2019
  16. sidhujag referenced this in commit 79655da96b on Dec 3, 2019
  17. MarcoFalke referenced this in commit ea756bc48c on Dec 10, 2019
  18. sidhujag referenced this in commit 67c61e5a9a on Dec 10, 2019
  19. theStack cross-referenced this on Dec 11, 2019 from issue test: add unit test for non-standard "scriptsig-not-pushonly" txs by theStack
  20. MarcoFalke referenced this in commit ec9b964cc9 on Jan 16, 2020
  21. theStack cross-referenced this on Jan 17, 2020 from issue test: add unit test for non-standard txs with too large tx size by theStack
  22. sidhujag referenced this in commit 6f3eaef555 on Jan 17, 2020
  23. laanwj referenced this in commit ceb3d45f7d on Feb 10, 2020
  24. sidhujag referenced this in commit a8cc03f1fe on Feb 18, 2020
  25. MarcoFalke referenced this in commit 98fbb2a184 on Mar 24, 2020
  26. theStack commented at 3:26 PM on March 24, 2020: contributor

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

  27. theStack closed this on Mar 24, 2020

  28. sidhujag referenced this in commit d6c5b753a2 on Mar 28, 2020
  29. jasonbcox referenced this in commit ea176cecdd on Nov 5, 2020
  30. sidhujag referenced this in commit 71802a6d21 on Nov 10, 2020
  31. sidhujag referenced this in commit bea79222be on Nov 10, 2020
  32. sidhujag referenced this in commit c4127b0278 on Nov 10, 2020
  33. sidhujag referenced this in commit 348d49416b on Nov 10, 2020
  34. sidhujag referenced this in commit b63e90ed86 on Nov 10, 2020
  35. PastaPastaPasta referenced this in commit 950e91d5c0 on Jun 27, 2021
  36. PastaPastaPasta referenced this in commit 881417d04f on Jun 27, 2021
  37. PastaPastaPasta referenced this in commit db3d2dd0af on Jun 28, 2021
  38. PastaPastaPasta referenced this in commit 77be9c7a14 on Jun 28, 2021
  39. PastaPastaPasta referenced this in commit 5466402f68 on Jun 29, 2021
  40. PastaPastaPasta referenced this in commit 5e4632e137 on Jun 29, 2021
  41. PastaPastaPasta referenced this in commit b1d9c8fe2d on Jul 1, 2021
  42. PastaPastaPasta referenced this in commit 000a5d3be2 on Jul 1, 2021
  43. PastaPastaPasta referenced this in commit 8e8bb36231 on Jul 1, 2021
  44. PastaPastaPasta referenced this in commit 3d92014caf on Jul 1, 2021
  45. PastaPastaPasta referenced this in commit 39de93769f on Jul 14, 2021
  46. PastaPastaPasta referenced this in commit 4c3e1a68c5 on Jul 14, 2021
  47. PastaPastaPasta referenced this in commit 0cb5bf6336 on Jul 14, 2021
  48. PastaPastaPasta referenced this in commit ec046ea48a on Jul 14, 2021
  49. PastaPastaPasta referenced this in commit 89af1a9f1e on Jul 15, 2021
  50. PastaPastaPasta referenced this in commit 8adf798623 on Jul 15, 2021
  51. bitcoin locked this on Feb 15, 2022

github-metadata-mirror

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: 2026-05-20 06:54 UTC