bench: BlockAssembler on a mempool with packages

glozow commented at 4:38 pm on December 13, 2022: member

Performance of block template building matters as miners likely want to be able to start mining on a block with transactions asap after a block is found. We would want to know if a mempool PR accidentally caused, for example, a 100x slowdown. An AssembleBlock() bench exists, but it operates on a mempool with 101 transactions, each with 0 ancestors or descendants and with the same fee. Adding a bench with a more complex mempool is useful because (1) it’s more realistic (2) updating packages can potentially cause the algorithm to take a long time.

glozow added the label Tests on Dec 13, 2022

DrahtBot commented at 4:38 pm on December 13, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, kevkevinpal, achow101
Concept ACK	pablomartin4btc
Stale ACK	jamesob, Emzy, hernanmarino

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

in src/test/util/mining.h:27 in 3f550b0887 outdated

23@@ -24,7 +24,7 @@ std::vector<std::shared_ptr<CBlock>> CreateBlockChain(size_t total_height, const
24 CTxIn MineBlock(const node::NodeContext&, const CScript& coinbase_scriptPubKey);
25 
26 /** Prepare a block to be mined */
27-std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext&, const CScript& coinbase_scriptPubKey);
28+std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext&, const CScript& coinbase_scriptPubKey, bool check);

jamesob commented at 9:08 pm on December 13, 2022:

https://github.com/bitcoin/bitcoin/pull/26695/commits/3f550b0887db0007633a26aac1dd80ec4723be55

Commit doesn’t compile; looks like the PrepareBlock() call in block_assemble.cpp needs to be updated:

0bench/block_assemble.cpp:45:9: error: no matching function for call to 'PrepareBlock'
1        PrepareBlock(test_setup->m_node, P2WSH_OP_TRUE);
2        ^~~~~~~~~~~~
3./test/util/mining.h:27:25: note: candidate function not viable: requires 3 arguments, but 2 were provided
4std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext&, const CScript& coinbase_scriptPubKey, bool check);
5                        ^
61 error generated.

glozow commented at 10:15 am on December 14, 2022:

Fixed

stickies-v commented at 1:36 pm on December 14, 2022:

check is kinda vague, what abouttest_validity? Also, default true seems appropriate?

0std::shared_ptr<CBlock> PrepareBlock(const node::NodeContext&, const CScript& coinbase_scriptPubKey, bool test_validity);

glozow commented at 2:20 pm on December 15, 2022:

This is no longer a bool, so resolving

jamesob commented at 9:11 pm on December 13, 2022: member

Approach ACK

Change and idea are good, small issue with the first commit.

maflcko removed the label Tests on Dec 14, 2022

DrahtBot added the label Tests on Dec 14, 2022

glozow force-pushed on Dec 14, 2022

jamesob approved

jamesob commented at 12:53 pm on December 14, 2022: member

ACK 4c20bee65009bd2c95c2a3426030b042f55007ad (jamesob/ackr/26695.2.glozow.bench_blockassembler_on)

Nice! Good thing to have.

