WIP: rework rescan logic - index side #23
Conversation
This file contains 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
added 9 commits
August 19, 2019 15:51
Added CBlockIndex parameter points to the block being disconnected. Pass block height in Chain::BlockConnected/Chain::BlockDisconnected.
Moving indexes as clients of the Chain interface means we should avoid to leak node internals to them if in the future they move further in their own memory spaces
…d undo file position Beyond processing block height/hash and its transactions, Chain client may need to know block position or filters in its database.
Instead of calling block disconnected multiple times and passing a block with irrelevant data we instead send common ancestor height between forked tip and new one. Thanks to this ancestor height Chain client can rollback its interior state and then process to sync forward until new tip.
Chain clients subscribing rescan requests to the Chain interface get their demand processed in ThreadServiceRequests. When tip is reached, thread call HandleNotifications to switch them to the Chain notifications handler, itself an adaptator between CValidationInterface and Chain clients.
Rescan is moving the rescan logic on the node side. A single thread (ThreadServiceRequests) read all received requests from clients and service them in order during initialization. This commit does not change behavior. It just a class that doesn't do anything yet.
Chain interface callers may register for notifications while initializing themselves. Init code starts notification delivery, shutdown code is responsible to stop it. Notification delivery runs in its own thread (ThreadServiceRequests) during rescan period then rely on CValidationInterface.
Instead of being direct consumers of the CValidationInterface indexes are moved as clients of Chain interface. Main benefit is to rescan in one sequence for all chain clients at initialization. Another benefit is to separate index from accessing node internals like direct chain query. This commit moves in this direction doesn't make separation complete. To do so, we adapt BaseIndex and its subclasses to Chain::Notifications model, remove CValidationInterface parts from BaseIndex, remove thread sync.
|
This is great! I looked over all the commits, and this definitely lines up with what I was trying to suggest in the gist, but even more ambitious because it looks ready to unify both the wallet rescans and index rescans at the the same time. Looking forward to seeing the wallet code, and let me know if there's anything I can do to help! |
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.
Right now, when bitcoind starts, we have multiple rescan loops reading the same scan ranges repeatedly out of order instead of just once in order.
This work moves the rescan logic on the node side in a new thread to be used both by wallets and indexes. To do so they need to respect the same class model, currently, index is a direct consumer of CValidationInterface, 1ff4272 makes it a client of Chain interface.
This is first part, still need to clean the wallet side and comment it better.
Work in progress, don't bother for a low-level review now, just commits order, general changes and new classes. There is currently a performance hit, I have ideas on why, working on the new thread loop to make it better.
Proposal & discussion : https://gist.github.com/ariard/89f9bcc3a7ab9576fc6d15d251032cfa