tests, bug fix: DisconnectedBlockTransactions rewrite followups

ismaelsadeeq commented at 3:51 pm on September 25, 2023: member

This PR is a follow-up to fix review comments and a bugfix from #28385

The PR

Updated DisconnectedBlockTransactions’s MAX_DISCONNECTED_TX_POOL from kb to bytes.
Moved DisconnectedBlockTransactions implementation code to kernel/disconnected_transactions.cpp.
AddTransactionsFromBlock now assume duplicate transactions are not passed by asserting after inserting each transaction to iters_by_txid.
Included a Bug fix: In the current master we are underestimating the memory usage of DisconnectedBlockTransactions.
- When adding and subtracting cachedInnerUsage we call RecursiveDynamicUsage with CTransaction which invokes this RecursiveDynamicUsage(const CTransaction& tx) version of RecursiveDynamicUsage, the output of that call only account for the memory usage of the inputs and outputs of the CTransaction, this omits the memory usage of the CTransaction object and the control block.
- This PR fixes this bug by calling RecursiveDynamicUsage with CTransactionRef when adding and subtracting cachedInnerUsage which invokes RecursiveDynamicUsage(const std::shared_ptr<X>& p) version of RecursiveDynamicUsage the output of the calculation accounts for the CTransaction object, the control blocks, inputs and outputs memory usage.
- see comment
Added test for DisconnectedBlockTransactions memory limit.

DrahtBot commented at 3:51 pm on September 25, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, BrandonOdiwuor, glozow
Concept ACK	theuni

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28690 (build: Introduce internal kernel library by TheCharlatan)

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.

DrahtBot added the label Refactoring on Sep 25, 2023

in src/kernel/disconnected_transactions.h:38 in 8c315c7541 outdated

33@@ -36,50 +34,27 @@ static const unsigned int MAX_DISCONNECTED_TX_POOL_SIZE = 20'000;
34  * of the list. After trimming, transactions can be re-added to the mempool from the back of the
35  * list to the front without running into missing inputs.
36  */
37-class DisconnectedBlockTransactions {
38+class DisconnectedBlockTransactions
39+{
40 private:

glozow commented at 3:58 pm on September 25, 2023:

Can delete this too, since it’s a class

in src/kernel/disconnected_transactions.h:43 in 8c315c7541 outdated

40 private:
41     /** Cached dynamic memory usage for the CTransactions (memory for the shared pointers is
42      * included in the container calculations). */
43-    uint64_t cachedInnerUsage = 0;
44+    uint64_t cachedInnerUsage{0};
45+    /* Maximum memory usage in bytes of the transactions stored  */

glozow commented at 3:59 pm on September 25, 2023:

A bit more accurate:

0    /* Maximum allowed total DynamicMemoryUsage, in bytes. To be enforced in AddTransactionsFromBlock.  */

0xETHengineer approved

in src/kernel/disconnected_transactions.cpp:1 in 8c315c7541

glozow commented at 4:25 pm on September 25, 2023:

Question: what’s the benefit of moving to cpp file? I thought maybe it saves users from including some things, but it seems about the same.

ismaelsadeeq commented at 3:24 pm on September 26, 2023:

Motivation is from @stickies-v review comment #28385#pullrequestreview-1629400522 Its a better practice to have interfaces in a .h and implementation code in .cpp file, allows for more efficient compilation by preventing redundant recompilation of unchanged code, ensuring faster build times I think.

I dont think there will be need to have implementation code in the header files. https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization

glozow commented at 1:46 pm on October 11, 2023:

nit on 436f53fd5b2ac1b3d9e11634d7f690dd32598844 commit message: it’s not “ensure” so much as “assume”

stickies-v commented at 3:44 pm on October 12, 2023:

Note for other reviewers: these are the memory allocation numbers I’m getting on my machine:

0(lldb) p MAP_1
1(const size_t) 32
2(lldb) p MAP_100
3(const size_t) 832
4(lldb) p TX_USAGE
5(const size_t) 640
6(lldb) p ENTRY_USAGE_OVERESTIMATE
7(const size_t) 896

ismaelsadeeq commented at 3:57 pm on October 13, 2023:

thanks updated

glozow commented at 12:59 pm on September 26, 2023: member

Thanks for opening a followup! Did you want to pick up the memusage unit test too? (https://github.com/bitcoin/bitcoin/commit/9dcef47e4d52b15ffe88be7ddf5ea626121e5f0d or a new one)

ismaelsadeeq force-pushed on Sep 26, 2023

BrandonOdiwuor commented at 9:06 am on September 27, 2023: contributor

ACK 2517862abb2c2e111c259ad2a6498e352f026c92

Great job on moving the implementation code from the header file to disconnected_transactions.cpp. This was a necessary step, especially considering how significantly the code had grown.

ismaelsadeeq force-pushed on Oct 5, 2023

ismaelsadeeq commented at 11:01 am on October 5, 2023: member

Added a commit to address review comment #28385 (review)

And rebased due to #28567 being merged for CI to be green

ismaelsadeeq force-pushed on Oct 10, 2023

ismaelsadeeq commented at 1:38 pm on October 10, 2023: member

Thanks for opening a followup! Did you want to pick up the memusage unit test too? (9dcef47 or a new one)

Added the the test.

in src/test/disconnected_transactions.cpp:40 in 225c9c3b25 outdated

34+
35+    const size_t TX_USAGE{RecursiveDynamicUsage(block_vtx.front())};
36+    for (const auto& tx : block_vtx)
37+        BOOST_CHECK_EQUAL(RecursiveDynamicUsage(tx), TX_USAGE);
38+
39+    const size_t ENTRY_USAGE_OVERESTIMATE{TX_USAGE + 8 * memusage::MallocUsage(sizeof(void*))};

ismaelsadeeq commented at 1:40 pm on October 10, 2023:

@glozow the ENTRY_USAGE_OVERESTIMATE in https://github.com/bitcoin/bitcoin/commit/9dcef47e4d52b15ffe88be7ddf5ea626121e5f0d is a bit much and two transaction are being added to disconnectpool for the first test that check that its big enough for one transaction. So reduced the overestimate to TX_USAGE + 8 * memusage::MallocUsage(sizeof(void*)

glozow commented at 2:04 pm on October 10, 2023:

Makes sense to me - the numbers were a little bit off when I originally wrote that test. Thanks for fixing :+1:

stickies-v commented at 1:51 pm on October 12, 2023:

I think these numbers need to be better documented. I think this means that we’re overestimating by 8 pointers? Why 8? Why do we need a safety margin? Labeling assumptions would be helpful to (future) reviewers.

Alternatively, would it make sense to use a relative safety margin (i.e. TX_USAGE * 1.1) instead of an absolute one?

glozow commented at 1:54 pm on October 13, 2023:

The 8 pointers isn’t to create an overestimation margin, it’s to account for the approximate usage of the data structures (pointers in the list nodes, etc.)

glozow commented at 2:31 pm on October 13, 2023:

Maybe a comment like this would suffice?

0// Our overall formula is unordered map overhead + usage per entry.
1// Implementations may vary, but we're trying to guess the usage of data structures:
2// list: per entry x (2 pointers + element itself, which is a CTransactionRef)
3// unordered map: hashtable overhead + per entry x (3 pointers + value itself, which is an iterator)
4// Usage per entry is thus approximately `TX_USAGE` + 6 pointers. Add another 2 pointers as a safety margin.

ismaelsadeeq commented at 3:54 pm on October 13, 2023:

Added the comment but I think, its word to also explain whats the pointers represent

0// Our overall formula is unordered map overhead + usage per entry.
1// Implementations may vary, but we're trying to guess the usage of data structures:
2// list: per entry x (2 pointers (prev, next) + element itself, which is a CTransactionRef)
3// unordered map: hashtable overhead + per entry x (3 pointers (key, value, internal structure for book keeping) + value itself, which is an iterator)
4// Usage per entry is thus approximately `TX_USAGE` + 6 pointers. Add another 2 pointers as a safety margin.

But not so sure if its correct for unordered map, I added as you suggested, if you think the explanation is worth it happy to repush

stickies-v commented at 12:02 pm on October 18, 2023:

In combination with the fix suggested here, what do you think about this?

It makes all the assumptions explicit in code (except for the pointers which are documented in docstring). It also removes the safety margin. I think we can leave that out until/if we observe it causing issues, e.g. on certain platforms?

 0diff --git a/src/test/disconnected_transactions.cpp b/src/test/disconnected_transactions.cpp
 1index b488866a37..bae9850de9 100644
 2--- a/src/test/disconnected_transactions.cpp
 3+++ b/src/test/disconnected_transactions.cpp
 4@@ -34,10 +34,14 @@ BOOST_AUTO_TEST_CASE(disconnectpool_memory_limits)
 5 
 6     // Our overall formula is unordered map overhead + usage per entry.
 7     // Implementations may vary, but we're trying to guess the usage of data structures:
 8-    // list: per entry x (2 pointers (next pointer and prev pointer) + element itself, which is a CTransactionRef)
 9-    // unordered map: hashtable overhead + per entry x (3 pointers + value itself, which is an iterator)
10-    // Usage per entry is thus approximately `TX_USAGE` + 6 pointers. Add another 2 pointers as a safety margin.
11-    const size_t ENTRY_USAGE_OVERESTIMATE{TX_USAGE + 8 * memusage::MallocUsage(sizeof(void*))};
12+    const size_t ENTRY_USAGE_OVERESTIMATE{
13+        TX_USAGE
14+        // list entry: 2 pointers (next pointer and prev pointer) + element itself
15+        + memusage::MallocUsage((2 * sizeof(void*)) + sizeof(decltype(block_vtx)::value_type))
16+        // unordered map: 1 pointer for the hashtable + key and value
17+        + memusage::MallocUsage(sizeof(void*) + sizeof(decltype(temp_map)::key_type)
18+                                + sizeof(decltype(temp_map)::value_type))
19+    };
20 
21     // DisconnectedBlockTransactions that's just big enough for 1 transaction.
22     {

ismaelsadeeq commented at 4:18 pm on October 18, 2023:

Added together with a clarification comment that we should overestimate if their is issues on other platform

glozow added the label Tests on Oct 11, 2023

glozow renamed this:
~~Refactor: move DisconnectedBlockTransactions implementation code to disconnected_transactions.cpp~~
tests, refactor: DisconnectedBlockTransactions rewrite followups
on Oct 11, 2023

glozow commented at 9:05 am on October 11, 2023: member

cc @theuni @stickies-v who made some of the suggestions being addressed and reviewed the test previously

in src/kernel/disconnected_transactions.cpp:85 in 72ebef1677 outdated

80+std::list<CTransactionRef> DisconnectedBlockTransactions::take()
81+{
82+    std::list<CTransactionRef> ret{std::move(queuedTx)};
83+    clear();
84+    return ret;
85+}

glozow commented at 1:54 pm on October 11, 2023:

72ebef1677a0ae61a1ca9dcde576553fb6e99fa6 newline?

ismaelsadeeq commented at 3:57 pm on October 13, 2023:

added 💯

in src/kernel/disconnected_transactions.cpp:6 in 225c9c3b25 outdated

0@@ -0,0 +1,86 @@
1+// Copyright (c) 2023 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <kernel/disconnected_transactions.h>
6+
7+#include <core_memusage.h>

stickies-v commented at 1:09 pm on October 12, 2023:

iwyu says:

0kernel/disconnected_transactions.cpp should add these lines:
1#include <assert.h>                  // for assert
2#include <algorithm>                 // for max
3#include <memory>                    // for __shared_ptr_access, shared_ptr
4#include <utility>                   // for move, pair
5#include "memusage.h"                // for DynamicUsage
6#include "primitives/transaction.h"  // for CTransactionRef, CTransaction
7#include "util/hasher.h"             // for SaltedTxidHasher

ismaelsadeeq commented at 4:16 pm on October 13, 2023:

Added Thanks

in src/kernel/disconnected_transactions.cpp:51 in 225c9c3b25 outdated

46+{
47+    iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
48+    for (auto block_it{vtx.rbegin()}; block_it != vtx.rend(); ++block_it) {
49+        auto it{queuedTx.insert(queuedTx.end(), *block_it)};
50+        auto emplace_result = iters_by_txid.emplace((*block_it)->GetHash(), it);
51+        assert(emplace_result.second);

stickies-v commented at 1:15 pm on October 12, 2023:

0        auto [_, inserted] = iters_by_txid.emplace((*block_it)->GetHash(), it);
1        assert(inserted);  // callers may never pass multiple transactions with the same txid

ismaelsadeeq commented at 3:51 pm on October 13, 2023:

fixed but Ive used used, already_exist instead since that what the boolean represent and what cpp reference describe the boolean

stickies-v commented at 3:55 pm on October 13, 2023:

I don’t think so?

Returns a pair consisting of an iterator to the inserted element, or the already-existing element if no insertion happened, and a bool denoting whether the insertion took place (true if insertion happened, false if it did not).

Conceptually, we exactly want to ensure that the txid does not exist yet, which is the opposite of what you’re stating now.

ismaelsadeeq commented at 4:07 pm on October 13, 2023:

Yes you are right, my bad. updated

in src/kernel/disconnected_transactions.cpp:10 in 225c9c3b25 outdated

 5+#include <kernel/disconnected_transactions.h>
 6+
 7+#include <core_memusage.h>
 8+
 9+DisconnectedBlockTransactions::DisconnectedBlockTransactions(size_t max_mem_usage)
10+    : m_max_mem_usage{max_mem_usage} {}

stickies-v commented at 1:28 pm on October 12, 2023:

Not sure this is worth moving, would just leave in the header

ismaelsadeeq commented at 3:58 pm on October 13, 2023:

its now in the header thanks

in src/test/disconnected_transactions.cpp:16 in 225c9c3b25 outdated

11+
12+//! Tests that DisconnectedBlockTransactions limits its own memory properly
13+BOOST_AUTO_TEST_CASE(disconnectpool_memory_limits)
14+{
15+    if (sizeof(void*) != 8) {
16+        BOOST_TEST_MESSAGE("Exiting disconnected_transactions memory usage test early due to unsupported arch");

stickies-v commented at 1:42 pm on October 12, 2023:

We’re not actually exiting though?

0        BOOST_TEST_MESSAGE("Exiting disconnected_transactions memory usage test early due to unsupported arch");
1        return;

glozow commented at 1:48 pm on October 13, 2023:

Well the good news is it looks like it passes on 32bit, so maybe just remove this block?

ismaelsadeeq commented at 3:51 pm on October 13, 2023:

Removed

in src/test/disconnected_transactions.cpp:46 in 225c9c3b25 outdated

41+    // DisconnectedBlockTransactions that's just big enough for 1 transaction.
42+    {
43+        DisconnectedBlockTransactions disconnectpool{MAP_1 + ENTRY_USAGE_OVERESTIMATE};
44+        // Just 2 transactions because the allocation depends on the number of txns passed in.
45+        std::vector<CTransactionRef> two_txns({block_vtx.at(0), block_vtx.at(1)});
46+        auto evicted_txns = disconnectpool.AddTransactionsFromBlock(two_txns);

stickies-v commented at 1:53 pm on October 12, 2023:

nit (in other places too):

0        auto evicted_txns{disconnectpool.AddTransactionsFromBlock(two_txns)};

ismaelsadeeq commented at 3:54 pm on October 13, 2023:

fixed thank you

in src/test/disconnected_transactions.cpp:45 in 225c9c3b25 outdated

39+    const size_t ENTRY_USAGE_OVERESTIMATE{TX_USAGE + 8 * memusage::MallocUsage(sizeof(void*))};
40+
41+    // DisconnectedBlockTransactions that's just big enough for 1 transaction.
42+    {
43+        DisconnectedBlockTransactions disconnectpool{MAP_1 + ENTRY_USAGE_OVERESTIMATE};
44+        // Just 2 transactions because the allocation depends on the number of txns passed in.

stickies-v commented at 1:55 pm on October 12, 2023:

I don’t really understand this docstring, what does it mean?

glozow commented at 1:51 pm on October 13, 2023:

I meant don’t pass in all 100 transactions because the memory usage won’t be what we expect and iirc all of them will be evicted. The function works by first allocating 100 buckets, adding all of the transactions, and then trimming. If we add all 100, there’s much more overhead and we end up not having room for anything.

stickies-v commented at 12:22 pm on October 18, 2023:

Okay, that makes sense. I didn’t understand what the alternative was or what the allocation referred to exactly, so here’s a suggestion to make this more explicit:

0        // Add just 2 (and not all 100) transactions to keep the unordered map's hashtable overhead
1        // to a minimum and avoid all (instead of all but 1) transactions getting evicted

ismaelsadeeq commented at 4:19 pm on October 18, 2023:

Added thank you.

in src/test/disconnected_transactions.cpp:89 in 225c9c3b25 outdated

79+        // Only 1 transaction needed to be evicted
80+        BOOST_CHECK_EQUAL(1, evicted_txns.size());
81+
82+        // Transactions are added from back to front and eviction is FIFO.
83+        // The last transaction of block_vtx should be the first to be evicted.
84+        BOOST_CHECK_EQUAL(block_vtx.back(), evicted_txns.front());

stickies-v commented at 3:52 pm on October 12, 2023:

It’s a good test to have but arguably not a disconnectpool_memory_limits. I think this could be a separate test case (and i also wouldn’t be opposed to unit testing the rest of the interface, i.e. AddTransactionsFromBlock, removeForBlock, take, clear.

ismaelsadeeq commented at 3:57 pm on October 13, 2023:

Yes, this checks the order in which transaction are evicted, do you think it should be in its own commit?

stickies-v commented at 12:23 pm on October 18, 2023:

I think it’s fine to have just a single test commit here. Testing the interface just could use a separate test from the memory management test.

ismaelsadeeq commented at 4:21 pm on October 18, 2023:

Removed the test, I will like to limit this to disconnectpool_memory_limits test.

stickies-v commented at 4:37 pm on October 18, 2023:

If you’re not adding it in a new test, I would leave it in here, it’s a useful check.

stickies-v commented at 3:54 pm on October 12, 2023: contributor

Approach ACK 225c9c3b25d24d0f8e7730fc17ae6a7b49a2b272

Even though they’re both correct and an improvement, I’d drop 6f5768d81272aeb81fd3660a26318be4e34ddd5e and 89c994acb1c12d02b072b66c7041b144fc65a6b4. For both, the fixups are arbitrary and incomplete (e.g. there are plenty of other places where we don’t use list initialization in validation.cpp, so why are only these two being fixed? “It’s a follow-up” is a bit of a wonky rationale. I’d say let’s not do it, or do it consistently and ideally enforce it (which I don’t think is suitable for this PR). Likewise for the clang-format fixes. Inconsistent style-only refactors are generally a hard sell.

note: I reviewed 72ebef1677a0ae61a1ca9dcde576553fb6e99fa6 with --color-moved=zebra --color-moved-ws="allow-indentation-change" (always useful to mention those things in PR description to help reviewers be more productive)

ismaelsadeeq commented at 4:40 pm on October 12, 2023: member

Thanks for your review @stickies-v, will drop the commits as you suggested and address comments.

refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes 81dfeddea7

ismaelsadeeq force-pushed on Oct 13, 2023

ismaelsadeeq commented at 4:29 pm on October 13, 2023: member

Force pushed https://github.com/bitcoin/bitcoin/commit/225c9c3b25d24d0f8e7730fc17ae6a7b49a2b272 to 8faa980089

Dropped the first two commits.
Added missing include in kernel/disconnected_transactions.cpp.
Updated #28385 (review) to destructure and capture the result of the emplace then assert.
Move the kernel/disconnected_transactions.cpp constructor back to header.
Removed the early return in src/test/disconnected_transactions.cpp since it passes on 32 bit.
Added a comment https://github.com/bitcoin/bitcoin/pull/28530/files#diff-3cd57ed0ff682b234d9991ad80eaf492edd9f4b01f9ea822d6344fee0eac183f to explain how ENTRY_USAGE_OVERESTIMATE is calculated.
Updated PR OP.

theuni commented at 6:52 pm on October 13, 2023: member

ACK the first 3 commits. I’ll pass on re-reviewing the test commit and confusing myself about memory accounting again :)

in src/kernel/disconnected_transactions.cpp:5 in 8faa980089 outdated

0@@ -0,0 +1,88 @@
1+// Copyright (c) 2023 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#include <assert.h> // for assert

stickies-v commented at 1:16 pm on October 16, 2023:

nit: we don’t mention these // for ... usage statements because they are cumbersome to maintain manually

stickies-v commented at 1:19 pm on October 16, 2023:

Also, we generally first include the “own” header, then the project headers, then the stdlib headers, so that would mean:

0#include <kernel/disconnected_transactions.h>
1
2#include <assert.h>
3#include <core_memusage.h>
4#include <memusage.h>
5#include <primitives/transaction.h>
6#include <util/hasher.h>
7
8#include <memory>
9#include <utility>

in src/kernel/disconnected_transactions.cpp:51 in 6d1948a08c outdated

46+
47+[[nodiscard]] std::vector<CTransactionRef> DisconnectedBlockTransactions::AddTransactionsFromBlock(const std::vector<CTransactionRef>& vtx)
48+{
49+    iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
50+    for (auto block_it{vtx.rbegin()}; block_it != vtx.rend(); ++block_it) {
51+        auto it{queuedTx.insert(queuedTx.end(), *block_it)};

stickies-v commented at 1:24 pm on October 16, 2023:

This is not a move-only change (list initialization). Same here and here. You can check this yourself by inspecting the commit with --color-moved=zebra --color-moved-ws="allow-indentation-change". Would revert the list initialization change.

ismaelsadeeq commented at 4:21 pm on October 18, 2023:

Thanks removed

move only: move implementation code to disconnected_transactions.cpp 29eb219c12

assume duplicate transactions are not added to `iters_by_txid`

In `AddTransactionsToBlock` description comment we have the asuumption
that callers will never pass multiple transactions with the same txid
We are asserting to assume that does not happen.

f4254e2098

ismaelsadeeq force-pushed on Oct 18, 2023

in src/test/disconnected_transactions.cpp:39 in 3c00943bb7 outdated

34+
35+    // Our overall formula is unordered map overhead + usage per entry.
36+    // Implementations may vary, but we're trying to guess the usage of data structures.
37+    // No safety margin is added. It is assumed that this is an accurate guess. If this
38+    // estimate causes issues on other platforms, then we may consider adding an additional
39+    // pointer as safety margin.

stickies-v commented at 4:36 pm on October 18, 2023:

I don’t know how useful this comment is. For ENTRY_USAGE_ESTIMATE, we’re just hardcoding how we calculate things in memusage, so this should always match, I think? I’d be fine removing it tbh.

ismaelsadeeq commented at 5:06 pm on October 18, 2023:

removed

in src/kernel/disconnected_transactions.cpp:55 in 73ed6fc053 outdated

50@@ -51,9 +51,8 @@ size_t DisconnectedBlockTransactions::DynamicMemoryUsage() const
51     iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
52     for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) {
53         auto it = queuedTx.insert(queuedTx.end(), *block_it);
54-        auto [_, inserted] = iters_by_txid.emplace((*block_it)->GetHash(), it);
55-        assert(inserted); // callers may never pass multiple transactions with the same txid

stickies-v commented at 4:40 pm on October 18, 2023:

I don’t think you meant to remove this? It reverts f4254e209801d6a790b5f0c251c0b32154a4e3cc

ismaelsadeeq commented at 5:07 pm on October 18, 2023:

interactive rebase mistake, fixed in latest push.

ismaelsadeeq force-pushed on Oct 18, 2023

in src/kernel/disconnected_transactions.cpp:55 in e9906c105d outdated

50+{
51+    iters_by_txid.reserve(iters_by_txid.size() + vtx.size());
52+    for (auto block_it = vtx.rbegin(); block_it != vtx.rend(); ++block_it) {
53+        auto it = queuedTx.insert(queuedTx.end(), *block_it);
54+        auto [_, inserted] = iters_by_txid.emplace((*block_it)->GetHash(), it);
55+        assert(inserted);

stickies-v commented at 1:28 pm on October 19, 2023:

nit: f4254e209801d6a790b5f0c251c0b32154a4e3cc has a docstring but 070f172a877a7c03fec5c9b1a032106b9a80b329 removes it:

0        assert(inserted); // callers may never pass multiple transactions with the same txid

tip to avoid these issues is to run through your commits one-by-one especially after doing rebase operations etc, it’s a pretty common mistake

ismaelsadeeq commented at 2:57 pm on October 19, 2023:

Thanks, will avoid.

in src/test/disconnected_transactions.cpp:42 in e9906c105d outdated

37+    const size_t ENTRY_USAGE_ESTIMATE{
38+        TX_USAGE
39+        // list entry: 2 pointers (next pointer and prev pointer) + element itself
40+        + memusage::MallocUsage((2 * sizeof(void*)) + sizeof(decltype(block_vtx)::value_type))
41+        // unordered map: 1 pointer for the hashtable + key and value
42+        + memusage::MallocUsage(sizeof(void*) + sizeof(decltype(temp_map)::key_type) + sizeof(decltype(temp_map)::value_type))};

stickies-v commented at 1:30 pm on October 19, 2023:

nit: exceeds line length

ismaelsadeeq commented at 2:58 pm on October 19, 2023:

https://github.com/bitcoin-core-review-club/bitcoin/tree/pr28368/contrib/devtools#clang-format-diffpy It was short before but contributor guide clang format e9906c105dffbb864e8c5f7680334480cdd19cd9 made the line longer?

stickies-v approved

stickies-v commented at 1:32 pm on October 19, 2023: contributor

ACK e9906c105dffbb864e8c5f7680334480cdd19cd9

Just 2 nits, happy to quickly re-ack since no other acks yet.

edit: PR description needs updating, would change title to tests, bugfix and add the bugfix in the description, as well as the “assume duplicate transactions are not added to iters_by_txid” commit

DrahtBot requested review from BrandonOdiwuor on Oct 19, 2023

DrahtBot requested review from theuni on Oct 19, 2023

DrahtBot removed review request from BrandonOdiwuor on Oct 19, 2023

DrahtBot requested review from BrandonOdiwuor on Oct 19, 2023

DrahtBot removed review request from BrandonOdiwuor on Oct 19, 2023

DrahtBot requested review from BrandonOdiwuor on Oct 19, 2023

ismaelsadeeq force-pushed on Oct 19, 2023

ismaelsadeeq renamed this:
~~tests, refactor: DisconnectedBlockTransactions rewrite followups~~
tests, bug fix: DisconnectedBlockTransactions rewrite followups
on Oct 19, 2023

in src/kernel/disconnected_transactions.cpp:72 in 2c40895591 outdated

67+    for (const auto& tx : vtx) {
68+        auto iter = iters_by_txid.find(tx->GetHash());
69+        if (iter != iters_by_txid.end()) {
70+            auto list_iter = iter->second;
71+            iters_by_txid.erase(iter);
72+            cachedInnerUsage -= RecursiveDynamicUsage(**list_iter);

stickies-v commented at 3:07 pm on October 19, 2023:

Hmm this didn’t get cherry-picked properly from https://github.com/stickies-v/bitcoin/commit/87f3f28b3acfc329eaed0b0cce6ff589271bfefd#diff-daf5057c112d6ec1338dd535558cd0f4e21e9a3e0241b610d88068fe2baba362R113 , probably because of rebase?

ismaelsadeeq commented at 3:17 pm on October 19, 2023:

Yes, fixed.

bugfix: correct DisconnectedBlockTransactions memory usage

With `queuedTx` owning the `CTransactionRef` shared ptrs, they (and
the managed objects) are entirely allocated on the heap. In
`DisconnectedBlockTransactions::DynamicMemoryUsage`, we account for
the 2 pointers that make up the shared_ptr, but not for the managed
object (CTransaction) or the control block.

Prior to this commit, by calculating the `RecursiveDynamicUsage` on
a `CTransaction` whenever modifying `cachedInnerUsage`, we account
for the dynamic usage of the `CTransaction`, i.e. the `vins` and
`vouts` vectors, but we do not account for the `CTransaction`
object itself, nor for the `CTransactionRef` control block.

This means prior to this commit, `DynamicMemoryUsage` underestimates
dynamic memory usage by not including the `CTransaction` objects and
the shared ptr control blocks.

Fix this by calculating `RecursiveDynamicUsage` on the
`CTransactionRef` instead of the `CTransaction` whenever modifying
`cachedInnerUsage`.

b2d0447964

[test] DisconnectedBlockTransactions::DynamicMemoryUsage 9b3da70bd0

ismaelsadeeq force-pushed on Oct 19, 2023

ismaelsadeeq commented at 4:44 pm on October 19, 2023: member

edit: PR description needs updating, would change title to tests, bugfix and add the bugfix in the description, as well as the “assume duplicate transactions are not added to iters_by_txid” commit

Thank you @stickies-v , I updated the PR description and address review comments.

stickies-v approved

stickies-v commented at 7:33 pm on October 19, 2023: contributor

ACK 9b3da70bd06b45482e7211aa95637a72bd115553 - nice work!

DrahtBot removed review request from BrandonOdiwuor on Oct 19, 2023

DrahtBot requested review from BrandonOdiwuor on Oct 19, 2023

BrandonOdiwuor approved

BrandonOdiwuor commented at 10:18 am on October 20, 2023: contributor

re ACK 9b3da70bd06b45482e7211aa95637a72bd115553

in src/kernel/disconnected_transactions.cpp:37 in b2d0447964 outdated

33@@ -34,7 +34,7 @@ std::vector<CTransactionRef> DisconnectedBlockTransactions::LimitMemoryUsage()
34 
35     while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) {
36         evicted.emplace_back(queuedTx.front());
37-        cachedInnerUsage -= RecursiveDynamicUsage(*queuedTx.front());
38+        cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());

glozow commented at 11:01 am on November 2, 2023:

For future reference for b2d04479647af64ad7cf5ebfb6175251b2f6b72e

Imagining DynamicMemoryUsage for a DisconnectedBlockTransactions with 1 transaction (ignoring the iters_by_txid part since that doesn’t change),

before

0DynamicUsage(queuedTx) + cachedInnerUsage
1=MallocUsage(sizeof(list_node<CTransactionRef>) + cachedInnerUsage
2=MallocUsage(sizeof(4 pointers) + RecursiveDynamicUsage(CTransaction)
3=MallocUsage(sizeof(4 pointers) + DynamicUsage(vin) + DynamicUsage(vout) + sum([RecursiveDynamicUsage(input) for input in vin]) + sum([RecursiveDynamicUsage(output) for output in vout])

after

0DynamicUsage(queuedTx) + cachedInnerUsage
1=MallocUsage(sizeof(list_node<CTransactionRef>) + cachedInnerUsage
2=MallocUsage(sizeof(4 pointers) + RecursiveDynamicUsage(CTransactionRef)
3=MallocUsage(sizeof(4 pointers) + DynamicUsage(CTransactionRef) + RecursiveDynamicUsage(CTransaction)
4=MallocUsage(sizeof(4 pointers) + MallocUsage(sizeof(CTransaction)) + MallocUsage(sizeof(stl_shared_counter)) + RecursiveDynamicUsage(CTransaction)
5=MallocUsage(sizeof(4 pointers) + MallocUsage(sizeof(CTransaction)) + MallocUsage(sizeof(stl_shared_counter)) + DynamicUsage(vin) + DynamicUsage(vout) + sum([RecursiveDynamicUsage(input) for input in vin]) + sum([RecursiveDynamicUsage(output) for output in vout])

So we account for

each list node, which is 2 pointers + a shared pointer object (2 pointers)
each shared pointer’s dynamically allocated memory i.e. the CTransaction object + its always dynamically allocated stuff (the vectors) + the control block

glozow commented at 11:07 am on November 2, 2023: member

ACK 9b3da70bd06b45482e7211aa95637a72bd115553

glozow merged this on Nov 2, 2023

glozow closed this on Nov 2, 2023

ismaelsadeeq deleted the branch on Jun 27, 2024

tests, bug fix: DisconnectedBlockTransactions rewrite followups #28530

Code Coverage

Reviews

Conflicts