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

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.

DrahtBot added the label Utils/log/libs on Feb 8, 2021

amitiuttarwar removed the label Utils/log/libs on Feb 8, 2021

amitiuttarwar added the label Tests on Feb 8, 2021

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

DrahtBot commented at 3:40 am on February 9, 2021: member

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

Conflicts

No conflicts as of last run.

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.

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.

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

0    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

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

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

0    COutPoint outpoint_to_spend(input_transaction->GetHash(), input_vout);

amitiuttarwar commented at 6:49 pm on February 12, 2021:

done

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:

0void 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:

0    const int height = 123;
1    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 :)

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

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

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.

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.

vasild commented at 3:12 pm on February 12, 2021: member

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

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.

amitiuttarwar force-pushed on Feb 12, 2021

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.

amitiuttarwar force-pushed on Feb 12, 2021

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.

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:

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

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:

0            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!

amitiuttarwar force-pushed on Feb 15, 2021

amitiuttarwar commented at 10:20 pm on February 15, 2021: contributor

updated to incorporate feedback from @vasild

vasild commented at 4:40 pm on February 16, 2021: member

ACK 903bbf7f1068da0c27b99401483444f037a17840

DrahtBot added the label Needs rebase on Feb 16, 2021

[test] Introduce a unit test helper to create a valid mempool transaction. 9a3bbe8fc5

[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

[util] Change GetMockTime to return chrono type instead of int df6a5fc1df

[util] Introduce a SetMockTime that takes chrono time 47a7a1687d

[doc / util] Use comments to clarify time unit for int64_t type. 1363b6c27d

amitiuttarwar force-pushed on Feb 16, 2021

amitiuttarwar commented at 8:24 pm on February 16, 2021: contributor

rebased

DrahtBot removed the label Needs rebase on Feb 16, 2021

vasild commented at 9:10 am on February 17, 2021: member

ACK 1363b6c27dbd2614fd555d148ea624ed8b95f14e

vasild approved

MarcoFalke merged this on Feb 17, 2021

MarcoFalke closed this on Feb 17, 2021

sidhujag referenced this in commit 3d9605586b on Feb 17, 2021

amitiuttarwar deleted the branch on Feb 17, 2021

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?

fanquake referenced this in commit f093310b2e on Feb 19, 2021

sidhujag referenced this in commit f83719ffdb on Feb 19, 2021

DrahtBot locked this on Aug 16, 2022

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

Conflicts