test: Validate oversized transactions or without inputs

paplorinc commented at 12:42 pm on April 12, 2024: contributor

Based on https://maflcko.github.io/b-c-cov/test_bitcoin.coverage/src/consensus/tx_check.cpp.gcov.html empty inputs and oversized transactions weren’t covered by Boost unit tests (though they’re covered by python tests).

I have tried including the empty transaction into tx_invalid.json, but it failed for another reason, so I added a separate test case for it in the end.

The oversized tx data is on the failure threshold now (lower threshold fails for a different reason, but I guess that’s fine, we’re testing the boundary here).

DrahtBot commented at 12:42 pm on April 12, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, glozow, achow101
Stale ACK	maflcko, BrandonOdiwuor, rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Apr 12, 2024

in src/test/transaction_tests.cpp:378 in 1240610fcf outdated

370+{
371+    TxValidationState state;
372+    bool isValid = CheckTransaction(CTransaction(CMutableTransaction()), state);
373+
374+    BOOST_CHECK_MESSAGE(!isValid, "Transaction with no inputs should be invalid.");
375+    BOOST_CHECK(state.GetRejectReason() == "bad-txns-vin-empty");

tdb3 commented at 0:34 am on April 14, 2024:

To check that unexpected reject reasons would cause test failure, added characters to the end of the reject reason check. Test failed as expected.

0BOOST_CHECK(state.GetRejectReason() == "bad-txns-vin-empty_ShouldCauseTestFailure");

tdb3 commented at 1:11 am on April 25, 2024:

Re-ran this sanity check on e2e8b21240da8d567a37d35ae29a6100fde65d2b Unit test failed as expected.

tdb3 commented at 0:34 am on April 14, 2024: contributor

ACK for 1240610fcf0651f217fd01de387e1047dc18485f

Great work adding more coverage. Built and ran all unit tests (including transaction_tests). Passed.

maflcko commented at 9:30 am on April 18, 2024: member

For reference, this is already covered in the total coverage: https://maflcko.github.io/b-c-cov/total.coverage/src/consensus/tx_check.cpp.gcov.html

maflcko commented at 9:31 am on April 18, 2024: member

lgtm ACK 1240610fcf0651f217fd01de387e1047dc18485f

BrandonOdiwuor approved

BrandonOdiwuor commented at 12:58 pm on April 23, 2024: contributor

ACK 1240610fcf0651f217fd01de387e1047dc18485f

Well done on including the unit test

paplorinc force-pushed on Apr 24, 2024

paplorinc renamed this:
~~test: Validate transaction without inputs~~
test: Validate transaction without inputs and oversized tx
on Apr 24, 2024

paplorinc renamed this:
~~test: Validate transaction without inputs and oversized tx~~
test: Validate transaction without inputs or oversized tx
on Apr 24, 2024

paplorinc renamed this:
~~test: Validate transaction without inputs or oversized tx~~
test: Validate oversized transactions or without inputs
on Apr 24, 2024

in src/test/transaction_tests.cpp:382 in e2e8b21240 outdated

377+
378+BOOST_AUTO_TEST_CASE(tx_oversized)
379+{
380+    CMutableTransaction tx;
381+    tx.vin.resize(1);
382+    auto largeOutput = CScript() << OP_RETURN << std::vector<unsigned char>(MAX_BLOCK_WEIGHT / WITNESS_SCALE_FACTOR, 0x00);

tdb3 commented at 1:11 am on April 25, 2024:

To check see that the test would fail without “bad-txns-oversize”, modified tx_oversized case temporarily with the following:

0auto largeOutput = CScript() << OP_RETURN << std::vector<unsigned char>(60, 0x00);

Unit test failed (as expected).

paplorinc commented at 8:40 am on April 25, 2024:

thanks for checking

tdb3 commented at 1:12 am on April 25, 2024: contributor

re-ACK for e2e8b21240da8d567a37d35ae29a6100fde65d2b

DrahtBot requested review from BrandonOdiwuor on Apr 25, 2024

DrahtBot requested review from maflcko on Apr 25, 2024

dasibcryptoidology approved

paplorinc force-pushed on May 11, 2024

tdb3 commented at 5:56 pm on May 12, 2024: contributor

re ACK for eae4aeb17503ce51ef34c60c94341e00fafc12c8

Range diff showed only metadata updates (e.g. email). Re-ran unit tests (passed).

paplorinc force-pushed on May 29, 2024

tdb3 approved

tdb3 commented at 10:19 pm on June 2, 2024: contributor

re ACK for 0aa7db42564408edb41b0d42103d39ba4c2787dc

Re-ran modifications of the tests to induce failure as described in #29862 (review) and https://github.com/bitcoin/bitcoin/pull/29862/commits/0aa7db42564408edb41b0d42103d39ba4c2787dc#r1578682415.

Unexpected reject reasons were caught (succesfully).

rkrux approved

rkrux commented at 9:17 am on June 17, 2024: none

tACK 0aa7db4

make and make check successful, so are all the functional tests.

Thanks for adding these unit tests and using SCRIPT_VERIFY_NONE instead of hardcoded 0.

in src/test/transaction_tests.cpp:382 in 0aa7db4256 outdated

374@@ -375,6 +375,18 @@ BOOST_AUTO_TEST_CASE(tx_no_inputs)
375     BOOST_CHECK(state.GetRejectReason() == "bad-txns-vin-empty");
376 }
377 
378+BOOST_AUTO_TEST_CASE(tx_oversized)
379+{
380+    CMutableTransaction tx;
381+    tx.vin.resize(1);
382+    auto largeOutput = CScript() << OP_RETURN << std::vector<unsigned char>(MAX_BLOCK_WEIGHT / WITNESS_SCALE_FACTOR, 0x00);

achow101 commented at 6:47 pm on June 17, 2024:

In 0aa7db42564408edb41b0d42103d39ba4c2787dc “Validate oversized transaction”

I think it would be more interesting for this test to be on the threshold exactly. The rest of the tx is 70 bytes, so MAX_BLOCK_WEIGHT / WITNESS_SCALE_FACTOR - 70 should pass CheckTransaction, while - 69 does not.

paplorinc commented at 7:58 pm on June 17, 2024:

I wanted a value that’s definitely oversized, to avoid being close to the threshold - but I think I managed to make it work, thanks for the hint.

paplorinc force-pushed on Jun 17, 2024

in src/test/transaction_tests.cpp:383 in fa9e680b0c outdated

378+    BOOST_CHECK(state.GetRejectReason() == "bad-txns-vin-empty");
379+}
380+
381+BOOST_AUTO_TEST_CASE(tx_oversized)
382+{
383+    auto maxPayloadSize = MAX_BLOCK_WEIGHT / WITNESS_SCALE_FACTOR - 70;

rkrux commented at 8:47 pm on June 17, 2024:

Would be nice to add a comment explaining the presence of this constant 70 for future reference.

tdb3 commented at 11:24 am on June 18, 2024:

This is a good suggestion. If the file changes again, an explanation of “70” would add value to future reviewers.

paplorinc commented at 1:45 pm on June 18, 2024:

I’ve calculated the maxPayloadSize by serializing the values that definitely overflows, subtracting the op_return data size from it, getting the overhead, which I can just subtract from (MAX_BLOCK_WEIGHT / WITNESS_SCALE_FACTOR) and get the threshold.

in src/test/transaction_tests.cpp:405 in fa9e680b0c outdated

399+
400+        TxValidationState state;
401+        BOOST_CHECK_MESSAGE(!CheckTransaction(CTransaction(tx), state), "Oversized transaction should be invalid");
402+        BOOST_CHECK(state.GetRejectReason() == "bad-txns-oversize");
403+    }
404+}

rkrux commented at 8:51 pm on June 17, 2024:

Nice to have: Duplication here can be removed by using lambda expressions and captures.

paplorinc commented at 1:46 pm on June 18, 2024:

I’ve done this as well as part of the above change.

rkrux approved

rkrux commented at 8:52 pm on June 17, 2024: none

reACK fa9e680

Re-tested by running make and make check, both successful. Mentioned couple suggestions but not blockers from my side, upto author.

DrahtBot requested review from tdb3 on Jun 17, 2024

in src/test/transaction_tests.cpp:390 in fa9e680b0c outdated

385+        CMutableTransaction tx;
386+        tx.vin.resize(1);
387+        tx.vout.emplace_back(1, CScript() << OP_RETURN << std::vector<unsigned char>(maxPayloadSize));
388+
389+        TxValidationState state;
390+        CheckTransaction(CTransaction(tx), state);

tdb3 commented at 11:43 am on June 18, 2024:

Am I missing something simple? In #29862 (review), it was mentioned that -70 should pass, but -69 should not. The test case appears to check that -70 fails with bad-txns-oversize. If -70 passes the size checking, it would fail for a reason other than bad-txns-oversize, which might be worth checking (i.e. shows that CheckTransaction is proceeding past the bad-txns-oversize check threshold).

Disclaimer: I didn’t check the txn byte math yet for 69/70 yet (e.g. version, inputcount, inputs,…), but the concept is the same. Checking below and above the threshold makes sense.

paplorinc commented at 1:47 pm on June 18, 2024:

I would need to construct fully valid transaction for it to pass - since this doesn’t have valid inputs it’s considered a coinbase transaction which would need some extra data, which would be outside the scope of this validation. Since I’m only changing the outputs size and that triggers the correct error, I consider that to be enough.

tdb3 commented at 11:48 am on June 18, 2024: contributor

Thanks. Most of the updates look great. Left a question.

DrahtBot requested review from tdb3 on Jun 18, 2024

paplorinc force-pushed on Jun 18, 2024

Use SCRIPT_VERIFY_NONE instead of hard-coded 0 in transaction_tests c3a8843189

Validate transaction without inputs 1984187840

Validate oversized transaction 327a31d1a4

Replace hard-coded constant in test 969e047cfb

paplorinc force-pushed on Jun 18, 2024

DrahtBot added the label CI failed on Jun 18, 2024

in src/test/transaction_tests.cpp:396 in 969e047cfb

391+
392+    auto maxPayloadSize = maxTransactionSize - oversizedTransactionBaseSize;
393+    {
394+        TxValidationState state;
395+        CheckTransaction(createTransaction(maxPayloadSize), state);
396+        BOOST_CHECK(state.GetRejectReason() != "bad-txns-oversize");

tdb3 commented at 11:42 pm on June 18, 2024:

Nice approach to test the boundary and handle non-bad-txns-oversize reject reasons.

Exercised this by inducing a failure with:

0-    auto maxPayloadSize = maxTransactionSize - oversizedTransactionBaseSize;
1+    auto maxPayloadSize = maxTransactionSize - oversizedTransactionBaseSize +1;

The test failed as expected.

in src/test/transaction_tests.cpp:403 in 969e047cfb

398+
399+    maxPayloadSize += 1;
400+    {
401+        TxValidationState state;
402+        BOOST_CHECK_MESSAGE(!CheckTransaction(createTransaction(maxPayloadSize), state), "Oversized transaction should be invalid");
403+        BOOST_CHECK(state.GetRejectReason() == "bad-txns-oversize");

tdb3 commented at 11:46 pm on June 18, 2024:

Similarly, exercised this by inducing a failure with:

0-    maxPayloadSize += 1;
1+    //maxPayloadSize += 1;

The test failed as expected.

tdb3 approved

tdb3 commented at 11:57 pm on June 18, 2024: contributor

ACK 969e047cfbab86e5819a2c9056e8d2dab17513a8 pending MSan, depends CI failure. Left comments showing what was exercised.

DrahtBot requested review from rkrux on Jun 18, 2024

DrahtBot removed the label CI failed on Jun 19, 2024

glozow commented at 11:35 am on June 19, 2024: member

utACK 969e047cfbab86e5819a2c9056e8d2dab17513a8

Last commit looks irrelevant to the added test and can be dropped.

paplorinc commented at 11:40 am on June 19, 2024: contributor

Last commit looks irrelevant to the added test and can be dropped.

Thanks for the review! The last commit is touching the existing bad-txns-oversize equivalent in Python.

achow101 commented at 5:23 pm on June 20, 2024: member

ACK 969e047cfbab86e5819a2c9056e8d2dab17513a8

achow101 merged this on Jun 20, 2024

achow101 closed this on Jun 20, 2024

paplorinc deleted the branch on Jun 20, 2024

paplorinc commented at 5:39 pm on June 20, 2024: contributor

Thanks for the reviews!

test: Validate oversized transactions or without inputs #29862

Code Coverage

Reviews