Skip to content

rpc: Use mempool from node context instead of global #17564

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
maflcko merged 3 commits into from
Dec 16, 2019
Merged

rpc: Use mempool from node context instead of global #17564

maflcko merged 3 commits into from
Dec 16, 2019

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Nov 22, 2019

Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.

@maflcko maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from faef91b to fa366b2 Compare November 22, 2019 21:25
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 22, 2019 •
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:

  • #16426 (Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard)
  • #15836 (Add feerate histogram to getmempoolinfo by jonasschnelli)

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.

@laanwj
Copy link
Member

laanwj commented Nov 23, 2019

Concept ACK

@maflcko maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from fa366b2 to fa963d3 Compare November 23, 2019 14:55
@practicalswift
Copy link
Contributor

practicalswift commented Nov 25, 2019

Concept ACK

ryanofsky
ryanofsky approved these changes Nov 25, 2019
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 fa963d3818afea82061615b9939361444e5b5c2d

@maflcko maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from fa963d3 to fa7c501 Compare November 25, 2019 16:10
ryanofsky
ryanofsky approved these changes Nov 25, 2019
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 fa7c5017a9056b33e230d78f7ff6879e3d8941a5. Only change since last review is suggested rpc test simplification

jnewbery
jnewbery reviewed Dec 2, 2019
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.

Concept ACK and approach ACK.

There are a few places where I think the new code is less clear than existing code, which I think could be improved.

src/rpc/blockchain.cpp Outdated
Copy link
Contributor

@jnewbery jnewbery Dec 2, 2019

Choose a reason for hiding this comment

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

I think in a future where running without a mempool is possible, we could make this friendlier by just defaulting include_mempool to false for nodes without mempools rather than forcing the user to always pass false as an argument.

(no change needed for this PR but something to add when -nomempool is added as an option)

@maflcko maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from fa7c501 to fa71de7 Compare December 5, 2019 18:41
rpc: Use mempool from node context instead of global
facbaf0 
Currently they are identical, but in the future we might want to turn
the mempool into a unique_ptr. Replacing the global with the mempool
pointer from the node context simplifies this step.
@maflcko maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch 2 times, most recently from fa0ae3b to fac3589 Compare December 5, 2019 19:19
@maflcko maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from fac3589 to faaa239 Compare December 5, 2019 19:22
ryanofsky
ryanofsky approved these changes Dec 13, 2019
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 faaa239. There were a lot of changes since my previous review, so I just reviewed it a second time.

@practicalswift
Copy link
Contributor

practicalswift commented Dec 13, 2019

ACK faaa239 -- diff looks correct

Thanks for doing this!

jnewbery
jnewbery reviewed Dec 13, 2019
View reviewed changes
@jnewbery
Copy link
Contributor

jnewbery commented Dec 15, 2019

It can be null because this version of EnsureMemPool only sets http status headers and doesn't throw an exception.

Ah, you're right. I was confused because the name suggests that it ensures the return value is valid, and the other version of EnsureMemPool throws if the mempool can't be found.

I've implemented a version of Russ's suggested changes here: jnewbery@d27fb22. What do you think, @MarcoFalke ?

promag
promag reviewed Dec 15, 2019
View reviewed changes
Copy link
Contributor

@promag promag 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 faaa239.

Why not also change ::mempool usages in ChainImpl?

@maflcko maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from faaa239 to fa8e650 Compare December 16, 2019 15:45
@maflcko
Copy link
Member Author

maflcko commented Dec 16, 2019

Fixed up and force pushed commit by @jnewbery with suggested rename

ryanofsky
ryanofsky approved these changes Dec 16, 2019
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 fa8e650, Only the discussed REST server changes since the last review.

@jnewbery
Copy link
Contributor

jnewbery commented Dec 16, 2019

Code review ACK fa8e650

maflcko pushed a commit that referenced this pull request Dec 16, 2019
Merge #17564: rpc: Use mempool from node context instead of global
48d64d7 
fa8e650 rest: Use mempool from node context instead of global (MarcoFalke)
fa660d6 node: Use mempool from node context instead of global (MarcoFalke)
facbaf0 rpc: Use mempool from node context instead of global (MarcoFalke)

Pull request description:

  Currently they are identical, but in the future we might want to turn
  the mempool into a unique_ptr. Replacing the global with the mempool
  pointer from the node context simplifies this step.

ACKs for top commit:
  jnewbery:
    Code review ACK fa8e650
  ryanofsky:
    Code review ACK fa8e650, Only the discussed REST server changes since the last review.

Tree-SHA512: 0836f3f39cf90306455962918446e5f8612e88c32072b92afc30929aea1f17430bbda0e2b3668d36c9d6b97d63a93cf4903185194571108642b7bf5a39b89125
@maflcko maflcko merged commit fa8e650 into bitcoin:master Dec 16, 2019
@maflcko maflcko deleted the 1911-rpcNoTxPoolGlobal branch December 16, 2019 21:07
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 16, 2019
Merge bitcoin#17564: rpc: Use mempool from node context instead of gl…
d81f32a 
…obal

fa8e650 rest: Use mempool from node context instead of global (MarcoFalke)
fa660d6 node: Use mempool from node context instead of global (MarcoFalke)
facbaf0 rpc: Use mempool from node context instead of global (MarcoFalke)

Pull request description:

  Currently they are identical, but in the future we might want to turn
  the mempool into a unique_ptr. Replacing the global with the mempool
  pointer from the node context simplifies this step.

ACKs for top commit:
  jnewbery:
    Code review ACK fa8e650
  ryanofsky:
    Code review ACK fa8e650, Only the discussed REST server changes since the last review.

Tree-SHA512: 0836f3f39cf90306455962918446e5f8612e88c32072b92afc30929aea1f17430bbda0e2b3668d36c9d6b97d63a93cf4903185194571108642b7bf5a39b89125
@maflcko maflcko mentioned this pull request Jan 25, 2020
Merged
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 16, 2020
@jasonbcox
Merge #17564: rpc: Use mempool from node context instead of global
73d648a 
Summary:
fa8e650b525e9493bdfa393c0c3e34cb22c78c08 rest: Use mempool from node context instead of global (MarcoFalke)
fa660d65d7cc401ad5bbfdc076a074de19a79329 node: Use mempool from node context instead of global (MarcoFalke)
facbaf092f1ab298943206603cff6e6e3d30d452 rpc: Use mempool from node context instead of global (MarcoFalke)

Pull request description:

  Currently they are identical, but in the future we might want to turn
  the mempool into a unique_ptr. Replacing the global with the mempool
  pointer from the node context simplifies this step.

ACKs for top commit:
  jnewbery:
    Code review ACK fa8e650b5
  ryanofsky:
    Code review ACK fa8e650b525e9493bdfa393c0c3e34cb22c78c08, Only the discussed REST server changes since the last review.

Backport of Core [[bitcoin/bitcoin#17564 | PR17564]]

Depends on D7965

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7966
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
Merge bitcoin#17564: rpc: Use mempool from node context instead of gl…
49046e1 
…obal

fa8e650 rest: Use mempool from node context instead of global (MarcoFalke)
fa660d6 node: Use mempool from node context instead of global (MarcoFalke)
facbaf0 rpc: Use mempool from node context instead of global (MarcoFalke)

Pull request description:

  Currently they are identical, but in the future we might want to turn
  the mempool into a unique_ptr. Replacing the global with the mempool
  pointer from the node context simplifies this step.

ACKs for top commit:
  jnewbery:
    Code review ACK fa8e650
  ryanofsky:
    Code review ACK fa8e650, Only the discussed REST server changes since the last review.

Tree-SHA512: 0836f3f39cf90306455962918446e5f8612e88c32072b92afc30929aea1f17430bbda0e2b3668d36c9d6b97d63a93cf4903185194571108642b7bf5a39b89125
@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

+3 more reviewers

@jnewbery jnewbery jnewbery left review comments

@promag promag promag left review comments

@instagibbs instagibbs instagibbs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @DrahtBot @laanwj @practicalswift @jnewbery @promag @instagibbs @ryanofsky