rpc: Add importmempool RPC #27460

maflcko · 2023-04-13T11:19:15Z

Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues:

Users aren't expected to fiddle with the datadir, possibly corrupting it
An existing mempool file in the datadir may be overwritten
The node needs to be restarted
Importing an untrusted file this way is dangerous, because it can corrupt the mempool

Fix all issues by adding a new RPC.

DrahtBot · 2023-04-13T11:19:18Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ajtowns, glozow, achow101
Concept ACK	kevkevinpal, instagibbs, michaelfolkson, svanstaa, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28207 (mempool: Persist with XOR by MarcoFalke)

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.

ghost · 2023-04-13T17:23:07Z

Hi @MarcoFalke

More than 5 tests are failing

maflcko · 2023-04-13T17:56:18Z

I changed the approach to force named RPC options, compatible with #26485

src/rpc/mempool.cpp

kevkevinpal · 2023-04-13T19:22:39Z

Concept ACK 846ff37fe7ac2a5b4f855faaadb3434158ed6364 (need to check latest commit)

tested by doing the following in regtest

bitcoin-cli createwallet "testwallet"
bitcoin-cli getnewaddress
bitcoin-cli generateto address 6 <previous commands output>
bitcoin-cli generate 20 (did this a couple times)
bitcoin-cli getbalance (validate you have a balance)
bitcoin-cli getnewaddress
bitcoin-cli send '{"<above output>": 0.1}' null "unset" null '{"fee_rate": 1}' (did this a few times)

shutdown bitcoind
copy mempool.dat from datadir to different location
delete mempool.dat from datadir
restart bitcoind
bitcoin-cli importmempool ./mempool.dat

Saw this in the logs
2023-04-13T19:12:01Z Imported mempool transactions from disk: 5 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
got {} as response from cli

Might test again and check the exact values

Checking diff of mempool.dat files

did git diff ./datadir/regtest/mempool.dat mempool.dat and it gave me no changes in the vim editor to see
but
diff ./datadir/regtest/mempool.dat mempool.dat gave me
Binary files ./datadir/regtest/mempool.dat and ./mempool.dat differ
not sure if that matters

ajtowns

Approach ACK

src/rpc/mempool.cpp

src/kernel/mempool_persist.h

src/rpc/mempool.cpp

instagibbs · 2023-04-14T13:29:21Z

concept ACK, will review

test/functional/mempool_persist.py

glozow

Concept ACK

test/functional/mempool_persist.py

src/kernel/mempool_persist.cpp

svanstaa · 2023-04-26T18:06:47Z

Why is use_current_time defaulting to true? As far as i understand, this would set all the broadcast times of the transactions in mempool.dat to the current time, effectively extending their lifetimes, to the point that (time-) expired txns would be dumped into the network again.
So I would suggest defaulting the value to false, unless there is some rationale behind it that is not obvious.

michaelfolkson · 2023-04-26T18:21:18Z

Also while we're sneaking PR review club questions on here what do people here use this for? I'm assuming it is useful for mempool dev work (specific details would be interesting) but I can't imagine it is common for a user to want this.

Concept ACK (the RPC approach seems superior assuming people are currently importing mempools without a RPC being available)

maflcko · 2023-04-27T07:41:49Z

The use case would be wanting to sync both the chain and the mempool to include everything/most known by the network or one of your other nodes. This is trivially possible for the chain, however for the mempool it is not.

One option would be to use the mempool P2P message type, or add a new message type. This would likely require lifting some restrictions or adding others to make it usable in a normal setting and still prevent privacy/DoS concerns. To what extent the existing mempool message type behavior can be modified will need to be considered in light of the discussion about its removal.

Another option would be to use a new RPC to import a mempool. In light of people potentially loading untrusted mempool data, I opted to use the same mempool acceptance defaults that would be picked as if the transactions were synced/submitted over P2P.

test/functional/mempool_persist.py

svanstaa · 2023-05-17T11:51:58Z

Concept ACK

kristapsk · 2023-05-17T12:11:13Z

Concept ACK

test_importmempool_union contributed by glozow Co-authored-by: glozow <[email protected]>

ajtowns · 2023-08-07T11:54:16Z

utACK fa776e6

glozow

reACK fa776e6

