Channel restoration of old format fee update #2558
Merged
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
cfromknecht
reviewed
Jan 29, 2019
Roasbeef
requested changes
Jan 30, 2019
halseth
force-pushed
the
channel-restoration-old-format-fee-update
branch
from
3b6d7f9 to
93fc204
Compare
Earlier versions did not write the log index to disk for fee updates, so they will be unset. To account for this we set them to to current update log index.
TestFeeUpdateOldDiskFormat tests that we properly recover FeeUpdates written to disk using the old format, where the logIndex was not written.
To avoid more bugs slipping through where the logIndex is not set, we panic to catch this. This was earlier done for Adds and the htlcCounter, which did lead us to find the resulting retoration bug.
halseth
force-pushed
the
channel-restoration-old-format-fee-update
branch
from
93fc204 to
47918bd
Compare
|
Comments addressed, and rebased. PTAL @Roasbeef @cfromknecht |
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.
This PR fixes a bug that was introduced in #2397, where we started restoring
FeeUpdates from disk as other HTLC updates. The problem arose if the prior version oflndwrote the updates to disk, and the new version attempted to restore. Since the old version did not write thelogIndexforFeeUpdates, this would be 0, causing it to be out of sync with the log update index, causing problems.We fix this by setting the
logIndexto the current log update index in case it is unset. A test that would previously cause this desync and following crash duringcompactLogsis included.The last commit contains a sanity check that ensures all updates have their log index properly set, to catch these bugs in the future. It is not strictly necessary, but I thought it could be nice to catch it already here, and not in some obscure situation down the road.
Fixes #2546