Add test util to populate mempool with random transactions, fix #24634 bug #24927

1. 
   glozow commented at 11:44 pm on April 19, 2022: member

   Fixes #24634 by using the testing_setup’s actual mempool rather than a locally-declared mempool for running check().

   Also creates a test utility for populating the mempool with a bunch of random transactions. I imagine this could be useful in other places as well; it was necessary here because we needed the mempool to contain transactions spending coins available in the current chainstate. The existing CreateOrderedCoins() is insufficient because it creates coins out of thin air.

   Also implements the separate suggestion to use the TestingSetup mempool in ComplexMemPool bench.

2. glozow force-pushed on Apr 20, 2022
3. 
   dongcarl commented at 0:06 am on April 20, 2022: member

   Concept ACK

   wondering if we shouldn’t copy:

   https://github.com/bitcoin/bitcoin/blob/254f3cc3684cdb7468e353596a89e8fe7a302b9b/src/init.cpp#L1274

   To ChainTestingSetup’s ctor. Right now it wouldn’t matter if you did

   0auto testing_setup = MakeNoLogFileContext<TestChain100Setup>(CBaseChainParams::REGTEST, {"-checkmempool=0"});
   

   The mempool will still be checked.

4. DrahtBot added the label Tests on Apr 20, 2022
5. 
   DrahtBot commented at 3:10 pm on April 20, 2022: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #24975 (bench: remove from available_coins with reference, vout size by Crypt-iQ)
   • #24232 (assumeutxo: add init and completion logic by jamesob)

   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.

6. glozow force-pushed on Apr 20, 2022
7. 
   glozow commented at 9:31 pm on April 20, 2022: member

   wondering if we shouldn’t copy…

   Agree, added something similar.

8. 
   theStack commented at 12:34 pm on April 25, 2022: member

   Concept ACK
9. 
   dongcarl commented at 3:30 pm on May 11, 2022: member

   @MarcoFalke @JeremyRubin would you be able to chime in on the correctness of this added function? I’m not too familiar with mempool usage. https://github.com/bitcoin/bitcoin/blob/e51f412d0a0bb532774fc2a68c596f730878b470/src/test/util/setup_common.cpp#L358-L393
10. dongcarl added this to the "WIP PRs" column in a project

    ---

11. dongcarl moved this from the "WIP PRs" to the "Ready for Review PRs" column in a project

    ---

12. dongcarl moved this from the "Ready for Review PRs" to the "Relevant External PRs" column in a project

    ---

13. 
    MarcoFalke commented at 3:44 pm on May 11, 2022: member

    Probably not correct, see CI:

    0SUMMARY: AddressSanitizer: heap-use-after-free (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/bench/bench_bitcoin+0xcd7816) (BuildId: 106aeeccd13eff54c783adff615bd4c5272e8a95) in __asan_memcpy
    

14. glozow force-pushed on May 25, 2022
15. 
    glozow commented at 3:07 pm on May 26, 2022: member

    Probably not correct, see CI:

    should be fixed now

