Skip to content

Replace OUR_MAX_HTLCS constant with a config knob #1990

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

Closed
TheBlueMatt opened this issue Jan 26, 2023 · 4 comments · Fixed by #2138
Closed

Replace OUR_MAX_HTLCS constant with a config knob #1990

TheBlueMatt opened this issue Jan 26, 2023 · 4 comments · Fixed by #2138
Labels
good first issue Good for newcomers
Milestone

Comments

@TheBlueMatt
Copy link
Collaborator

Basically what it says on the tin - add a new config knob and use it in place of OUR_MAX_HTLCS, writing an even TLV if its not 50 to break forwards compat.

@TheBlueMatt TheBlueMatt added the good first issue Good for newcomers label Jan 26, 2023
@TheBlueMatt TheBlueMatt added this to the 0.0.115 milestone Jan 26, 2023
@ariard
Copy link

ariard commented Jan 27, 2023

The config knob should warn the post-anchor fee-bumping reserves should be scale in consequence (or we do that under the hood). And probably few other warnings.

@swilliamson5
Copy link
Contributor

I'd like to help! I'll take this as a first issue. I'll need some guidance and I might also post in the Discord for help if needed.

@swilliamson5
Copy link
Contributor

Sorry for the delay, working on trying to figure out how to properly test this. I'm relatively new to LDK and Lightning development so I could be in over my head here. Here's a code snippet for the "even TLV" change. Also, I added a new field to the Channel struct in channel.rs to replace the referenced constant where possible. Let me know if I am way off!

write_tlv_fields!(writer, {
...
  (27, self.channel_keys_id, required),
  (28, Some(self.max_accepted_htlcs), option),
});

@valentinewallace
Copy link
Contributor

Sorry for the delay, working on trying to figure out how to properly test this. I'm relatively new to LDK and Lightning development so I could be in over my head here. Here's a code snippet for the "even TLV" change. Also, I added a new field to the Channel struct in channel.rs to replace the referenced constant where possible. Let me know if I am way off!

write_tlv_fields!(writer, {
...
  (27, self.channel_keys_id, required),
  (28, Some(self.max_accepted_htlcs), option),
});

You can write it as required and read it as an option, which is a bit more succinct, but what you have would work. Matt also mentioned in the top-level comment that it should only be written if it's not 50.

But, this sounds like a fine start, feel free to open a draft PR and you can get feedback there :)

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
None yet
Development

Successfully merging a pull request may close this issue.

4 participants