[test] Small unit test improvements, including helper to make mempool transaction #21121

pull amitiuttarwar wants to merge 5 commits into bitcoin:master from amitiuttarwar:2021-01-unit-test-valid-tx changing 6 files +90 −7
  1. amitiuttarwar commented at 6:53 PM on February 8, 2021: contributor

    Some miscellaneous improvements that came up when working on #21061

    • The first commit is a helper to make valid mempool transactions & submit via ATMP. Introducing in this PR, using in #21061.
    • The second commit is a small improvement in miner_tests.cpp that uses BOOST_REQUIRE_EQUAL to properly terminate the program instead of segfaulting in the failure scenario where the blocks do not include the expected number of transactions.
    • The third commit changes the function signature of GetMockTime() to return a chrono type.
    • The fourth & fifth commit overload SetMockTime to also accept chrono type, and adds documentation to indicate that the int64_t function signature is deprecated.
  2. amitiuttarwar cross-referenced this on Feb 8, 2021 from issue [p2p] Introduce node rebroadcast module by amitiuttarwar
  3. DrahtBot added the label Utils/log/libs on Feb 8, 2021
  4. amitiuttarwar removed the label Utils/log/libs on Feb 8, 2021
  5. amitiuttarwar added the label Tests on Feb 8, 2021
  6. amitiuttarwar commented at 1:54 AM on February 9, 2021: contributor

    the failing test seems unrelated. The failure is in feature_assumevalid.py, which seems hard to impact from changes that exclusively touch the unit tests, unit test framework, and some comments :)

  7. DrahtBot commented at 3:40 AM on February 9, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  8. DrahtBot cross-referenced this on Feb 9, 2021 from issue tree-wide: De-globalize ChainstateManager by dongcarl
  9. DrahtBot cross-referenced this on Feb 9, 2021 from issue validation: UTXO snapshot activation by jamesob
  10. in src/util/time.h:50 in e7573304b7 outdated 
      39 | @@ -40,9 +40,16 @@ int64_t GetTimeMicros();
  40 |  /** Returns the system time (not mockable) */
  41 |  int64_t GetSystemTimeInSeconds(); // Like GetTime(), but not mockable
  42 |  
  43 | -/** For testing. Set e.g. with the setmocktime rpc, or -mocktime argument */
  44 | +/**
  45 | + * For testing. Set e.g. with the setmocktime rpc, or -mocktime argument
  46 | + *
  47 | + * @param[in] nMockTimeIn Time in seconds.
    laanwj commented at 1:09 PM on February 10, 2021:

    Probably more of a hassle, but another way to document would be to use std::chrono types with explicit units.

    amitiuttarwar commented at 6:43 PM on February 10, 2021:

    yeah, I took a quick look at changing the signature to a chrono type, but ended up taking the efficient/lazy way for now. there are a couple tweaks that would need to be made to switch it over, nothing difficult but not currently at the top of my priority list

    so I thought leaving a comment was the smallest way to help :)

    amitiuttarwar commented at 6:46 PM on February 12, 2021:

    in the latest push, I updated the function signature of GetMockTime, with updated call sites. I also overloaded SetMockTime to take in chronos. This does mean I introduced another function that doesn't get used until #21061, so I can remove if you'd prefer.

  11. laanwj commented at 1:11 PM on February 10, 2021: member

    Code review ACK e7573304b7e112b6b7f49c79c25fce36a5440209

    The new function is added but not used (so not tested), I would prefer if it was, but I guess it can wait until #21061.

  12. in src/test/util/setup_common.cpp:245 in e7573304b7 outdated 
     240 | +    // Transaction we will submit to the mempool
 241 | +    CMutableTransaction mempool_txn;
 242 | +
 243 | +    // Create an input
 244 | +    COutPoint outpoint_to_spend = COutPoint(input_transaction->GetHash(), input_vout);
 245 | +    CTxIn input(outpoint_to_spend, CScript(), CTxIn::SEQUENCE_FINAL);
    vasild commented at 10:36 AM on February 11, 2021:

    nit

        CTxIn input(outpoint_to_spend);

    The last two arguments have default values equal to the above.

    amitiuttarwar commented at 6:48 PM on February 12, 2021:

    done

  13. in src/test/util/setup_common.cpp:262 in e7573304b7 outdated 
     257 | +    CCoinsView coins_view;
 258 | +    CCoinsViewCache coins_cache(&coins_view);
 259 | +    AddCoins(coins_cache, *input_transaction.get(), /* check_for_overwrite */ false);
 260 | +    // - Use GetCoin to properly populate utxo_to_spend,
 261 | +    Coin utxo_to_spend;
 262 | +    coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend);
    vasild commented at 10:45 AM on February 11, 2021:

    Maybe assert() or BOOST_REQUIRE() that GetCoin() returns true?

    amitiuttarwar commented at 6:48 PM on February 12, 2021:

    done

  14. in src/test/util/setup_common.cpp:244 in e7573304b7 outdated 
     239 | +{
 240 | +    // Transaction we will submit to the mempool
 241 | +    CMutableTransaction mempool_txn;
 242 | +
 243 | +    // Create an input
 244 | +    COutPoint outpoint_to_spend = COutPoint(input_transaction->GetHash(), input_vout);
    vasild commented at 10:46 AM on February 11, 2021:

    nit

        COutPoint outpoint_to_spend(input_transaction->GetHash(), input_vout);
    amitiuttarwar commented at 6:49 PM on February 12, 2021:

    done

  15. in src/test/util/setup_common.cpp:259 in e7573304b7 outdated 
     254 | +    FillableSigningProvider keystore;
 255 | +    keystore.AddKey(coinbaseKey);
 256 | +    // - Populate a CoinsViewCache with the unspent output
 257 | +    CCoinsView coins_view;
 258 | +    CCoinsViewCache coins_cache(&coins_view);
 259 | +    AddCoins(coins_cache, *input_transaction.get(), /* check_for_overwrite */ false);
    vasild commented at 10:54 AM on February 11, 2021:

    The prototype of AddCoins() is:

    void AddCoins(CCoinsViewCache& cache, const CTransaction& tx, int nHeight, bool check = false);

    I think the above call implicitly converts false to 0 for the 3rd argument nHeight. Maybe that should be:

        const int height = 123;
    AddCoins(coins_cache, *input_transaction.get(), height);
    amitiuttarwar commented at 6:51 PM on February 12, 2021:

    good catch! I updated so the height is a param that's passed in. It seems to not matter for my use cases, but since this is a helper it definitely is better to have consistency in the data structure. thanks :)

  16. in src/test/util/setup_common.cpp:269 in e7573304b7 outdated 
     264 | +    std::map<COutPoint, Coin> input_coins;
 265 | +    input_coins.insert({outpoint_to_spend, utxo_to_spend});
 266 | +    // - Default signature hashing type
 267 | +    int nHashType = SIGHASH_ALL;
 268 | +    std::map<int, std::string> input_errors;
 269 | +    SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors);
    vasild commented at 10:57 AM on February 11, 2021:

    assert() or BOOST_REQUIRE() a successful sign?

    amitiuttarwar commented at 6:51 PM on February 12, 2021:

    done

  17. in src/test/util/setup_common.cpp:273 in e7573304b7 outdated 
     268 | +    std::map<int, std::string> input_errors;
 269 | +    SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors);
 270 | +
 271 | +    // Add transaction to the mempool
 272 | +    TxValidationState state;
 273 | +    WITH_LOCK(cs_main, AcceptToMemoryPool(*m_node.mempool.get(), state, MakeTransactionRef(mempool_txn), nullptr, false));
    vasild commented at 11:00 AM on February 11, 2021:

    assert() or BOOST_REQUIRE() a success here?

    amitiuttarwar commented at 6:52 PM on February 12, 2021:

    done

  18. in src/test/util/setup_common.h:141 in e7573304b7 outdated 
     120 | @@ -121,6 +121,11 @@ struct TestChain100Setup : public RegTestingSetup {
 121 |      CBlock CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns,
 122 |                                   const CScript& scriptPubKey);
 123 |  
 124 | +    CMutableTransaction CreateValidMempoolTransaction(CTransactionRef input_transaction,
 125 | +                                                      int input_vout,
 126 | +                                                      CScript output_destination,
 127 | +                                                      CAmount output_amount = CAmount(1 * COIN));
    vasild commented at 11:10 AM on February 11, 2021:

    The implementation assumes that the key to spend input_transaction:input_vout is this->coinbaseKey. Indeed all callers of this in #21061 pass some tx from this->m_coinbase_txns[].

    This is not clear from the above prototype. Maybe document this explicitly or even better - pass the spend key here, together with input_transaction and input_vout.

    amitiuttarwar commented at 6:52 PM on February 12, 2021:

    good point! extracted input_signing_key as a param to be passed in.

  19. amitiuttarwar commented at 4:26 AM on February 12, 2021: contributor

    @vasild- thanks for the review! I've taken all your suggestions locally, but I'm currently getting a linker error when trying to #include <boost/test/unit_test.hpp> in setup_common.cpp (to use BOOST_REQUIRE). I'll dig into it more tomorrow, but just dropping a line here incase you (or anybody else) knows the cause off hand.

  20. vasild commented at 3:12 PM on February 12, 2021: contributor

    @amitiuttarwar, I see no boost code is used in setup_common.cpp. Maybe it would be easier to use assert() inside CreateValidMempoolTransaction() instead or somehow signal a failure from that method and make the callers use BOOST_REQUIRE() to ensure that it succeeded.

  21. MarcoFalke commented at 3:27 PM on February 12, 2021: member

    It is only allowed to use boost in the unit tests, not the fuzz tests (or other tests). setup_common is used by all test and bench libraries.

  22. amitiuttarwar force-pushed on Feb 12, 2021
  23. amitiuttarwar commented at 7:00 PM on February 12, 2021: contributor

    ah interesting. used asserts instead. thanks!

    took all the feedback.

    • added asserts to CreateValidMempool helper
    • changed GetMockTime to return chrono time, updated the call sites
    • introduced a SetMockTime that takes in chrono time, did not update call sites, but documented in the notes that the int64_t is deprecated and the chrono one should be used

    this means that in this commit, I introduce CreateValidMempoolTransaction and SetMockTime(std::chrono::seconds), but they are currently unused. I tested them by locally applying #21061, but ofc there is no guarantee that PR will be merged. if reviewers prefer, I can remove commit 88e62f78a7920bbeec01a05126bc8dd7b1407c4b Introduce a SetTMockTime that takes chrono time.

  24. amitiuttarwar force-pushed on Feb 12, 2021
  25. amitiuttarwar commented at 7:45 PM on February 12, 2021: contributor

    fixed a silent merge conflict since the function signature of ATMP has changed. updated #21061 to use e020cd6 to test the helper.

  26. in src/logging.cpp:207 in 82de557f8c outdated 
     202 | @@ -203,9 +203,9 @@ std::string BCLog::Logger::LogTimestampStr(const std::string& str)
 203 |              strStamped.pop_back();
 204 |              strStamped += strprintf(".%06dZ", nTimeMicros%1000000);
 205 |          }
 206 | -        int64_t mocktime = GetMockTime();
 207 | -        if (mocktime) {
 208 | -            strStamped += " (mocktime: " + FormatISO8601DateTime(mocktime) + ")";
 209 | +        std::chrono::seconds mocktime = GetMockTime();
 210 | +        if (mocktime.count() == 0) {
    vasild commented at 1:54 PM on February 15, 2021:

    Shouldn't that be != instead of ==? Or:

            if (mocktime > 0) {
    amitiuttarwar commented at 10:01 PM on February 15, 2021:

    🤦‍♀️ good catch.

    amitiuttarwar commented at 10:19 PM on February 15, 2021:

    fixed. can't compare chrono time directly to an int, so I updated to if (mocktime > 0s) to keep the comparison in chronos.

  27. in src/logging.cpp:208 in 82de557f8c outdated 
     202 | @@ -203,9 +203,9 @@ std::string BCLog::Logger::LogTimestampStr(const std::string& str)
 203 |              strStamped.pop_back();
 204 |              strStamped += strprintf(".%06dZ", nTimeMicros%1000000);
 205 |          }
 206 | -        int64_t mocktime = GetMockTime();
 207 | -        if (mocktime) {
 208 | -            strStamped += " (mocktime: " + FormatISO8601DateTime(mocktime) + ")";
 209 | +        std::chrono::seconds mocktime = GetMockTime();
 210 | +        if (mocktime.count() == 0) {
 211 | +            strStamped += " (mocktime: " + FormatISO8601DateTime(mocktime.count()) + ")";
    vasild commented at 1:57 PM on February 15, 2021: 
                strStamped += " (mocktime: " + FormatISO8601DateTime(count_seconds(mocktime)) + ")";

    This will prevent silent surprises if GetMockTime() is changed some day to return something else than std::chrono::seconds, e.g. std::chrono:milliseconds.

    amitiuttarwar commented at 10:19 PM on February 15, 2021:

    cool, didn't know about these helpers. updated. thanks!

  28. amitiuttarwar force-pushed on Feb 15, 2021
  29. amitiuttarwar commented at 10:20 PM on February 15, 2021: contributor

    updated to incorporate feedback from @vasild

  30. vasild commented at 4:40 PM on February 16, 2021: contributor

    ACK 903bbf7f1068da0c27b99401483444f037a17840

  31. DrahtBot added the label Needs rebase on Feb 16, 2021
  32. [test] Introduce a unit test helper to create a valid mempool transaction. 9a3bbe8fc5
  33. [test] Throw error instead of segfaulting in failure scenario
    If the miner code is faulty and does not include any transactions in a block,
the code segfaults when it tries to access block transactions. Instead, add a
check that safely aborts the process.
    a2d908e1da
  34. [util] Change GetMockTime to return chrono type instead of int df6a5fc1df
  35. [util] Introduce a SetMockTime that takes chrono time 47a7a1687d
  36. [doc / util] Use comments to clarify time unit for int64_t type. 1363b6c27d
  37. amitiuttarwar force-pushed on Feb 16, 2021
  38. amitiuttarwar commented at 8:24 PM on February 16, 2021: contributor

    rebased

  39. DrahtBot removed the label Needs rebase on Feb 16, 2021
  40. vasild commented at 9:10 AM on February 17, 2021: contributor

    ACK 1363b6c27dbd2614fd555d148ea624ed8b95f14e

  41. vasild approved
  42. MarcoFalke merged this on Feb 17, 2021
  43. MarcoFalke closed this on Feb 17, 2021
  44. sidhujag referenced this in commit 3d9605586b on Feb 17, 2021
  45. amitiuttarwar deleted the branch on Feb 17, 2021
  46. in src/test/miner_tests.cpp:126 in 1363b6c27d 
     122 | @@ -123,6 +123,7 @@ void MinerTestingSetup::TestPackageSelection(const CChainParams& chainparams, co
 123 |      m_node.mempool->addUnchecked(entry.Fee(50000).Time(GetTime()).SpendsCoinbase(false).FromTx(tx));
 124 |  
 125 |      std::unique_ptr<CBlockTemplate> pblocktemplate = AssemblerForTest(chainparams).CreateNewBlock(scriptPubKey);
 126 | +    BOOST_REQUIRE_EQUAL(pblocktemplate->block.vtx.size(), 4);
    jonasschnelli commented at 7:31 PM on February 18, 2021:

    Somehow this leads to a "comparison of integers of different signs" on bitcoinbuilds.org: https://bitcoinbuilds.org/index.php?ansilog=982c61cf-6969-4001-bebc-dc215e5d29a4.log#l1906 Maybe only happening on older boost versions?

  47. jonasschnelli cross-referenced this on Feb 18, 2021 from issue test: Avoid comparision of integers with different signs by jonasschnelli
  48. fanquake referenced this in commit f093310b2e on Feb 19, 2021
  49. sidhujag referenced this in commit f83719ffdb on Feb 19, 2021
  50. bitcoin locked this on Aug 16, 2022
Contributors
amitiuttarwarDrahtBotlaanwjvasildMarcoFalkejonasschnelli
Labels
Tests
Linked (view graph)
#21061 [p2p] Introduce node rebroadcast module #21228 test: Avoid comparision of integers with different signs
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-05-20 06:54 UTC