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 --ignore-fee-rates on mutual close, don't massively overpay with LDK nodes #7252

Conversation

rustyrussell
Copy link
Contributor

@rustyrussell rustyrussell commented Apr 23, 2024

#7242 describes the pain this causes :(

Fixes #7242

@rustyrussell rustyrussell added this to the v24.05 milestone Apr 23, 2024
@TonyGiorgio
Copy link
Contributor

Concept ACK, appreciate this. Have not tested this yet but from investigations on our side, it sounds like this would fix the issue.

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 0567ac4

Make sense, iirc we use this ignore fee to avoid disable the channel but when we need to touch the chain it is good that we enforce our point of view, this should also possible bugs

Thanks

@vincenzopalazzo
Copy link
Collaborator

Ok the CI cached the following crash, that looks like unrelated, but I will give another un before merging the PR

lightningd-1 2024-04-23T08:08:42.059Z DEBUG   lightningd: Adding block 101: 560466b8148c0f2f6cab1910d1d8eecd043617ccba5cbf94bf0312d56aac271e
lightningd-1 2024-04-23T08:08:42.721Z DEBUG   gossipd: REPLY WIRE_GOSSIPD_NEW_BLOCKHEIGHT_REPLY with 0 fds
lightningd-1 2024-04-23T08:08:42.878Z INFO    plugin-pay: Killing plugin: exited during normal operation
lightningd-1 2024-04-23T08:08:42.878Z **BROKEN** plugin-pay: Plugin marked as important, shutting down lightningd!

@rustyrussell rustyrussell force-pushed the guilt/mutual-close-no-ignore branch 2 times, most recently from 0ee1f8f to fc45c1f Compare May 9, 2024 01:45
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

ACK fc45c1f

Thought it's intuitive, it's not documented to, and LDK's current behavior makes it quite dangerous to do so.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@rustyrussell rustyrussell force-pushed the guilt/mutual-close-no-ignore branch 2 times, most recently from ed92370 to f040f58 Compare May 22, 2024 02:39
@rustyrussell
Copy link
Contributor Author

Fixed timeout caused by close taking forever in test_fee_limits...

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.

genidx doc/index.rst
Checking for generated files being changed by make
git diff --exit-code HEAD
diff --git a/contrib/pyln-grpc-proto/pyln/grpc/node_pb2.py b/contrib/pyln-grpc-proto/pyln/grpc/node_pb2.py
index 2ea52d86d..665aa592c 100644
--- a/contrib/pyln-grpc-proto/pyln/grpc/node_pb2.py
+++ b/contrib/pyln-grpc-proto/pyln/grpc/node_pb2.py
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 # Generated by the protocol buffer compiler.  DO NOT EDIT!
 # source: node.proto
+# Protobuf Python Version: 4.25.1
 """Generated protocol buffer code."""
 from google.protobuf import descriptor as _descriptor
 from google.protobuf import descriptor_pool as _descriptor_pool
diff --git a/contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py b/contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py
index b55614db6..955eb91af 100644
--- a/contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py
+++ b/contrib/pyln-grpc-proto/pyln/grpc/primitives_pb2.py
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 # Generated by the protocol buffer compiler.  DO NOT EDIT!
 # source: primitives.proto
+# Protobuf Python Version: 4.25.1
 """Generated protocol buffer code."""
 from google.protobuf import descriptor as _descriptor
 from google.protobuf import descriptor_pool as _descriptor_pool
make: *** [Makefile:599: check-gen-updated] Error 1

Not clear why this is happening, but looks like the diff for the pylon-grpc-proto package is not necessary.

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 a86e31a

LDK will pick the *upper* limit (see: lightningdevkit/rust-lightning#3014)!

It should not do this, but since you can set a manual range for mutual close, it's probably better to disable this option for close, as it's even more dangerous than documented.

Changelog-Changed: config/JSON: --ignore-fee-limits / setchannel ignorefeelimits no longer applies to mutual close.
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fixes: ElementsProject#7242
@endothermicdev endothermicdev merged commit 877df5a into ElementsProject:master May 22, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLN choosing absurd fee rates for cooperative closures
5 participants