Skip to content
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

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

xanoni
Copy link
Contributor

@xanoni xanoni commented Jul 27, 2021

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

Copy link
Collaborator

@guggero guggero left a 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.

@@ -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 " +
Copy link
Collaborator

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 ...?

@xanoni
Copy link
Contributor Author

xanoni commented Jul 27, 2021

Thanks for the fix. The new error message is definitely more understandable. Added suggestion of my own, hope you don't mind.

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.

@xanoni
Copy link
Contributor Author

xanoni commented Jul 28, 2021

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.

Copy link
Collaborator

@carlaKC carlaKC left a 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! :)

@xanoni
Copy link
Contributor Author

xanoni commented Jul 30, 2021

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:

  • Understand what message is thrown when the other node doesn't have sufficient funds, and if that message needs to be clarified as well
  • Add release notes

@xanoni xanoni force-pushed the master branch 3 times, most recently from 51f38fd to 2e82851 Compare August 2, 2021 09:38
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
@xanoni
Copy link
Contributor Author

xanoni commented Aug 2, 2021

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.)

@guggero guggero merged commit 44971f0 into lightningnetwork:master Aug 2, 2021
@xanoni
Copy link
Contributor Author

xanoni commented Aug 3, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants