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

Allow to listen on and advertise multiple SocketAddr #187

Merged

Conversation

alexanderwiederin
Copy link
Contributor

@alexanderwiederin alexanderwiederin commented Nov 10, 2023

Resolves #92

  • Allows nodes to listen and advertise on multiple SocketAddr.
  • Changes random_config in testing to set two listening addresses.

Also fixes a small typo in an error message

@alexanderwiederin alexanderwiederin marked this pull request as draft November 10, 2023 14:54
@alexanderwiederin alexanderwiederin force-pushed the 92-allow-multiple-listen branch 3 times, most recently from 4c401fc to fd3e74a Compare November 12, 2023 16:43
@alexanderwiederin alexanderwiederin marked this pull request as ready for review November 12, 2023 17:31
@alexanderwiederin alexanderwiederin marked this pull request as draft November 13, 2023 10:56
src/builder.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@alexanderwiederin alexanderwiederin force-pushed the 92-allow-multiple-listen branch 3 times, most recently from dec253a to 758e614 Compare November 13, 2023 15:34
@alexanderwiederin alexanderwiederin marked this pull request as ready for review November 13, 2023 23:00
src/builder.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/test/utils.rs Outdated Show resolved Hide resolved
@alexanderwiederin alexanderwiederin force-pushed the 92-allow-multiple-listen branch 2 times, most recently from 7d52e50 to 6d40944 Compare November 14, 2023 11:39
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Pretty much looks good to me, only 1-2 comments and some nits.

Also, could you rebase the branch on current main to see if CI passes?

src/lib.rs Outdated Show resolved Hide resolved
src/builder.rs Outdated Show resolved Hide resolved
src/test/utils.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@alexanderwiederin alexanderwiederin force-pushed the 92-allow-multiple-listen branch 4 times, most recently from c3549f9 to 18f567c Compare November 15, 2023 14:27
@alexanderwiederin
Copy link
Contributor Author

alexanderwiederin commented Nov 15, 2023

Rebased and adapted python tests to set a list of listening_addresses.

Running the tests in the CI, we get:

TMP DIR 1: /tmp/tmpj9y442xu_ldk_node_1
Node ID 1: 034edc368e2707c09293c2035cd7b197edfe5736bb55e55493374592797b376c2e
TMP DIR 2: /tmp/tmpn11v_7gj_ldk_node_2
E/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/tempfile.py:860: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/tmpj9y442xu_ldk_node_1'>
  _warnings.warn(warn_message, ResourceWarning)
thread '<unnamed>' panicked at src/logger.rs:70:14:
Failed to open log file: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
/home/runner/work/_temp/a1a4800b-6608-4c01-8400-1a6843ab90bd.sh: line 2:  6461 Aborted                 (core dumped) python3 -m unittest discover -s src/ldk_node

Running the test locally, returns a similar error, but on a different line.

Will continue to look into this, but if you have an idea, let me know!

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Rebased and adapted python tests to set multiple listening_addresses.

Running the tests in the CI, we get:

TMP DIR 1: /tmp/tmpj9y442xu_ldk_node_1
Node ID 1: 034edc368e2707c09293c2035cd7b197edfe5736bb55e55493374592797b376c2e
TMP DIR 2: /tmp/tmpn11v_7gj_ldk_node_2
E/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/tempfile.py:860: ResourceWarning: Implicitly cleaning up <TemporaryDirectory '/tmp/tmpj9y442xu_ldk_node_1'>
  _warnings.warn(warn_message, ResourceWarning)
thread '<unnamed>' panicked at src/logger.rs:70:14:
Failed to open log file: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
/home/runner/work/_temp/a1a4800b-6608-4c01-8400-1a6843ab90bd.sh: line 2:  6461 Aborted                 (core dumped) python3 -m unittest discover -s src/ldk_node

Running the test locally, returns a similar error, but on a different line.

Will continue looking into this, but if you have an idea, let me know!

Yes, this is what the Python tests put out when they panic (essentially we panic, shutdown, write some logs, while the Python test env already tries to cleanup the temporary folder). See the comment on why it fails.

bindings/python/src/ldk_node/test_ldk_node.py Show resolved Hide resolved
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@tnull tnull merged commit beb2345 into lightningdevkit:main Nov 15, 2023
11 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.

Allow to listen on and advertise multiple NetAddresses
2 participants