-
Notifications
You must be signed in to change notification settings - Fork 23
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
Update bdk
to version 1.0.0-alpha.6
#2094
Conversation
60963b0
to
b3f0b1f
Compare
b3f0b1f
to
d20d0fb
Compare
@luckysori will this also fix #1960 and https://github.com/get10101/meta/issues/351 ? |
Good point. I will try your repro and report back. |
64b1529
to
32202c9
Compare
@holzeis: really hard to say locally, because the sync is so fast. I didn't run into it though. |
7d85daf
to
26ef02c
Compare
dec3efd
to
b37cb30
Compare
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.
Looks good so far. Didn't manage to look at all changes yet, but will continue tomorrow.
b37cb30
to
ba0c3ae
Compare
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.
very nice 👍
A lot of feel good changes in there :)
|
||
// This is a standard base weight (without inputs or change outputs) for on-chain DLCs. We | ||
// assume that this value is still correct for DLC channels. | ||
let funding_tx_base_weight = 212; |
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.
🔧 That would probably be better placed as constant.
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.
True, although I didn't change this code with this PR.
In any case, I think it disappears with #2062, so we can wait for that one to merge.
crates/ln-dlc-node/src/dlc_wallet.rs
Outdated
}; | ||
|
||
coin_selector | ||
.run_bnb(metric, 100_000) |
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.
❓ What is the 100_000
defining?
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.
From the docs:
/// The method keeps trying until no better solution can be found, or we reach `max_rounds`.
I just took this value (a long time ago 😛) from the bdk_coin_select
docs/examples. I can add a constant for it in a separate commit.
.run_bnb(metric, 100_000) | ||
.map_err(|e| dlc_manager::error::Error::WalletError((format!("{e:#}")).into()))?; | ||
|
||
debug_assert!(coin_selector.is_target_met(target)); |
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.
🤔 Is it safe to continue here in prod? since we only debug_assert
, or should we also gracefully print an error and return?
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.
Old code. I just copied this from the bdk_coin_select
docs/examples. It's safe because a coin selection that didn't meet its target will just fail later.
But I agree that we should fail early. I can add this to #2062.
use std::sync::Arc; | ||
use tokio::sync::watch; | ||
|
||
pub type PendingInterceptedHtlcs = Arc<Mutex<HashMap<PublicKey, InterceptionDetails>>>; | ||
pub type EventSender = watch::Sender<Option<Event>>; |
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.
❓ is this still used?
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 would love to remove it. I remember keeping it on purpose because I wasn't sure. I can try to get rid of it now.
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.
AFAICT, after this PR it's only used by the app to generate an EventType::SpendableOutputs
.
The app uses that as one of the triggers to create a new backup. This is obviously completely irrelevant for most of the apps in production, so I wouldn't mind removing it.
What do you think?
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 will open a separate PR to remove it.
I'm interested in including a patch[^1] which will forward errors from the Esplora backend. This will make debugging easier. [^1]: bitcoindevkit/rust-esplora-client@269360f.
With the previous patch we can now use the `just maker` recipe knowing that it will work for Linux too.
Fixes #788.
Fixes #2016.
Fixes #2014.
Fixes #1990.
Fixes #1960 (reopen if not true after testing on public regtest).