Could possibly later populate the mempool with a more stressful load, something like 10,000 transactions?

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 4c20bee65009bd2c95c2a3426030b042f55007ad ([`jamesob/ackr/26695.2.glozow.bench_blockassembler_on`](https://github.com/jamesob/bitcoin/tree/ackr/26695.2.glozow.bench_blockassembler_on))
 4
 5Nice! Good thing to have.
 6
 7Could possibly later populate the mempool with a more stressful load, something
 8like 10,000 transactions? 
 9-----BEGIN PGP SIGNATURE-----
10
11iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmOZx4sACgkQepNdrbLE
12TwVdwA//bHGKG97lUP5JPiknjulnwVzPvfPyXJi8azZQ2XoK3T1HEA/kJo31wHrY
132mdRgis+kst/emY7wfu/LAAlc06M8cKt5DuPfEgJIPzkidV8JAePMvw8QpDgbd/K
14mTu+6mdeGq5Mspy0eS2ItsQEeC4NvZeQSIIf9yCSIhlnCBmeshqBEydN3tduW6Ot
15nr6RKGizFO7yJ1ecWX+UA9eYNUYUz4isLehBgoyTh5hz0IJobWIFRmzVCXUQci0y
16S3WxFf4RlGnJpWMOOwd/6WZHAdsFOtZ6T4Nux9HEC7nboxb+pfib47CvqCWc0kG3
174mAsJtxasIna/0HTMFD9gApOteG3NByNDKfOkhG1gZYu8tnRA6wm+61WKX6BgJT9
18MHHd4WMlov9ye4YaQQazpV8ApXqbjMbFpOLxSeCn3vad/bXo4IWq2x+rpQZTpLJs
19+pPNTvqahtk96njeqZcukbnu+JYuEbst4sSlOGMd29fV52M2lyOICK5Jt0PgjfBR
20f/lZXhNdd7+1ydj5XOkKE4RAqaQhEf1FYhvR0u+HmFcIVUsEfIYQkvB4z89j28KI
21hFGQD3eFPxFPdxBV32GMWHKQ52KBgvzL0Hbbe4a7D/FSbE8YAnRhFXx3jd4AhlAV
228xCDcN/eAthz45ZsTvvoF6NagwRfoXQoP4jzjzFIsNDGD0CKri0=
23=cB3y
24-----END PGP SIGNATURE-----

0Tested on Linux-6.0.11-arch1-1-x86_64-with-glibc2.36
1
2Configured with ./configure --without-bdb 'CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare ' --disable-gui-tests --disable-fuzz-binary --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare  -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha  i
5
6Compiler version: clang version 14.0.6

in src/test/util/mining.cpp:79 in b42cd786d0 outdated

73@@ -74,10 +74,12 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
74     return CTxIn{block->vtx[0]->GetHash(), 0};
75 }
76 
77-std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
78+std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey, bool check)
79 {
80+    BlockAssembler::Options options;

maflcko commented at 1:05 pm on December 14, 2022:

not sure if it matters, but previously this was (implicitly) calling DefaultOptions, now it calls Options.

glozow commented at 12:05 pm on December 15, 2022:

True, there’s no difference here but it means the testing setup ignores -blockmintxfee and -blockmaxweight options when calling PrepareBlock().

glozow commented at 3:54 pm on December 15, 2022:

Added an ApplyArgsManOptions so we can both manually manipulate the options and get the config from ArgsManager.

in src/test/util/mining.cpp:77 in b42cd786d0 outdated

73@@ -74,10 +74,12 @@ CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
74     return CTxIn{block->vtx[0]->GetHash(), 0};
75 }
76 
77-std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
78+std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey, bool check)

maflcko commented at 1:14 pm on December 14, 2022:

nit: Could pass in the options struct to avoid having to change this again if another option is added? Could also add a redirect function without the argument, similar to BlockAssembler, to avoid having to change existing call sites?

Feel free to ignore, but then you could use clang-tidy named-args to at least document the bool literals.

glozow commented at 3:51 pm on December 15, 2022:

Good idea, did this instead of the bool

in src/bench/block_assemble.cpp:54 in 4c20bee650 outdated

49+{
50+    FastRandomContext det_rand{true};
51+    auto testing_setup = MakeNoLogFileContext<TestChain100Setup>();
52+    CTxMemPool& pool = *testing_setup.get()->m_node.mempool;
53+    LOCK2(cs_main, pool.cs);
54+    const auto transactions = testing_setup->PopulateMempool(det_rand, 1000, true);

maflcko commented at 1:16 pm on December 14, 2022:

nit: could use clang-tidy named args?

glozow commented at 3:50 pm on December 15, 2022:

Added

in src/bench/block_assemble.cpp:53 in 4c20bee650 outdated

44@@ -45,5 +45,18 @@ static void AssembleBlock(benchmark::Bench& bench)
45         PrepareBlock(test_setup->m_node, P2WSH_OP_TRUE, true);
46     });
47 }
48+static void BlockAssemblerAddPackageTxns(benchmark::Bench& bench)
49+{
50+    FastRandomContext det_rand{true};
51+    auto testing_setup = MakeNoLogFileContext<TestChain100Setup>();
52+    CTxMemPool& pool = *testing_setup.get()->m_node.mempool;
53+    LOCK2(cs_main, pool.cs);

maflcko commented at 1:18 pm on December 14, 2022:

nit: why?

glozow commented at 3:50 pm on December 15, 2022:

Removed, thanks

maflcko approved

maflcko commented at 1:18 pm on December 14, 2022: member

lgtm. Left some nits

in src/test/util/mining.cpp:64 in 4c20bee650 outdated

60@@ -61,7 +61,7 @@ std::vector<std::shared_ptr<CBlock>> CreateBlockChain(size_t total_height, const
61 
62 CTxIn MineBlock(const NodeContext& node, const CScript& coinbase_scriptPubKey)
63 {
64-    auto block = PrepareBlock(node, coinbase_scriptPubKey);
65+    auto block = PrepareBlock(node, coinbase_scriptPubKey, true);

stickies-v commented at 1:33 pm on December 14, 2022:

0    auto block{PrepareBlock(node, coinbase_scriptPubKey, /*check=*/true)};

glozow commented at 3:51 pm on December 15, 2022:

Line is removed now

in src/node/miner.h:140 in 4c20bee650 outdated

134@@ -135,6 +135,9 @@ class BlockAssembler
135     unsigned int nBlockMaxWeight;
136     CFeeRate blockMinFeeRate;
137 
138+    // Whether to call TestBlockValidity() at the end of CreateNewBlock().
139+    const bool test_block_validity;

stickies-v commented at 1:38 pm on December 14, 2022:

Perhaps out of scope for this PR but… We could avoid this duplication by just adding Options as a member to BlockAssembler?

glozow commented at 12:08 pm on December 15, 2022:

Fwiw since you brought it up - I think it’s better to keep Options as an argument for ctoring BlockAssembler rather than a member, as I think it would be the best way to decouple BlockAssembler from ArgsManager (e.g. something like https://github.com/bitcoin/bitcoin/pull/25290/commits/f1941e8bfd2eecc478c7660434b1ebf6a64095a0).

~~Looking back to when they were first separated in #9868, it seems not accessing gArgs from the BlockAssembler ctor was a motivation there as well.~~

Ah I misunderstood and thought you were suggesting we remove the struct entirely, apologies!

in src/node/miner.cpp:176 in 4c20bee650 outdated

172@@ -170,7 +173,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
173     pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
174 
175     BlockValidationState state;
176-    if (!TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, GetAdjustedTime, false, false)) {
177+    if (test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, GetAdjustedTime, false, false)) {

stickies-v commented at 1:42 pm on December 14, 2022:

nit: while touching, would be nice to add parameter hints

0    if (test_block_validity && !TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, GetAdjustedTime, /*fCheckPOW=*/false, /*fCheckMerkleRoot=*/false)) {

glozow commented at 3:59 pm on December 15, 2022:

Done

in src/test/util/setup_common.cpp:400 in 4c20bee650 outdated

396@@ -397,15 +397,15 @@ std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContex
397             unspent_prevouts.pop_front();
398         }
399         const size_t num_outputs = det_rand.randrange(24) + 1;
400-        // Approximately 1000sat "fee," equal output amounts.
401-        const CAmount amount_per_output = (total_in - 1000) / num_outputs;
402+        const CAmount fee = 100 * det_rand.randrange(30);

stickies-v commented at 1:49 pm on December 14, 2022:

nit

0        const CAmount fee{100 * det_rand.randrange(30)};

stickies-v commented at 2:20 pm on December 14, 2022:

it’s not immediately obvious to me what the benefit is of randomizing the fee, perhaps useful to add rationale to the commit msg?

glozow commented at 3:52 pm on December 15, 2022:

Updated commit message. This is a narrowing conversion - randrange returns uint64_t, CAmount is a 32.

in src/bench/block_assemble.cpp:54 in 4c20bee650 outdated

50+{
51+    FastRandomContext det_rand{true};
52+    auto testing_setup = MakeNoLogFileContext<TestChain100Setup>();
53+    CTxMemPool& pool = *testing_setup.get()->m_node.mempool;
54+    LOCK2(cs_main, pool.cs);
55+    const auto transactions = testing_setup->PopulateMempool(det_rand, 1000, true);

stickies-v commented at 2:22 pm on December 14, 2022:

nit

0    auto testing_setup{MakeNoLogFileContext<TestChain100Setup>()};
1    CTxMemPool& pool{*testing_setup.get()->m_node.mempool};
2    LOCK2(cs_main, pool.cs);
3    const auto transactions{testing_setup->PopulateMempool(det_rand, 1000, true)};

glozow commented at 3:50 pm on December 15, 2022:

Some taken, most lines deleted

in src/bench/block_assemble.cpp:63 in 4c20bee650 outdated

58+        PrepareBlock(testing_setup->m_node, P2WSH_OP_TRUE, false);
59     });
60 }
61 
62 BENCHMARK(AssembleBlock, benchmark::PriorityLevel::HIGH);
63+BENCHMARK(BlockAssemblerAddPackageTxns, benchmark::PriorityLevel::HIGH);

stickies-v commented at 2:57 pm on December 14, 2022:

This is among the slowest benchmarks in the suite atm. On my machine, --sanity-check takes ~1.2s. I think that’s an acceptable additional slowdown for make check, but perhaps we should consider downgrading this to PriorityLevel::MEDIUM?

kevkevinpal commented at 2:02 am on December 21, 2022:

this is what I get when trying to run the BlockAssemblerAddPackageTxns benchmark

Specs:

0mac book pro 2019
12.3 GHz 8-Core Intel Core i9
216 GB 2667 MHz DDR4
3AMD Radeon Pro 5500M 4 GB

Results:

0./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns --sanity-check
1Running with --sanity-check option, benchmark results will be useless.
2
3|               ns/op |                op/s |    err% |     total | benchmark
4|--------------------:|--------------------:|--------:|----------:|:----------
5|      813,714,108.00 |                1.23 |    0.0% |      0.81 | `BlockAssemblerAddPackageTxns`

Without --sanity-check:

0./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns
1
2|               ns/op |                op/s |    err% |     total | benchmark
3|--------------------:|--------------------:|--------:|----------:|:----------
4|      830,079,779.00 |                1.20 |    1.4% |      9.16 | `BlockAssemblerAddPackageTxns`

achow101 commented at 11:31 pm on December 21, 2022:

When compiled with --enable-debug, this slows down make check by ~30s for me. When run without --sanity-check, it takes ~280s. I think this needs to be a low priority benchmark.

glozow commented at 11:56 am on December 22, 2022:

Changed to low, I think we don’t have medium? Is priority based on time only, and if so at what time threshold would we want to knock the priority down?

stickies-v commented at 3:20 pm on December 22, 2022:

Yeah sorry, we had medium in the initial PR but it was removed later on, so agreed with changing it to low. We haven’t defined any guidelines on when to downgrade to low afaik, but since devs run make check often, I think adding benches with meaningful (e.g. >1s?) --sanity-check overhead should run in low priority - unless their complexity/importance warrants otherwise.

stickies-v commented at 11:57 am on December 15, 2022: contributor

Concept ACK - nice to have this kind of benchmark, will be helpful in e.g. #26289

glozow force-pushed on Dec 15, 2022

glozow force-pushed on Dec 16, 2022

jamesob approved

jamesob commented at 5:05 pm on December 16, 2022: member

ACK 628ce92c494c6d3b6181d2ee6f64494de38a67e3 (jamesob/ackr/26695.3.glozow.bench_blockassembler_on)

re-ACK

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 628ce92c494c6d3b6181d2ee6f64494de38a67e3 ([`jamesob/ackr/26695.3.glozow.bench_blockassembler_on`](https://github.com/jamesob/bitcoin/tree/ackr/26695.3.glozow.bench_blockassembler_on))
 4
 5re-ACK
 6-----BEGIN PGP SIGNATURE-----
 7
 8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmOcpWIACgkQepNdrbLE
 9TwW/5w/+MpLKrQ5eLWc7uXQ2P1ML82Q1EKDXzGC4bmBFZZ2t7Ynj78enPUf1ukD2
10o8hT28touxw5XSWF2Zcwjhh+DRWU6sqYJInpOXhW1366iqS2DcvDQZOctwEEUasO
11RM6lcRfNNvz4FyuiV4Cfn/liVHK6ipCQoPY88mjyy/I32XzwB6wXKNopRCV8GfFV
12pIU10BM9tVobwHJhFRvTQve3nhwXjIbwJ0Vn9peUqKRSmuxDkFf2R6l/3YUwEFjk
13iPJQ+SW4fo0h6bDThZ8GOfT8F4u52cnsx97zOck9f6wmehQoKUDIj6s2Epa/n/0q
14WZXx6+xI2RS3GuS7fM9et1+0H8TmyHhkvSpzkSiOg+npWBqPl+8tCgqQftUS4s+D
15HY/BoP1arD2nHsQ/yBksymASmnFKMyR0RWz8r8JheUY3gGXRDCQZkUCkVMMjpV3O
16tOIx6k1h8dfyaJqxW2e6o3r2dNho79Ap2nQS7UnRfFu80KebgyV0SZYusK3+OT4a
17Wkm4VBO3l36BQJwOBmu6nQ3mpbgF4TvQOhj9F65GUaLrsAKrNn/YrcnV2U+g/RmS
18JaXsSUio7DukkYccLLLOh9WvYOXkxohIXNA7DBpk+VES8PBDN5Q5ZBuCmJSRt0HC
19VwSxSbNA4Pedk6pmaebOAgTUcQxY9yJOaBQLEqyhR3SRE4YgrVQ=
20=rMYG
21-----END PGP SIGNATURE-----

0Tested on Linux-6.0.11-arch1-1-x86_64-with-glibc2.36
1
2Configured with ./configure --without-bdb 'CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare ' --disable-gui-tests --disable-fuzz-binary --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/bin/clang++ CC=/usr/bin/clang PYTHONPATH= --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare  -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha  i
5
6Compiler version: clang version 14.0.6

jamesob approved

jamesob commented at 5:09 pm on December 16, 2022: member

ACK b05347204d2d1cad030fc794aae2b0d29c35db7d (jamesob/ackr/26695.4.glozow.bench_blockassembler_on)

Oops, previous ACK already stale. New ACK incorporates

0-            m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, 1000, 0, 1, false, 4, lp));
1+            m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, /*fee=*/(total_in - num_outputs * amount_per_output),
2+                                                         /*time=*/0, /*entry_height=*/1,
3+                                                         /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
4         }
5         --num_transactions;
6     }

glozow commented at 5:10 pm on December 16, 2022: member

Yes apologies @jamesob, I noticed I was making the tx fee random but still setting the mempool entry fee to 1000. Thanks for such a quick review!

in src/bench/block_assemble.cpp:55 in b05347204d outdated

50+{
51+    FastRandomContext det_rand{true};
52+    auto testing_setup{MakeNoLogFileContext<TestChain100Setup>()};
53+    testing_setup->PopulateMempool(det_rand, /*num_transactions=*/1000, /*submit=*/true);
54+    node::BlockAssembler::Options assembler_options;
55+    assembler_options.test_block_validity = false;

maflcko commented at 9:46 am on December 19, 2022:

Is there any value in disabling it? Sure, the benchmark will be less “micro”, but at the same time it would be missing if a “package” block is slowing down CNB via TBV. Or is taking TBV a lot longer than the remainder of CNB, which would make this benchmark ineffective?

glozow commented at 10:10 am on December 19, 2022:

One reason is TBV taking time, another is TBV would fail because PopulateMempool() doesn’t sign the txns and spends immature coinbases. I can change it to generate keys and mine 100 blocks in between creating and submitting the transactions if people want.

stickies-v commented at 2:45 pm on December 22, 2022:

I think I’d prefer keeping TBV disabled here. Block validation is important to benchmark too, but since this scales linearly with the amount of transactions selected - I think the current AssembleBlock benchmark suffices there? Narrower benchmarks help more easily pinpoint where performance issues are.

maflcko approved

kevkevinpal commented at 2:48 am on December 21, 2022: contributor

I saw in 50218324dac18556d87688dc1a8e89bbe4d5f69e you change LOCK2(m_node.mempool->cs, cs_main); -> LOCK2(cs_main, m_node.mempool->cs); I’m just curious as to the reason

glozow commented at 10:02 am on December 21, 2022: member

I saw in 5021832 you change LOCK2(m_node.mempool->cs, cs_main); -> LOCK2(cs_main, m_node.mempool->cs); I’m just curious as to the reason

It’s an inconsistent order of acquiring mutexes, which along with other things can cause deadlock. I missed it when first implementing PopulateMempool() - until this PR, it was only called with both locks already acquired. Compiling with --enable-debug / -DDEBUG_LOCKORDER found it.

kevkevinpal commented at 2:04 pm on December 21, 2022: contributor

I saw in 5021832 you change LOCK2(m_node.mempool->cs, cs_main); -> LOCK2(cs_main, m_node.mempool->cs); I’m just curious as to the reason

It’s an inconsistent order of acquiring mutexes, which along with other things can cause deadlock. I missed it when first implementing PopulateMempool() - until this PR, it was only called with both locks already acquired. Compiling with --enable-debug / -DDEBUG_LOCKORDER found it.

Ok I’ll try compiling with --enable-debug thanks that helps explain

in src/node/miner.cpp:77 in b05347204d outdated

73@@ -72,18 +74,22 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool
74     nBlockMaxWeight = std::max<size_t>(4000, std::min<size_t>(MAX_BLOCK_WEIGHT - 4000, options.nBlockMaxWeight));
75 }
76 
77-static BlockAssembler::Options DefaultOptions()
78+void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options)

stickies-v commented at 4:06 pm on December 21, 2022:

0void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options)

Emzy commented at 6:21 pm on December 21, 2022: contributor

Tested ACK b05347204d2d1cad030fc794aae2b0d29c35db7d

this is what I get when running the BlockAssemblerAddPackageTxns benchmark

Specs: mac book pro 2019 2,6 GHz 6-Core Intel Core i7 16 GB 2667 MHz DDR4 macos 13.0.1 (22A400)

0$ ./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns --sanity-check
1Running with --sanity-check option, benchmark results will be useless.
2
3|               ns/op |                op/s |    err% |     total | benchmark
4|--------------------:|--------------------:|--------:|----------:|:----------
5|      777,853,228.00 |                1.29 |    0.0% |      0.78 | `BlockAssemblerAddPackageTxns`

Without –sanity-check:

0$ ./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns 
1
2|               ns/op |                op/s |    err% |     total | benchmark
3|--------------------:|--------------------:|--------:|----------:|:----------
4|      787,270,151.00 |                1.27 |    0.8% |      8.94 | `BlockAssemblerAddPackageTxns`

Specs: Thin Client 2,6 GHz 2-Core AMD G-T56N Processor 4 GB Ubuntu 20.04.5 LTS

 0$ ./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns --sanity-check
 1Running with --sanity-check option, benchmark results will be useless.
 2Warning, results might be unstable:
 3* CPU frequency scaling enabled: CPU 0 between 825.0 and 1,650.0 MHz
 4* CPU governor is 'ondemand' but should be 'performance'
 5* Turbo is enabled, CPU frequency will fluctuate
 6
 7Recommendations
 8* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf
 9
