Remove CTxMemPool params from ATMP #23465

lsilva01 · 2021-11-08T04:43:37Z

Remove CTxMemPool parameter from AcceptToMemoryPool function, as suggested in #23437 (comment) .

This requires that CChainState has access to MockedTxPool in tx_pool.cpp as mentioned #23173 (comment). So the MockedTxPool is attributed to CChainState::m_mempool before calling AcceptToMemoryPool.

Requires #23437.

DrahtBot · 2021-11-08T05:01:35Z

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

Conflicts

No conflicts as of last run.

mjdietzx · 2021-11-15T20:29:18Z

Code Review ACK 988cfd8

stratospher

Concept ACK. This PR removes CTxMemPool pool argument from AcceptToMemoryPool() since it can be obtained from CChainState active_chainstate.m_mempool

src/validation.cpp

promag

Code review ACK 988cfd8.

This change simplifies ATMP since chain params and the mempool are accessible from the chainstate.

It also removes the intermediate AcceptToMemoryPoolWithTime causing all ATMP callers to explicit set accept_time, usually GetTime().

src/validation.h

lsilva01 · 2021-12-02T06:11:37Z

I changed the approach. Now, the m_mempool remains private and the getter and setter were added, as suggested by @stratospher in #23448 (review) and @promag in #23465 (comment) .

Not sure if this is idiomatic (c++ style). Otherwise, I can revert to original approach (with public m_mempool).

src/validation.h

src/test/fuzz/tx_pool.cpp

src/validation.h

src/validation.cpp

src/test/fuzz/tx_pool.cpp

src/validation.cpp

jonatack

Approach ACK 2cc5a38, will review properly.

src/validation.cpp

Co-authored-by: John Newbery <1063656+jnewbery@users.noreply.github.com> Co-authored-by: Jon Atack <jon@atack.com>

jnewbery · 2021-12-08T08:48:11Z

utACK f1f10c0

maflcko

left some nits in the test, but no blockers

review ACK f1f10c0 🔙

Show signature

Signature:

maflcko · 2021-12-08T08:54:47Z

src/validation.cpp

+    assert(active_chainstate.GetMempool() != nullptr);
+    CTxMemPool& pool{*active_chainstate.GetMempool()};


Suggested change

assert(active_chainstate.GetMempool() != nullptr);

CTxMemPool& pool{*active_chainstate.GetMempool()};

CTxMemPool& pool{*Assert(active_chainstate.GetMempool())};

nit: Can be written shorter

maflcko · 2021-12-08T08:55:56Z

src/test/fuzz/tx_pool.cpp

+class DummyChainState final : public CChainState
+{
+public:


Suggested change

class DummyChainState final : public CChainState

{

public:

struct DummyChainState final : public CChainState {

nit: Can be written shorter, but might be controversial to use struct here. I think in tests it is fine.

I'd prefer to declare this as a class, even if it requires an additional line for the access specifier (since it's a class and not just a data structure).

maflcko · 2021-12-08T08:56:32Z

src/test/fuzz/tx_pool.cpp

+    void SetMempool(CTxMemPool* mempool)
+    {
+        m_mempool = mempool;


nit: mempool is never null, so can use a reference

Suggested change

void SetMempool(CTxMemPool* mempool)

{

m_mempool = mempool;

void SetMempool(CTxMemPool& mempool)

{

m_mempool = &mempool;

jonatack · 2021-12-08T11:56:11Z

Post-merge ACK, modulo that I'm not sure if the CChainState::GetMempool() getter adds value. I've proposed a follow-up to remove it that picks up some of Marco's review feedback.

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

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: Co-authored-by: John Newbery <1063656+jnewbery@users.noreply.github.com> Co-authored-by: Jon Atack <jon@atack.com> This is a backport of [[bitcoin/bitcoin#23465 | core#23465]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12457

lsilva01 force-pushed the remove_pool_params_atmp branch from 5c548b1 to b028df7 Compare November 8, 2021 05:17

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

lsilva01 mentioned this pull request Nov 8, 2021

refactor: AcceptToMemoryPool #23437

Merged

DrahtBot added the Needs rebase label Nov 9, 2021

lsilva01 force-pushed the remove_pool_params_atmp branch from b028df7 to 17eb627 Compare November 13, 2021 06:15

lsilva01 changed the title ~~Remove CChainParams and CTxMemPool params from ATMP~~ Remove CTxMemPool params from ATMP Nov 13, 2021

lsilva01 force-pushed the remove_pool_params_atmp branch from 17eb627 to 988cfd8 Compare November 13, 2021 06:35

DrahtBot removed the Needs rebase label Nov 13, 2021

This was referenced Nov 18, 2021

scripted-diff: Use clang-tidy syntax for C++ named arguments #23545

Closed

scripted-diff: Use clang-tidy syntax for C++ named arguments (tests only) #23546

Merged

stratospher reviewed Nov 25, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

promag reviewed Nov 25, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Dec 1, 2021

lsilva01 force-pushed the remove_pool_params_atmp branch from 988cfd8 to 1d96cbc Compare December 2, 2021 04:55

DrahtBot removed the Needs rebase label Dec 2, 2021

lsilva01 force-pushed the remove_pool_params_atmp branch from 1d96cbc to 358f94a Compare December 2, 2021 05:12

maflcko reviewed Dec 2, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

maflcko reviewed Dec 2, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

lsilva01 force-pushed the remove_pool_params_atmp branch from 358f94a to 3f7d1c1 Compare December 2, 2021 19:20

maflcko reviewed Dec 3, 2021

View reviewed changes

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

lsilva01 force-pushed the remove_pool_params_atmp branch from 3f7d1c1 to 83b3697 Compare December 3, 2021 22:21

jnewbery reviewed Dec 7, 2021

View reviewed changes

lsilva01 force-pushed the remove_pool_params_atmp branch 2 times, most recently from b6489b9 to d784d1e Compare December 7, 2021 14:53

jnewbery reviewed Dec 7, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

lsilva01 force-pushed the remove_pool_params_atmp branch from d784d1e to 2cc5a38 Compare December 7, 2021 18:32

jonatack reviewed Dec 7, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

lsilva01 force-pushed the remove_pool_params_atmp branch from 9e426c9 to 7503606 Compare December 7, 2021 21:52

Remove CTxMemPool params from ATMP

f1f10c0

Co-authored-by: John Newbery <1063656+jnewbery@users.noreply.github.com> Co-authored-by: Jon Atack <jon@atack.com>

lsilva01 force-pushed the remove_pool_params_atmp branch from 7503606 to f1f10c0 Compare December 7, 2021 21:57

maflcko added Refactoring and removed RPC/REST/ZMQ P2P Validation Utils/log/libs labels Dec 8, 2021

maflcko approved these changes Dec 8, 2021

View reviewed changes

maflcko merged commit 84d921e into bitcoin:master Dec 8, 2021

jonatack mentioned this pull request Dec 8, 2021

validation, refactor: remove CChainState::GetMempool() #23704

Closed

lsilva01 deleted the remove_pool_params_atmp branch December 8, 2021 12:06

bitcoin locked and limited conversation to collaborators Dec 8, 2022

		assert(active_chainstate.GetMempool() != nullptr);
		CTxMemPool& pool{*active_chainstate.GetMempool()};

	assert(active_chainstate.GetMempool() != nullptr);
	CTxMemPool& pool{*active_chainstate.GetMempool()};
	CTxMemPool& pool{*Assert(active_chainstate.GetMempool())};

Remove CTxMemPool params from ATMP #23465

Remove CTxMemPool params from ATMP #23465

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

mjdietzx commented Nov 15, 2021

Uh oh!

stratospher 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!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lsilva01 commented Dec 2, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Dec 8, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 8, 2021

Choose a reason for hiding this comment

Uh oh!

jonatack commented Dec 8, 2021

Uh oh!

Uh oh!

lsilva01 commented Nov 8, 2021 •

edited

Loading

DrahtBot commented Nov 8, 2021 •

edited

Loading

stratospher left a comment •

edited

Loading