Skip to content

Remove mempool global from interfaces #19848

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Sep 5, 2020
Merged

Remove mempool global from interfaces #19848

merged 3 commits into from
Sep 5, 2020

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 31, 2020

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
Copy link
Contributor

promag commented Aug 31, 2020

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

@maflcko
Copy link
Member Author

maflcko commented Aug 31, 2020

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))

@maflcko maflcko mentioned this pull request Aug 31, 2020
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 31, 2020 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Member

hebasto commented Aug 31, 2020

Concept ACK.

@practicalswift
Copy link
Contributor

Concept ACK

darosior
darosior reviewed Aug 31, 2020
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

This was referenced Aug 31, 2020
Merged
Merged
@jnewbery
Copy link
Contributor

jnewbery commented Sep 2, 2020

utACK fa5fa7e

@DrahtBot DrahtBot mentioned this pull request Sep 2, 2020
Closed
hebasto
hebasto reviewed Sep 2, 2020
View reviewed changes
ryanofsky
ryanofsky approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa5fa7e. 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
Copy link
Member Author

maflcko commented Sep 3, 2020

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
hebasto requested changes Sep 3, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK fa9124d

@maflcko
Copy link
Member Author

maflcko commented Sep 4, 2020

fixed up and force pushed last doc commit

hebasto
hebasto approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fadb436

jnewbery
jnewbery reviewed Sep 4, 2020
View reviewed changes
Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ACK fadb436

@DrahtBot DrahtBot mentioned this pull request Sep 4, 2020
Closed
ryanofsky
ryanofsky approved these changes Sep 4, 2020
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

darosior
darosior approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested ACK fadb436

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

@maflcko
Copy link
Member Author

maflcko commented Sep 5, 2020

Changed first commit according to feedback.

Untitled png

darosior
darosior approved these changes Sep 5, 2020
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa9ee52

hebasto
hebasto approved these changes Sep 5, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

jnewbery commented Sep 5, 2020

utACK fa9ee52

@maflcko maflcko merged commit 3ba25e3 into bitcoin:master Sep 5, 2020
@maflcko maflcko deleted the 2008-nomemint branch September 5, 2020 14:00
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jul 15, 2021
@PiRK
Remove mempool global from interfaces
d4e8a4c 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@jnewbery jnewbery jnewbery left review comments

@darosior darosior darosior approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@maflcko @promag @DrahtBot @hebasto @practicalswift @jnewbery @ryanofsky @darosior