-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
multi: fix loopvar and enhance logging around channel reestablishment #8220
multi: fix loopvar and enhance logging around channel reestablishment #8220
Conversation
7b18870
to
d012578
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🌮
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
channeldb/channel.go
Outdated
log.Debugf("Skipped htlc due to onion has not "+ | ||
"matched: id=%v, update=%v incoming=%v", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange wording. Perhaps "Skipped htlc since the onion does not match" or "Skipped htlc due to onion mismatch"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🔥
564310c
to
8a601dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix, LGTM ⭐️
|
||
// TestExtractPayDescs asserts that `extractPayDescs` can correctly turn a | ||
// slice of htlcs into two slices of PaymentDescriptors. | ||
func TestExtractPayDescs(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important test 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch 🎣
channeldb/channel.go
Outdated
onionHash := sha256.Sum256(htlc.OnionBlob[:]) | ||
if _, ok := remoteHtlcs[onionHash]; !ok { | ||
log.Debugf("Skipped htlc due to onion mismatched: "+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need these logs now that the issue is fixed? I think the debug logs might get spammier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah downgraded to trace instead
c804390
to
6b489c0
Compare
6b489c0
to
ae705e5
Compare
Needs a rebase! (release notes conflict) |
ae705e5
to
4845b7a
Compare
Attempt to provide more logs so we can find out what's the root cause in #8130