-
Notifications
You must be signed in to change notification settings - Fork 97
Fix channel manager race panic #78
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
Merged
Merged
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
45b91b8
to
ae08821
Compare
G8XSU
approved these changes
Oct 19, 2022
jkczyz
reviewed
Oct 19, 2022
Needs rebase, should pass CI now. Thanks! |
eae9614
to
9f08ea3
Compare
CI passed, ready for re-review @TheBlueMatt @jkczyz |
jkczyz
reviewed
Nov 2, 2022
The race: - (1) If a channel manager is starting for the first time, it gets its initial block hash by calling BlockSource::get_blockchain_info() and constructing a BestBlock out of that. - (2) During the initial chain sync, if chain_tip.is_none() then the best chain_tip is retrieved using lightning_block_sync::init::validate_best_block_header() which similarly uses BlockSource::get_best_block() - If blocks were mined between (1) and (2), the channel manager’s block hash will be at a lower height than that of the block hash contained inside the chain_tip fed to the SpvClient. Suppose that this results in the channel manager being at block 1 and the SpvClient being at block 2. Once block 3 comes in and the SpvClient detects it via poll_best_tip(), the SpvClient computes the ChainDifference between its saved chain_tip value and block 3, concluding that it just needs to connect block 3. However, when Listen::filtered_block_connected is called using the data in block 3, the channel manager panics with “Blocks must be connected in chain-order - the connected header must build on the last connected header” - because block 3’s prev block hash is block 2’s block hash, rather than block 1’s block hash which the channel manager expects This commit fixes it by removing the fresh channel manager's query to the block source, instead deriving its best block from the validated block header using a new `.to_best_block()` method.
9f08ea3
to
430084f
Compare
jkczyz
approved these changes
Nov 2, 2022
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.
The race:
This commit fixes it by removing the fresh channel manager's query to the block source, instead deriving its best block from the validated block header using a new
.to_best_block()
method.Depends on lightningdevkit/rust-lightning#1777.
This can be tested locally by adding the following to
ldk-sample
'sCargo.toml
:Fixes #74.