Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Lock used utxos #913

Merged
merged 11 commits into from
Jul 7, 2023
Merged

Lock used utxos #913

merged 11 commits into from
Jul 7, 2023

Conversation

bonomat
Copy link
Contributor

@bonomat bonomat commented Jul 6, 2023

fixes #873

@bonomat bonomat requested review from holzeis and luckysori July 6, 2023 08:07
luckysori
luckysori approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@luckysori luckysori left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I've left some comments to clean this up a bit, but I think it's good.

crates/ln-dlc-node/src/fee_rate_estimator.rs Outdated
pub trait EstimateFeeRate {
fn estimate(&self, target: ConfirmationTarget) -> FeeRate;
async fn update(&self) -> Result<()>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 I think this should be defined next to the component that requires this contract i.e. the wallet.

I would keep the get method and just call it when implementing the foreign trait method estimate.

crates/ln-dlc-node/src/fee_rate_estimator.rs Outdated
#[async_trait]
pub trait EstimateFeeRate {
fn estimate(&self, target: ConfirmationTarget) -> FeeRate;
async fn update(&self) -> Result<()>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I don't think update needs to be part of this trait. The wallet only cares about getting the estimate, not how it is internally updated.

crates/ln-dlc-node/src/fee_rate_estimator.rs Outdated
}

#[autometrics]
pub(crate) async fn update(&self) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 This shouldn't be async anymore. I'll fix that separately.

crates/ln-dlc-node/src/ldk_node_wallet.rs
@@ -25,32 +25,32 @@ use std::sync::Arc;
use std::time::Instant;
use tokio::sync::RwLock;

pub struct Wallet<D>
pub struct Wallet<D, B, F>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃 Since we only need the generics for testing, I would isolate the rest of the modules from them by providing a pub type which is Wallet<sled::Tree, EsploraBlockchain, FeeRateEstimator>.

I can do this afterwards though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I'l leave it for now then.

crates/ln-dlc-node/src/ldk_node_wallet.rs Outdated
Comment on lines 117 to 120
let mut locked_out_points = self.locked_outpoints.lock();
for out_point in locked_out_points.iter() {
tx_builder.add_unspendable(*out_point);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃 Let's use outpoint instead of out_point consistently.

crates/ln-dlc-node/src/ldk_node_wallet.rs Outdated
.map(|input| input.previous_output)
.collect::<Vec<_>>();

locked_out_points.extend(prev_outpoints);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 It might be a good idea to take this lock twice rather than holding it for an extended period of time. What do you think, @klochowicz?

crates/ln-dlc-node/src/ldk_node_wallet.rs Outdated
}

#[tokio::test]
async fn wallet_with_two_utxo_should_be_able_to_fund_twice_but_not_three_times() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙃 Let's move the test to the top of the module, please.

crates/ln-dlc-node/src/ldk_node_wallet.rs Outdated
Comment on lines 395 to 397
async fn update(&self) -> Result<()> {
unimplemented!()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this confirms that we don't need it in the trait.

@bonomat bonomat force-pushed the fix/dont-reuse-utxo branch 2 times, most recently from 581ec3f to ae8d138 Compare July 6, 2023 11:51
holzeis
holzeis approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

crates/ln-dlc-node/src/ldk_node_wallet.rs
@@ -90,6 +93,8 @@ where
"Finished on-chain sync",
);

self.locked_outpoints.lock().clear();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 is it safe to clear the utxos here? or do we even have to wait for the locked utxo to get a confirmation, before we can safely remove it from the locked_outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is because during sync with on-chain we should catch-up with all transactions which are in the mempool

@bonomat
chore: extract traits for testability
e46ed6d 
by extracting traits we can easier test the wallet and pass in dummies.
@bonomat
fix: lock used utxos
b12c539 
to prevent accidentally reusing utxos for different funding transactions we lock a used utxo. On next wallet sync we clear the locked utxos because the wallet should pick up said utxos from the mempool
@bonomat
chore: hardcode fee rate to 1.0 s/vbyte
ac62f96 
This fixes a weird situation where bitcoind decides to go for an absurdly high fee of 10,000 sats/vbyte which confuses the fee estimator.
@bonomat
chore: add timeout to balance check
c3cac68 
We don't want this to run forever. Timing this out if no balance can be found after some time helps us to get better errors in the tests.
@bonomat
chore: add more utxos to coordinator
29e0d0e 
We are blocking utxos after we used them. Because of this the coordinator needs to start with more utxos.
@bonomat
feat: add fn for sending to address
3097f89
@bonomat
chore: ensure that coordinator has enough money
7af079f 
After repeated running tests, the coordinator might run out of utxos/money. Hence, we ensure that he has an available utxo before the test runs
@bonomat
fix: ensure that lnd faucet works
46f3be2
@bonomat
Copy link
Contributor Author

bonomat commented Jul 7, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 7, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit aa9a372 into main Jul 7, 2023
7 checks passed
@bors bors bot deleted the fix/dont-reuse-utxo branch July 7, 2023 09:31
@holzeis holzeis mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holzeis holzeis holzeis approved these changes

@luckysori luckysori luckysori approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Channel without funding transaction on chain
3 participants
@bonomat @holzeis @luckysori