test: add unit test for non-standard txs w/ too large tx size #17570

1. 
   KaanKC commented at 10:01 am on November 23, 2019: none

   Adds one of the missing tests of issue #17394: the function IsStandardTx() returns rejection reason tx-size if the transaction’s size is larger than MAX_STANDARD_TX_WEIGHT, which is as of now defined as 400000.

   Simply increasing the size of vin seemed the easiest way to test this case.

   Heavily inspired by #17480

2. test: add unit test for non-standard txs w/ too large tx size

   The function IsStandardTx() returns rejection reason tx-size if the transaction's size is larger than MAX_STANDARD_TX_WEIGHT, which is as of now defined as 400000.

   f31ab9105e
3. DrahtBot added the label Tests on Nov 23, 2019
4. 
   DrahtBot commented at 11:40 am on November 23, 2019: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #17502 (test: add unit test for non-standard bare multisig txs by theStack)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

5. test: use random large number for non-standard tx-size unit test

   Using numbers to close to the MAX_STANARD_TX_WEIGHT, might cause problems down the road. Since the actual tx weight depends on more than just the size of the input array, the test might fail if it is moved above or under other tests, which mutate the tx object. Was also the reason why the initial commit failed the test

   efe165c326
6. 
   instagibbs commented at 5:33 pm on December 3, 2019: member

   Would it be easier to just make a new transaction, make the vin size large enough to just break the size, rather than using an arbitrarily large number?
7. 
   KaanKC commented at 9:54 am on December 4, 2019: none

   Sure. Completely new transaction object or reset the existing one to the default values?
8. DrahtBot added the label Needs rebase on Dec 10, 2019
9. 
   DrahtBot commented at 6:52 pm on December 10, 2019: member
10. 
    theStack commented at 12:17 pm on January 13, 2020: member

    Hi @KaanKC, thanks for your pull request!

    Sure. Completely new transaction object or reset the existing one to the default values?

    There is no need for a completely new object, you can just reuse the existing one (CMutableTransaction t) which also used for all other thests in the same function– first reset it, then resize t.vin to a size where it passes and then fails. Your second commit is not needed then.

11. 
    MarcoFalke commented at 8:43 pm on January 16, 2020: member

    Is anyone still working on this?
12. 
    theStack commented at 11:19 am on January 17, 2020: member

    Is anyone still working on this?

    Since this PR’s author doesn’t seem to be working on the unit test anymore, I created a new PR: #17947.

13. 
    fanquake commented at 11:21 am on January 17, 2020: member

    Going to close in favour of #17947.
14. fanquake closed this on Jan 17, 2020

    ---

15. KaanKC deleted the branch on Jan 17, 2020
16. DrahtBot locked this on Feb 15, 2022

Contributors
KaanKC DrahtBot instagibbs theStack MarcoFalke fanquake

Labels
Tests Needs rebase