test: add tests in transaction_tests.cpp covering live mutants #36130

pull ViniciusCestarii wants to merge 3 commits into bitcoin:master from ViniciusCestarii:kill-tx_verify-mutants changing 1 files +64 −0
  1. ViniciusCestarii commented at 2:08 PM on August 31, 2026: contributor

    Kills some live mutants on tx_verify.cpp that affect consensus found with https://github.com/ViniciusCestarii/mutant-harness. They are:

    <details> <summary>tx_verify.cpp (killed by 5c35785d6ddda80d5147616342e42d759490e6b9): <code>IsFinalTx</code>: sequence loop returns on the first input instead of requiring all of them</summary>

    diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
    index e580a9d..46009a6 100644
    --- a/src/consensus/tx_verify.cpp
    +++ b/src/consensus/tx_verify.cpp
    @@ -35,11 +35,7 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
         // also check that the spending input's nSequence != SEQUENCE_FINAL,
         // ensuring that an unsatisfied nLockTime value will actually cause
         // IsFinalTx() to return false here:
    -    for (const auto& txin : tx.vin) {
    -        if (!(txin.nSequence == CTxIn::SEQUENCE_FINAL))
    -            return false;
    -    }
    -    return true;
    +    return std::ranges::any_of(tx.vin, [](const CTxIn& txin) { return txin.nSequence == CTxIn::SEQUENCE_FINAL; });
     }
     
     std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>& prevHeights, const CBlockIndex& block)
    

    </details>

    <details> <summary>tx_verify.cpp (killed by 3ef559d9a5cb79e4721b68427ad679d9f4f6392a): <code>CalculateSequenceLocks</code>: <code>tx.version >= 2</code> -> <code>tx.version == 2</code></summary>

    diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
    index e580a9d..0faaa55 100644
    --- a/src/consensus/tx_verify.cpp
    +++ b/src/consensus/tx_verify.cpp
    @@ -54,7 +54,7 @@ std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags
         int nMinHeight = -1;
         int64_t nMinTime = -1;
     
    -    bool fEnforceBIP68 = tx.version >= 2 && flags & LOCKTIME_VERIFY_SEQUENCE;
    +    bool fEnforceBIP68 = tx.version == 2 && flags & LOCKTIME_VERIFY_SEQUENCE;
     
         // Do not enforce sequence numbers as a relative lock time
         // unless we have been instructed to
    

    </details>

    <details> <summary>tx_verify.cpp (killed by 1944eb409055d88eeaf7888b18a75289c506a943): <code>GetLegacySigOpCount</code>: <code>scriptSig.GetSigOpCount(false)</code> -> <code>GetSigOpCount(true)</code></summary>

    diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
    index e580a9d..0b98597 100644
    --- a/src/consensus/tx_verify.cpp
    +++ b/src/consensus/tx_verify.cpp
    @@ -120,7 +120,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
         unsigned int nSigOps = 0;
         for (const auto& txin : tx.vin)
         {
    -        nSigOps += txin.scriptSig.GetSigOpCount(false);
    +        nSigOps += txin.scriptSig.GetSigOpCount(true);
         }
         for (const auto& txout : tx.vout)
         {
    

    </details>

    Recommend reviewing per commit.

  2. DrahtBot added the label Tests on Aug 31, 2026
  3. DrahtBot commented at 2:08 PM on August 31, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36130.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK brunoerg, jeanpablojp, instagibbs, sedited
    Stale ACK mercie-ux

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36087 (util: Add and use AssertUnreachable by maflcko)
    • #36074 (scripted-diff: [test] Add util/check.h includes for assertions by maflcko)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. jeanpablojp commented at 9:18 AM on September 2, 2026: contributor

    Concept ACK

    I ran the three mutants on the merge base and all three survive the whole unit suite and seven functional tests.

  5. in src/test/transaction_tests.cpp:1228 in 29bbec6e34
    1223 | +        const CBlockIndex block{};
    1224 | +        const auto lock_pair{CalculateSequenceLocks(CTransaction{mtx}, LOCKTIME_VERIFY_SEQUENCE, prev_heights, block)};
    1225 | +        BOOST_CHECK_EQUAL(lock_pair.first, expected_min_height);
    1226 | +    }};
    1227 | +
    1228 | +    // BIP68 exempts version 1 only, and applies to every version from 2 onward.
    


    jeanpablojp commented at 9:18 AM on September 2, 2026:

    nit: >= 2 exempts version 0 too, not just version 1.


    ViniciusCestarii commented at 12:20 PM on September 2, 2026:

    Thanks! I forgot that version 0 is consensus valid, fixed the comment and also added check for version 0 too: 3ef559d9a5cb79e4721b68427ad679d9f4f6392a

  6. ViniciusCestarii force-pushed on Sep 2, 2026
  7. ViniciusCestarii commented at 12:21 PM on September 2, 2026: contributor

    Thanks for the review. Forced push 1944eb409055d88eeaf7888b18a75289c506a943 addressing #36130 (review).

  8. mercie-ux commented at 6:41 PM on September 2, 2026: none

    ACK 29bbec6e34 the three test clearly correspond to the described live mutants.

  9. mercie-ux commented at 6:42 PM on September 2, 2026: none

    ACK 29bbec6e34 the three test clearly correspond to the described live mutants.

  10. fanquake requested review from instagibbs on Sep 3, 2026
  11. fanquake requested review from brunoerg on Sep 3, 2026
  12. in src/test/transaction_tests.cpp:1175 in 5c35785d6d outdated
    1166 | @@ -1167,6 +1167,30 @@ BOOST_AUTO_TEST_CASE(checktxinputs_invalid_transactions_test)
    1167 |                    TxValidationResult::TX_PREMATURE_SPEND, /*expected_reason=*/"bad-txns-premature-spend-of-coinbase");
    1168 |  }
    1169 |  
    1170 | +BOOST_AUTO_TEST_CASE(isfinaltx_sequences_test)
    1171 | +{
    1172 | +    constexpr int height{100};
    1173 | +    constexpr uint32_t non_final{CTxIn::SEQUENCE_FINAL - 1};
    1174 | +
    1175 | +    // Every transaction here has the same unsatisfied nLockTime, so only the
    


    instagibbs commented at 12:52 PM on September 3, 2026:

    potentially unsatisfied? since it's not unsatisfied if it's not active


    ViniciusCestarii commented at 1:26 PM on September 3, 2026:

    nLockTime is unsatisfied in all three cases here (nLockTime = height fails the < check), it's just ignored when every input is SEQUENCE_FINAL. I used the same wording IsFinalTx function own comment uses: "Even if tx.nLockTime isn't satisfied by nBlockHeight/nBlockTime, a transaction is still considered final if all inputs' nSequence == SEQUENCE_FINAL". Happy to reword if you still find it confusing.


    instagibbs commented at 1:32 PM on September 3, 2026:

    I never found locktimes easy to reason about :)


    ViniciusCestarii commented at 2:04 PM on September 3, 2026:

    Same here lol

  13. in src/test/transaction_tests.cpp:1173 in 5c35785d6d
    1166 | @@ -1167,6 +1167,30 @@ BOOST_AUTO_TEST_CASE(checktxinputs_invalid_transactions_test)
    1167 |                    TxValidationResult::TX_PREMATURE_SPEND, /*expected_reason=*/"bad-txns-premature-spend-of-coinbase");
    1168 |  }
    1169 |  
    1170 | +BOOST_AUTO_TEST_CASE(isfinaltx_sequences_test)
    1171 | +{
    1172 | +    constexpr int height{100};
    1173 | +    constexpr uint32_t non_final{CTxIn::SEQUENCE_FINAL - 1};
    


    instagibbs commented at 12:53 PM on September 3, 2026:

    can just use CTxIn::MAX_SEQUENCE_NONFINAL


    ViniciusCestarii commented at 2:06 PM on September 3, 2026:

    Thanks. Done on bba1d41

  14. in src/test/transaction_tests.cpp:1215 in 3ef559d9a5
    1210 | +    }};
    1211 | +
    1212 | +    // BIP68 only applies to versions 2 and up
    1213 | +    check_min_height(/*version=*/0, /*expected_min_height=*/-1);
    1214 | +    check_min_height(/*version=*/1, /*expected_min_height=*/-1);
    1215 | +    for (uint32_t version{2}; version <= TX_MAX_STANDARD_VERSION; ++version) {
    


    instagibbs commented at 12:56 PM on September 3, 2026:

    maybe just use 3 instead of TX_MAX_STANDARD_VERSION since we don't care about standardness per se and it could theoretically drift in value as a policy choice?


    brunoerg commented at 1:15 PM on September 3, 2026:

    I was going to ask the same, perhaps it could be:

    @@ -1225,12 +1226,11 @@ BOOST_AUTO_TEST_CASE(calculatesequencelocks_tx_version_test)
             BOOST_CHECK_EQUAL(lock_pair.first, expected_min_height);
         }};
     
         // BIP68 only applies to versions 2 and up
         check_min_height(/*version=*/0, /*expected_min_height=*/-1);
         check_min_height(/*version=*/1, /*expected_min_height=*/-1);
    -    for (uint32_t version{2}; version <= TX_MAX_STANDARD_VERSION; ++version) {
    -        check_min_height(version, /*expected_min_height=*/coin_height);
    -    }
    +    check_min_height(/*version=*/2, /*expected_min_height=*/coin_height);
    +    check_min_height(/*version=*/std::numeric_limits<uint32_t>::max(), /*expected_min_height=*/coin_height);
    
    

    ViniciusCestarii commented at 1:38 PM on September 3, 2026:

    My original intention was to avoid patching this test for every new transaction version, but I agree TX_MAX_STANDARD_VERSION isn't a good choice. I will apply @brunoerg suggestion since testing the boundaries is enough.


    ViniciusCestarii commented at 2:07 PM on September 3, 2026:

    Thanks. Done on a5fc82e

  15. instagibbs commented at 1:03 PM on September 3, 2026: member

    Maybe move the sigops test next to GetSigOpCount tests in src/test/sigopcount_tests.cpp?

  16. in src/test/transaction_tests.cpp:1181 in 5c35785d6d
    1176 | +    // sequences decide the outcome.
    1177 | +    auto check_final{[](const std::vector<uint32_t>& sequences, bool expected_final) {
    1178 | +        CMutableTransaction mtx;
    1179 | +        mtx.nLockTime = height;
    1180 | +        for (const uint32_t sequence : sequences) {
    1181 | +            mtx.vin.emplace_back(COutPoint{}, CScript() << OP_TRUE, sequence);
    


    brunoerg commented at 1:08 PM on September 3, 2026:

    5c35785d6ddda80d5147616342e42d759490e6b9: You don't need CScript() << OP_TRUE here, an empty CScript would be enough since IsFinalTx doesn't check it.


    ViniciusCestarii commented at 2:13 PM on September 3, 2026:

    True, Done on bba1d41. Also updated calculatesequencelocks_tx_version_test since CalculateSequenceLocks also doesn't check the scripts on a5fc82e2b1403b7bf0f1ad494a62ccff793f99d0.

  17. brunoerg commented at 1:40 PM on September 3, 2026: contributor

    Concept ACK, the mutants are relevant and I verified that no existing test kills them.

    WIll review again as soon as the relevant comments are addressed.

  18. test: cover IsFinalTx requires every input to be SEQUENCE_FINAL bba1d4150e
  19. test: cover enforce BIP68 to tx versions higher than 2 a5fc82e2b1
  20. test: cover legacy sigops count CHECKMULTISIG inaccurately 5ce3a0b4aa
  21. ViniciusCestarii force-pushed on Sep 3, 2026
  22. ViniciusCestarii commented at 2:10 PM on September 3, 2026: contributor

    Thanks for the reviews! Forced push 5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44 addressing @instagibbs and @brunoerg comments.

  23. brunoerg approved
  24. brunoerg commented at 7:17 PM on September 3, 2026: contributor

    ACK 5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44

  25. DrahtBot requested review from jeanpablojp on Sep 3, 2026
  26. jeanpablojp commented at 8:28 PM on September 3, 2026: contributor

    tACK 5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44

    I re-ran everything on the new head, the three mutants are still live on the merge base and each one still dies only on its own test. Narrowing the guard to tx.version >= 2 && tx.version <= 3 survived the earlier loop form and dies on the max-version case now.

  27. instagibbs commented at 5:22 PM on September 4, 2026: member
  28. sedited approved
  29. sedited commented at 12:18 PM on September 5, 2026: contributor

    ACK 5ce3a0b4aab5ad9ec710e803f88d79139b3b3c44

  30. sedited merged this on Sep 5, 2026
  31. sedited closed this on Sep 5, 2026


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-09-09 07:56 UTC