-
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
lnwallet/wallet.go: clarify anchor chan error str #5577
Conversation
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 the fix. The new error message is definitely more understandable. Added suggestion of my own, hope you don't mind.
All PRs need an entry in the release notes (this one should go into 0.14.0) too.
lnwallet/wallet.go
Outdated
@@ -59,7 +59,8 @@ var ( | |||
// transaction that would take the walletbalance below what we require | |||
// to keep around to fee bump our open anchor channels. | |||
ErrReservedValueInvalidated = errors.New("reserved wallet balance " + | |||
"invalidated") | |||
"invalidated: transaction would not leave sufficient funds to " + |
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.
nit: how about would leave insufficient reserve for fee bumping ...
instead of not leave sufficient funds to fee bump ...
?
Yeah sure it was late and that simple sentence took me far too long to construct, but I also noticed that it was still a bit bumpy, haha. Saw the failing release notes test but assumed this was too insignificant to be mentioned. It's probably all explained in the contributor guidelines. Will update all that later and read these guidelines. |
Quick update, I remember that you mentioned that both sides of the channel need the 10k reserve. I need to look at the code again to understand if it would raise the same error message if the other node is lacking funds, and if yes, include that hint in the error string. |
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.
Great improvement to this error!
Just need to add this to the 0.14 release notes, no PR is too small! :)
Thank you! Life's been busy but I'll definitely get to it this weekend. TODO:
|
51f38fd
to
2e82851
Compare
Improve 'ErrReservedValueInvalidated' error string to explain that the error is triggered by a transaction that would deplete funds reserved for potential future anchor channel closings (via CPFP) Add hint that further details can be found in the debug log Update strings in 'lntest/itest/log_error_whitelist.txt' correspondingly
I think this should be ready now ... I updated the strings as suggested by @guggero and added the PR to the release notes. I also checked whether the same error is thrown when the counterparty node doesn't have sufficient funds, but couldn't see that that's the case. So either only the local node is checked, or the balance of the other node is checked somewhere else and a different error is thrown. (I'm not sure how exactly this would be done, given we shouldn't be able to see the UTXOs of someone else's node... so it would have to tell us.) |
Thank you! |
See #5523
Improve 'ErrReservedValueInvalidated' error string to explain that the
error is triggered by a transaction that would deplete funds reserved for
potential future anchor channel closings (via CPFP)
Add hint that further details can be found in the debug log
Update strings in 'lntest/itest/log_error_whitelist.txt' correspondingly