achow101 · 2023-08-15T14:03:42Z

ACK fa776e6

fa776e6 Add importmempool RPC (MarcoFalke) fa20d73 refactor: Add and use kernel::ImportMempoolOptions (MarcoFalke) fa88669 doc: Clarify the getmempoolinfo.loaded RPC field documentation (MarcoFalke) 6888886 Remove Chainstate::LoadMempool (MarcoFalke) Pull request description: Currently it is possible to import a mempool by placing it in the datadir and starting the node. However this has many issues: * Users aren't expected to fiddle with the datadir, possibly corrupting it * An existing mempool file in the datadir may be overwritten * The node needs to be restarted * Importing an untrusted file this way is dangerous, because it can corrupt the mempool Fix all issues by adding a new RPC. ACKs for top commit: ajtowns: utACK fa776e6 achow101: ACK fa776e6 glozow: reACK fa776e6 Tree-SHA512: fcb1a92d6460839283c546c47a2d930c363ac1013c4c50dc5215ddf9fe5e51921d23fe0abfae0a5a7631983cfc7e2fff3788b70f95937d0a989a203be4d67546

This fixes a silent conflict betwen bitcoin#28123 and bitcoin#27460

…ng oneline description 2394314 rpc: remove one more quote from non-string oneline description (Martin Zumsande) Pull request description: This fixes a silent conflict between bitcoin/bitcoin#28123 (which removed all `\"options\"`) and bitcoin/bitcoin#27460 (which added a new one). It should fix the current CI failures. ACKs for top commit: ajtowns: utACK 2394314 MarcoFalke: lgtm ACK 2394314 jonatack: ACK 2394314 hebasto: ACK 2394314 Tree-SHA512: feb0c2b936a77be45d9c65aa7d738277b2266b5153665fee3b1413045de521195dc7d5efa2fc8b37b22f16e9b8d0ee8de25bfd151a428666122b31f64056557a

…ne description 2394314 rpc: remove one more quote from non-string oneline description (Martin Zumsande) Pull request description: This fixes a silent conflict between bitcoin#28123 (which removed all `\"options\"`) and bitcoin#27460 (which added a new one). It should fix the current CI failures. ACKs for top commit: ajtowns: utACK 2394314 MarcoFalke: lgtm ACK 2394314 jonatack: ACK 2394314 hebasto: ACK 2394314 Tree-SHA512: feb0c2b936a77be45d9c65aa7d738277b2266b5153665fee3b1413045de521195dc7d5efa2fc8b37b22f16e9b8d0ee8de25bfd151a428666122b31f64056557a

fanquake · 2023-10-11T18:12:10Z

Release notes added in #28637.

Co-authored-by: glozow <[email protected]>

1b672eb doc: add release note for #27460 (new `importmempool` RPC) (Sebastian Falbesoner) Pull request description: This PR adds a missing release note for #27460. ACKs for top commit: glozow: ACK 1b672eb Tree-SHA512: 89deadbfd6779e6eb19801c9fe7459a9876b920d44e09df102774c1eb8b3c0716462613dc99d1711eda4bd959ea61595b33f4528424ac02cf1af6cb4e5f1f0e9

…portmempool` RPC) 1b672eb doc: add release note for bitcoin#27460 (new `importmempool` RPC) (Sebastian Falbesoner) Pull request description: This PR adds a missing release note for bitcoin#27460. ACKs for top commit: glozow: ACK 1b672eb Tree-SHA512: 89deadbfd6779e6eb19801c9fe7459a9876b920d44e09df102774c1eb8b3c0716462613dc99d1711eda4bd959ea61595b33f4528424ac02cf1af6cb4e5f1f0e9

DrahtBot changed the title ~~rpc: Add importmempool RPC~~ rpc: Add importmempool RPC Apr 13, 2023

DrahtBot added the RPC/REST/ZMQ label Apr 13, 2023

glozow mentioned this pull request Apr 13, 2023

Remove BIP35 mempool p2p message #27426

Closed

maflcko force-pushed the 2304-import-mempool-rpc- branch from 846ff37 to d31a87f Compare April 13, 2023 17:54

kevkevinpal reviewed Apr 13, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

ajtowns reviewed Apr 14, 2023

View reviewed changes

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

src/kernel/mempool_persist.h Outdated Show resolved Hide resolved

src/rpc/mempool.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Apr 14, 2023

View reviewed changes

test/functional/mempool_persist.py Outdated Show resolved Hide resolved

maflcko force-pushed the 2304-import-mempool-rpc- branch from d31a87f to 1ef7f7a Compare April 14, 2023 14:59

glozow reviewed Apr 17, 2023

View reviewed changes

test/functional/mempool_persist.py Outdated Show resolved Hide resolved

src/kernel/mempool_persist.cpp Outdated Show resolved Hide resolved

maflcko force-pushed the 2304-import-mempool-rpc- branch 2 times, most recently from dad18a4 to fab8b37 Compare April 20, 2023 16:50

DrahtBot added CI failed and removed CI failed labels Apr 20, 2023

dergoegge mentioned this pull request Apr 21, 2023

mempool: keep CPFP'd transactions when loading from mempool.dat #27476

Closed

ismaelsadeeq reviewed Apr 27, 2023

View reviewed changes

test/functional/mempool_persist.py Show resolved Hide resolved

This was referenced May 14, 2023

kernel: Remove util/system from kernel library, interface_ui from validation. #27636

Merged

kernel: Remove args, settings, chainparams, chainparamsbase from kernel library #27576

Merged

DrahtBot added the CI failed label May 17, 2023

DrahtBot removed the CI failed label May 17, 2023

maflcko force-pushed the 2304-import-mempool-rpc- branch from 5d18c30 to fa60af6 Compare May 17, 2023 12:08

Add importmempool RPC

fa776e6

test_importmempool_union contributed by glozow Co-authored-by: glozow <[email protected]>

maflcko force-pushed the 2304-import-mempool-rpc- branch from fad4eaf to fa776e6 Compare August 7, 2023 09:51

DrahtBot requested a review from glozow August 7, 2023 11:54

glozow reviewed Aug 9, 2023

View reviewed changes

glozow requested review from instagibbs and mzumsande August 9, 2023 12:58

achow101 merged commit cd43a84 into bitcoin:master Aug 15, 2023

maflcko deleted the 2304-import-mempool-rpc- branch August 15, 2023 14:26

mzumsande added a commit to mzumsande/bitcoin that referenced this pull request Aug 17, 2023

rpc: remove one more quote from non-string oneline description

2394314

This fixes a silent conflict betwen bitcoin#28123 and bitcoin#27460

mzumsande mentioned this pull request Aug 17, 2023

rpc: remove one more quote from non-string oneline description #28289

Merged

fanquake mentioned this pull request Aug 18, 2023

refactor: Make IsInitialBlockDownload & NotifyHeaderTip not require a Chainstate #28218

Merged

ajtowns mentioned this pull request Aug 23, 2023

rpc, test: add sendmsgtopeer rpc and a test for net-level deadlock situation #28287

Merged

theStack mentioned this pull request Oct 11, 2023

doc: add release note for #27460 (new importmempool RPC) #28637

Merged

theStack added a commit to theStack/bitcoin that referenced this pull request Oct 15, 2023

doc: add release note for bitcoin#27460 (new importmempool RPC)

1b672eb

Co-authored-by: glozow <[email protected]>

bitcoin locked and limited conversation to collaborators Oct 10, 2024

rpc: Add importmempool RPC #27460

rpc: Add importmempool RPC #27460

Uh oh!

Conversation

maflcko commented Apr 13, 2023

Uh oh!

DrahtBot commented Apr 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ghost commented Apr 13, 2023

Uh oh!

maflcko commented Apr 13, 2023

Uh oh!

Uh oh!

kevkevinpal commented Apr 13, 2023

tested by doing the following in regtest

Checking diff of mempool.dat files

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Apr 14, 2023

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

svanstaa commented Apr 26, 2023

Uh oh!

michaelfolkson commented Apr 26, 2023

Uh oh!

maflcko commented Apr 27, 2023

Uh oh!

Uh oh!

svanstaa commented May 17, 2023

Uh oh!

kristapsk commented May 17, 2023

Uh oh!

ajtowns commented Aug 7, 2023

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 15, 2023

Uh oh!

fanquake commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

DrahtBot commented Apr 13, 2023 •

edited

Loading