tests, bug fix: DisconnectedBlockTransactions rewrite followups #28530

ismaelsadeeq · 2023-09-25T15:51:47Z

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 · 2023-09-25T15:51:50Z

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.

glozow

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

src/kernel/disconnected_transactions.h

src/kernel/disconnected_transactions.cpp

BrandonOdiwuor

ACK 2517862

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 · 2023-10-05T11:01:20Z

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

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

ismaelsadeeq · 2023-10-10T13:38:17Z

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.

src/test/disconnected_transactions.cpp

glozow · 2023-10-11T09:05:13Z

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

stickies-v

Approach ACK 225c9c3

Even though they're both correct and an improvement, I'd drop 6f5768d and 89c994a. 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 72ebef1 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)

src/kernel/disconnected_transactions.cpp

src/test/disconnected_transactions.cpp

stickies-v · 2023-10-12T15:44:11Z

src/test/disconnected_transactions.cpp

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

(lldb) p MAP_1 (const size_t) 32 (lldb) p MAP_100 (const size_t) 832 (lldb) p TX_USAGE (const size_t) 640 (lldb) p ENTRY_USAGE_OVERESTIMATE (const size_t) 896

src/test/disconnected_transactions.cpp

ismaelsadeeq · 2023-10-12T16:40:34Z

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

src/kernel/disconnected_transactions.cpp

src/test/disconnected_transactions.cpp

ismaelsadeeq · 2023-10-13T16:29:57Z

Force pushed 225c9c3 to 8faa980

Dropped the first two commits.
Added missing include in kernel/disconnected_transactions.cpp.
Updated [refactor] rewrite DisconnectedBlockTransactions to not use boost #28385 (comment) 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 · 2023-10-13T18:52:53Z

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

src/kernel/disconnected_transactions.cpp

src/test/disconnected_transactions.cpp

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.

src/test/disconnected_transactions.cpp

src/kernel/disconnected_transactions.cpp

stickies-v

ACK e9906c1

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

src/kernel/disconnected_transactions.cpp

src/test/disconnected_transactions.cpp

src/kernel/disconnected_transactions.cpp

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`.

ismaelsadeeq · 2023-10-19T16:44:56Z

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

ACK 9b3da70 - nice work!

BrandonOdiwuor

re ACK 9b3da70

glozow

ACK 9b3da70

glozow · 2023-11-02T11:01:39Z

src/kernel/disconnected_transactions.cpp

@@ -34,7 +34,7 @@ std::vector<CTransactionRef> DisconnectedBlockTransactions::LimitMemoryUsage()

    while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) {
        evicted.emplace_back(queuedTx.front());
-        cachedInnerUsage -= RecursiveDynamicUsage(*queuedTx.front());
+        cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());


For future reference for b2d0447

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

before

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

after

DynamicUsage(queuedTx) + cachedInnerUsage =MallocUsage(sizeof(list_node<CTransactionRef>) + cachedInnerUsage =MallocUsage(sizeof(4 pointers) + RecursiveDynamicUsage(CTransactionRef) =MallocUsage(sizeof(4 pointers) + DynamicUsage(CTransactionRef) + RecursiveDynamicUsage(CTransaction) =MallocUsage(sizeof(4 pointers) + MallocUsage(sizeof(CTransaction)) + MallocUsage(sizeof(stl_shared_counter)) + RecursiveDynamicUsage(CTransaction) =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

DrahtBot added the Refactoring label Sep 25, 2023

ismaelsadeeq mentioned this pull request Sep 25, 2023

[refactor] rewrite DisconnectedBlockTransactions to not use boost #28385

Merged

0xETHengineer approved these changes Sep 25, 2023

View reviewed changes

glozow reviewed Sep 26, 2023

View reviewed changes

src/kernel/disconnected_transactions.h Show resolved Hide resolved

src/kernel/disconnected_transactions.h Outdated Show resolved Hide resolved

src/kernel/disconnected_transactions.cpp Show resolved Hide resolved

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch from 8c315c7 to 2517862 Compare September 26, 2023 16:45

BrandonOdiwuor reviewed Sep 27, 2023

View reviewed changes

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch from 2517862 to 436f53f Compare October 5, 2023 10:56

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch from 8742350 to 225c9c3 Compare October 10, 2023 13:33

ismaelsadeeq commented Oct 10, 2023

View reviewed changes

src/test/disconnected_transactions.cpp Outdated Show resolved Hide resolved

glozow added the Tests label Oct 11, 2023

glozow changed the title ~~Refactor: move DisconnectedBlockTransactions implementation code to disconnected_transactions.cpp~~ tests, refactor: DisconnectedBlockTransactions rewrite followups Oct 11, 2023

stickies-v reviewed Oct 12, 2023

View reviewed changes

refactor: update MAX_DISCONNECTED_TX_POOL from kb to bytes

81dfedd

glozow reviewed Oct 13, 2023

View reviewed changes

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch 2 times, most recently from 6d922c3 to 8faa980 Compare October 13, 2023 16:15

stickies-v reviewed Oct 18, 2023

View reviewed changes

ismaelsadeeq added 2 commits October 18, 2023 15:26

move only: move implementation code to disconnected_transactions.cpp

29eb219

assume duplicate transactions are not added to iters_by_txid

f4254e2

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.

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch from 8faa980 to 3c00943 Compare October 18, 2023 16:16

stickies-v reviewed Oct 18, 2023

View reviewed changes

src/test/disconnected_transactions.cpp Outdated Show resolved Hide resolved

src/test/disconnected_transactions.cpp Show resolved Hide resolved

src/kernel/disconnected_transactions.cpp Outdated Show resolved Hide resolved

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch from 3c00943 to e9906c1 Compare October 18, 2023 17:05

stickies-v approved these changes Oct 19, 2023

View reviewed changes

src/kernel/disconnected_transactions.cpp Outdated Show resolved Hide resolved

src/test/disconnected_transactions.cpp Outdated Show resolved Hide resolved

DrahtBot requested review from BrandonOdiwuor and theuni and removed request for BrandonOdiwuor October 19, 2023 13:32

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch from e9906c1 to 2c40895 Compare October 19, 2023 14:55

ismaelsadeeq changed the title ~~tests, refactor: DisconnectedBlockTransactions rewrite followups~~ tests, bug fix: DisconnectedBlockTransactions rewrite followups Oct 19, 2023

stickies-v reviewed Oct 19, 2023

View reviewed changes

src/kernel/disconnected_transactions.cpp Outdated Show resolved Hide resolved

stickies-v and others added 2 commits October 19, 2023 16:14

[test] DisconnectedBlockTransactions::DynamicMemoryUsage

9b3da70

ismaelsadeeq force-pushed the 09-2023-follow-up-28385 branch from 2c40895 to 9b3da70 Compare October 19, 2023 15:15

stickies-v approved these changes Oct 19, 2023

View reviewed changes

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor October 19, 2023 19:33

BrandonOdiwuor approved these changes Oct 20, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 20, 2023

build: Introduce internal kernel library #28690

Open

glozow reviewed Nov 2, 2023

View reviewed changes

glozow merged commit 023418a into bitcoin:master Nov 2, 2023

hebasto mentioned this pull request Nov 7, 2023

Rebased cmake-staging branch (post PR 44) hebasto/bitcoin#46

Closed

ismaelsadeeq deleted the 09-2023-follow-up-28385 branch June 27, 2024 11:16

bitcoin locked and limited conversation to collaborators Jun 27, 2025

tests, bug fix: DisconnectedBlockTransactions rewrite followups #28530

tests, bug fix: DisconnectedBlockTransactions rewrite followups #28530

Uh oh!

Conversation

ismaelsadeeq commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrandonOdiwuor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq commented Oct 5, 2023

Uh oh!

ismaelsadeeq commented Oct 10, 2023

Uh oh!

Uh oh!

glozow commented Oct 11, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ismaelsadeeq commented Oct 12, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ismaelsadeeq commented Oct 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Oct 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ismaelsadeeq commented Oct 19, 2023

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq commented Sep 25, 2023 •

edited

Loading

DrahtBot commented Sep 25, 2023 •

edited

Loading

BrandonOdiwuor left a comment •

edited

Loading

stickies-v Oct 12, 2023 •

edited

Loading

ismaelsadeeq commented Oct 13, 2023 •

edited

Loading

stickies-v left a comment •

edited

Loading