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 strict-forwarding option #7606

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

Lagrang3
Copy link
Collaborator

@Lagrang3 Lagrang3 commented Aug 24, 2024

Add strict forwarding option

Description

When receiving a forward request lightningd selects the best channel with
the next peer based on its ability to add an HTLC and its spendable amount.
If two forwarding request arrive at the same time pointing to the same
peer we can have a race condition in which both HTLCs compete
concurrently for the same spendable amount.

There shouldn't be a race condition if both requests are supposed to be
allocated on different parallel channels. However, non-strict forwarding is allowed
in the protocol and it is reasonable that node operators can decide which channel
to use when forwarding to a peer.

Therefore we add configuration option dev-strict-forwarding default to false,
to enforce strict forwarding. With that flag we no longer need to skip parallel channel
tests in renepay (9d88ce3).

Related Issues

Changes Made

  • Feature: the ability to turn off non-strict forwarding as a configuration option.

Checklist

Ensure the following tasks are completed before submitting the PR:

  • Changelog has been added in relevant commit/s.
  • Tests have been added or updated to cover the changes.
  • Documentation has been updated as needed.
  • Any relevant comments or TODOs have been addressed or removed.

Additional Notes

This is not a solution to the race condition, it only avoids it by removing the freedom to choose the forward channel.

@Lagrang3 Lagrang3 added the bug label Aug 24, 2024
@Lagrang3 Lagrang3 force-pushed the fix-reserve-race branch 2 times, most recently from d6df8d2 to 4170c09 Compare August 24, 2024 10:35
@Lagrang3 Lagrang3 marked this pull request as draft August 24, 2024 12:00
@Lagrang3
Copy link
Collaborator Author

OK, so 495403d, breaks the test_zeroconf_multichan_forward on tests/test_opening.py, because the test expects a different forward channel than the one chosen by the payer...

Shall we add a configuration option to enable/disable non-strict forwarding?

@Lagrang3 Lagrang3 changed the title Fix reserve race Add strict-forwarding option Aug 25, 2024
@Lagrang3 Lagrang3 removed the bug label Aug 25, 2024
@Lagrang3 Lagrang3 marked this pull request as ready for review August 25, 2024 06:31
@rustyrussell
Copy link
Contributor

If the channels have any fee differences, they will also not be considered equivalent. So, varying feerates serves to override, too. This will affect renepay's algorithm too, unfortunately :(

@Lagrang3 Lagrang3 added this to the v24.11 milestone Oct 29, 2024
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack d0ca1d7

Changelog-Add: add option dev-strict-forwarding
@rustyrussell
Copy link
Contributor

Trivial rebase for silly conflict in test_pay.py.

@vincenzopalazzo vincenzopalazzo merged commit bc419b4 into ElementsProject:master Nov 12, 2024
37 of 39 checks passed
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.

Unable to pay extreme multipart payment in tests
3 participants