test: add unit test for non-standard txs with too large tx size #17947

pull theStack wants to merge 1 commits into bitcoin:master from theStack:20200116-test-check-for-non-standard-txs-with-too-large-tx-size changing 1 files +20 −0

theStack commented at 11:15 AM on January 17, 2020: member

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

test: add unit test for non-standard txs with too large tx size

The function IsStandardTx() returns rejection reason "tx-size" if the
transaction weight is larger than MAX_STANDARD_TX_WEIGHT (=400000 vbytes).

4537ba5f21

fanquake added the label Tests on Jan 17, 2020
fanquake requested review from instagibbs on Jan 17, 2020
theStack commented at 11:18 AM on January 17, 2020: member

Note that there is still an open PR #17570 for this unit test but the author hasn't been active for more than five weeks now.
DrahtBot commented at 2:58 PM on January 17, 2020: 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:
- #17720 (test: add unit test for non-standard "scriptsig-not-pushonly" 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.
Empact commented at 7:34 PM on January 17, 2020: member

Code Review ACK https://github.com/bitcoin/bitcoin/pull/17947/commits/4537ba5f21ad8afb705325cd8e15dd43877eb28f
in src/test/transaction_tests.cpp:828 in 4537ba5f21
```
 820 | @@ -821,9 +821,29 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
 821 |      BOOST_CHECK(!IsStandardTx(CTransaction(t), reason));
 822 |      BOOST_CHECK_EQUAL(reason, "scriptsig-size");
 823 |  
 824 | +    // Check tx-size (non-standard if transaction weight is > MAX_STANDARD_TX_WEIGHT)
 825 | +    t.vin.clear();
 826 | +    t.vin.resize(2438); // size per input (empty scriptSig): 41 bytes
 827 | +    t.vout[0].scriptPubKey = CScript() << OP_RETURN << std::vector<unsigned char>(19, 0); // output size: 30 bytes
 828 | +    // tx header:                12 bytes =>     48 vbytes
```
instagibbs commented at 8:41 PM on January 17, 2020:

micro-nit: I don't think this big block of text is necessary for the test reader. We trust that GetTransactionWeight does the right thing, and checking that IsStandardTx does what we expect.

instagibbs commented at 8:42 PM on January 17, 2020:

no need to invalidate ACKs for this, just noting!

theStack commented at 7:32 PM on January 18, 2020:

The idea was to reason how we end up magically from two numbers (2438 and 19) to a transaction size with exactly 400000 vbytes. In case this needs to be altered in the future to reach another tx weight, a test writer has it easier to figure out what to change (I admit that it is very unlikely to ever happen). I agree that it could be a bit shorter though.
instagibbs commented at 8:42 PM on January 17, 2020: member

ACK https://github.com/bitcoin/bitcoin/commit/4537ba5f21ad8afb705325cd8e15dd43877eb28f
theStack commented at 11:27 AM on February 7, 2020: member

Since there has been no activity for three weeks: is there anything else I can do for this PR?
laanwj referenced this in commit ceb3d45f7d on Feb 10, 2020
laanwj merged this on Feb 10, 2020
laanwj closed this on Feb 10, 2020
sidhujag referenced this in commit a8cc03f1fe on Feb 18, 2020
sidhujag referenced this in commit b63e90ed86 on Nov 10, 2020
theStack deleted the branch on Dec 1, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#17394 Add tests checking missing reject reasons for function IsStandardTx #30254 test: doc: fix units in tx-size standardness test (s/vbytes/weight units)

test: add unit test for non-standard txs with too large tx size #17947

Conflicts