Skip to content

Conversation

glozow
Copy link
Member

@glozow glozow commented Apr 19, 2022

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.

@dongcarl
Copy link
Contributor

Concept ACK

wondering if we shouldn't copy:

int check_ratio = std::min<int>(std::max<int>(args.GetIntArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000);

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

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

The mempool will still be checked.

@DrahtBot DrahtBot added the Tests label Apr 20, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 20, 2022

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.

@glozow glozow force-pushed the 2022-04-test-util-populate-mempool branch from fbeff51 to e51f412 Compare April 20, 2022 21:30
@glozow
Copy link
Member Author

glozow commented Apr 20, 2022

wondering if we shouldn't copy...

Agree, added something similar.

@theStack
Copy link
Contributor

Concept ACK

@dongcarl
Copy link
Contributor

@MarcoFalke @JeremyRubin would you be able to chime in on the correctness of this added function? I'm not too familiar with mempool usage.

std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit)
{
std::vector<CTransactionRef> mempool_transactions;
std::deque<std::pair<COutPoint, CAmount>> unspent_prevouts;
std::transform(m_coinbase_txns.begin(), m_coinbase_txns.end(), std::back_inserter(unspent_prevouts),
[](const auto& tx){ return std::make_pair(COutPoint(tx->GetHash(), 0), 50 * COIN); });
CScript witnessScript = CScript() << OP_DROP << OP_TRUE;
CScript scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
while (num_transactions > 0 && !unspent_prevouts.empty()) {
CMutableTransaction mtx = CMutableTransaction();
const auto& [prevout, amount] = unspent_prevouts.front();
unspent_prevouts.pop_front();
mtx.vin.push_back(CTxIn(prevout, CScript()));
// Create a 1-input, n-output transaction.
// The number of outputs is random, between 1 and 24.
const size_t num_outputs = det_rand.randrange(24) + 1;
// Approximately 1000sat "fee," equal output amounts.
const CAmount amount_per_output = (amount - 1000) / num_outputs;
for (size_t n{0}; n < num_outputs; ++n) {
CScript spk = CScript() << CScriptNum(num_transactions + n);
mtx.vout.push_back(CTxOut(amount_per_output, spk));
}
CTransactionRef ptx = MakeTransactionRef(mtx);
mempool_transactions.push_back(ptx);
for (size_t n{0}; n < num_outputs; ++n) {
unspent_prevouts.push_back(std::make_pair(COutPoint(ptx->GetHash(), n), amount_per_output));
}
if (submit) {
LOCK2(m_node.mempool->cs, cs_main);
LockPoints lp;
m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, 1000, 0, 1, false, 4, lp));
}
--num_transactions;
}
return mempool_transactions;
}

@maflcko
Copy link
Member

maflcko commented May 11, 2022

Probably not correct, see CI:

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

@glozow
Copy link
Member Author

glozow commented May 26, 2022

Probably not correct, see CI:

should be fixed now

@glozow glozow force-pushed the 2022-04-test-util-populate-mempool branch from d1dcb41 to e69bfa3 Compare May 30, 2022 21:54
@glozow glozow force-pushed the 2022-04-test-util-populate-mempool branch from e69bfa3 to d2f8f1b Compare May 30, 2022 23:00
@glozow
Copy link
Member Author

glozow commented May 31, 2022

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.

@laanwj
Copy link
Member

laanwj commented Jun 2, 2022

Code review ACK d2f8f1b

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.

laanwj added a commit to laanwj/bitcoin-core-gui that referenced this pull request Jun 2, 2022
…andom transactions, fix #24634 bug

d2f8f1b use testing setup mempool in ComplexMemPool bench (glozow)
aecc332 create and use mempool transactions using real coins in MempoolCheck (glozow)
2118750 [test util] to populate mempool with random transactions/packages (glozow)
5374dfc [test util] use -checkmempool for TestingSetup mempool check ratio (glozow)
d7d9c7b [test util] add chain name to TestChain100Setup ctor (glozow)

Pull request description:

  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.
@laanwj laanwj merged commit a100c42 into bitcoin:master Jun 2, 2022
@glozow
Copy link
Member Author

glozow commented Jun 3, 2022

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!

@glozow glozow deleted the 2022-04-test-util-populate-mempool branch June 3, 2022 00:16
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 3, 2022
laanwj added a commit that referenced this pull request Jun 16, 2022
d273e53 bench/rpc_mempool: Create ChainTestingSetup, use its CTxMemPool (Carl Dong)
020caba bench: Use existing CTxMemPool in TestingSetup (Carl Dong)
86e732d scripted-diff: test: Use CTxMemPool in TestingSetup (Carl Dong)
213457e test/policyestimator: Use ChainTestingSetup's CTxMemPool (Carl Dong)
319f0ce rest/getutxos: Don't construct empty mempool (Carl Dong)
03574b9 tree-wide: clang-format CTxMemPool references (Carl Dong)

Pull request description:

  This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18

  This PR reduces the number of call sites where we explicitly construct CTxMemPool. This is done in preparation for later PRs which decouple the mempool module from `ArgsManager`, eventually all of libbitcoinkernel will be decoupled from `ArgsManager`.

  The changes in this PR:

  - Allows us to have less code churn as we modify `CTxMemPool`'s constructor in later PRs
  - In many cases, we can make use of existing `CTxMemPool` instances, getting rid of extraneous constructions
  - In other cases, we construct a `ChainTestingSetup` and use the `CTxMemPool` there, so that we can rely on the logic in `setup_common` to set things up correctly

  ## Notes for Reviewers

  ### A note on using existing mempools

  When evaluating whether or not it's appropriate to use an existing mempool in a `*TestingSetup` struct, the key is to make sure that the mempool has the same lifetime as the `*TestingSetup` struct.

  Example 1: In [`src/fuzz/tx_pool.cpp`](https://github.com/bitcoin/bitcoin/blob/b4f686952a60bbadc7ed2250651d0d6af0959f4d/src/test/fuzz/tx_pool.cpp), the `TestingSetup` is initialized in `initialize_tx_pool` and lives as a static global, while the `CTxMemPool` is in the `tx_pool_standard` fuzz target, meaning that each time the `tx_pool_standard` fuzz target gets run, a new `CTxMemPool` is created. If we were to use the static global `TestingSetup`'s CTxMemPool we might run into problems since its `CTxMemPool` will carry state between subsequent runs. This is why we don't modify `src/fuzz/tx_pool.cpp` in this PR.

  Example 2: In [`src/bench/mempool_eviction.cpp`](https://github.com/bitcoin/bitcoin/blob/b4f686952a60bbadc7ed2250651d0d6af0959f4d/src/bench/mempool_eviction.cpp), we see that the `TestingSetup` is in the same scope as the constructed `CTxMemPool`, so it is safe to use its `CTxMemPool`.

  ### A note on checking `CTxMemPool` ctor call sites

  After the "tree-wide: clang-format CTxMemPool references" commit, you can find all `CTxMemPool` ctor call sites with the following command:

  ```sh
  git grep -E -e 'make_unique<CTxMemPool>' \
              -e '\bCTxMemPool\s+[^({;]+[({]' \
              -e '\bCTxMemPool\s+[^;]+;' \
              -e '\bnew\s+CTxMemPool\b'
  ```

  At the end of the PR, you will find that there are still quite a few call sites that we can seemingly get rid of:

  ```sh
  $ git grep -E -e 'make_unique<CTxMemPool>' -e '\bCTxMemPool\s+[^({;]+[({]' -e '\bCTxMemPool\s+[^;]+;' -e '\bnew\s+CTxMemPool\b'
  # rearranged for easier explication
  src/init.cpp:        node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
  src/test/util/setup_common.cpp:    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
  src/rpc/mining.cpp:        CTxMemPool empty_mempool;
  src/test/util/setup_common.cpp:    CTxMemPool empty_pool;
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  src/test/fuzz/rbf.cpp:    CTxMemPool pool;
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/validation_load_mempool.cpp:    CTxMemPool pool{};
  src/txmempool.h:    /** Create a new CTxMemPool.
  ```
  Let's break them down one by one:

  ```
  src/init.cpp:        node.mempool = std::make_unique<CTxMemPool>(node.fee_estimator.get(), mempool_check_ratio);
  src/test/util/setup_common.cpp:    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
  ```

  Necessary

  -----

  ```
  src/rpc/mining.cpp:        CTxMemPool empty_mempool;
  src/test/util/setup_common.cpp:    CTxMemPool empty_pool;
  ```

  These are fixed in #25223 where we stop requiring the `BlockAssembler` to have a `CTxMemPool` if it's not going to consult it anyway (as is the case in these two call sites)

  -----

  ```
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  src/bench/mempool_stress.cpp:    CTxMemPool pool;
  ```

  Fixed in #24927.

  -----

  ```
  src/test/fuzz/rbf.cpp:    CTxMemPool pool;
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/tx_pool.cpp:    CTxMemPool tx_pool_{/*estimator=*/nullptr, /*check_ratio=*/1};
  src/test/fuzz/validation_load_mempool.cpp:    CTxMemPool pool{};
  ```

  These are all cases where we don't want the `CTxMemPool` state to persist between runs, see the previous section "A note on using existing mempools"

  -----

  ```
  src/txmempool.h:    /** Create a new CTxMemPool.
  ```

  It's a comment (someone link me to a grep that understands syntax plz thx)

ACKs for top commit:
  laanwj:
    Code review ACK d273e53

Tree-SHA512: c4ff3d23217a7cc4a7145defc7b901725073ef73bcac3a252ed75f672c87e98ca0368d1d8c3f606b5b49f641e7d8387d26ef802141b650b215876f191fb6d5f9
@bitcoin bitcoin locked and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Status: Done or Closed or Rethinking
Development

Successfully merging this pull request may close these issues.

bench: MempoolCheck actually runs with check_ratio = 0
7 participants