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

Add API to change HTLC relay parameters #216

Closed
yuntai opened this issue Oct 20, 2018 · 7 comments · Fixed by #1527
Closed

Add API to change HTLC relay parameters #216

yuntai opened this issue Oct 20, 2018 · 7 comments · Fixed by #1527
Labels
good first issue Good for newcomers
Milestone

Comments

@yuntai
Copy link
Contributor

yuntai commented Oct 20, 2018

I wonder we need user-facing API to update relay parameter for

  • cltv_expiry_delta
  • htlc_minimum_msat
  • fee_base_msat
  • fee_proportional_millionths

and fire channel_update message (if there is actual update). (related to #169?)

While writing test code for #157, I found that it'd be much easier (and more realistic) to actually change channel config to simulate error condition.

@yuntai
Copy link
Contributor Author

yuntai commented Oct 20, 2018

Confused about how htlc_minimum_msat is described in BOLT but came up with a theory. Could you guys please check on this?

In the Requirement section for channel_update in BOLT7,

- MUST set htlc_minimum_msat to the minimum HTLC value (in millisatoshi) that the final node will accept.

The final seems to be defined as the final recipient of the message. So the above condition sounds strange. Perhaps 'the minimum HTLC value in the peer(the other end of the channel) node'?

In BOLT4,
'amount_below_minimum' onion error is raised when

The HTLC amount was below the htlc_minimum_msat of the channel from the processing node.

and

if the HTLC amount is less than the currently specified minimum amount:
report the amount of the incoming HTLC and the current channel setting for the outgoing channel.

Then the HTLC amount here must mean amt_to_forward in the onion packet(while still reporting the incoming amount).

Since there is no message to notify the changes of their own htlc_mimimum_msat between peers, 'htlc_mimimum_msat' is fixed after the initial announcement in open/accept, so we don't need API to change it. And, so is fixed the htlc_minimum_msat field in channel_update.

@ariard
Copy link

ariard commented Oct 21, 2018

Seems to agree with you doesn't make sense as final node isn't known by origin node at channel_update build-time, it's a broadcast all if I get it right.

So htlc_minimum_msat seems currently static in channel_update but in the future maybe nodes'll be able to renegotiate this parameter if they want to respond to a channel capacity increase..

@yuntai
Copy link
Contributor Author

yuntai commented Oct 23, 2018

Found #477 in RFC repo addresses my first concern in BOLT #7.

- MUST set htlc_minimum_msat to the minimum HTLC value (in millisatoshi) that the final node will accept.

'the final node' => 'channel peer'

@TheBlueMatt
Copy link
Collaborator

re: htlc_minimum_msat - indeed, the spec is confusing (and wrong).

re: ChannelUpdate firing - indeed, we should have an API to change things such as fee_proportional_millionths which would, by side effect, generate channel_updates. Don't think we really want a channel_update-generation API specifically because those should only be sent when there is an actual update.

@yuntai yuntai changed the title API to fire ChannelUpdate Add API to change HTLC relay parameters Oct 23, 2018
@TheBlueMatt TheBlueMatt added this to the 0.2 milestone Oct 17, 2021
@TheBlueMatt TheBlueMatt added the good first issue Good for newcomers label Oct 17, 2021
@TheBlueMatt
Copy link
Collaborator

Tagging this good-first-issue, at least for the fee-update parts - having an api to change forwarding_fee_proportional_millionths, forwarding_fee_base_msat, and cltv_expiry_delta should be relatively straightforward and also a pretty huge win in terms of missing functionality. We already have all the logic to regenerate channel_updates, so we basically just need to have an api to change these values and then generate an update.

@TheBlueMatt TheBlueMatt modified the milestones: 0.2, 0.1 Nov 10, 2021
@TheBlueMatt
Copy link
Collaborator

Tagging 0.1 cause apparently we have some users who want this sooner rather than later.

@ConorOkus ConorOkus moved this to In Progress in LDK Roadmap Dec 7, 2021
@matchacactus
Copy link

Working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants