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

Allow maintaining the blockfilterindex when using prune #15946

Merged
merged 6 commits into from Feb 18, 2021

Conversation

jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented May 3, 2019

Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).

This PR allows running the blockfilterindex(es) in conjunction with pruning.

  • Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable -blockfilterindex when we already have pruned)
  • manual block pruning is disabled during blockfilterindex sync
  • auto-pruning is delayed during blockfilterindex sync

ToDos:

  • Functional tests

@DrahtBot
Copy link
Contributor

DrahtBot commented May 3, 2019

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.

src/index/base.cpp Show resolved Hide resolved
src/index/base.cpp Show resolved Hide resolved
src/index/base.h Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
test/lint/lint-circular-dependencies.sh Show resolved Hide resolved
@promag
Copy link
Member

promag commented May 6, 2019

Closes #15867?

@jonasschnelli jonasschnelli force-pushed the 2019/05/prune_blockfilter branch 3 times, most recently from b9537bf to 011904a Compare May 9, 2019
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented May 9, 2019

Rebased and fixed reported points.

src/index/base.cpp Outdated Show resolved Hide resolved
src/index/base.cpp Outdated Show resolved Hide resolved
src/index/base.cpp Outdated Show resolved Hide resolved
src/index/base.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
@laanwj laanwj added the Feature label Sep 30, 2019
@Roasbeef
Copy link

Roasbeef commented Jan 8, 2020

Any chance this will be revived? This is a blocker for lnd to support a pruned bitcoind node, as we want to be able to fetch filters from bitcoind, then manually fetch blocks ourselves (if bitcoind doesn't have them) to scan them for rescans or just normal wallet sync.

@prusnak
Copy link
Contributor

prusnak commented Jul 8, 2020

This feature increases the value of running a pruned node significantly. +1 for adding this!

@luke-jr
Copy link
Member

luke-jr commented Jul 8, 2020

I suspect #19463 could be useful for this

@andronoob andronoob mentioned this pull request Jul 28, 2020
4 tasks
@jonasschnelli jonasschnelli force-pushed the 2019/05/prune_blockfilter branch 3 times, most recently from c49aa4a to 6c7b84c Compare Dec 10, 2020
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Dec 10, 2020

Rewrote this PR and tried to fix the reported point by @promag, @MarcoFalke and @luke-jr.
Added -fastprune debug parameter to test pruning more effectively (added a functional-test as well).

@jonasschnelli jonasschnelli force-pushed the 2019/05/prune_blockfilter branch from e2251df to 84716b1 Compare Feb 16, 2021
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Feb 16, 2021

Thanks @ryanofsky and @fjahr! Fixed the reported points. Thanks for a quick retest.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Code review ACK 84716b1. Only changes since last review were suggested new FindFilesToPrune argument and test.

Change is neat and straightforward and I think close to ready for merge.

This should also get release notes at some point since it adds a new feature and a new error condition that wasn't checked before.

Copy link
Contributor

@benthecarman benthecarman left a comment

tACK 84716b1

@fjahr
Copy link
Contributor

fjahr commented Feb 17, 2021

Code review ACK 84716b1

Checked that only changes since last review were small improvements on the functional test and the added parameter to FindFilesToPrune().

@@ -17,6 +17,7 @@
#include <cuckoocache.h>
#include <flatfile.h>
#include <hash.h>
#include <index/blockfilterindex.h>
Copy link
Member

@jnewbery jnewbery Feb 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this making blockfilterindex part of validation code?

@jamesob jamesob mentioned this pull request May 27, 2021
16 tasks
MarcoFalke added a commit that referenced this pull request Dec 13, 2021
9600ea0 test: Add edge case of pruning up to index height (Martin Zumsande)
698c524 index: Fix backwards search for bestblock (Martin Zumsande)

Pull request description:

  This PR attempts to fix an intermittent Init issue encountered during the stress testing of #23289, which relates to the pruning-compatible filter reconstruction logic introduced in #15946.

  The problem would occur when the node starts with `-txindex=1` but `ThreadSync` is interrupted after it sets `m_best_block_index` to Genesis, and before it gets do any further work.
  In that case, during the next restart of the node, an Init error would be thrown because  `BaseIndex::Init()` tries to backtrack from the tip to the last block which has been successfully indexed (here: Genesis), but the backtracking logic didn't work properly in this case:
  The loop
  `while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))` checks if a predecessor exists **before** performing the check `block_to_test == block` and then possbily setting `prune_violation = false`
  If `block_to_test` and `block` are the Genesis block this check will not be reached because `block->pprev` does not exist.

  To reproduce this bug on regtest:
  1) start a node with a fresh datadir using `-txindex=1` (or any other index)
  2) stop and restart without any index
  3) mine a block
  3) stop and restart again with the index enabled
  ->InitError `Error: txindex best block of the index goes beyond pruned data. (...)`

  Fix this by requiring that we have the data for the block of the current iteration `block` (instead of requiring it for the predecessor `block->pprev`)
  That way, the check for `block_to_test == block` is also reached when `block_to_test` is the Genesis block.
  No longer requiring the data of `block->pprev` also means that we can now prune up to `m_best_block_index` height without requiring a reindex (one block more than before). I added this edge case to `feature_blockfilterindex_prune.py`, the new version should fail on master.

