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.

Sign up for GitHub

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

Jump to bottom

test: feature_blockfilterindex_prune.py improvements #21297

Merged
merged 2 commits into from
Feb 27, 2021
Merged

test: feature_blockfilterindex_prune.py improvements #21297

merged 2 commits into from
Feb 27, 2021

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Feb 24, 2021 •
edited
Loading

  • improves the assertions
  • removes an unneeded node, reducing from two to one, and some unneeded extra_arg code

@fanquake fanquake added the Tests label Feb 24, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 25, 2021 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Feb 25, 2021
Merged
@jonatack jonatack closed this Feb 25, 2021
@maflcko
Copy link
Member

maflcko commented Feb 25, 2021

Happy to ACK after a rebase, if still relevant

@jonatack jonatack reopened this Feb 25, 2021
@jonatack jonatack force-pushed the feature_blockfilterindex_prune-improvements branch from 764ac92 to 3c3e061 Compare February 25, 2021 14:20
@jonatack jonatack marked this pull request as ready for review February 25, 2021 14:22
@jonatack
Copy link
Member Author

Thanks -- rebased, updated, and now removes an unneeded node to use one instead of two.

@jonatack jonatack changed the title test: feature_blockfilterindex_prune.py improvements test: feature_blockfilterindex_prune.py improvements Feb 25, 2021
maflcko
maflcko approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

I presume the first node was used as "network node" to serve the blockchain, though nothing is done with the chain, so seems fine to remove it

test/functional/feature_blockfilterindex_prune.py Outdated Show resolved Hide resolved
@jonatack jonatack force-pushed the feature_blockfilterindex_prune-improvements branch from 3c3e061 to 88c4b9b Compare February 25, 2021 17:07
@maflcko
Copy link
Member

maflcko commented Feb 25, 2021

ACK 88c4b9b

@jonatack
Copy link
Member Author

Updated per feedback to git diff 3c3e061 88c4b9b (thanks!)

brunoerg
brunoerg approved these changes Feb 27, 2021
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

Tested ACK 88c4b9b

  • Run it on MacOS 11.1
  • Code looks great for me

@maflcko maflcko merged commit fb67cae into bitcoin:master Feb 27, 2021
@jonatack jonatack deleted the feature_blockfilterindex_prune-improvements branch February 27, 2021 17:52
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 27, 2021
Merge bitcoin#21297: test: feature_blockfilterindex_prune.py improvem…
8c51d27 
…ents

88c4b9b test: remove unneeded node from feature_blockfilterindex_prune.py (Jon Atack)
ace3f4c test: improve assertions in feature_blockfilterindex_prune.py (Jon Atack)

Pull request description:

  - improves the assertions
  - removes an unneeded node, reducing from two to one, and some unneeded `extra_arg` code

ACKs for top commit:
  MarcoFalke:
    ACK 88c4b9b
  brunoerg:
    Tested ACK 88c4b9b

Tree-SHA512: 295700da3a5f583ee02ae2d184db93cce0e13aba69115d5db07f83e96a66b4b850adaff2c3725b6585799565b9ee654b1fee8a6245eaba8c21e1cb5ce524eb2b
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 5, 2022
@jonasschnelli @PiRK
Allow maintaining the blockfilterindex when using prune
eacbda7 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@brunoerg brunoerg brunoerg approved these changes

@maflcko maflcko maflcko approved these changes

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jonatack @DrahtBot @maflcko @brunoerg @fanquake