10|               ns/op |                op/s |    err% |     total | benchmark
11|--------------------:|--------------------:|--------:|----------:|:----------
12|    3,488,170,147.00 |                0.29 |    0.0% |      3.49 | `BlockAssemblerAddPackageTxns`

Without –sanity-check:

 0$ ./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns
 1Warning, results might be unstable:
 2* CPU frequency scaling enabled: CPU 0 between 825.0 and 1,650.0 MHz
 3* CPU governor is 'ondemand' but should be 'performance'
 4* Turbo is enabled, CPU frequency will fluctuate
 5
 6Recommendations
 7* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf
 8
 9|               ns/op |                op/s |    err% |     total | benchmark
10|--------------------:|--------------------:|--------:|----------:|:----------
11|    3,477,761,468.00 |                0.29 |    0.0% |     38.30 | `BlockAssemblerAddPackageTxns`

hernanmarino commented at 6:31 pm on December 21, 2022: contributor

ACK b05347204d2d1cad030fc794aae2b0d29c35db7d . Tested / bench’ed correctly

kevkevinpal commented at 6:33 pm on December 21, 2022: contributor

Tested ACK b053472

left some test results here #26695 (review)

in src/node/miner.cpp:89 in b05347204d outdated

86         options.blockMinFeeRate = CFeeRate{parsed.value_or(DEFAULT_BLOCK_MIN_TX_FEE)};
87     } else {
88         options.blockMinFeeRate = CFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
89     }
90+}
91+static BlockAssembler::Options DefaultOptions()

pablomartin4btc commented at 7:00 pm on December 21, 2022:

