Prune locks #19463

pull luke-jr wants to merge 10 commits into bitcoin:master from luke-jr:prune_locks changing 11 files +339 −38
  1. 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.

  2. luke-jr force-pushed on Jul 8, 2020
  3. 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.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK fjahr

    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:

    • #30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
    • #29668 (prune, rpc: Check undo data when finding pruneheight by fjahr)
    • #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.

  4. DrahtBot added the label RPC/REST/ZMQ on Jul 8, 2020
  5. DrahtBot added the label UTXO Db and Indexes on Jul 8, 2020
  6. DrahtBot added the label Validation on Jul 8, 2020
  7. DrahtBot added the label Wallet on Jul 8, 2020
  8. 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.
  9. 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.
  10. 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…

  11. laanwj added the label Block storage on Jul 9, 2020
  12. laanwj removed the label Wallet on Jul 9, 2020
  13. laanwj removed the label Validation on Jul 9, 2020
  14. laanwj removed the label UTXO Db and Indexes on Jul 9, 2020
  15. luke-jr force-pushed on Jul 24, 2020
  16. luke-jr force-pushed on Jul 28, 2020
  17. 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.

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

  19. luke-jr force-pushed on Jul 29, 2020
  20. luke-jr force-pushed on Jul 29, 2020
  21. luke-jr commented at 5:45 pm on July 29, 2020: member
    Removed wallet support for this PR, rebased, added test, and passing CI.
  22. in src/rpc/blockchain.cpp:930 in 8d43c3cbc0 outdated
    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"},
    


    kristapsk commented at 6:52 pm on July 29, 2020:
    This line should be removed I think (duplicate “height” with wrong description).

    luke-jr commented at 7:44 pm on July 29, 2020:
    Fixed
  23. luke-jr force-pushed on Jul 29, 2020
  24. DrahtBot added the label Needs rebase on Sep 22, 2020
  25. luke-jr force-pushed on Nov 19, 2020
  26. DrahtBot removed the label Needs rebase on Nov 19, 2020
  27. DrahtBot added the label Needs rebase on Dec 1, 2020
  28. 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.
  29. luke-jr force-pushed on Jul 7, 2021
  30. luke-jr commented at 0:39 am on July 7, 2021: member
    Rebased
  31. DrahtBot removed the label Needs rebase on Jul 7, 2021
  32. maflcko commented at 3:36 pm on July 20, 2021: member
    0Error: RPC command "listprunelocks" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.
    
  33. DrahtBot added the label Needs rebase on Jul 20, 2021
  34. 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)
  35. luke-jr closed this on Feb 4, 2022

  36. fanquake referenced this in commit 34ae04d775 on Apr 26, 2022
  37. luke-jr reopened this on Nov 12, 2022

  38. luke-jr force-pushed on Nov 12, 2022
  39. luke-jr force-pushed on Nov 12, 2022
  40. luke-jr commented at 5:04 am on November 12, 2022: member
    Rebased
  41. DrahtBot removed the label Needs rebase on Nov 12, 2022
  42. maflcko commented at 8:11 am on November 12, 2022: member
    CI is red
  43. luke-jr force-pushed on Nov 12, 2022
  44. 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)

  45. DrahtBot added the label Needs rebase on Feb 17, 2023
  46. achow101 commented at 3:29 pm on April 25, 2023: member
    Are you still working on this?
  47. luke-jr force-pushed on Jul 8, 2023
  48. 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.

  49. achow101 commented at 4:15 pm on September 20, 2023: member
    Are you still working on this?
  50. achow101 marked this as a draft on Sep 20, 2023
  51. achow101 commented at 4:16 pm on September 20, 2023: member
    Converting to draft as this is not ready for review.
  52. luke-jr force-pushed on Dec 19, 2023
  53. DrahtBot added the label CI failed on Dec 19, 2023
  54. DrahtBot removed the label Needs rebase on Dec 19, 2023
  55. luke-jr force-pushed on Dec 19, 2023
  56. luke-jr force-pushed on Dec 21, 2023
  57. DrahtBot removed the label CI failed on Dec 21, 2023
  58. luke-jr marked this as ready for review on Dec 21, 2023
  59. luke-jr commented at 11:13 pm on December 21, 2023: member
    Rebased
  60. luke-jr requested review from kristapsk on Dec 21, 2023
  61. DrahtBot added the label CI failed on Jan 24, 2024
  62. achow101 removed review request from kristapsk on Apr 9, 2024
  63. achow101 requested review from mzumsande on Apr 9, 2024
  64. DrahtBot commented at 5:43 pm on April 17, 2024: contributor
    If still relevant, this needs rebase due to silent merge conflict.
  65. DrahtBot marked this as a draft on Apr 23, 2024
  66. DrahtBot commented at 6:44 am on April 23, 2024: contributor
    Converted to draft due to failing CI and inactivity
  67. DrahtBot added the label Needs rebase on Jul 11, 2024
  68. DrahtBot commented at 2:00 am on July 11, 2024: contributor

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

  69. Roll back (tip - 1) prune locks when doing a reorg ad4f3e472f
  70. Move prune lock checking into BlockManager 64de1480d3
  71. blockstorage: Add height_last to PruneLockInfo 1ebacb60f8
  72. Add internal interfaces for prune locks
    Including desc member of PruneLockInfo for a human-readable description
    55fc577c9a
  73. Support for persisting prune locks in blocks/index db ee8bcadb84
  74. RPC: blockchain: Add listprunelocks and setprunelock methods 6337da5696
  75. QA: Test prune locks via RPC aeb14e6f8e
  76. RPC/Blockchain: Optimise setprunelock "delete all" slightly 24f1597337
  77. Bugfix: QA/fuzz: Add listprunelocks and setprunelock to RPC_COMMANDS_SAFE_FOR_FUZZING 53e5476b40
  78. Bugfix: RPC: blockchain: Actually include "temporary" flag in listprunelocks result 11e1e09759
  79. luke-jr force-pushed on Sep 3, 2024

github-metadata-mirror

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: 2024-10-04 13:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me