Fixes #8433 and builds upon #8392.
This adds a file mempool.dat to the data directory, which is created every 10 minutes, and at shutdown. At startup, it is loaded after loading/reindexing/activation of blocks (in the background).
It only grabs a mempool lock while copying and sorting mempool shared pointers (similar to responding to a mempool BIP35 message), which takes 50-100ms here for 1 GB mempool. Loading from disk seems to take up to dozens of seconds but happens in the background.
mempool.dat to doc/files.md
Concept ACK.
Why not behind a command line flag?
Agreed. It can be enabled by default. But people may want to disable this functionality, the per-10 minute dump could interfere with benchmarks and such for example.
6792@@ -6794,6 +6793,107 @@ ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::D
6793 return VersionBitsState(chainActive.Tip(), params, pos, versionbitscache);
6794 }
6795
6796+bool LoadMempool(void)
6797+{
6798+ FILE* filestr = fopen((GetDataDir() / "mempool.dat").c_str(), "r");
.string().c_str()
IIRC the fopen parameters are restrict (or often are in the impl), and assumed to be there for the lifetime of the file pointer… I don’t think this temporary holds those variants and might cause UB?
I could be wrong.
edit: that may only be in the C standard, not C++
I suspect it is completely irrelevant, seeing how prevalent this pattern is throughout the code.
6792@@ -6794,6 +6793,107 @@ ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::D
6793 return VersionBitsState(chainActive.Tip(), params, pos, versionbitscache);
6794 }
6795
6796+bool LoadMempool(void)
6797+{
6798+ FILE* filestr = fopen((GetDataDir() / "mempool.dat").c_str(), "r");
6799+ CAutoFile file(filestr, SER_DISK, CLIENT_VERSION);
6800+ if (file.IsNull()) {
6801+ return error("Failed to open mempool file from disk");
6804+ int64_t count = 0;
6805+
6806+ try {
6807+ uint64_t version;
6808+ file >> version;
6809+ if (version != 1) {
MEMPOOL_DUMP_VERSION or so
Concept ACK
0000000e0 7a 8e 6e 84 83 ab c4 ca 20 fb 9b 88 ac 00 00 00 |z.n..... .......|
1000000f0 00 00 00 00 00 0f 6a 0d 48 61 6c 6c 6f 55 62 69 |......j.HalloUbi|
200000100 72 63 68 74 21 00 00 00 00 0d 9a a1 57 00 00 00 |rcht!.......W...|
As we dump raw txes from the mempool, this will conflict with current status of “antivirus-compliant” storage and dumping mempool.dat can stop the bitcoind from running, ie. new DoS vector.
As we dump raw txes from the mempool, this will conflict with current status of “antivirus-compliant” storage and dumping mempool.dat can stop the bitcoind from running, ie. new DoS vector.
A fair point. Though a missing, or corrupted mempool.dat should never cause bitcoind to stop running. At most this will lose the mempool contents (at next start) due to the file being quarantined.
At most this will lose the mempool contents due to the file being quarantined.
AV can be configured to stop bitcoind with a dialog: “This app is writing a virus to your disk. OK | Error”. This is outside of bitcoind…
Concept ACK and the code size is surprisingly small!
The only thing that I miss (not directly related to this PR) is an opportunity to keep a history of some major mempool data (feerate, size, tx rate, etc.). Another option would be keeping ~24*6 dumps per day that would could be stored in file-diff-deltas to allow generating any types of statistics.
I don’t really see a huge benefit in doing this. It seems like unless you are immediately restarting, you’ll be wasting a lot of time trying to load already confirmed transactions or low fee spam. That said I don’t object to making it a command line argument, but I would suggest it default to off.
What’s the rationale for running it every 10 minutes? You can copy it from one running node to start another or to handle crashes?
I think it should be off by default. And when it is, we can even skip XORing it, I think as it is expert level feature anyway.
Or the other way: create a RPC to dump mempool and that’s all. But this way we loose the save-on-restart feature which is nice (which can be worked around by dumpmempool and restart).
@morcos The reason for dumping every 10 minutes is so that prioritization data is not lost in case of a crash.
But perhaps we should have separate files for that. prioritization.dat which is continuously dumped (maybe only after mapDeltas changed?), and mempool.dat which is only saved at shutdown?
wasting a lot of time trying to load already confirmed transactions or low fee spam
Could it help to save the current block height to the mempool.dat and then discard mempool.dat on start when current height > 3 + height when mempool was saved?
This has split into two threads, really:
Persisting bits of state like prio deltas is useful, and seems applicable to a continuous-log or database solution, that can be restored at startup.
As for mempool restore…
Standard rhetorical questions: Who will really use mempool restore, and how often?
mempool restore seems likely to be a little used, expert-only feature.
mempool restore seems likely to be a little used, expert-only feature.
Somewhat agreed, and as a regular ‘power’ user of bitcoind, I personally already have my own RPC calls equivalent to dumpmempool for this.
@jgarzik This is functionality directly requested by miners, the absence of it creates an operational burden because nodes then end up making small blocks after being restarted.
persisting prio deltas across restarts & crashes
This isn’t useful without the transactions themselves– peers will not re-relay transactions they previously sent. So you could save priorities, but all of them would be almost completely useless.
users really do not like features that pause shutdown for a long time.
Indeed, if it takes a long time that is an issue. But this should be writing no more than about 150 MB of data, which should take under a second. If it’s taking a long time then that needs to be addressed.
One solution if that is an issue is to simply not write it at shutdown, but to only use the regular snapshots.
and introduce additional I/O stress points for remote attackers to exploit
I don’t follow how remote attackers interact here; it wouldn’t make sense to have any of this behavior be remotely triggerable.
if restart is not rapid, much of the data will be stale,
It takes several hours for a full mempool to lose its utility.
I suppose it would be reasonable to not even bother trying to load it if it’s more than three days old.
Mempool sync is also a bandwidth concern for future mempool sync efforts– if it’s lost on restart, than every quick restart would waste bandwidth.
Similarly for block transmission– latency and bandwidth usage will be adversely effected if mempool is lost on restart. It takes about 12 hours in my tests for a node to recover its compact block reconstruction performance after restart, and it can take several seconds to accept a block due to the empty signature cache.
These pressures against restart are an operational burden, because then you’re pressured to try to find ways to avoid restarting– e.g. running extra nodes, or avoiding changing settings.
Separately, the cold mempool means that users and developers spend time in a non-representative state after they try changing settings; which makes it much harder to iterate around settings changes.
( Sorry if some of these points are redundant with #8433 )
Concept ACK, and a very useful feature!
Though I’d suggest making it off by default for now so that nodes that don’t need this don’t have the risk of a bug creating a restart loop - less risk if we start in a clean state by default.
@petertodd With the exception of the priority deltas the transactions are going through normal mempool accept logic.
I can’t see this causing a restart loop unless there’s a bug in the priority handling logic or a remotely exploitable DoS issue anyways.
I can’t see this causing a restart loop unless there’s a bug in the priority handling logic or a remotely exploitable DoS issue anyways.
Which may only be trigger-able under particular circumstance, of which may be captured in the current mempool state?
It seems like unless you are immediately restarting, you’ll be wasting a lot of time trying to load already confirmed transactions or low fee spam.
Eloipool does a save/restore state when it restarts, and one thing it does is write a timestamp early on in the dump. This way, when loading from the dump, it can see if the data is irrelevant and ignore the file. Perhaps that would make sense here too?
Any reason to write out the entire thing every interval, rather than just having another LevelDB database?
@gmaxwell Those concerns seem to be met by remote mempool download also, which @morcos suggested elsewhere in the thread – and indeed was one original purpose of BIP 35 mempool message – maintaining miner’s mempools and not losing lucrative miner fees due to restart.
Remote download over LAN is also potentially faster, something Google noticed when seeing increased performance through remote page caches: http://static.googleusercontent.com/media/research.google.com/en//pubs/archive/43886.pdf
Remote download also gets you fresh transactions which the db/log will not have - fuller blocks, more miners fees.
and introduce additional I/O stress points for remote attackers to exploit
I don’t follow how remote attackers interact here; it wouldn’t make sense to have any of this behavior be remotely triggerable.
The relevant portion that answers your confusion was elided: “Alternatives to dump-on-shutdown […]” This refers to, e.g. periodic dumps, streaming transactions to a leveldb or WAL log, which occur at runtime not shutdown.
Those concerns seem to be met by remote mempool download also, which @morcos suggested elsewhere in the thread – and indeed was one original purpose of BIP 35 mempool message – maintaining miner’s mempools and not losing lucrative miner fees due to restart.
Preservation of the mempool is a prerequisite for mempool sync. Otherwise you’d have 150 MB of transfer (burning resources of remote nodes) each time someone restarts to twiddle some configure. I think the mempool message is unsuitable for synchronization generally, due to overhead.
[and while the latency concerns related to compact blocks would be addressed by a bulk download, the bandwidth savings would be lost]
Remote download over LAN is also potentially faster, something Google
In Google datacenters not on our P2P network; in any case I believe the load is bottlenecked on any ordinary host. (and were it faster, it wouldn’t matter– resource usage needs to be heavily weighed).
and introduce additional I/O stress points for remote attackers to exploit
I don’t follow how remote attackers interact here; it wouldn’t make sense to have any of this behavior be remotely triggerable.
The relevant portion that answers your confusion was elided: “Alternatives to dump-on-shutdown […]” This refers to, e.g. periodic dumps, streaming transactions to a leveldb or WAL log, which occur at runtime not shutdown.
There isn’t any leveldb involved here; but regardless– I still don’t see where the “remote attackers” part that I was asking about comes in.
Notable how remote mempool load on startup could be increase the effects of a DoS attack due to all the extra bandwidth wasted as nodes restart after crashing; it’d be especially harmful if a remote crash exploit could be found if people ever setup nodes to restart automatically.
On 3 August 2016 21:56:34 GMT-07:00, Gregory Maxwell notifications@github.com wrote:
Those concerns seem to be met by remote mempool download also, which @morcos suggested elsewhere in the thread – and indeed was one original purpose of BIP 35 mempool message – maintaining miner’s mempools and not losing lucrative miner fees due to restart.
Preservation of the mempool is a prerequisite for mempool sync. Otherwise you’d have 150 MB of transfer (burning resources of remote nodes) each time someone restarts to twiddle some configure. I think the mempool message is unsuitable for synchronization generally, due to overhead.
Remote download over LAN is also potentially faster, something Google
In Google datacenters not on our P2P network; in any case I believe the load is bottlenecked on any ordinary host. (and were it faster, it wouldn’t matter– resource usage needs to be heavily weighed).
and introduce additional I/O stress points for remote attackers to exploit
I don’t follow how remote attackers interact here; it wouldn’t make sense to have any of this behavior be remotely triggerable.
The relevant portion that answers your confusion was elided: “Alternatives to dump-on-shutdown […]” This refers to, e.g. periodic dumps, streaming transactions to a leveldb or WAL log, which occur at runtime not shutdown.
There isn’t any leveldb involved here; but regardless– I still don’t see where the “remote attackers” part that I was asking about comes in.
You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #8448 (comment)
@sipa Could be off for now– though sad to lose the BIP152 improvement but as you say I guess most users aren’t going up and down that often and I suppose it could be defaulted on after mempool sync. Though I have a little concern that defaulting off would be another point of divergence between what virtually every developer runs and most users run.
only for prioritized transactions,
And their unconfirmed parents? meh.
Remote download over LAN is also potentially faster, something Google
In Google datacenters not on our P2P network
The P2P network is not a LAN. It was not suggested that miners download the mempool over a WAN. The referenced, driving use case is miners who requested this feature, with almost everyone else defaulting this feature ‘off’ In that context for that user subset, LAN environments are the norm.
Furthermore, miners will inevitably want the fastest solution to node restart - thus the doc reference. Based on current LAN technology and research, all network with no additional disk I/O - either periodic (leveldb/log) or at startup/shutdown - is likely to be the performant solution.
There isn’t any leveldb involved here
Please re-read this thread. Several alternatives to dump-on-shutdown are discussed. Specifically, leveldb was suggested as an alternative solution, mostly recently a few comments back by @luke-jr My comment referenced these alternate solutions.
This matches history, where the idea of storing the mempool in leveldb has been suggested many times over the years. It is a relevant discussion in the current thread context.
XORing a key: sigh, ok
I think this could be done in a follow up PR.
[…] Those concerns seem to be met by remote mempool download also, which @morcos suggested elsewhere in the thread – and indeed was one original purpose of BIP 35 mempool message – maintaining miner’s mempools and not losing lucrative miner fees due to restart.
IMO BIP35’s mempool command is a fingerprinting and DOS attack vector. Its extremely resource hungry (some SPV nodes call the filtered mempool command over and over again).
Also, since 0.13, we do disable the mempool p2p command when NODE_BLOOM is disabled.
See #8078
Furthermore, miners will inevitably want the fastest solution to node restart
This PR does not slow down startup. The mempool file is loaded in the background, while the node is already running. Dumping takes a second on my VPS with pretty slow I/O.
Idea: instead of not loading the mempool file when it’s too old, just skip loading entries that have expired by the time we load them. That has the same effect, and is a pure optimization.
Assuming that skipping entries is extremely cheap, I no longer see a benefit in turning the feature off by default.
6823+ CAmount amountdelta = nFeeDelta;
6824+ if (amountdelta) {
6825+ mempool.PrioritiseTransaction(tx.GetHash(), tx.GetHash().ToString(), prioritydummy, amountdelta);
6826+ }
6827+ CValidationState state;
6828+ AcceptToMemoryPool(mempool, state, tx, true, NULL, nTime, 0, false);
cs_main.
AcceptToMemoryPoolWorker has a AssertLockHeld(cs_main);
it will have an impact in reducing overall p2p bandwidth consumption
For the power cycles you refer to, [on average] how much of the mempool stays relevant? (no doubt also dependent on maxmempool)
Btcdrak showed me some graphs showing that nodes often go on and off throughout the day.
IMO the main benefit to turning it off is eliminating 2mbit/sec worth of extra writing that might reduce the life time of low end flash devices from the periodic writes. Perhaps only disable the periodic writes by default?
198@@ -199,6 +199,7 @@ void Shutdown()
199 StopNode();
200 StopTorControl();
201 UnregisterNodeSignals(GetNodeSignals());
202+ DumpMempool();
644@@ -644,6 +645,9 @@ void ThreadImport(std::vector<boost::filesystem::path> vImportFiles)
645 LogPrintf("Stopping after block import\n");
646 StartShutdown();
647 }
648+
649+ LoadMempool();
650+ ScheduleDumpMempool(*scheduler);
2758@@ -2758,7 +2759,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara
2759 // ignore validation errors in resurrected transactions
2760 list<CTransaction> removed;
2761 CValidationState stateDummy;
2762- if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL, true)) {
2763+ if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL, block.nTime, true, 0)) {
6816+ CTransaction tx;
6817+ int64_t nTime;
6818+ int64_t nFeeDelta;
6819+ file >> tx;
6820+ file >> nTime;
6821+ file >> nFeeDelta;
6826+ }
6827+ CValidationState state;
6828+ AcceptToMemoryPool(mempool, state, tx, true, NULL, nTime, 0, false);
6829+ count += state.IsValid();
6830+ }
6831+ std::map<uint256, CAmount> mapDeltas;
300@@ -299,7 +301,7 @@ void PruneAndFlush();
301
302 /** (try to) add transaction to memory pool **/
303 bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree,
304- bool* pfMissingInputs, bool fOverrideMempoolLimit=false, const CAmount nAbsurdFee=0);
305+ bool* pfMissingInputs, int64_t nTime, bool fOverrideMempoolLimit, const CAmount nAbsurdFee);
I’d really prefer if the every 10 minute scheduled dump was removed. I think useful features are:
The idea of not being able to get any of those benefits without having to write 100MB+ every 10 minutes kind of bothers me…
I agree with @morcos. I’d also prefer not to do the automatic periodic writing, but just on shutdown, and when prompted through RPC. Bitcoind generates enough I/O as it is :( Could enable this only when there is prioritization data to save in the first place?
If it is to be enabled by default, at the least there needs to be an option to disable it, e.g. for when benchmarking/measuring.
Rebased with the following changes:
XORed dump is not implemented, this can be done later. As the regular dump seemed controversial, I’ve removed it now (it can always be added back later).
7040+ file << mapDeltas;
7041+ FileCommit(file.Get());
7042+ file.fclose();
7043+ RenameOver(GetDataDir() / "mempool.dat.new", GetDataDir() / "mempool.dat");
7044+ int64_t last = GetTimeMicros();
7045+ LogPrintf("Dumped mempool: %gs to copy, %gs to dump\n", (mid-start)*0.000001, (last-mid)*0.000001);
This results in
02016-11-01 08:30:08 Dumped mempool: 1.9e-05s to copy, 0.006452s to dump
here on testnet. What about using ms instead?
ACK https://github.com/bitcoin/bitcoin/pull/8448/commits/582068aa907127c12db53fbd65667dc7810d42b0
XOR dumping nit can be ignored now, because we are dumping mempool on shutdown only.