Allow shutdown during LoadMempool, dump only when necessary #9408
Conversation
| @@ -557,7 +557,14 @@ static const unsigned int REJECT_CONFLICT = 0x102; | |||
| /** Dump the mempool to disk. */ | |||
| void DumpMempool(); | |||
|
|
|||
| /** Result states for loading the mempool from disk */ | |||
| enum LoadMempoolResult { | |||
There was a problem hiding this comment.
I think this is a bit overkill. bool is there already. And returning true only in case of successful mempool.dat load is enough. The only question is what to return in case of corrupted mempool.dat. Returning true instead of false makes sense here. And when true is returned, dump mempool at shutdown.
There was a problem hiding this comment.
IMO a simple true|false would not work. You need to distinct between...
- not yet loaded = don't dump during shutdown
- user_aborted (shutdown) = don't dump during shutdown
- failed = dump during shutdown
- successful = dump during shutdown
Returning false in case of a user-triggered shutdown could work, but then you would need to return true in case of an input error (somehow not good).
There was a problem hiding this comment.
If user aborts at T=5, and doesn't shut down the node, when it does finally shutdown at T=5000, you would want to dump it. This isn't possible, but the enum name doesn't make that clear. So a simply two-value enum with DoNotSave vs PleaseSave might be best?
|
@luke-jr Two value enum aka bool? ;-) |
|
I might be missing something here but imo it is intentional to always dump the full mempool. In case the user feels it is taking too long (to load the mempool) on their hardware, they can switch to a smaller mempool with |
|
@MarcoFalke Imagine |
|
@paveljanik Using an enum would give it clear values which cannot be accidentally cast to/from a number. |
|
From a software design perspective, LoadMempool has three major states... The decision if we should dump during shutdown should probably not be part of |
|
If you only have a partially loaded mempool file, but your node has been
running for a week after that, we should probably dump again...
|
Yes. Partially loaded mempool is only possible with a user-triggered shutdown during |
|
@jonasschnelli No, when the file is corrupted in the middle, you end up with partially loaded mempool. |
66fcca9 to
ef479e1
Compare
|
@paveljanik: Yes. Your right, I somehow though the mempool gets clear if a corruption was detected. |
| @@ -130,6 +130,7 @@ static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat"; | |||
| // | |||
|
|
|||
| std::atomic<bool> fRequestShutdown(false); | |||
| std::atomic<bool> fDumpMempoolLater(false); | |||
There was a problem hiding this comment.
nit: fDumpMempoolOnShutdown seems more self-explanatory and precise.
|
testing this. seems to be working well so far. |
|
utACK |
|
utACK ef479e19e9e9279c8e22cff50227ea0722f59df3 |
| @@ -1105,6 +1105,15 @@ void BitcoinGUI::detectShutdown() | |||
| } | |||
| } | |||
|
|
|||
| void BitcoinGUI::shouldQuit() | |||
There was a problem hiding this comment.
Please don't make the qt shutdown sequence even more complicated :(
Wouldn't a better idea be to add StartShutdown here to app.requestShutdown, called from https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoin.cpp#L691
There was a problem hiding this comment.
Argh. Yes. This is much simpler. Force push changed.
Request retests.
ef479e1 to
325e400
Compare
|
utACK 325e400 I'd call this a pretty bad regression if we cant get this for 0.14. |
|
utACK 325e400 |
325e400 [Qt] Do proper shutdown (Jonas Schnelli) 9479f8d Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli)
… necessary 325e400 [Qt] Do proper shutdown (Jonas Schnelli) 9479f8d Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli)
… necessary 325e400 [Qt] Do proper shutdown (Jonas Schnelli) 9479f8d Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli)
… necessary 325e400 [Qt] Do proper shutdown (Jonas Schnelli) 9479f8d Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli)
9b9c616 Fix missing zapwallettxes mode in wallet_hd.py functional test (furszy) d6d0ad9 [logs] fix zapwallettxes startup logs (John Newbery) 006c503 [wallet] fix zapwallettxes interaction with persistent mempool (John Newbery) c6d45c6 Adapting and connecting mempool_persist.py functional test to the test runner. (furszy) 4f26a4e Control mempool persistence using a command line parameter. (John Newbery) 5d949de [Qt] Do proper shutdown (Jonas Schnelli) e60da98 Allow shutdown during LoadMempool, dump only when necessary (Jonas Schnelli) c0a0e81 Moving TxMempoolInfo tx member to CTransactionRef (furszy) f0c2255 Add mempool.dat to doc/files.md (furszy) 8e52226 Add DumpMempool and LoadMempool (Pieter Wuille) 44c635d Add AcceptToMemoryPoolWithTime function (Pieter Wuille) 6bbc6a9 Add feedelta to TxMempoolInfo (Pieter Wuille) 9979f3d [mempool] move removed and conflicts transaction ref list to vector. (furszy) 4f672c2 Make removed and conflicted arguments optional to remove (Pieter Wuille) e51c4b8 Bypass removeRecursive in removeForReorg (Pieter Wuille) 54cf7c0 Get rid of CTxMempool::lookup() entirely (furszy) 35bc2a9 Finished the switch CTransaction storage in mempool to CTransactionRef and introduced the timeLastMempoolReq coming from btc#8080. (furszy) d10583b An adapted version of btc@b5599147533103efea896a1fc4ff51f2d3ad5808 (furszy) cb4fc6c An adapted version of btc@ed7068302c7490e8061cb3a558a0f83a465beeea (furszy) 9645775 Split up and optimize transaction and block inv queues (furszy) 68bc68f Don't do mempool lookups for "mempool" command without a filter (furszy) 7624823 mapNextTx: use pointer as key, simplify value (furszy) 191c62e Return mempool queries in dependency order (Pieter Wuille) 23c9f3e Eliminate TX trickle bypass, sort TX invs for privacy and priority. (furszy) 6ebfd17 tiny test fix for mempool_tests (Alex Morcos) 8c0016e Check all ancestor state in CTxMemPool::check() (furszy) 91c6096 Add ancestor feerate index to mempool (Suhas Daftuar) 64e84e2 Add ancestor tracking to mempool This implements caching of ancestor state to each mempool entry, similar to descendant tracking, but also including caching sigops-with-ancestors (as that metric will be helpful to future code that implements better transaction selection in CreatenewBlock). (furszy) 8325bb4 Fix mempool limiting for PrioritiseTransaction Redo the feerate index to be based on mining score, rather than fee. (furszy) 1fa40ac Remove work limit in UpdateForDescendants() The work limit served to prevent the descendant walking algorithm from doing too much work by marking the parent transaction as dirty. However to implement ancestor tracking, it's not possible to similarly mark those descendant transactions as dirty without having to calculate them to begin with. This commit removes the work limit altogether. With appropriate chain limits (-limitdescendantcount) the concern about doing too much work inside this function should be mitigated. (furszy) ba32375 Rename CTxMemPool::remove -> removeRecursive remove is no longer called non-recursively, so simplify the logic and eliminate an unnecessary parameter (furszy) c30fa16 CTxMemPool::removeForBlock now uses RemoveStaged (furszy) Pull request description: Ending up 2020 with a large PR :). Included a good number of performance and privacy improvements over the mempool and inv processing/sending areas + added mempool cache dump/load. Almost finishing with #1726 work, getting closer to 9725, and getting closer to be able to decouple the message processing thread <-> wallet and the wallet <-> GUI locks dependencies (which will be another long story.. but well, step by step). The final goal is clear, a much faster syncing process using pivx-qt (a good speed up for pivxd as well), smoother visual navigation when big wallets are syncing, less thread synchronization issues, among all of the improvements that will be coming with the backports adaptations. Adapted the following PRs to our sources: bitcoin#7062 —> only eb30666. bitcoin#7174 —> complete. bitcoin#7562 —> only c5d746a. bitcoin#7594 —> complete. bitcoin#7840 —> complete. bitcoin#7997 —> complete. bitcoin#8080 —> complete bitcoin#8126 —> except e9b4780 (we don't have the mapRelay) and c2a4724 (we don't have the relay expiration vector). bitcoin#8448 —> complete bitcoin#8515 —> complete bitcoin#9408 —> complete bitcoin#9966 —> complete bitcoin#10330 —> complete ACKs for top commit: random-zebra: ACK 9b9c616 Fuzzbawls: ACK 9b9c616 Tree-SHA512: 186bd09bbb19b55ec0d46dc27291663a0df2d60d8238c661a8c9b8cbf3677b6f8a92fa56bd7346a3cb5293712eeccc5d6542ee3c441d35a4f61e7d12e2ce489a
A try to address #9398.
Right now,
LoadMempool()has now "interruption point". If one triggers a shutdown, It will always wait untilLoadMempool()has been completed.This PR does detect a shutdown during the
LoadMempool()while also taking care of not-dumping the mempool during shutdown if the user has request it before the import was completed.Includes also a GUI fix that ensure proper shutdowns (right now,
StartShutdown()is not called when quitting the GUI).ping @sipa
Needs testing (maybe @instagibbs?)