ACKs for top commit:
  ryanofsky:
    Partial code review ACK 9600ea0 for the code change, not the test changes. (Test changes are indirect and little over my head.) It seems obvious that previous code `prune_violation = true, while (block->pprev)` would incorrectly detect a prune violation at the genesis block, and the fix here make sense and looks correct.

Tree-SHA512: c717f372cee8fd49718b1b18bfe237aa6ba3ff4468588c10e1272d7a2ef3981d10af4e57de51dec295e2ca72d441bc6c2812f7990011a94d7f818775e3ff1a38
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 13, 2021
9600ea0 test: Add edge case of pruning up to index height (Martin Zumsande)
698c524 index: Fix backwards search for bestblock (Martin Zumsande)

Pull request description:

  This PR attempts to fix an intermittent Init issue encountered during the stress testing of bitcoin#23289, which relates to the pruning-compatible filter reconstruction logic introduced in bitcoin#15946.

  The problem would occur when the node starts with `-txindex=1` but `ThreadSync` is interrupted after it sets `m_best_block_index` to Genesis, and before it gets do any further work.
  In that case, during the next restart of the node, an Init error would be thrown because  `BaseIndex::Init()` tries to backtrack from the tip to the last block which has been successfully indexed (here: Genesis), but the backtracking logic didn't work properly in this case:
  The loop
  `while (block_to_test && block->pprev && (block->pprev->nStatus & BLOCK_HAVE_DATA))` checks if a predecessor exists **before** performing the check `block_to_test == block` and then possbily setting `prune_violation = false`
  If `block_to_test` and `block` are the Genesis block this check will not be reached because `block->pprev` does not exist.

  To reproduce this bug on regtest:
  1) start a node with a fresh datadir using `-txindex=1` (or any other index)
  2) stop and restart without any index
  3) mine a block
  3) stop and restart again with the index enabled
  ->InitError `Error: txindex best block of the index goes beyond pruned data. (...)`

  Fix this by requiring that we have the data for the block of the current iteration `block` (instead of requiring it for the predecessor `block->pprev`)
  That way, the check for `block_to_test == block` is also reached when `block_to_test` is the Genesis block.
  No longer requiring the data of `block->pprev` also means that we can now prune up to `m_best_block_index` height without requiring a reindex (one block more than before). I added this edge case to `feature_blockfilterindex_prune.py`, the new version should fail on master.

ACKs for top commit:
  ryanofsky:
    Partial code review ACK 9600ea0 for the code change, not the test changes. (Test changes are indirect and little over my head.) It seems obvious that previous code `prune_violation = true, while (block->pprev)` would incorrectly detect a prune violation at the genesis block, and the fix here make sense and looks correct.

Tree-SHA512: c717f372cee8fd49718b1b18bfe237aa6ba3ff4468588c10e1272d7a2ef3981d10af4e57de51dec295e2ca72d441bc6c2812f7990011a94d7f818775e3ff1a38
kittywhiskers added a commit to kittywhiskers/dash that referenced this pull request Feb 26, 2022
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 5, 2022
Summary:
PR description:
> Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).
>
> This PR allows running the blockfilterindex(es) in conjunction with pruning.
>
> - Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable -blockfilterindex when we already have pruned)
> - manual block pruning is disabled during blockfilterindex sync
> - auto-pruning is delayed during blockfilterindex sync

----

> Allow blockfilter in conjunction with prune
bitcoin/bitcoin@6abe9f5

----

> index: Fix backwards search for bestblock
bitcoin/bitcoin@698c524

----

> Avoid accessing nullpointer in BaseIndex::GetSummary()
bitcoin/bitcoin@00d57ff

----

> Avoid pruning below the blockfilterindex sync height
bitcoin/bitcoin@5e11226

----

> Add debug startup parameter -fastprune for more effective pruning tests
bitcoin/bitcoin@00d57ff

----

> Add functional test for blockfilterindex in prune-mode
bitcoin/bitcoin@ab3a0a2

----

> Fix several bugs in feature_blockfilterindex_prune.py
[[bitcoin/bitcoin#21230 | core#21230]]

----

> test: Intermittent issue in feature_blockfilterindex_prune
[[bitcoin/bitcoin#21252 | core#21252]]

----

> test: improve assertions in feature_blockfilterindex_prune.py
> test: remove unneeded node from feature_blockfilterindex_prune.py
[[bitcoin/bitcoin#21297 | core#21297]]

----

> test: Add edge case of pruning up to index height
bitcoin/bitcoin@9600ea0

This is a backport of [[bitcoin/bitcoin#15946 | core#15946]], [[bitcoin/bitcoin#21230 | core#21230]], [[bitcoin/bitcoin#21252 | core#21252]], [[bitcoin/bitcoin#21297 | core#21297]] and  [[bitcoin/bitcoin#23365 |  core#23365]]

Backport notes:
 - the test was buggy and ugly code  after the first commit, so I had to squash all these commits to reach an acceptable quality.
 - I had to use different numbers of blocks that are generated and pruned, because Bitcoin ABC can fit more blocks into each blk?????.dat file than core because in this test the witness data of the coinbase transaction makes core blocks larger. Comments were added to explain this where needed.
 - I used the shortcut `node = self.nodes[0]` to make the code a bit more readable (shorter lines)

Test Plan:
`ninja all check-all`

Run IBD with pruning enabled
` src/bitcoind -prune=2000 -blockfilterindex=1`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D11299
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
@fanquake
Copy link
Member

fanquake commented Sep 15, 2022

This change was part of 22.0, removing "Needs release note".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet