This PR adds a new benchmark for adding/evicting orphan transactions to/from the mapOrphanTransactions.
This benchmark could be useful while considering relevant code changes (e.g., #19374) or changing the DEFAULT_MAX_ORPHAN_TRANSACTIONS parameter.
The output example:
0$ ./src/bench/bench_bitcoin -filter="OrphanTxPool"
1# Benchmark, evals, iterations, total, min, max, median
2OrphanTxPool, 5, 10000, 2.3938, 4.76101e-05, 4.83722e-05, 4.77407e-0
LimitOrphanTxSize() which is only called after the 101st orphan has been added. Picking a random element from a 101-entry container is always going to be fast, whatever the underlying data structure, so I don’t think there’s any benefit to having a microbenchmark for this. On the other hand, there is a cost to adding it - it means that there’s more work involved in changing or reviewing changes to the interface.
microbenchmarks are never going to match the exact usage pattern in the end product. This is true for all existing microbenchmarks (like the mempool eviction one, etc …). Absent the details, I think having a benchmark is slightly preferable to not having a benchmark because it might catch accidental performance regressions.
And since we don’t have any user-facing compatibility concerns for tests in general, adding and removing tests can be done much more easily than (let’s say) an RPC method. In general I think that the cost of a pull request that adds a test that passes can only be negative or null (when it is testing nothing). And it should always be an uncontroversial option to remove a not-too-useful test that is in the way of other changes/refactoring/features.
I haven’t looked at the specifics of this pull request, but I think in general we need more benchmarks (or fuzzers) for p2p tx relay, as that is the primary DoS attack vector on the live network.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
In general I think that the cost of a pull request that adds a test that passes can only be negative or null (when it is testing nothing). And it should always be an uncontroversial option to remove a not-too-useful test that is in the way of other changes/refactoring/features.
I’m sorry - this is wrong. All code added to the repo has a cost - in maintenance, in review, in discussion as to whether to remove it later. Tests/benches should only be added if they’re useful.
1018@@ -1018,6 +1019,13 @@ unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
1019 return nEvicted;
1020 }
1021
1022+unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans)
1023+{
1024+ LOCK(g_cs_orphans);
1025+ SweepOutExpiredOrphans();
1026+ return EvictOrphanTxs(nMaxOrphans);
1027+}
To benchmark EvictOrphanTxs() without sweeping out functionality of the current LimitOrphanTxSize().
What could you suggest to change?
12+#include <script/script.h>
13+#include <sync.h>
14+
15+#include <vector>
16+
17+static constexpr CAmount CENT{1000000};
14+
15+#include <vector>
16+
17+static constexpr CAmount CENT{1000000};
18+extern bool AddOrphanTx(const CTransactionRef& tx, NodeId peer);
19+extern unsigned int EvictOrphanTxs(unsigned int nMaxOrphans);
extern needed here?
Updated 059c8e6b31d55cad2ccf1dbb1cec0163cd58108d -> 2ee8c0b4be9e524f51572aba9ce4aafccbc0efef (pr19377.01 -> pr19377.02, diff):
32+ tx.vout[0].scriptPubKey << OP_1;
33+ txs.emplace_back(MakeTransactionRef(tx));
34+ }
35+
36+ bench.batch(txs.size()).unit("byte").run([&] {
37+ WITH_LOCK(g_cs_orphans, for (const auto& tx : txs) { AddOrphanTx(tx, 0); });
Concept ACK. Had a look as this “conflicts” with #22284, rebased it to current master and began building; it needs updating here and line 17 above:
0bench/orphan_tx_pool.cpp:17:83: error: use of undeclared identifier 'g_cs_orphans'
1bool AddOrphanTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
2 ^
3bench/orphan_tx_pool.cpp:37:19: error: use of undeclared identifier 'g_cs_orphans'
4 WITH_LOCK(g_cs_orphans, for (const auto& tx : txs) { AddOrphanTx(tx, 0); });
5 ^
6bench/orphan_tx_pool.cpp:37:19: error: use of undeclared identifier 'g_cs_orphans'
73 errors generated.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.