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

bolt07: enforce htlc_maximum_msat >= htlc_minimum_msat #1089

Merged
merged 1 commit into from
Jun 25, 2023

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Jun 22, 2023

Bolt 7 specifies how to set hltc_minimum_msat and htlc_maximum_msat but does not explicitly enforce hltc_minimum_msat <= htlc_maximum_msat.

Since a channel where the largest HTLC that the advertising node will send through it is smaller than the minimum value the peer will accept is unusable, it seems reasonable to blacklist this type of gossip?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

I don't see why we should ban an entire node for such a thing, like I could see a bug on this resulting in this happening, but we should just ignore the channel, not ban the node and all their channels, no? Did you come across this somewhere?

@carlaKC
Copy link
Contributor Author

carlaKC commented Jun 22, 2023

I don't see why we should ban an entire node for such a thing, like I could see a bug on this resulting in this happening, but we should just ignore the channel, not ban the node and all their channels, no?

I think ignore is also fine. Was following the pattern from "if htlc_maximum_msat is greater than channel capacity" since that's also a reaction to a bug?

Did you come across this somewhere?

No, just noticed it when I was checking up on some route blinding stuff.

@TheBlueMatt
Copy link
Collaborator

I think ignore is also fine. Was following the pattern from "if htlc_maximum_msat is greater than channel capacity" since that's also a reaction to a bug?

Yea, and arguably that also shouldn't result in us punishing a node entirely - we reject the gossip anyway, but at least that one is maybe harder to hit?

@carlaKC
Copy link
Contributor Author

carlaKC commented Jun 22, 2023

Yea, and arguably that also shouldn't result in us punishing a node entirely

Yeah fair point, bumped this one to should ignore during route construction.

Copy link
Collaborator

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

ACK d6b2177

Copy link
Contributor

@ProofOfKeags ProofOfKeags left a comment

Choose a reason for hiding this comment

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

ACK d6b2177

@t-bast t-bast merged commit e0995c9 into lightning:master Jun 25, 2023
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.

4 participants