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

Set initial relay fees during channel open #1610

Merged
merged 1 commit into from
Dec 8, 2020
Merged

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Nov 19, 2020

It can be useful to override the default relay fees when opening channels to specific nodes.

Note that these initial relay fees are not persisted in the DB. That means that if your node reboots before the funding transaction confirms, the channel will be opened with the default relay fees, not the overridden values.

Here is the command to open a channel with overridden relay fees:

eclair-cli open --nodeId=<nodeId> --fundingSatoshis=500000 --feeBaseMsat=1000 --feeProportionalMillionths=10000

Fixes #1507

It can be useful to override the default relay fees when opening channels
to specific nodes.

Note that these initial relay fees are not persisted in the DB. That means
that if your node reboots before the funding transaction confirms, the
channel will be opened with the default relay fees, not the overridden values.

Fixes #1507
@pm47 pm47 self-requested a review November 27, 2020 14:16
Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

Have you considered going with a simpler best-effort non-persisted implementation ?

If we go with a change that involves a db migration, we may as well squeeze in #1616?

@t-bast
Copy link
Member Author

t-bast commented Dec 7, 2020

Have you considered going with a simpler best-effort non-persisted implementation ?

But this is the best effort non-persisted implementation... 🤔
Can you clarify why you're saying otherwise?

@pm47
Copy link
Member

pm47 commented Dec 7, 2020

Have you considered going with a simpler best-effort non-persisted implementation ?

But this is the best effort non-persisted implementation... 🤔
Can you clarify why you're saying otherwise?

🤦‍♂️ I saw that you modified channel types and associated codecs, and jumped to the conclusion.

That said even if we are not changing the database, we are still changing the codecs and a regression here could be fatal. Let me try to convince myself that there is absolutely no risk with the provide method.

@t-bast
Copy link
Member Author

t-bast commented Dec 7, 2020

I get it, yes I had to change the codec but only to provide a None value, so I think it should be ok, but it's worth an independent review ;)

I did test this E2E on regtest, with the following scenarios:

  • Upgrading from the latest release
  • Create channel with initial relay fees set
  • Create channel with initial relay fees set but restart before it confirms

Everything looked good.

pushAmount: MilliSatoshi,
initialFeeratePerKw: FeeratePerKw,
fundingTxFeeratePerKw: FeeratePerKw,
initialRelayFees_opt: Option[(MilliSatoshi, Int)],
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you have considered defining a like so?

case class RelayFees(base: MilliSatoshi, proportional: Int)

The only issue I would have is that I'm not sure how we should then define ChannelUpdate. Except if we define this RelayFees in the scope of the channel? In the future we will probably play a lot more with those feerates.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I did experiment with this, but I felt that then it made sense to use it (almost) everywhere in the codebase if we added this class, and it touched many files, so I figured we would do it in a separate PR to avoid hiding the real change.

@t-bast t-bast merged commit a4d1845 into master Dec 8, 2020
@t-bast t-bast deleted the channel-open-relay-fees branch December 8, 2020 08:51
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.

Feature request: Open channel with specific relay and base fee setting
2 participants