Drop last bits of rescan as its too complicated to be worth having #730
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, we had a concept of "rescaning" blocks when we detected a need to monitor for a new set of outputs in future blocks while connecting a block. In such cases, we'd need to possibly learn about these new spends later in the *same block*, requiring clients who filter blocks to get a newly-filtered copy of the same block. While redoing the chain access API, it became increasingly clear this was an overly complicated API feature, and it seems likely most clients will not use it anyway. Further, any client who *does* filter blocks can simply update their filtering algorithm to include any descendants of matched transactions in the filter results, avoiding the need for rescan support entirely. Thus, it was decided that we'd move forward without rescan support in lightningdevkit#649, however to avoid significant further changes in the already-large 649, we decided to fully remove support in a follow-up. Here, we remove the API features that existed for rescan and fix the few tests to not rely on it. After this commit, we now only ever have one possible version of block connection transactions, making it possible to be significantly more confident in our test coverage actually capturing all realistic scenarios.
jkczyz
approved these changes
Oct 2, 2020
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Oct 2, 2020
|
Post-merge Code Review ACK 2bd571d |
|
Post-Merge review is best (ok, only almost, but still) review. |
TheBlueMatt
added a commit
to TheBlueMatt/rust-lightning
that referenced
this pull request
Oct 8, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, we had a concept of "rescaning" blocks when we detected
a need to monitor for a new set of outputs in future blocks while
connecting a block. In such cases, we'd need to possibly learn about
these new spends later in the same block, requiring clients who
filter blocks to get a newly-filtered copy of the same block. While
redoing the chain access API, it became increasingly clear this was
an overly complicated API feature, and it seems likely most clients
will not use it anyway.
Further, any client who does filter blocks can simply update their
filtering algorithm to include any descendants of matched
transactions in the filter results, avoiding the need for rescan
support entirely.
Thus, it was decided that we'd move forward without rescan support
in #649, however to avoid significant further changes in the
already-large 649, we decided to fully remove support in a
follow-up.
Here, we remove the API features that existed for rescan and fix
the few tests to not rely on it.
After this commit, we now only ever have one possible version of
block connection transactions, making it possible to be
significantly more confident in our test coverage actually
capturing all realistic scenarios.
I'd also started down the path of enforcing in-order block connections/disconnections, but our tests end up using all kinds of crazy chain setups which make this impractical to do in short order.