refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

TheCharlatan · 2023-09-02T09:47:52Z

Motivation

It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
Code external to mempool should ideally use its public helper methods instead of accessing mapTx or its iterators directly.
Reduce the number of complex boost multi index type interactions
Also see RFC: Remove boost usage from kernel api / headers #28335 for further context/motivation. This PR together with [refactor] rewrite DisconnectedBlockTransactions to not use boost #28385 simplifies that one.

Overview of things done in this PR:

Make vTxHashes a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with GetEntry instead of being cached in vTxHashes.
Introduce GetEntry helper method to replace the more involved GetIter where applicable
Replace mapTx access with CTxMemPool helper methods
Simplify checkChainLimits call in node/interfaces.cpp
Make CTxMemPoolEntrys lockPointsmutable such that they can be changed with a const iterator directly instead of going through mapTx
Make BlockAssembler's inBlock and failedTx sets of transaction hashes.

DrahtBot · 2023-09-02T09:47:56Z

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, glozow, maflcko
Concept ACK	Sjors
Stale ACK	ismaelsadeeq

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:

#28368 (Fee Estimator updates from Validation Interface/CScheduler thread by ismaelsadeeq)
#28335 (RFC: Remove boost usage from kernel api / headers 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 · 2023-09-05T13:32:01Z

Concept ACK fwiw
As additional motivation, I imagine this would also make it easier to change the internal implementation of CTxMemPool without touching all these files.

ismaelsadeeq

Concept ACK on the changes on in this PR.

This refactor all seems relevant.
The commits are really atomic and easier to review. 💯

src/node/interfaces.cpp

src/test/blockencodings_tests.cpp

TheCharlatan · 2023-09-25T10:56:12Z

Rebased a5bdd7e -> f0577ea (simplifyMemPoolInteractions_0 -> simplifyMemPoolInteractions_1, compare)

The behavior is the same as CalculateMemPoolAncestors. The only difference is the string returned, and the string is discarded anyway since checkChainLimits only cares about pass/fail.

Allows calling UpdateLockPoints() with a (const) txiter. Note that this was already possible for caller using mapTx.modify(txiter). The point here is to not be accessing mapTx when doing so.

vTxHashes exposes a complex mapTx iterator type that its external users don't need. Directly populate it with CTransactionRef instead.

-BEGIN VERIFY SCRIPT- git grep -l "vTxHashesIdx" src | xargs sed -i "s/vTxHashesIdx/idx_randomized/g" git grep -l "vTxHashes" src | xargs sed -i "s/vTxHashes/txns_randomized/g" -END VERIFY SCRIPT-

Use the helper function instead of reaching into the mapTx member object.

TheCharlatan · 2023-11-10T16:06:55Z

Updated 3b73acb -> 4dd94ca (simplifyMemPoolInteractions_15 -> simplifyMemPoolInteractions_16, compare)

Applied @stickies-v's patch

src/kernel/mempool_entry.h

stickies-v

re-ACK 4dd94ca

glozow · 2023-11-10T17:00:32Z

reACK 4dd94ca

maflcko · 2023-11-11T16:22:47Z

re-ACK 4dd94ca 👝

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b 👝
bfX5G2mlfoBXb55qisEcFZjk5n7/wUwHbMjlDI8yW0PZkGts+ABxmKmEYnMVoT8uNoolCpfL0MahaEHSXhgUCQ==

sdaftuar · 2023-12-07T03:19:00Z

src/txmempool.cpp

+
+    std::vector<CTxMemPoolEntryRef> ret;
+    ret.reserve(mapTx.size());
+    for (const auto& it : GetSortedDepthAndScore()) {


Is there a reason that we are invoking GetSortedDepthAndScore() here, ie do any of the callers rely on a particular sort order?

(I'm planning to eliminate this function in #28676, so just want to understand if this behavior needs to be preserved for some reason.)

This function is supposed to mimic infoAll, which was originally used in this patchset to retrieve mempool information instead of directly accessing mapTx. Looking at its usage you are right, the way I read this too there were no order promises made prior to this patch. I opened #29019 to instead just iterate through mapTx.

…mits` error message to wallet 8dec9c5 wallet, mempool: propagete `checkChainLimits` error message to wallet (ismaelsadeeq) Pull request description: * Requested in [#28391 comment](bitcoin/bitcoin#28391 (comment)) * The error message is static when a new transaction is created and package limit is reached. `Transaction has too long of a mempool chain` While the [`CTxMempool::CheckPackageLimits`](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/txmempool.cpp#L199) provide explicit information about the error message. * This PR updates [`CTxMempool::CheckPackageLimits`](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/txmempool.cpp#L199) return type to `util::Result<void>`, `CheckPackageLimits` now returns void when package limit is not hit, and returns the error string whenever package limit is hit instead of using out parameter `errString`. * The PR updates [`checkChainLimits`](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/node/interfaces.cpp#L703) return type to `util::Result<void>`. * Now the wallet `CreateTransactionInternal` will have access to the package limit error string whenever its hit. * Also Updated functional test to reflect the error message from `CTxMempool::CheckPackageLimits` output. ACKs for top commit: glozow: utACK 8dec9c5 Sjors: utACK 8dec9c5 TheCharlatan: Re-ACK 8dec9c5 Tree-SHA512: ddeac18aeba6f8e3be0e3fe76bf3db655352e3b415169f1f83ea1e8976a2f3e3de021c8da6880eb8382ab52d545e418e3f4d57adcc68ecb4f390339710ee6f30

glozow · 2024-01-05T16:42:20Z

Should we backport 453b481?

Instead of reaching into the mapTx data structure, use a helper method that provides the required vector of CTxMemPoolEntry pointers. Github-Pull: bitcoin#28391 Rebased-From: 453b481

11f3a7e Use hardened runtime on macOS release builds. (Mark Friedenbach) ac1b9a5 [test] import descriptor wallet with reorged parent + IsFromMe child in mempool (glozow) ecb8ebc [test] rescan legacy wallet with reorged parent + IsFromMe child in mempool (Gloria Zhao) 438ac29 snapshots: don't core dump when running -checkblockindex after `loadtxoutset` (Mark Friedenbach) 7ec3455 [log] mempool loading (glozow) fe0f8fe net: create I2P sessions with both ECIES-X25519 and ElGamal encryption (Jon Atack) fc62271 [refactor] Add helper for iterating through mempool entries (stickies-v) Pull request description: Backports for 26.x. Includes: - 453b481 from #28391 - #29179 - #29200 - #29227 - #28791 - #29127 ACKs for top commit: stickies-v: ACK 11f3a7e Tree-SHA512: 20ef871ec768f2328056d83f958e595b36ae9b8baa8a6e8b0e1f02c3df4b16965a8e05dcb4323afbcc9ecc4bdde10931232512022c39ee7e12baf9795bf25bf1

DrahtBot added the Refactoring label Sep 2, 2023

TheCharlatan changed the title ~~refactor: Simplify CTxMempool / Miner fields, remove some external mapTx usage~~ refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx usage Sep 2, 2023

TheCharlatan force-pushed the simplifyMemPoolInteractions branch from 9f854df to 18503e0 Compare September 2, 2023 13:52

This was referenced Sep 2, 2023

Fee Estimator updates from Validation Interface/CScheduler thread #28368

Merged

Fix waste calculation in SelectionResult #28366

Merged

RFC: Remove boost usage from kernel api / headers #28335

Closed

DrahtBot added the CI failed label Sep 2, 2023

DrahtBot mentioned this pull request Sep 2, 2023

Bump unconfirmed ancestor transactions to target feerate #26152

Merged

TheCharlatan changed the title ~~refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx usage~~ refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access Sep 3, 2023

DrahtBot removed the CI failed label Sep 4, 2023

glozow mentioned this pull request Sep 4, 2023

[refactor] rewrite DisconnectedBlockTransactions to not use boost #28385

Merged

TheCharlatan marked this pull request as ready for review September 5, 2023 06:22

TheCharlatan force-pushed the simplifyMemPoolInteractions branch from 18503e0 to 93be096 Compare September 6, 2023 05:12

DrahtBot mentioned this pull request Sep 11, 2023

wallet: Receive silent payment transactions #28453

Closed

DrahtBot added the Needs rebase label Sep 14, 2023

TheCharlatan force-pushed the simplifyMemPoolInteractions branch from 93be096 to a5bdd7e Compare September 15, 2023 05:48

DrahtBot added CI failed and removed Needs rebase labels Sep 15, 2023

ismaelsadeeq reviewed Sep 24, 2023

View reviewed changes

src/node/interfaces.cpp Outdated Show resolved Hide resolved

src/test/blockencodings_tests.cpp Show resolved Hide resolved

TheCharlatan force-pushed the simplifyMemPoolInteractions branch from a5bdd7e to f0577ea Compare September 25, 2023 10:56

DrahtBot removed the CI failed label Sep 25, 2023

DrahtBot mentioned this pull request Sep 27, 2023

validate package transactions with their in-package ancestor sets #26711

Closed

glozow requested a review from stickies-v October 3, 2023 14:28

DrahtBot mentioned this pull request Oct 18, 2023

[WIP] Cluster mempool implementation #28676

Draft

8 tasks

DrahtBot mentioned this pull request Oct 26, 2023

doc: Fix bugprone-lambda-function-name errors #28737

Merged

DrahtBot added the Needs rebase label Oct 30, 2023

TheCharlatan force-pushed the simplifyMemPoolInteractions branch from f0577ea to 5fb9ddf Compare October 30, 2023 16:17

glozow and others added 10 commits November 10, 2023 16:44

[refactor] remove access to mapTx from rpc/mempool.cpp

8892d6b

[refactor] remove access to mapTx in blockencodings_tests.cpp

f80909e

[refactor] remove access to mapTx.find in mempool_tests.cpp

dbc5bdb

[refactor] use CheckPackageLimits for checkChainLimits

1bf4855

The behavior is the same as CalculateMemPoolAncestors. The only difference is the string returned, and the string is discarded anyway since checkChainLimits only cares about pass/fail.

[txmempool] make CTxMemPoolEntry::lockPoints mutable

333367a

Allows calling UpdateLockPoints() with a (const) txiter. Note that this was already possible for caller using mapTx.modify(txiter). The point here is to not be accessing mapTx when doing so.

[refactor] remove access to mapTx in validation.cpp

938643c

[refactor] rewrite vTxHashes as a vector of CTransactionRef

a03aef9

vTxHashes exposes a complex mapTx iterator type that its external users don't need. Directly populate it with CTransactionRef instead.

scripted-diff: rename vTxHashes to txns_randomized

55b0939

-BEGIN VERIFY SCRIPT- git grep -l "vTxHashesIdx" src | xargs sed -i "s/vTxHashesIdx/idx_randomized/g" git grep -l "vTxHashes" src | xargs sed -i "s/vTxHashes/txns_randomized/g" -END VERIFY SCRIPT-

[refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids

d0cd2e8

[refactor] remove access to mapTx in validation_block_tests

4dd94ca

Use the helper function instead of reaching into the mapTx member object.

TheCharlatan force-pushed the simplifyMemPoolInteractions branch from 3b73acb to 4dd94ca Compare November 10, 2023 16:06

maflcko reviewed Nov 10, 2023

View reviewed changes

src/kernel/mempool_entry.h Show resolved Hide resolved

stickies-v approved these changes Nov 10, 2023

View reviewed changes

DrahtBot requested a review from maflcko November 10, 2023 16:48

DrahtBot removed the request for review from glozow November 10, 2023 17:00

DrahtBot removed the request for review from maflcko November 11, 2023 16:22

fanquake merged commit dd5f571 into bitcoin:master Nov 13, 2023
16 checks passed

ismaelsadeeq mentioned this pull request Nov 13, 2023

wallet, mempool: propagete checkChainLimits error message to wallet #28863

Merged

sdaftuar reviewed Dec 7, 2023

View reviewed changes

TheCharlatan mentioned this pull request Dec 7, 2023

mempool: Don't sort in entryAll #29019

Closed

glozow mentioned this pull request Jan 9, 2024

[26.x] more backports #29209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

TheCharlatan commented Sep 2, 2023 •

edited

DrahtBot commented Sep 2, 2023 •

edited

glozow commented Sep 5, 2023

ismaelsadeeq left a comment

TheCharlatan commented Sep 25, 2023

TheCharlatan commented Nov 10, 2023

stickies-v left a comment

glozow commented Nov 10, 2023

maflcko commented Nov 11, 2023

sdaftuar Dec 7, 2023

TheCharlatan Dec 7, 2023

glozow commented Jan 5, 2024

refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access #28391

Conversation

TheCharlatan commented Sep 2, 2023 • edited

DrahtBot commented Sep 2, 2023 • edited

Code Coverage

Reviews

Conflicts

glozow commented Sep 5, 2023

ismaelsadeeq left a comment

Choose a reason for hiding this comment

TheCharlatan commented Sep 25, 2023

TheCharlatan commented Nov 10, 2023

stickies-v left a comment

Choose a reason for hiding this comment

glozow commented Nov 10, 2023

maflcko commented Nov 11, 2023

sdaftuar Dec 7, 2023

Choose a reason for hiding this comment

TheCharlatan Dec 7, 2023

Choose a reason for hiding this comment

glozow commented Jan 5, 2024

TheCharlatan commented Sep 2, 2023 •

edited

DrahtBot commented Sep 2, 2023 •

edited