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

pay: still use channels for routehints even if peer says it's disabled. #6556

Merged

Conversation

rustyrussell
Copy link
Contributor

We have a report that LND said our (unannounced) channel was disabled, so we didn't use it for routehints. We're better off ignoring that in this case (if the peer is actually not connected, the routehint code will check that and ignore anyway).

Fixes: #6555
Changelog-Changed: pay: use channels in routehints even if peer says they're "disabled" (LND compat)

We have a report that LND said our (unannounced) channel was disabled, so we didn't
use it for routehints.  We're better off ignoring that in this case (if the peer is
actually not connected, the routehint code will check that and ignore anyway).

Fixes: ElementsProject#6555
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Changelog-Changed: pay: use channels in routehints even if peer says they're "disabled" (LND compat)
@rustyrussell rustyrussell added lnd-compat protocol These issues are protocol level issues that should be discussed on the protocol spec repo labels Aug 13, 2023
@rustyrussell rustyrussell added this to the v23.08 milestone Aug 13, 2023
Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

This LGTM but the CI looks like that catch somethings

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/test_opening.py::test_rbf_broadcast_close_inflights - AssertionError: assert '0446f4874120bebf7f20e378010b3602fee31274f8903dad7570e6bf5b4714a5' in ['5a74e9f4b30c81ddc3996f748a6909875694faa2cf13d516058a484b8fe593e3']
 +  where ['5a74e9f4b30c81ddc3996f748a6909875694faa2cf13d516058a484b8fe593e3'] = <function SimpleBitcoinProxy.__getattr__.<locals>.f at 0x7fe35e04af70>()
 +    where <function SimpleBitcoinProxy.__getattr__.<locals>.f at 0x7fe35e04af70> = <pyln.testing.utils.SimpleBitcoinProxy object at 0x7fe35e04e460>.getrawmempool
 +      where <pyln.testing.utils.SimpleBitcoinProxy object at 0x7fe35e04e460> = <pyln.testing.utils.BitcoinD object at 0x7fe35e03de50>.rpc

Not sure if this is unrelated but it does not seems flaky test

@cdecker
Copy link
Member

cdecker commented Aug 14, 2023

This LGTM but the CI looks like that catch somethings

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================== short test summary info ============================
FAILED tests/test_opening.py::test_rbf_broadcast_close_inflights - AssertionError: assert '0446f4874120bebf7f20e378010b3602fee31274f8903dad7570e6bf5b4714a5' in ['5a74e9f4b30c81ddc3996f748a6909875694faa2cf13d516058a484b8fe593e3']
 +  where ['5a74e9f4b30c81ddc3996f748a6909875694faa2cf13d516058a484b8fe593e3'] = <function SimpleBitcoinProxy.__getattr__.<locals>.f at 0x7fe35e04af70>()
 +    where <function SimpleBitcoinProxy.__getattr__.<locals>.f at 0x7fe35e04af70> = <pyln.testing.utils.SimpleBitcoinProxy object at 0x7fe35e04e460>.getrawmempool
 +      where <pyln.testing.utils.SimpleBitcoinProxy object at 0x7fe35e04e460> = <pyln.testing.utils.BitcoinD object at 0x7fe35e03de50>.rpc

Not sure if this is unrelated but it does not seems flaky test

Seems unrelated: the topology and on-chain footprint have not changed, so why would we suddenly fail to see an on-chain transaction?

@vincenzopalazzo
Copy link
Collaborator

Seems unrelated: the topology and on-chain footprint have not changed, so why would we suddenly fail to see an on-chain transaction?

No idea :) I just post the CI failure because I was travelling and I was not able a sanity check in the code

Copy link
Collaborator

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK 8bae3d1

@rustyrussell rustyrussell merged commit af6b535 into ElementsProject:master Aug 14, 2023
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lnd-compat protocol These issues are protocol level issues that should be discussed on the protocol spec repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unpublished channel with LND peer leading to incoming direction getting disabled.
3 participants