Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prune locks #19463

Closed
wants to merge 6 commits into from
Closed

Prune locks #19463

wants to merge 6 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Jul 8, 2020

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.

One caveat in this PR is that it exposes the BDB "fileid" to wallet logic. This is to ensure the same prune lock is used/updated even if the wallet is moved or renamed later. It might seem ugly at face value to do it this way, but I'm not sure we have any other unique identifier, and it should be trivial for any new db backend to simply migrate the fileid to a dedicated key.

TODO:

  • Tests
  • Check endian safety of BDB fileid
  • GUI management of prune locks (followup PR?)
  • Ability to enable for wallets from the GUI (followup PR?)

Edit: Removed wallet usage for this PR

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 8, 2020

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

Conflicts

Reviewers, 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.

@andronoob
Copy link

andronoob commented Jul 9, 2020

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
Copy link

andronoob commented Jul 9, 2020

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
Copy link
Member Author

luke-jr commented Jul 9, 2020

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...

@andronoob
Copy link

andronoob commented Jul 28, 2020

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
Copy link

andronoob commented Jul 28, 2020

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.

@fjahr
Copy link
Contributor

fjahr commented Apr 19, 2021

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
Copy link
Member Author

luke-jr commented Jul 7, 2021

Rebased

@MarcoFalke
Copy link
Member

MarcoFalke commented Jul 20, 2021

Error: RPC command "listprunelocks" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 20, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@luke-jr luke-jr mentioned this pull request Nov 27, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 22, 2021

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@luke-jr
Copy link
Member Author

luke-jr commented Feb 4, 2022

Closing this in favour of #21726 for now (this PR had some more functionality, but that can be added in later)

@luke-jr luke-jr closed this Feb 4, 2022
fanquake added a commit that referenced this pull request Apr 26, 2022
71c3f03 move-only: Rename index + pruning functional test (Fabian Jahr)
de08932 test: Update test for indices on pruned nodes (Fabian Jahr)
825d198 Index: Allow coinstatsindex with pruning enabled (Fabian Jahr)
f08c9fb Index: Use prune locks for blockfilterindex (Fabian Jahr)
2561823 blockstorage: Add prune locks to BlockManager (Fabian Jahr)
231fc7b refactor: Introduce GetFirstStoredBlock helper function (Fabian Jahr)

Pull request description:

  # Motivation
  The main motivation of this change and only behavior change noticeable by user is to allow running `coinstatsindex` on pruned nodes as has been requested [here for example](https://twitter.com/benthecarman/status/1388170854140452870?s=20).

  # Background
  `coinstatsindex` on pruned nodes can be enabled in a much simpler than it is done here but it comes with downside. The ability to run `blockfilterindex`on pruned nodes was added in #15946 but it also added the `blockfilterindex` as a dependency to `validation` and it introduced two new circular dependencies. Enabling `coinstatsindex` on pruned nodes in a similar way would add it as a dependency as well and introduce another circular dependency.

  Instead, this PR introduces a `m_prune_blockers` map to `BlockManager` as a flexible approach to block pruning. Entities like `blockfilterindex`, for example, can add a key and a height to block pruning over that height. These entities need to update that value to allow more pruning when they are ready.

  # Alternative approach
  Upon completing the first draft of this PR I found #19463 as an alternative that follows the same but follows a very different approach. I am listing the main differences here as I see them:
  - Usage of globals
  - Blocks pruning with a start and a stop height
  - Can persist blockers across restarts
  - Blockers can be set/unset via RPCs

  Personally, I don't think any of these are necessary to be added here but if the general approach or specific features are more appealing to reviewers I am happy to change to a solution based on that PR or port over specific parts of it here.

ACKs for top commit:
  mzumsande:
    Code review ACK 71c3f03
  ryanofsky:
    Code review ACK 71c3f03. Changes since last review: just tweaking comments and asserts, and rebasing
  w0xlt:
    tACK 71c3f03 on signet.

Tree-SHA512: de7efda08b44aa31013fbebc47a02cd2de32db170b570f9643e1f013fee0e8e7ca3068952d1acc6e5e74a70910735c5f263437981ad73df841ad945b52d36b71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants