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

Ignore fees per channel #6398

Merged

Conversation

rustyrussell
Copy link
Contributor

We always had a global ignore-fee-limits: this adds an equivalent on a per-channel basis. This is a workaround, of course: letting peers set arbitrary fees is fraught, but there are cases where this can help. For example, there seems to be a problem where LND nodes insist on retransmitting the same fee, even if it knows it's now wrong (e.g. neutrino, or bitcoind restart). We will keep closing the connection on them.

With this, you could temporarily setchannel id=<channel> ignorefeelimits=true and it will hopefully make progress. You can also use it to "trust" peers, and not get upset if there are fee disagreements.

@rustyrussell rustyrussell added feature config Optech Make Me Famous! Look! Look! Look! COOL NEW FEATURE! labels Jul 10, 2023
@rustyrussell rustyrussell added this to the v23.08 milestone Jul 10, 2023
@vincenzopalazzo
Copy link
Collaborator

If you have time to rebase it, I will give a shot for the review today :)

@rustyrussell
Copy link
Contributor Author

If you have time to rebase it, I will give a shot for the review today :)

Done, thanks!

@wtogami
Copy link
Contributor

wtogami commented Jul 20, 2023

https://fedorapeople.org/~wtogami/a/2023/0001-DO-NOT-COMMIT-Temporary-workaround-for-LND-update_fe.patch
Temporary workaround for v23.05.2

Your setchannel approach of runtime switchable ignore fees is far superior to temporary hack that requires restarting CLN.

Would it be possible to add a print to let you know every time an ignore fee event happened?

What I like about my ugly hack is the print tells you exactly what is happening. It is diagnostically useful to know because it should happen JUST ONCE per channel to recover from LND's bug after their fee estimator was broken in the past. If it happens more than once the remote peer's fee estimator is currently broken. You want to know that so there is a chance to talk to the sysadmin to be able to fix it. It appears this PR currently would be silent when it ignores which is less informative?

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The critical logic LGMT but I had some comments from the API point of view.

I was wondering if the following CI failure is related?

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
ERROR tests/test_opening.py::test_anchor_min_emergency - ValueError: 
Node errors:
 - lightningd-1: had bad gossip messages
Global errors:
======= 59 passed, 3 skipped, 4 warnings, 1 error in 2441.54s (0:40:41) ========

lightningd/peer_control.c Show resolved Hide resolved
lightningd/peer_control.c Show resolved Hide resolved
For now, it's set from the global config.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
…llers.

Since it's going to be per-channel, this makes more sense.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `listpeerchannels` has a new field `ignore_fee_limits`
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
It's always true, now deprecated APIs removed.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Added: JSON-RPC: `setchannel` adds a new `ignorefeelimits` parameter to allow peer to set arbitrary commitment transaction fees on a per-channel basis.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 7ec9faa

@rustyrussell
Copy link
Contributor Author

https://fedorapeople.org/~wtogami/a/2023/0001-DO-NOT-COMMIT-Temporary-workaround-for-LND-update_fe.patch Temporary workaround for v23.05.2

Your setchannel approach of runtime switchable ignore fees is far superior to temporary hack that requires restarting CLN.

Would it be possible to add a print to let you know every time an ignore fee event happened?

What I like about my ugly hack is the print tells you exactly what is happening. It is diagnostically useful to know because it should happen JUST ONCE per channel to recover from LND's bug after their fee estimator was broken in the past. If it happens more than once the remote peer's fee estimator is currently broken. You want to know that so there is a chance to talk to the sysadmin to be able to fix it. It appears this PR currently would be silent when it ignores which is less informative?

This may be possible, but the current PR works by telling channeld an infinite allowable range. I think we can do logging in lightningd however, if the actual feerate used is weird.

I will do this as a separate PR.

@rustyrussell rustyrussell merged commit 7372b51 into ElementsProject:master Jul 22, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config feature Optech Make Me Famous! Look! Look! Look! COOL NEW FEATURE!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants