Remove mempool global from interfaces by maflcko · Pull Request #19848 · bitcoin/bitcoin

maflcko · 2020-08-31T14:50:16Z

The chain interface has an m_node member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See #19556

promag · 2020-08-31T15:03:22Z

Correct me if I'm wrong but m_node.mempool is always set right?

maflcko · 2020-08-31T15:08:42Z

bitcoin/src/init.cpp

Line 1374 in 068bc21

node.mempool = &::mempool;

Currently it will be set during init until shortly before shutdown is done. Though, in the future it may be nullptr for the whole runtime of the program (see also #19629 (comment))

DrahtBot · 2020-08-31T16:55:09Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Avoid locking CTxMemPool::cs recursively in some cases #19872 (Avoid locking CTxMemPool::cs recursively in some cases by hebasto)
ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
Remove mempool global #19556 (Remove mempool global by MarcoFalke)
Wallet passive startup #15719 (Wallet passive startup by ryanofsky)
RPC: Add ancestor{count,size,fees} to listunspent output #12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

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.

hebasto · 2020-08-31T17:42:36Z

Concept ACK.

practicalswift · 2020-08-31T19:09:09Z

Concept ACK

darosior

Concept ACK

jnewbery · 2020-09-02T08:31:34Z

utACK fa5fa7e025164f9ac73741ece697e3ea4e7ed64c

src/policy/rbf.cpp

ryanofsky

Code review ACK fa5fa7e025164f9ac73741ece697e3ea4e7ed64c. Seems good for these calls to function without a mempool. Agree with hebasto it would be nice to clean up locking assertions within IsRBFOptIn. But if that requires more invasive changes to IsRBFOptIn, it would probably be better to do it in a targeted PR before or after this one.

maflcko · 2020-09-03T08:08:06Z

Changed the first commit to remove NO_THREAD_SAFETY_ANALYSIS. Let me know if this is better or if I should revert to the initial version.

hebasto

Approach ACK fa9124d93527fd5ff741000b790a3772f2961a52

src/policy/rbf.h

maflcko · 2020-09-04T06:48:13Z

fixed up and force pushed last doc commit

hebasto

ACK fadb436745515f16efb1330b36e6f3831c0ac5fb

jnewbery

I'm not very keen on the static const mempool and have an alternative.

ACK fadb436745515f16efb1330b36e6f3831c0ac5fb

src/policy/rbf.cpp

ryanofsky

Code review ack fadb436745515f16efb1330b36e6f3831c0ac5fb. Since last review reverted changes to IsRBFOptIn with different IsRBFOptInEmptyMempool workaround. John's third workaround seems good too.

darosior

tested ACK fadb436745515f16efb1330b36e6f3831c0ac5fb

I also prefer jnewberry's patch, but not against merging this without it.

Co-authored-by: John Newbery <[email protected]>

maflcko · 2020-09-05T09:49:06Z

Changed first commit according to feedback.

darosior

ACK fa9ee52

hebasto

re-ACK fa9ee52, since my previous review:

applied @jnewbery's patch
added @jnewbery as a co-author of the "refactor: Add IsRBFOptInEmptyMempool" commit
rebased

jnewbery · 2020-09-05T10:13:07Z

utACK fa9ee52

Summary: > The chain interface has an m_node member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See [[bitcoin/bitcoin#19556 | core#19556]] This is a backport of [[bitcoin/bitcoin#19848 | core#19848]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D9784

maflcko added the Refactoring label Aug 31, 2020

maflcko mentioned this pull request Aug 31, 2020

Remove mempool global #19556

Merged

darosior reviewed Aug 31, 2020

View reviewed changes

This was referenced Aug 31, 2020

RPC: Add ancestor{count,size,fees} to listunspent output #12677

Merged

ZMQ: Create "sequence" notifier, enabling client-side mempool tracking #19572

Merged

DrahtBot mentioned this pull request Sep 2, 2020

Wallet passive startup #15719

Closed

hebasto reviewed Sep 2, 2020

View reviewed changes

src/policy/rbf.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Sep 2, 2020

View reviewed changes

maflcko force-pushed the 2008-nomemint branch from fa5fa7e to fa9124d Compare September 3, 2020 07:38

hebasto requested changes Sep 3, 2020

View reviewed changes

src/policy/rbf.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2008-nomemint branch from fa9124d to fadb436 Compare September 4, 2020 06:43

hebasto approved these changes Sep 4, 2020

View reviewed changes

jnewbery reviewed Sep 4, 2020

View reviewed changes

src/policy/rbf.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Sep 4, 2020

Avoid locking CTxMemPool::cs recursively in some cases #19872

Closed

ryanofsky approved these changes Sep 4, 2020

View reviewed changes

darosior approved these changes Sep 4, 2020

View reviewed changes

MarcoFalke and others added 3 commits September 5, 2020 11:44

refactor: Add IsRBFOptInEmptyMempool

fa83168

Co-authored-by: John Newbery <[email protected]>

Remove mempool global from interfaces

faef4fc

doc: Add doxygen comment to IsRBFOptIn

fa9ee52

maflcko force-pushed the 2008-nomemint branch from 47e4f69 to fa9ee52 Compare September 5, 2020 09:47

darosior approved these changes Sep 5, 2020

View reviewed changes

hebasto approved these changes Sep 5, 2020

View reviewed changes

maflcko merged commit 3ba25e3 into bitcoin:master Sep 5, 2020

maflcko deleted the 2008-nomemint branch September 5, 2020 14:00

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

maflcko commented Aug 31, 2020

Uh oh!

promag commented Aug 31, 2020

Uh oh!

maflcko commented Aug 31, 2020

Uh oh!

DrahtBot commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Aug 31, 2020

Uh oh!

practicalswift commented Aug 31, 2020

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 2, 2020

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 3, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Sep 4, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 5, 2020

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Sep 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Aug 31, 2020 •

edited

Loading