rpc: Use mempool from node context instead of global by maflcko · Pull Request #17564 · bitcoin/bitcoin

maflcko · 2019-11-22T20:31:40Z

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.

DrahtBot · 2019-11-22T23:41:03Z

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 · 2019-11-23T13:44:29Z

Concept ACK

practicalswift · 2019-11-25T00:09:26Z

Concept ACK

ryanofsky

Code review ACK fa963d3818afea82061615b9939361444e5b5c2d

src/test/rpc_tests.cpp

ryanofsky

Code review ACK fa7c5017a9056b33e230d78f7ff6879e3d8941a5. Only change since last review is suggested rpc test simplification

jnewbery

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/rest.cpp

jnewbery · 2019-12-02T16:30:48Z

src/rpc/blockchain.cpp

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)

src/rpc/mining.cpp

src/rest.cpp

src/node/coin.h

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.

ryanofsky

Code review ACK faaa239. There were a lot of changes since my previous review, so I just reviewed it a second time.

practicalswift · 2019-12-13T22:41:08Z

ACK faaa239 -- diff looks correct

Thanks for doing this!

src/rest.cpp

jnewbery · 2019-12-15T03:52:11Z

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

Code review ACK faaa239.

Why not also change ::mempool usages in ChainImpl?

src/rest.cpp

maflcko · 2019-12-16T15:47:12Z

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

ryanofsky

Code review ACK fa8e650, Only the discussed REST server changes since the last review.

jnewbery · 2019-12-16T20:49:14Z

Code review ACK fa8e650

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

…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

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

…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 added Refactoring RPC/REST/ZMQ labels Nov 22, 2019

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

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

ryanofsky approved these changes Nov 25, 2019

View reviewed changes

src/test/rpc_tests.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from fa963d3 to fa7c501 Compare November 25, 2019 16:10

ryanofsky approved these changes Nov 25, 2019

View reviewed changes

jnewbery reviewed Dec 2, 2019

View reviewed changes

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 force-pushed the 1911-rpcNoTxPoolGlobal branch 2 times, most recently from fa0ae3b to fac3589 Compare December 5, 2019 19:19

node: Use mempool from node context instead of global

fa660d6

maflcko force-pushed the 1911-rpcNoTxPoolGlobal branch from fac3589 to faaa239 Compare December 5, 2019 19:22

ryanofsky approved these changes Dec 13, 2019

View reviewed changes

jnewbery reviewed Dec 13, 2019

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

src/rest.cpp Outdated Show resolved Hide resolved

promag reviewed Dec 15, 2019

View reviewed changes

src/rest.cpp Outdated Show resolved Hide resolved

rest: Use mempool from node context instead of global

fa8e650

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

ryanofsky approved these changes Dec 16, 2019

View reviewed changes

maflcko merged commit fa8e650 into bitcoin:master Dec 16, 2019

maflcko deleted the 1911-rpcNoTxPoolGlobal branch December 16, 2019 21:07

maflcko mentioned this pull request Jan 25, 2020

refactor: Remove mempool global from net #17997

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

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

Conversation

maflcko commented Nov 22, 2019

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

laanwj commented Nov 23, 2019

Uh oh!

practicalswift commented Nov 25, 2019

Uh oh!

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

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Dec 13, 2019

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Dec 15, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 16, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented Nov 22, 2019 •

edited

Loading