0static BlockAssembler::Options CustomOptions()

nit: as discussed on the PR review club, perhaps the "DefaultOptions" could be renamed to avoid confusion ("CustomOptions" could be an alternative), unless there’s a reason for not doing it.

glozow commented at 11:35 am on December 22, 2022:

Renamed to ConfiguredOptions

pablomartin4btc commented at 7:03 pm on December 21, 2022: member

Tested ACK. I’ve executed the bench successfully; compiled with --enable-debug.

[refactor] add helper to apply ArgsManager to BlockAssembler::Options

This allows us to both manually manipulate options and grab values from
ArgsManager (i.e. -blockmaxweight and -blockmintxfee config options)
when constructing BlockAssembler::Options. Prior to this change, the
only way to apply the config options is by ctoring BlockAssembler with
no options, which calls DefaultOptions().

a2de971ba1

[refactor] parameterize BlockAssembler::Options in PrepareBlock c058852308

[miner] allow bypassing TestBlockValidity

Allows us to test BlockAssembler on transactions without signatures or
mature coinbases (which is what PopulateMempool creates). Also means
that `TestBlockValidity()` is not included in the bench timing.

cba5934eb6

[test util] randomize fee in PopulateMempool

This makes the contents of the mempool more realistic and iterating by
ancestor feerate order more meaningful. If transactions have varying
feerates, it's also more likely that packages will need to be updated
during block template assembly.

8791410662

[test util] lock cs_main before pool.cs in PopulateMempool 6ce265acf4

[bench] BlockAssembler with mempool packages

The current BlockAssembler bench only tests on a mempool where all
transactions have 0 ancestors or descendants, which does not exercise
any of the package-handling logic in BlockAssembler

04528054fc

glozow force-pushed on Dec 22, 2022

stickies-v approved

stickies-v commented at 3:27 pm on December 22, 2022: contributor

ACK 04528054f

I think this comment can be resolved in a follow-up if preferable but I’d prefer the gArgs parameter be renamed in this PR already since it’s quite confusing.

kevkevinpal commented at 0:08 am on December 23, 2022: contributor

Tested ACK 0452805 Compiled this time using --enable-debug

Specs:

mac book pro 2019 2.3 GHz 8-Core Intel Core i9 16 GB 2667 MHz DDR4 AMD Radeon Pro 5500M 4 GB

➜ bitcoin git:(PR26695) ✗ ./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns --sanity-check

0Running with --sanity-check option, benchmark results will be useless.
1Warning, results might be unstable:
2* DEBUG defined
3
4Recommendations
5* Make sure you compile for Release
6
7|               ns/op |                op/s |    err% |     total | benchmark
8|--------------------:|--------------------:|--------:|----------:|:----------
9|   45,742,309,331.00 |                0.02 |    0.0% |     45.74 | `BlockAssemblerAddPackageTxns`

➜ bitcoin git:(PR26695) ✗ ./src/bench/bench_bitcoin -filter=BlockAssemblerAddPackageTxns

0Warning, results might be unstable:
1* DEBUG defined
2
3Recommendations
4* Make sure you compile for Release
5
6|               ns/op |                op/s |    err% |     total | benchmark
7|--------------------:|--------------------:|--------:|----------:|:----------
8|   43,927,650,019.00 |                0.02 |    0.7% |    488.03 | `BlockAssemblerAddPackageTxns`

achow101 commented at 11:02 pm on January 11, 2023: member

ACK 04528054fcde61aa00e009dbbe1ac350ca1cf748

achow101 merged this on Jan 11, 2023

achow101 closed this on Jan 11, 2023

glozow deleted the branch on Jan 12, 2023

stickies-v commented at 1:38 pm on January 12, 2023: contributor

I think this comment can be resolved in a follow-up if preferable but I’d prefer the gArgs parameter be renamed in this PR already since it’s quite confusing.

Follow-ups in https://github.com/bitcoin/bitcoin/pull/26883

sidhujag referenced this in commit b974575c37 on Jan 12, 2023

glozow referenced this in commit 08b65df1bb on Feb 20, 2023

sidhujag referenced this in commit 40eec9149b on Feb 25, 2023

bitcoin locked this on Jan 12, 2024

bench: BlockAssembler on a mempool with packages #26695

Reviews

Conflicts