16. in src/test/util/setup_common.cpp:363 in d1dcb410b8 outdated

    354@@ -355,6 +355,43 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio
    355     return mempool_txn;
    356 }
    357 
    358+std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit)
    359+{
    360+    std::vector<CTransactionRef> mempool_transactions;
    361+    std::deque<std::pair<COutPoint, CAmount>> unspent_prevouts;
    362+    std::transform(m_coinbase_txns.begin(), m_coinbase_txns.end(), std::back_inserter(unspent_prevouts),
    363+        [](const auto& tx){ return std::make_pair(COutPoint(tx->GetHash(), 0), 50 * COIN); });
    

    ---

    ---

    

    JeremyRubin commented at 5:02 pm on May 26, 2022:

    This is not “correct” necessarily – you should inspect thetx->vout[0].value and not guess 50 coins.

    ---

    

    glozow commented at 8:33 pm on May 30, 2022:

    Sure, fixed.
17. in src/test/util/setup_common.cpp:365 in d1dcb410b8 outdated

    360+    std::vector<CTransactionRef> mempool_transactions;
    361+    std::deque<std::pair<COutPoint, CAmount>> unspent_prevouts;
    362+    std::transform(m_coinbase_txns.begin(), m_coinbase_txns.end(), std::back_inserter(unspent_prevouts),
    363+        [](const auto& tx){ return std::make_pair(COutPoint(tx->GetHash(), 0), 50 * COIN); });
    364+    CScript witnessScript = CScript() << OP_DROP << OP_TRUE;
    365+    CScript scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
    

    ---

    ---

    

    JeremyRubin commented at 5:05 pm on May 26, 2022:

    seems to be unused?

    ---

    

    glozow commented at 8:33 pm on May 30, 2022:

    Thanks, fixed.
18. in src/test/util/setup_common.cpp:369 in d1dcb410b8 outdated

    364+    CScript witnessScript = CScript() << OP_DROP << OP_TRUE;
    365+    CScript scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
    366+    while (num_transactions > 0 && !unspent_prevouts.empty()) {
    367+        CMutableTransaction mtx = CMutableTransaction();
    368+        const auto& [prevout, amount] = unspent_prevouts.front();
    369+        mtx.vin.push_back(CTxIn(prevout, CScript()));
    

    ---

    ---

    

    JeremyRubin commented at 5:07 pm on May 26, 2022:

    Should you set the scriptWitness on vin.back()?

    ---

    

    glozow commented at 8:30 pm on May 30, 2022:

    vin is initialized to be empty with the default CMutableTransaction ctor, so vin.back() cannot be dereferenced.

    ---

    

    JeremyRubin commented at 2:57 am on May 31, 2022:

    but you just push back’d vin, so it is initialized and dereferencable…
19. in src/test/util/setup_common.cpp:387 in d1dcb410b8 outdated

    376+            CScript spk = CScript() << CScriptNum(num_transactions + n);
    377+            mtx.vout.push_back(CTxOut(amount_per_output, spk));
    378+        }
    379+        unspent_prevouts.pop_front();
    380+        CTransactionRef ptx = MakeTransactionRef(mtx);
    381+        mempool_transactions.push_back(ptx);
    

    ---

    ---

    

    JeremyRubin commented at 5:09 pm on May 26, 2022:

    would it be better to do a swap with random index after this push_back?

    That way you get a bit more of a ‘arbitrary’ txn graph, v.s. this one which will have a very specific structure.

    ---

    

    glozow commented at 8:42 pm on May 30, 2022:

    The order of mempool_transactions will not affect the contents of the resulting mempool. Random swapping may result in a vector that is no longer topologically sorted, so the caller may have trouble submitting them.

    ---

    

    glozow commented at 9:54 pm on May 30, 2022:

    Oh, I think you mean random swap for unspent_prevouts after insert? Added that. I think we have a decent amount of randomness in the structure that way.

    ---

    

    JeremyRubin commented at 2:58 am on May 31, 2022:

    correct. random swap with unspent_prevouts.
20. glozow force-pushed on May 30, 2022
21. [test util] add chain name to TestChain100Setup ctor

    This allows calling MakeNoLogFileContext<TestChain100Setup>

    d7d9c7b266
22. [test util] use -checkmempool for TestingSetup mempool check ratio 5374dfc4e3
23. [test util] to populate mempool with random transactions/packages 2118750631
24. create and use mempool transactions using real coins in MempoolCheck aecc332a71
25. use testing setup mempool in ComplexMemPool bench d2f8f1b307
26. glozow force-pushed on May 30, 2022
27. 
    glozow commented at 4:15 am on May 31, 2022: member

    Addressed @JeremyRubin’s comments and made the graph more random. Ran the bench 100 times, doesn’t seem like the extra randomness increases the variance of the bench results which is good. Also rebased to resolve CI issue.
28. 
    laanwj commented at 1:22 pm on June 2, 2022: member

    Code review ACK d2f8f1b307b056d1a54fb02a99da2cb664570904

    Ran the bench 100 times, doesn’t seem like the extra randomness increases the variance of the bench results which is good

    It shouldn’t, right, because of the deterministic random context it will do the same every run? But good to check, of course.

29. laanwj referenced this in commit fe28aa8cc6 on Jun 2, 2022
30. laanwj merged this on Jun 2, 2022
31. laanwj closed this on Jun 2, 2022

    ---

32. laanwj moved this from the "Relevant External PRs" to the "Done or Closed or Rethinking" column in a project

    ---

33. 
    glozow commented at 0:15 am on June 3, 2022: member

    It shouldn’t, right, because of the deterministic random context it will do the same every run? But good to check, of course.

    Ah my bad, I didn’t realize that! Thanks!

34. glozow deleted the branch on Jun 3, 2022
35. sidhujag referenced this in commit 7064748e84 on Jun 3, 2022
36. laanwj referenced this in commit 489b587669 on Jun 16, 2022
37. DrahtBot locked this on Jun 3, 2023

Contributors
glozow dongcarl DrahtBot theStack MarcoFalke JeremyRubin laanwj

Labels
Tests