Cut 0.0.125 with a few bugfixes #3361
Closed
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
When we upgrade from LDK 0.0.123 or prior, we need to intialize `holder_commitment_point` with commitment point(s). In 1f7f3a3 we changed the point(s) which we fetch from both the current and next per-commitment-point (setting the value to `HolderCommitmentPoint::Available` on upgrade) to only fetching the current per-commitment-point (setting the value to `HolderCommitmentPoint::PendingNext` on upgrade). In `commitment_signed` handling, we expect the next per-commitment-point to be available (allowing us to `advance()` the `holder_commitment_point`), as it was included in the `revoke_and_ack` we most recently sent to our peer, so must've been available at that time. Sadly, these two interact negatively with each other - on upgrade, assuming the channel is at a steady state and there are no pending updates, we'll not make the next per-commitment-point available but if we receive a `commitment_signed` from our peer we'll assume it is. As a result, in debug mode, we'll hit an assertion failure, and in production mode we'll force-close the channel. Instead, here, we fix the upgrade logic to always upgrade directly to `HolderCommitmentPoint::Available`, making the next per-commitment-point available immediately. We also attempt to resolve the next per-commitment-point in `get_channel_reestablish`, allowing any channels which were upgraded to LDK 0.0.124 and are in this broken state to avoid the force-closure, as long as they don't receive a `commitment_signed` in the interim.
If we manage to pull a `node_counter` from `removed_node_counters` for reuse, `add_channel_between_nodes` would `unwrap_or` with the `next_node_counter`-incremented value. This visually looks right, except `unwrap_or` is always called, causing us to always increment `next_node_counter` even if we don't use it. This will result in the `node_counter`s always growing any time we add a new node to our graph, leading to somewhat larger memory usage when routing and a debug assertion failure in `test_node_counter_consistency`. The fix is trivial, this is what `unwrap_or_else` is for.
Previously, the `ChainListenerSet` `Listen` implementation wouldn't forward to the listeners `block_connected` implementation outside of tests. This would result in the default implementation of `Listen::block_connected` being used and the listeners implementation never being called.
When we're calculating the success probability for min-/max-bucket pairs and are looking at the 0th' min-bucket, we only look at the highest max-bucket to decide the success probability. We ignore max-buckets which have a value below `BUCKET_FIXED_POINT_ONE` to only consider values which aren't substantially decayed. However, if all of our data is substantially decayed, this filter causes us to conclude that the highest max-bucket is bucket zero even though we really should then be looking at any bucket. We make this change here, looking at the highest non-zero max-bucket if no max-buckets have a value above `BUCKET_FIXED_POINT_ONE`.
Collaborator
Author
|
Oops, wrong branch, #3362 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Several important bugfixes and an 0.0.125 release. The most important bugfix follows, but see individual commits for more info.
When we upgrade from LDK 0.0.123 or prior, we need to intialize
holder_commitment_pointwith commitment point(s). In1f7f3a3 we changed the point(s)
which we fetch from both the current and next per-commitment-point
(setting the value to
HolderCommitmentPoint::Availableonupgrade) to only fetching the current per-commitment-point (setting
the value to
HolderCommitmentPoint::PendingNexton upgrade).In
commitment_signedhandling, we expect the nextper-commitment-point to be available (allowing us to
advance()the
holder_commitment_point), as it was included in therevoke_and_ackwe most recently sent to our peer, so must've beenavailable at that time.
Sadly, these two interact negatively with each other - on upgrade,
assuming the channel is at a steady state and there are no pending
updates, we'll not make the next per-commitment-point available but
if we receive a
commitment_signedfrom our peer we'll assume itis. As a result, in debug mode, we'll hit an assertion failure, and
in production mode we'll force-close the channel.
Instead, here, we fix the upgrade logic to always upgrade directly
to
HolderCommitmentPoint::Available, making the nextper-commitment-point available immediately.
We also attempt to resolve the next per-commitment-point in
get_channel_reestablish, allowing any channels which wereupgraded to LDK 0.0.124 and are in this broken state to avoid the
force-closure, as long as they don't receive a
commitment_signedin the interim.