test: Validate oversized transactions or without inputs #29862

pull l0rinc wants to merge 4 commits into bitcoin:master from l0rinc:paplorinc/consensus_test_coverage changing 2 files +59 −24
  1. l0rinc 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). <img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/57a74ff5-5466-401f-a4fe-d79e36964adf">

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

  2. DrahtBot commented at 12:42 PM on April 12, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

  3. DrahtBot added the label Tests on Apr 12, 2024
  4. 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 12: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.

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

  5. tdb3 commented at 12: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.

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

  7. maflcko commented at 9:31 AM on April 18, 2024: member

    lgtm ACK 1240610fcf0651f217fd01de387e1047dc18485f

  8. BrandonOdiwuor approved
  9. BrandonOdiwuor commented at 12:58 PM on April 23, 2024: contributor

    ACK 1240610fcf0651f217fd01de387e1047dc18485f

    Well done on including the unit test

  10. l0rinc force-pushed on Apr 24, 2024
  11. l0rinc renamed this:
    test: Validate transaction without inputs
    test: Validate transaction without inputs and oversized tx
    on Apr 24, 2024
  12. l0rinc renamed this:
    test: Validate transaction without inputs and oversized tx
    test: Validate transaction without inputs or oversized tx
    on Apr 24, 2024
  13. l0rinc renamed this:
    test: Validate transaction without inputs or oversized tx
    test: Validate oversized transactions or without inputs
    on Apr 24, 2024
  14. 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:

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

    Unit test failed (as expected).

    l0rinc commented at 8:40 AM on April 25, 2024:

    thanks for checking

  15. tdb3 commented at 1:12 AM on April 25, 2024: contributor

    re-ACK for e2e8b21240da8d567a37d35ae29a6100fde65d2b

  16. DrahtBot requested review from BrandonOdiwuor on Apr 25, 2024
  17. DrahtBot requested review from maflcko on Apr 25, 2024
  18. dasibcryptoidology approved
  19. l0rinc force-pushed on May 11, 2024
  20. 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).

  21. l0rinc force-pushed on May 29, 2024
  22. tdb3 approved
  23. 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).

  24. rkrux approved
  25. rkrux commented at 9:17 AM on June 17, 2024: contributor

    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.

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

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

  27. l0rinc force-pushed on Jun 17, 2024
  28. 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.

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

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

    l0rinc commented at 1:46 PM on June 18, 2024:

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

  30. rkrux approved
  31. rkrux commented at 8:52 PM on June 17, 2024: contributor

    reACK fa9e680

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

  32. DrahtBot requested review from tdb3 on Jun 17, 2024
  33. 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.

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

  34. tdb3 commented at 11:48 AM on June 18, 2024: contributor

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

  35. DrahtBot requested review from tdb3 on Jun 18, 2024
  36. l0rinc force-pushed on Jun 18, 2024
  37. Use SCRIPT_VERIFY_NONE instead of hard-coded 0 in transaction_tests c3a8843189
  38. Validate transaction without inputs 1984187840
  39. Validate oversized transaction 327a31d1a4
  40. Replace hard-coded constant in test 969e047cfb
  41. l0rinc force-pushed on Jun 18, 2024
  42. DrahtBot added the label CI failed on Jun 18, 2024
  43. 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:

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

    The test failed as expected.

  44. 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:

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

    The test failed as expected.

  45. tdb3 approved
  46. tdb3 commented at 11:57 PM on June 18, 2024: contributor

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

  47. DrahtBot requested review from rkrux on Jun 18, 2024
  48. DrahtBot removed the label CI failed on Jun 19, 2024
  49. 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.

  50. l0rinc 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.

  51. achow101 commented at 5:23 PM on June 20, 2024: member

    ACK 969e047cfbab86e5819a2c9056e8d2dab17513a8

  52. achow101 merged this on Jun 20, 2024
  53. achow101 closed this on Jun 20, 2024
  54. l0rinc deleted the branch on Jun 20, 2024
  55. l0rinc commented at 5:39 PM on June 20, 2024: contributor

    Thanks for the reviews!

  56. bitcoin locked this on Jun 20, 2025
Contributors
l0rincDrahtBottdb3maflckoBrandonOdiwuordasibcryptoidologyrkruxachow101glozow


rkruxmaflckoBrandonOdiwuor

Labels
Tests
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-04-24 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me