-
Notifications
You must be signed in to change notification settings - Fork 156
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
47dbaec
91b8b7a
727bc62
fe91312
1aa6618
a5628e3
fbb18cf
bda78cc
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 |
|---|---|---|
|
|
@@ -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 | ||
|
|
||
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.
We map 400 here to ChainSourceNotSupported because Bitcoin Core v26 returns a RPC error when submitting the dummy package, which maps to error code 400.
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.
Grrr, this is all very brittle. I can't wait to drop all of this logic again once we can just assume
submitpackageis available if we have any chain source at all.