Uh oh!
There was an error while loading. Please reload this page.
Handle retrying sign_counterparty_commitment failures - #2554
Closed
TheBlueMatt wants to merge 6 commits into
Closed
Conversation
If sign_counterparty_commitment fails (i.e. because the signer is temporarily disconnected), this really indicates that we should retry the message sending later, rather than force-closing the channel (which probably won't even work if the signer is missing). Here we add initial handling of sign_counterparty_commitment failing during normal channel operation, setting a new flag in `ChannelContext` which indicates we should retry sending the commitment update later. We don't yet add any ability to do that retry.
If sign_counterparty_commitment fails (i.e. because the signer is temporarily disconnected), this really indicates that we should retry the message sending which required the signature later, rather than force-closing the channel (which probably won't even work if the signer is missing). Here we add initial handling of sign_counterparty_commitment failing during outbound channel funding, setting a new flag in `ChannelContext` which indicates we should retry sending the `funding_created` later. We don't yet add any ability to do that retry.
If sign_counterparty_commitment fails (i.e. because the signer is temporarily disconnected), this really indicates that we should retry the message sending which required the signature later, rather than force-closing the channel (which probably won't even work if the signer is missing). Here we add initial handling of sign_counterparty_commitment failing during inbound channel funding, setting a flag in `ChannelContext` which indicates we should retry sending the `funding_signed` later. We don't yet add any ability to do that retry.
If sign_counterparty_commitment fails (i.e. because the signer is temporarily disconnected), this really indicates that we should retry the message sending which required the signature later, rather than force-closing the channel (which probably won't even work if the signer is missing). This commit adds initial retrying of failures, specifically regenerating commitment updates, attempting to re-sign the `CommitmentSigned` message, and sending it to our peers if we succed.
If sign_counterparty_commitment fails (i.e. because the signer is temporarily disconnected), this really indicates that we should retry the message sending which required the signature later, rather than force-closing the channel (which probably won't even work if the signer is missing). This commit adds retrying of outbound funding_created signing failures, regenerating the `FundingCreated` message, attempting to re-sign, and sending it to our peers if we succeed.
waterson
reviewed
Sep 6, 2023
Uh oh!
There was an error while loading. Please reload this page.
waterson
reviewed
Sep 6, 2023
waterson
left a comment
Contributor
There was a problem hiding this comment.
All existing CI tests (in particular payment tests) pass with the change in channel.rs to fix the counterparty commitment transaction number we provide to the monitor.
If sign_counterparty_commitment fails (i.e. because the signer is temporarily disconnected), this really indicates that we should retry the message sending which required the signature later, rather than force-closing the channel (which probably won't even work if the signer is missing). This commit adds retrying of inbound funding_created signing failures, regenerating the `FundingSigned` message, attempting to re-sign, and sending it to our peers if we succeed.
TheBlueMattforce-pushed
the
2023-09-counterparty-sign-retry
branch
from
September 6, 2023 16:47
237ceff to
48aafb5CompareTheBlueMatt
commented
Sep 6, 2023
CollaboratorAuthor
Mmm, good catch, yea. I went ahead and took your change, but feel free to do whatever you want with this PR. |
codecov-commenter
commented
Sep 6, 2023
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@## main #2554 +/- ##
=======================================
Coverage 90.61% 90.61% =======================================
Files 110 110 Lines 57573 57586 +13 Branches 57573 57586 +13 =======================================
+ Hits 52167 52180 +13
Misses 5406 5406
☔ View full report in Codecov by Sentry. |
TheBlueMatt
commented
Sep 6, 2023
CollaboratorAuthor
Supersceded by #2558 |
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
If sign_counterparty_commitment fails (i.e. because the signer is
temporarily disconnected), this really indicates that we should
retry the message sending which required the signature later,
rather than force-closing the channel (which probably won't even
work if the signer is missing).
This adds implements this properly.
It is missing tests and even breaks a test, but I wrote it as a demo and I think it'd basically be land-able if there were tests. Up for grabs - if someone wants to write tests, do it, and I'll close this PR and let you replace it.