mempool: Persist with XOR #28207

maflcko · 2023-08-03T09:09:44Z

Currently the mempool.dat file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.

While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or may cause block relay to be slower.

Fix this, similar to #6650, by rolling a random XOR pattern over the dat file when writing or reading it.

Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat file. Any program that intentionally wants to mess with the dat file can still trivially do so.

DrahtBot · 2023-08-03T09:09:46Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, ismaelsadeeq, achow101
Concept ACK	Sjors, 0xB10C, luke-jr, RandyMcMillan
Stale ACK	hernanmarino, pablomartin4btc, darosior

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:

#28438 (Use serialization parameters for CTransaction by ajtowns)
#28132 (policy: Enable full-rbf by default by petertodd)

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.

maflcko · 2023-08-03T09:12:10Z

Currently opened as draft to wait for initial feedback. There is no option to disable this feature, because I am not aware of anyone reading the mempool.dat, is there? (The getrawmempool RPC is the recommended way to get the mempool, and using the savemempool RPC instead seems like an edge-case?)

Sjors · 2023-08-03T09:21:36Z

Concept ACK.

It may be difficult to find out if anyone relies on parsing mempool.dat. If it's not too hard, we might as well add an option (default 1) and deprecate it in a few releases. That also makes it easier to toggle between master and the last release (by setting it to 0).

You may also want to use a bit flag instead of increasing the version. The added complexity of that could be an argument to not make this optional.

maflcko · 2023-08-03T09:31:38Z

I don't think bit flags make sense in this context, because the mempool.dat frequently changes and is at most expected to be read by the previous version. However, your suggestion to use a setting makes sense. The setting could have several options, like "write version 1 mempool.dat", and "use 0 for all random numbers" (if needed), and "use prng".

Edit: Hmm, I think just having a boolean option to say "write version 1 mempool.dat" is enough. If there are any users that need "use 0 for all random numbers", they can create a feature request after they read the release notes.

src/kernel/mempool_persist.cpp

0xB10C · 2023-08-03T18:28:14Z

Concept ACK. I opened issue #16721 a while ago but closed it as it didn't get much attention back then.

I am not aware of anyone reading the mempool.dat, is there?

I've written a mempool.dat parser a few years ago for fun. However, as you said, the RPCs are powerful enough and if someone really really wants to read the file, they can implement XOR functionality. Similar to block0000.dat files, these files are not something considered an interface for others to rely on.

maflcko · 2023-08-04T05:41:59Z

Just for context: For testing I used -datacarriersize=9999999 and then put the following nulldata raw() descriptor into the mempool.dat: raw(6a4458354f2150254041505b345c505a58353428505e2937434329377d2445494341522d5354414e444152442d414e544956495255532d544553542d46494c452124482b482a). On current master I got several hits by different virus scanners. On this pull, all virus scanners were green and didn't put the mempool.dat in the quarantine.

maflcko · 2023-08-04T12:24:08Z

Added some code to make the Windows CI pass

darosior

Concept and approach ACK. I don't think there is harm in making it the default behaviour immediately.

src/init.cpp

src/kernel/mempool_options.h

luke-jr · 2023-08-05T16:02:31Z

Concept ACK, I don't think we need to concern with external programs reading this. If they do exist, they can adapt easily.

luke-jr · 2023-08-05T16:02:52Z

(Although downgrading might be something we want to support?)

maflcko · 2023-08-06T08:51:45Z

(Although downgrading might be something we want to support?)

It is. You can simply set -persistmempoolv1=1.

glozow · 2023-08-07T15:06:53Z

concept ACK

RandyMcMillan · 2023-08-07T15:41:31Z

Concept ACK

ismaelsadeeq

light code-review ACK fac48d2

Idk why but I try testing the behavior on master 5aa67eb with the steps you provided here but no anti virus quarantined the mempool.dat file, I used Intego, bitfinder.

When we want to drop support for writing in the legacy format (v1), mempool_compatibilty.py functional test should be deleted along the CL I think.

Do we also want fee_estimates.dat to be persisted with XOR?

src/kernel/mempool_persist.cpp

maflcko · 2023-08-23T14:07:22Z

but no anti virus quarantined the mempool.dat file, I used Intego, bitfinder.

The majority won't detect the eicar test virus I used. I used an online service to scan the mempool.dat and IIRC 4/50 were red (or so), but I don't recall which.

Do we also want fee_estimates.dat to be persisted with XOR?

Why? IIUC correctly, it only stores integers and floating point number calculated locally, or am I missing something? peers.dat may be a better choice for XOR?

glozow

ACK fa52084
Code LGTM, tried a few roundtrips of stop/start/savemempool/importmempool with combinations of the options. I originally thought -persistmempool=0 should imply something for -persistmempoolv1 but makes sense that it applies to savemempool when we're not persisting.

test/functional/mempool_compatibility.py

src/kernel/mempool_persist.cpp

glozow

reACK fa6b053

ismaelsadeeq

ACK fa6b053

Used this PR to successfully read a mempool.dat saved in the legacy format.
Additionally, the persistmempoolv1 option downgraded and allowed dumping using the legacy format.

Code looks good to me.

ismaelsadeeq · 2023-11-13T13:55:54Z

src/init.cpp

@@ -458,6 +458,11 @@ void SetupServerArgs(ArgsManager& argsman)
    argsman.AddArg("-par=<n>", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)",
        -GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    argsman.AddArg("-persistmempool", strprintf("Whether to save the mempool on shutdown and load on restart (default: %u)", DEFAULT_PERSIST_MEMPOOL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    argsman.AddArg("-persistmempoolv1",


This PR is an improvement, this patch allows you to read mempool.dat dumped using the legacy format.
Whats the a rationale to temporary retention of persistmempoolv1, and when can we expect to remove this option?

Should be fine to remove in the second next release after this is merged. I presume the read code (// Leave XOR-key empty ) can stay forever/longer, because it is just one line of code, so no strong opinion.

AFAIU we are persisting with XOR to "protect against programs that accidentally and unintentionally are trying to mess with the dat file" so I think it will be better if we completely prevent dumping without XOR.

achow101 · 2023-11-13T16:16:57Z

re-ACK fa6b053

DrahtBot added the Mempool label Aug 3, 2023

ismaelsadeeq reviewed Aug 3, 2023

View reviewed changes

src/kernel/mempool_persist.cpp Show resolved Hide resolved

DrahtBot added the CI failed label Aug 3, 2023

DrahtBot mentioned this pull request Aug 3, 2023

rpc: Add importmempool RPC #27460

Merged

maflcko force-pushed the 2308-xor-memepool- branch 2 times, most recently from a7d36ce to fa0f249 Compare August 4, 2023 07:20

maflcko marked this pull request as ready for review August 4, 2023 07:21

maflcko force-pushed the 2308-xor-memepool- branch from edae44e to fa3276d Compare August 4, 2023 12:23

DrahtBot mentioned this pull request Aug 4, 2023

policy: Enable full-rbf by default #28132

Open

DrahtBot removed the CI failed label Aug 4, 2023

darosior reviewed Aug 5, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/kernel/mempool_options.h Show resolved Hide resolved

maflcko force-pushed the 2308-xor-memepool- branch from fa3276d to fa6827c Compare August 6, 2023 09:05

maflcko force-pushed the 2308-xor-memepool- branch from fa6827c to fac18cd Compare August 15, 2023 14:27

DrahtBot added the CI failed label Aug 15, 2023

maflcko force-pushed the 2308-xor-memepool- branch from fac18cd to fac48d2 Compare August 16, 2023 12:33

DrahtBot removed the CI failed label Aug 16, 2023

ismaelsadeeq reviewed Aug 23, 2023

View reviewed changes

DrahtBot requested a review from hernanmarino November 9, 2023 11:33

glozow reviewed Nov 9, 2023

View reviewed changes

test/functional/mempool_compatibility.py Show resolved Hide resolved

src/kernel/mempool_persist.cpp Show resolved Hide resolved

DrahtBot requested review from hernanmarino and removed request for hernanmarino November 9, 2023 13:59

mempool: persist with XOR

fa6b053

maflcko force-pushed the 2308-xor-memepool- branch from fa52084 to fa6b053 Compare November 9, 2023 18:45

glozow reviewed Nov 10, 2023

View reviewed changes

DrahtBot requested review from darosior, hernanmarino and achow101 and removed request for hernanmarino November 10, 2023 17:41

ismaelsadeeq reviewed Nov 13, 2023

View reviewed changes

DrahtBot requested review from hernanmarino and removed request for hernanmarino November 13, 2023 13:56

DrahtBot requested review from hernanmarino and removed request for achow101 and hernanmarino November 13, 2023 16:17

achow101 merged commit d232e36 into bitcoin:master Nov 13, 2023
16 checks passed

maflcko deleted the 2308-xor-memepool- branch November 13, 2023 16:32

Sjors mentioned this pull request Feb 23, 2024

rpc: Optimize serialization disk space of dumptxoutset #26045

Closed

LarryRuane mentioned this pull request Apr 11, 2024

investigate obfuscating (xor) database blocks files (like chainstate) LarryRuane/bitcoin#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mempool: Persist with XOR #28207

mempool: Persist with XOR #28207

maflcko commented Aug 3, 2023 •

edited

DrahtBot commented Aug 3, 2023 •

edited

maflcko commented Aug 3, 2023 •

edited

Sjors commented Aug 3, 2023

maflcko commented Aug 3, 2023 •

edited

0xB10C commented Aug 3, 2023

maflcko commented Aug 4, 2023 •

edited

maflcko commented Aug 4, 2023

darosior left a comment

luke-jr commented Aug 5, 2023

luke-jr commented Aug 5, 2023

maflcko commented Aug 6, 2023

glozow commented Aug 7, 2023

RandyMcMillan commented Aug 7, 2023

ismaelsadeeq left a comment

maflcko commented Aug 23, 2023

glozow left a comment

glozow left a comment

ismaelsadeeq left a comment

ismaelsadeeq Nov 13, 2023 •

edited

maflcko Nov 13, 2023

ismaelsadeeq Nov 13, 2023

achow101 commented Nov 13, 2023

mempool: Persist with XOR #28207

mempool: Persist with XOR #28207

Conversation

maflcko commented Aug 3, 2023 • edited

DrahtBot commented Aug 3, 2023 • edited

Code Coverage

Reviews

Conflicts

maflcko commented Aug 3, 2023 • edited

Sjors commented Aug 3, 2023

maflcko commented Aug 3, 2023 • edited

0xB10C commented Aug 3, 2023

maflcko commented Aug 4, 2023 • edited

maflcko commented Aug 4, 2023

darosior left a comment

Choose a reason for hiding this comment

luke-jr commented Aug 5, 2023

luke-jr commented Aug 5, 2023

maflcko commented Aug 6, 2023

glozow commented Aug 7, 2023

RandyMcMillan commented Aug 7, 2023

ismaelsadeeq left a comment

Choose a reason for hiding this comment

maflcko commented Aug 23, 2023

glozow left a comment

Choose a reason for hiding this comment

glozow left a comment

Choose a reason for hiding this comment

ismaelsadeeq left a comment

Choose a reason for hiding this comment

ismaelsadeeq Nov 13, 2023 • edited

Choose a reason for hiding this comment

maflcko Nov 13, 2023

Choose a reason for hiding this comment

ismaelsadeeq Nov 13, 2023

Choose a reason for hiding this comment

achow101 commented Nov 13, 2023

maflcko commented Aug 3, 2023 •

edited

DrahtBot commented Aug 3, 2023 •

edited

maflcko commented Aug 3, 2023 •

edited

maflcko commented Aug 3, 2023 •

edited

maflcko commented Aug 4, 2023 •

edited

ismaelsadeeq Nov 13, 2023 •

edited