luke-jr
commented at 4:03 am on July 8, 2020:
member
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:
Ensuring any known Core wallet won’t become unable to sync due to pruning when it is unloaded. (Included in this PR)
Ensuring pruning doesn’t go too far for a desired-functional wallet backup. (Questionable, since you probably lose the node if you need the backup anyway?)
Allowing users to temporarily disable block indexes and filters, without pruning cutting them off from catching up later.
Avoiding pruning out from under external wallets (Armory, EPS, etc) that might need block data.
External block indexes/filters, etc.
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.
luke-jr force-pushed
on Jul 8, 2020
DrahtBot
commented at 4:15 am on July 8, 2020:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
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:
#31860 (init: Take lock on blocks directory in BlockManager ctor by TheCharlatan)
#29652 (wallet: Avoid potentially writing incorrect best block locator by ryanofsky)
#29418 (rpc: provide per message stats for global traffic via new RPC ‘getnetmsgstats’ by vasild)
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.
DrahtBot added the label
RPC/REST/ZMQ
on Jul 8, 2020
DrahtBot added the label
UTXO Db and Indexes
on Jul 8, 2020
DrahtBot added the label
Validation
on Jul 8, 2020
DrahtBot added the label
Wallet
on Jul 8, 2020
andronoob
commented at 4:41 am on July 9, 2020:
none
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.
andronoob
commented at 4:45 am on July 9, 2020:
none
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.
luke-jr
commented at 4:04 pm on July 9, 2020:
member
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.
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.)
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.
Aside, that’s a pretty serious bug. It’s the receiver’s responsibility to rebroadcast the transaction until it confirms…
laanwj added the label
Block storage
on Jul 9, 2020
laanwj removed the label
Wallet
on Jul 9, 2020
laanwj removed the label
Validation
on Jul 9, 2020
laanwj removed the label
UTXO Db and Indexes
on Jul 9, 2020
luke-jr force-pushed
on Jul 24, 2020
luke-jr force-pushed
on Jul 28, 2020
andronoob
commented at 7:18 am on July 28, 2020:
none
Allowing users to temporarily disable block indexes and filters, without pruning cutting them off from catching up later.
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?
External block indexes/filters, etc.
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).
Avoiding pruning out from under external wallets (Armory, EPS, etc) that might need block data.
Ensuring any known Core wallet won’t become unable to sync due to pruning when it is unloaded. (Included in this PR)
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.
andronoob
commented at 7:58 am on July 28, 2020:
none
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.)
Yes, prune locks indeedly have its utility, especially to lock needed/related historical blocks. I just think that I see greater problems.
luke-jr force-pushed
on Jul 29, 2020
luke-jr force-pushed
on Jul 29, 2020
luke-jr
commented at 5:45 pm on July 29, 2020:
member
Removed wallet support for this PR, rebased, added test, and passing CI.
in
src/rpc/blockchain.cpp:930
in
8d43c3cbc0outdated
925+ {
926+ {RPCResult::Type::OBJ, "", "",
927+ {
928+ {RPCResult::Type::STR, "id", "A unique identifier for the lock"},
929+ {RPCResult::Type::STR, "desc", "A description of the lock's purpose"},
930+ {RPCResult::Type::STR, "height", "A description of the lock's purpose"},
DrahtBot added the label
Needs rebase
on Sep 22, 2020
luke-jr force-pushed
on Nov 19, 2020
DrahtBot removed the label
Needs rebase
on Nov 19, 2020
DrahtBot added the label
Needs rebase
on Dec 1, 2020
fjahr
commented at 0:01 am on April 19, 2021:
contributor
Concept ACK on solving this for internal purposes. I started working on the same issue with #21726 before I found this. I am only focused on finding a simple solution for internal issues, specifically indices that are still syncing. I am not sure if there is much need for the additional features that this PR provides (RPCs etc.). But happy to consolidate ideas/approaches if that seems useful to other reviewers.
luke-jr force-pushed
on Jul 7, 2021
luke-jr
commented at 0:39 am on July 7, 2021:
member
Rebased
DrahtBot removed the label
Needs rebase
on Jul 7, 2021
DrahtBot added the label
Needs rebase
on Jul 20, 2021
luke-jr
commented at 7:31 pm on February 4, 2022:
member
Closing this in favour of #21726 for now (this PR had some more functionality, but that can be added in later)
luke-jr closed this
on Feb 4, 2022
fanquake referenced this in commit
34ae04d775
on Apr 26, 2022
luke-jr reopened this
on Nov 12, 2022
luke-jr force-pushed
on Nov 12, 2022
luke-jr force-pushed
on Nov 12, 2022
luke-jr
commented at 5:04 am on November 12, 2022:
member
Rebased
DrahtBot removed the label
Needs rebase
on Nov 12, 2022
luke-jr force-pushed
on Nov 12, 2022
luke-jr
commented at 4:58 pm on November 12, 2022:
member
A new circular dependency in the form of “node/blockstorage -> txdb -> node/blockstorage” appears to have been introduced.
CI is falsely equating headers and source files. I don’t see a clean way to resolve this (besides fixing CI to distinguish properly)
DrahtBot added the label
Needs rebase
on Feb 17, 2023
achow101
commented at 3:29 pm on April 25, 2023:
member
Are you still working on this?
luke-jr force-pushed
on Jul 8, 2023
luke-jr
commented at 1:58 am on July 8, 2023:
member
Are you still working on this?
Yes, though it seems even rebasing on branch-25 still results in conflicts, so I’ll have to get back to it after Knots v25.
achow101
commented at 4:15 pm on September 20, 2023:
member
Are you still working on this?
achow101 marked this as a draft
on Sep 20, 2023
achow101
commented at 4:16 pm on September 20, 2023:
member
Converting to draft as this is not ready for review.
luke-jr force-pushed
on Dec 19, 2023
DrahtBot added the label
CI failed
on Dec 19, 2023
DrahtBot removed the label
Needs rebase
on Dec 19, 2023
luke-jr force-pushed
on Dec 19, 2023
luke-jr force-pushed
on Dec 21, 2023
DrahtBot removed the label
CI failed
on Dec 21, 2023
luke-jr marked this as ready for review
on Dec 21, 2023
luke-jr
commented at 11:13 pm on December 21, 2023:
member
Rebased
luke-jr requested review from kristapsk
on Dec 21, 2023
DrahtBot added the label
CI failed
on Jan 24, 2024
achow101 removed review request from kristapsk
on Apr 9, 2024
achow101 requested review from mzumsande
on Apr 9, 2024
DrahtBot marked this as a draft
on Apr 23, 2024
DrahtBot added the label
Needs rebase
on Jul 11, 2024
luke-jr force-pushed
on Sep 3, 2024
Roll back (tip - 1) prune locks when doing a reorg8ee1214157
Move prune lock checking into BlockManager0b4bd4e134
blockstorage: Add height_last to PruneLockInfof4b3368b13
Add internal interfaces for prune locks
Including desc member of PruneLockInfo for a human-readable description
b4e1c9614e
Support for persisting prune locks in blocks/index db4822c21812
RPC: blockchain: Add listprunelocks and setprunelock methodsb64d1e9b7b
Bugfix: QA/fuzz: Add listprunelocks and setprunelock to RPC_COMMANDS_SAFE_FOR_FUZZING1d1c92d40d
Bugfix: RPC: blockchain: Actually include "temporary" flag in listprunelocks result162f0dba2f
luke-jr force-pushed
on Apr 6, 2025
luke-jr marked this as ready for review
on Apr 6, 2025
DrahtBot removed the label
Needs rebase
on Apr 6, 2025
DrahtBot removed the label
CI failed
on Apr 6, 2025
DrahtBot closed this
on Apr 7, 2025
DrahtBot reopened this
on Apr 7, 2025
in
src/validation.cpp:3094
in
8ee1214157outdated
3087@@ -3088,13 +3088,13 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
3088 Ticks<MillisecondsDouble>(SteadyClock::now() - time_start));
30893090 {
3091- // Prune locks that began at or after the tip should be moved backward so they get a chance to reorg
3092+ // Prune locks that began around the tip should be moved backward so they get a chance to reorg
3093 const int max_height_first{pindexDelete->nHeight - 1};
3094 for (auto& prune_lock : m_blockman.m_prune_locks) {
3095- if (prune_lock.second.height_first <= max_height_first) continue;
3096+ if (prune_lock.second.height_first < max_height_first) continue;
Commit 8ee12141570140f56f5cfefd793825a8787656a6:
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?
in
src/node/blockstorage.h:100
in
f4b3368b13outdated
96@@ -97,6 +97,7 @@ struct CBlockIndexHeightOnlyComparator {
97 98 struct PruneLockInfo {
99 int height_first{std::numeric_limits<int>::max()}; //! Height of earliest block that should be kept and not pruned
100+ int height_last{std::numeric_limits<int>::max()}; //! Height of latest block that should be kept and not pruned
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?
in
src/rpc/blockchain.cpp:972
in
b64d1e9b7boutdated
967+ }
968+ } else {
969+ if (lockid == "*") throw JSONRPCError(RPC_INVALID_PARAMETER, "id \"*\" only makes sense when deleting");
970+ if (!height_param[0].isNum()) throw JSONRPCError(RPC_TYPE_ERROR, "Invalid start height");
971+ lock_info.height_first = height_param[0].getInt<uint64_t>();
972+ if (!height_param[1].isNull()) {
Should probably return an error if height_first > height_last.
mzumsande
commented at 9:40 pm on April 7, 2025:
contributor
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?
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2025-04-27 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me