-
Notifications
You must be signed in to change notification settings - Fork 55
Prepare LN for large networks #737
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
base: main
Are you sure you want to change the base?
Conversation
c84d74b
to
e4ed4b6
Compare
@macgyver13 I touched a lot of your LN API code, lemme know if you see anything smelly or frustrating in here,,, |
Tested with 10, 50, 100 node networks + simln: https://github.com/pinheadmz/ln100-warnet On digital ocean cluster: 100 vCPUs / 200 GB Memory / 1.22 TB Disk |
Concept ACK - I like the separation of concerns, nice work - much needed clean up 👏 I will test later this week and even consider testing eclair with these changes to see if an underlying issue was causing instability with eclair nodes in warnet. There are some nice improvements in ln.py in the eclair PR that may make sense to follow this PR if you see value - around the "use_rpc" concept. |
Well, the way it was before we only reset connection if there was a problem and my new idea is, ln.py doesn't deal with problems at all. I also had to deal with this already in scenarios based on the functional test framework: warnet/resources/scenarios/commander.py Lines 87 to 94 in e4ed4b6
Unlike the test framework, we can't expect an HTTP connection to survive the entire lifespan of a warnet, so we refresh the connection on every call. |
e4ed4b6
to
08cb3e4
Compare
it's supposed to be a 32-byte hash anyway
@@ -204,6 +204,8 @@ def check_logging_required(directory: Path): | |||
return True | |||
if default_file.get("metricsExport", False): | |||
return True | |||
if default_file.get("lnd", False).get("metricsExport"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to use {} for the default value
if default_file.get("lnd", {}).get("metricsExport"):
@@ -216,6 +218,8 @@ def check_logging_required(directory: Path): | |||
return True | |||
if node.get("metricsExport", False): | |||
return True | |||
if node.get("lnd", False).get("metricsExport"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^^ same problem
ln.py
toln_init.py
. Meaning the API calls have no error handling on their own and propagate back up to the caller. This is to ensure consistency in theln_init
scenario which is opinionated about what is allowed to fail. Ideally the scenario completes successfully, terminates with an error, or runs endlessly because something is broken. In all those cases,ln_init
needs to directly handle all API calls to LN nodes.ln_init
for large networks (specificallytest/data/LN_100.json
with >500 channels) ensuring LN nodes always have enough funds to open all their channels, that channel opens are always tx output 0 and channel open TXs are ordered as expected in their blocks.