Prune locks #19463
Prune locks #19463
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/19463. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I think redownloading specified block could be much more useful, just like the case that scantxoutset gives the corresponding block height, but the actual block had already been pruned - the ability to redownload (and then rescan) specified block fix this problem. |
|
Recently I heard (could be inaccurate/misunderstanding of myself) that some projects like BTCPay didn't keep full transaction data of received funds. Only the UTXO entry (txid, vout, amount and scriptPubkey) was kept. Therefore they are facing problems to fix the BIP143 vulnerability, because the full data of the previous transaction had been already discarded. I think the ability to redownload & rescan specified block(s) can relieve this problem. |
That's useful too, but for different use cases. You wouldn't want to be constantly redownloading the same block over and over. (Prune locks might be useful for it, though, to prevent the redownloaded block from being immediately repruned before use.)
Aside, that's a pretty serious bug. It's the receiver's responsibility to rebroadcast the transaction until it confirms... |
734ec24 to
423f89f
Compare
Honestly I don't get what's the point of this - to save a little more disk space under extreme conditions, like almostly exhausted disk space, so that the node could then keep running for a littel more time?
This seems to justify the above point, however I think one of the most-wanted features is "external (and pluggable) block storage", rather than "external indices/filters etc", although the later also seems to be nice (despite both would complicate the things further).
Then, (without some filter/matching mechanism) it will in fact disable (pause) pruning until the wallet reloads, won't it? Because the node won't know whether a newly received block is unrelated (prunable), therefore the only choice is to stop pruning. It has been a significant usability problem since quite a long time ago, that the user cannot load/import wallets/keys/addrs freely - nobody likes triggering the troubles of blockchain rescanning. I think #15946 (together with something like #15845) should be able to fix such usability problem in a simpler,better way, that: With local blockfilterindex, the blocks could be simply discarded (pruned). Then, the once pruned blocks could be redownloaded according to local blockfilterindex while reloading wallet/importing keys/addrs - no need to worry about whether a block could be needed in the future anymore. However I still agree that a button allowing the user to simply "pause/resume" pruning would be nice, too - I hope that this could be displayed on some dashboard. |
Yes, prune locks indeedly have its utility, especially to lock needed/related historical blocks. I just think that I see greater problems. |
Including desc member of PruneLockInfo for a human-readable description
There was a problem hiding this comment.
I can see how this could be useful for external applications in theory, but it would be good to know if there is an actual demand for this - I wonder if there are any applications out there that would actually use this / generate prune locks if this option was available for them?
| const int max_height_first{pindexDelete->nHeight - 1}; | ||
| for (auto& prune_lock : m_blockman.m_prune_locks) { | ||
| if (prune_lock.second.height_first <= max_height_first) continue; | ||
| if (prune_lock.second.height_first < max_height_first) continue; |
There was a problem hiding this comment.
Commit 8ee1214:
would be good to have an explanation why this is being changed in the commit message.
I guess this means that if we have a chain at height N, have an index synced to height N-1 and disconnect the tip, we would change the prune lock of the index from N-1 to N-2. Why?
src/node/blockstorage.h
Outdated
| @@ -97,6 +97,7 @@ struct CBlockIndexHeightOnlyComparator { | |||
|
|
|||
| struct PruneLockInfo { | |||
| int height_first{std::numeric_limits<int>::max()}; //! Height of earliest block that should be kept and not pruned | |||
| int height_last{std::numeric_limits<int>::max()}; //! Height of latest block that should be kept and not pruned | |||
There was a problem hiding this comment.
Is there a use case for allowing users to specify height_last, which would allow blocks higher than that to be pruned, so that anything requiring to sync the entire chain at a later point wouldn't work anymore?
| if (lockid == "*") throw JSONRPCError(RPC_INVALID_PARAMETER, "id \"*\" only makes sense when deleting"); | ||
| if (!height_param[0].isNum()) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid start height"); | ||
| lock_info.height_first = height_param[0].getInt<uint64_t>(); | ||
| if (!height_param[1].isNull()) { |
There was a problem hiding this comment.
Should probably return an error if height_first > height_last.
| static RPCHelpMan listprunelocks() | ||
| { | ||
| return RPCHelpMan{"listprunelocks", | ||
| "\nReturns a list of pruning locks.\n", |
There was a problem hiding this comment.
| "\nReturns a list of pruning locks.\n", | |
| "Returns a list of pruning locks.\n", |
the ci failure is a bit vague, but this should fix it
|
|
||
| // Since there is no reasonable expectation for any follow-up to this prune lock, actually ensure it gets committed to disk immediately | ||
| if (!m_block_tree_db->DeletePruneLock(name)) { | ||
| LogError("%s: failed to %s prune lock '%s'\n", __func__, "erase", name); |
There was a problem hiding this comment.
nit: the error log message looks already unique enough and the __func__ can be removed
|
Turned into draft for now, due to failing CI and inactivity for two months. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
This adds the ability to have any number of internal or external locks to prevent pruning specific block ranges.
Some examples where this would be useful:
By design, users retain complete control over prune locks, and can delete them out from under applications using them. This is to avoid circumstances where a prune lock has been set, by an application that may no longer be used.