-
Notifications
You must be signed in to change notification settings - Fork 157
Follow-ups to #660 #992
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?
Follow-ups to #660 #992
Changes from all commits
0e76fe8
3ef0f3b
0f909c7
05a0c32
ebef8da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -292,6 +292,16 @@ impl Node { | |
| return Err(Error::AlreadyRunning); | ||
| } | ||
|
|
||
| match self.start_inner(&mut is_running_lock) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex:
Seems that might be worth an additional PR though. Let me know if you prefer I pick that up.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me know what you think of the commit below, we move the fallible operations before starting the tasks.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we'd want to do this, as we deliberately allow inbound connections as one of the last steps after we're positive we're ready-to-go.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Help me understand thank you, once we've passed all the can-fail calls, aren't we positive we are ready to go ?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I think I was misunderstanding what you're doing. Still, just moving code around so we don't start tasks before possibly emitting an error seems very brittle. We might easily add another error'ing case sometime soon, and then have the same issue again. We probably need to fix it 'properly' by signalling stop to the tasks etc., but doing this is probably out-of-scope for this PR as mentioned above.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good I kept the commit below, and we can add the proper signalling in the next PR.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I'd prefer to drop the extra diff there, as splitting the listener logic in two just makes it harder to follow going forward. We'll want to keep that code block together, but need to address the 'wind down spawned tasks on error otherwise'.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I dropped the diff, and filed an issue. |
||
| Ok(()) => Ok(()), | ||
| Err(e) => { | ||
| self.chain_source.stop(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex:
|
||
| Err(e) | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| fn start_inner(&self, is_running_lock: &mut bool) -> Result<(), Error> { | ||
| log_info!( | ||
| self.logger, | ||
| "Starting up LDK Node with node ID {} on network: {}", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ use crate::logger::{log_error, LdkLogger}; | |
| use crate::types::Wallet; | ||
| use crate::Error; | ||
|
|
||
| const BCAST_PACKAGE_QUEUE_SIZE: usize = 50; | ||
| const BCAST_PACKAGE_QUEUE_SIZE: usize = 256; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went back and forth with codex on this one, for now I lean against drawing transactions at random order: we do not do this now, but in the future we might want to make sure we broadcast parents before children no ?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, in short, I'm not sure we could even begin to guarantee this? Broadcast is inherently fallible, we never know when the network connection could drop, when the backend won't accept anything into the mempool, and when it will just decide to drop any transaction again. So without mempool introspection I'd always lean on treating broadcast as an entirely opaque operation: we submit and retry, and only stop once we see what we expect confirmed in a block.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good added the random draw below |
||
|
|
||
| /// A package of transactions that LDK handed to the broadcaster in one `broadcast_transactions` | ||
| /// call, along with each transaction's type. Queued until the background task classifies and | ||
|
|
||
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.
Codex:
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.
Done below.