Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 161
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
Uh oh!
There was an error while loading. Please reload this page.
Follow-ups to #660 #992
Changes from all commits
0e76fe83ef0f3b0f909c705a0c32ebef8daFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
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. ContributorAuthor 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. ContributorAuthor 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. ContributorAuthor 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'. ContributorAuthor 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; | ||
| ||
| /// 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 | ||
Uh oh!
There was an error while loading. Please reload this page.
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.