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

chainntnfs: modify all historical rescans to scan backwards #2211

Merged

Conversation

Roasbeef
Copy link
Member

In this commit, we modify all existing historical rescans for
ChainNotifier backends to scan backwards rather than forwards. If we
know that a transaction has been confirmed, or outpoint spent, the it's
likely that the event has recently transpired assuming we've been
offline for a short period of time. Therefore, if we scan backwards
rather than forwards, then we can save potentially hundreds or thousands
of block fetches if the event recently happened close to the tip of the
chain.

@halseth
Copy link
Contributor

halseth commented Nov 23, 2018

We could also consider updating the height hint caches while doing rescans (not after the rescan is over), to let rescans pick up where they left when restarted. Not sure how well that would play with this change though.

@Roasbeef
Copy link
Member Author

Yep we can follow up with that, would prefer to contain the diff to just these three lines. In order to do that properly, we may need to modify the way height hints are interpreted to mark some as "incomplete". Assuming this goes through, then if we check point these heights, these are now starting points in the backwards search, rather then starting points in the forwards scan.

In this commit, we modify all existing historical rescans for
ChainNotifier backends to scan backwards rather than forwards. If we
know that a transaction has been confirmed, or outpoint spent, the it's
likely that the event has recently transpired assuming we've been
offline for a short period of time. Therefore, if we scan backwards
rather than forwards, then we can save potentially hundreds or thousands
of block fetches if the event recently happened close to the tip of the
chain.

We bound this search at the genesis block, to ensure we don't underflow
the uint32 used throughout the package in the main loop.
@Roasbeef Roasbeef force-pushed the notifier-reverse-scan branch from 31ba7bd to 28eb847 Compare November 24, 2018 21:50
@Roasbeef
Copy link
Member Author

Pushed out a new version to fix the test flake. Other option was to modify the tests, but this route allows the loop to not error out w/o a status code on simnet genesis chains.

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Should keep in mind that this does not cover spend rescans for the btcd and neutrino backends. LGTM ⚡️

@cfromknecht
Copy link
Contributor

cfromknecht commented Nov 26, 2018

Updating the height hint caches w/ partial rescan results is tricky, and was left out of the original PRs due to increased complexity. Scanning backwards indeed makes this more complicated if we don't interpret height hint caches as forward-scanning lower bounds. My guess is that with these changes, partial checkpoints won't be necessary as rescan duration should be shortened considerably, but can revisit if lengthy rescans persist

@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour notifications P2 should be fixed if one has time needs testing PR hasn't yet been actively tested on testnet/mainnet optimization labels Nov 27, 2018
Copy link
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM! 🙏

@cfromknecht cfromknecht merged commit ae46d2e into lightningnetwork:master Dec 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to existing features / behaviour needs testing PR hasn't yet been actively tested on testnet/mainnet notifications optimization P2 should be fixed if one has time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants