refactor: AcceptToMemoryPool #23437

lsilva01 · 2021-11-04T01:36:49Z

This PR refactors AcceptToMemoryPool.

. Remove AcceptToMemoryPoolWithTime (after #23173, this function is no longer needed).
. Remove the CChainParams chainparams parameter from ATMP as they can be inferred from the current chain state.
. Update the tx_pool test with new function signature.

DrahtBot · 2021-11-04T08:23:31Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23546 (scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) by MarcoFalke)
#23465 (Remove CTxMemPool params from ATMP by lsilva01)

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.

jnewbery

Concept ACK

I also have a branch that does this (amongst other things) here: https://github.com/jnewbery/bitcoin/tree/2021-09-process-transaction-2. Feel free to take anything you find useful from that branch.

src/validation.h

glozow

Concept ACK, good cleanup.

src/validation.h

lsilva01 · 2021-11-05T04:01:04Z

@jnewbery and @glozow thanks for reviews.
I addressed your suggestions in 034e827.

As these changes are already implemented in another branch, with more other commits, I can close this PR if it's better.

jnewbery

A few more comments inline

As these changes are already implemented in another branch, with more other commits, I can close this PR if it's better.

No, please continue with this PR! Just feel free to take anything from the other branch that you find useful.

src/validation.h

src/test/fuzz/tx_pool.cpp

src/validation.cpp

theStack · 2021-11-05T14:48:26Z

Concept ACK

theStack · 2021-11-07T01:31:09Z

Possible follow-up idea that came to my mind while reviewing this PR: couldn't we simplify the interface of AcceptToMemoryPool by getting rid of both the CChainParams and CTxMemPool parameters, since those can be deduced from the chainstate anyway? The change would look something like

-MempoolAcceptResult AcceptToMemoryPool(const CChainParams& chainparams, CTxMemPool& pool,
-                                       CChainState& active_chainstate, const CTransactionRef& tx,
+MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, const CTransactionRef& tx,
                                        int64_t accept_time, bool bypass_limits, bool test_accept)
     EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
+    const CChainParams& chainparams{active_chainstate.m_params};
+    assert(active_chainstate.m_mempool != nullptr);
+    CTxMemPool& pool{*active_chainstate.m_mempool};
     std::vector<COutPoint> coins_to_uncache;
     MemPoolAccept::ATMPArgs args { chainparams, accept_time, bypass_limits, coins_to_uncache,
                                    test_accept, /* m_allow_bip125_replacement */ true };

I couldn't think of a case where passing chainparams or a mempool instance that doesn't belong to the active chainstate makes much sense. Drawback though: currently the member m_mempool is protected and thus not accessible from outside, that one would need to move to the public section, like it was done for m_params in the last commit of this PR.

lsilva01 · 2021-11-08T06:19:23Z

Thanks for the review @theStack . I opened PR #23465 to test your suggestion.
It seems to work fine.

jnewbery · 2021-11-10T13:55:33Z

#23173 is merged. Please rebase and I'll review.

src/test/fuzz/tx_pool.cpp

src/validation.h

jnewbery · 2021-11-11T12:11:04Z

Please update the PR description now that #23173 is merged.

lsilva01 · 2021-11-22T19:15:53Z

@theStack and @stratospher thanks for the review.
The commit were squashed, as suggested.

src/test/fuzz/tx_pool.cpp

theStack

re-ACK 4ec8b96

jnewbery · 2021-12-01T13:27:06Z

utACK 4ec8b96

Happy to rereview this once it's rebased.

theStack

re-ACK 123f5de

jnewbery · 2021-12-01T16:33:04Z

reACK 123f5de

glozow · 2021-12-01T16:37:54Z

light code review ACK 123f5de

maflcko · 2021-12-01T16:53:32Z

src/validation.h

 * @param[in]  tx                 The transaction to submit for mempool acceptance.
+ * @param[in]  accept_time        The timestamp for adding the transaction to the mempool. Usually
+ *                                the current system time, but may be different.


nit: I don't think we need to explain what the argument means at the call site.

f1f10c0 Remove CTxMemPool params from ATMP (lsilva01) Pull request description: Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in bitcoin/bitcoin#23437 (comment) . This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned bitcoin/bitcoin#23173 (comment). So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`. Requires #23437. ACKs for top commit: jnewbery: utACK f1f10c0 MarcoFalke: review ACK f1f10c0 🔙 Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349

f1f10c0 Remove CTxMemPool params from ATMP (lsilva01) Pull request description: Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in bitcoin#23437 (comment) . This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned bitcoin#23173 (comment). So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`. Requires bitcoin#23437. ACKs for top commit: jnewbery: utACK f1f10c0 MarcoFalke: review ACK f1f10c0 🔙 Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349

93134c7 Remove calls to global Params() in tx_pool test (lsilva01) bfdb258 Remove AcceptToMemoryPoolWithTime (lsilva01) Pull request description: This PR refactors AcceptToMemoryPool. . Remove `AcceptToMemoryPoolWithTime` (after #23173, this function is no longer needed). . Remove the `CChainParams chainparams` parameter from ATMP as they can be inferred from the current chain state. . Update the `tx_pool` test with new function signature. ACKs for top commit: jnewbery: reACK 93134c7 glozow: light code review ACK 93134c7 theStack: re-ACK 93134c7 Tree-SHA512: b672d9f091f5fcb4d163c0a443aa469b2ce6f37201136803201c620c3fa329ffb41abd50c5b528787fd1c47e8e09af5ec20ddaa0a4235352c9c619e5691dddb6

efbb49e Remove CTxMemPool params from ATMP (lsilva01) Pull request description: Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in bitcoin/bitcoin#23437 (comment) . This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned bitcoin/bitcoin#23173 (comment). So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`. Requires #23437. ACKs for top commit: jnewbery: utACK efbb49e MarcoFalke: review ACK efbb49e 🔙 Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349

Summary: This is a backport of [[bitcoin/bitcoin#23437 | core#23437]] bitcoin/bitcoin@9360778 The fuzzer part of the first commit and the second commit are not applicable, due to missing fuzzer backports. Depends on D12236 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12238

DrahtBot added P2P RPC/REST/ZMQ Utils/log/libs Validation labels Nov 4, 2021

jnewbery reviewed Nov 4, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

glozow reviewed Nov 4, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 4, 2021

net_processing: lock clean up #21527

Closed

lsilva01 force-pushed the remove_atmp_time branch from 20867dc to 034e827 Compare November 5, 2021 03:46

DrahtBot mentioned this pull request Nov 5, 2021

validation/refactor: refactoring for package submission #23381

Merged

jnewbery reviewed Nov 5, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 5, 2021

validation: mempool validation and submission for packages of 1 child + parents #22674

Merged

lsilva01 force-pushed the remove_atmp_time branch 2 times, most recently from 1333ddf to aac6548 Compare November 5, 2021 19:21

lsilva01 mentioned this pull request Nov 5, 2021

refactor, consensus: remove calls to global Params() in validation layer #23448

Closed

DrahtBot mentioned this pull request Nov 6, 2021

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

lsilva01 mentioned this pull request Nov 8, 2021

Remove CTxMemPool params from ATMP #23465

Merged

DrahtBot added the Needs rebase label Nov 9, 2021

lsilva01 force-pushed the remove_atmp_time branch from aac6548 to 5b2f49d Compare November 10, 2021 18:29

DrahtBot removed the Needs rebase label Nov 10, 2021

lsilva01 force-pushed the remove_atmp_time branch from 5b2f49d to 9f96751 Compare November 10, 2021 20:31

jnewbery reviewed Nov 11, 2021

View reviewed changes

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

maflcko mentioned this pull request Nov 11, 2021

rpc: Only allow specific types to be P2(W)SH wrapped in decodescript #23486

Merged

lsilva01 force-pushed the remove_atmp_time branch from 9f96751 to c3d1519 Compare November 11, 2021 21:54

maflcko added the Refactoring label Nov 22, 2021

maflcko changed the title ~~refactor, mempool: refactor AcceptToMemoryPool~~ refactor: AcceptToMemoryPool Nov 22, 2021

maflcko requested review from glozow and jnewbery November 22, 2021 09:26

lsilva01 force-pushed the remove_atmp_time branch from 863703c to 8f46eb5 Compare November 22, 2021 19:10

theStack reviewed Nov 23, 2021

View reviewed changes

src/test/fuzz/tx_pool.cpp Outdated Show resolved Hide resolved

lsilva01 force-pushed the remove_atmp_time branch from 8f46eb5 to 4ec8b96 Compare November 24, 2021 03:02

theStack approved these changes Nov 27, 2021

View reviewed changes

DrahtBot added the Needs rebase label Dec 1, 2021

lsilva01 added 2 commits December 1, 2021 10:44

Remove AcceptToMemoryPoolWithTime

9360778

Remove calls to global Params() in tx_pool test

123f5de

lsilva01 force-pushed the remove_atmp_time branch from 4ec8b96 to 123f5de Compare December 1, 2021 15:10

theStack approved these changes Dec 1, 2021

View reviewed changes

DrahtBot removed the Needs rebase label Dec 1, 2021

maflcko reviewed Dec 1, 2021

View reviewed changes

maflcko merged commit 1c06e1a into bitcoin:master Dec 1, 2021

lsilva01 deleted the remove_atmp_time branch December 1, 2021 17:00

This was referenced Dec 1, 2021

test: Extend test coverage of BIP125 and document confusing/inconsistent behavior #22867

Closed

Implement RBF inherited signaling and fix getmempoolentry returned bip125-replaceable status #22698

Closed

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 2, 2021

Merge bitcoin#23437: refactor: AcceptToMemoryPool

3734ee4

bitcoin locked and limited conversation to collaborators Dec 2, 2022

refactor: AcceptToMemoryPool #23437

refactor: AcceptToMemoryPool #23437

Uh oh!

Conversation

lsilva01 commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsilva01 commented Nov 5, 2021

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theStack commented Nov 5, 2021

Uh oh!

theStack commented Nov 7, 2021

Uh oh!

lsilva01 commented Nov 8, 2021

Uh oh!

jnewbery commented Nov 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Nov 11, 2021

Uh oh!

lsilva01 commented Nov 22, 2021

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 1, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 1, 2021

Uh oh!

glozow commented Dec 1, 2021

Uh oh!

maflcko Dec 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsilva01 commented Nov 4, 2021 •

edited

Loading

DrahtBot commented Nov 4, 2021 •

edited

Loading