Never delete spent utxos from the database #515

danielabrozzoni · 2021-12-31T17:37:10Z

Description

A is_spent field is added to LocalUtxo; when a txo is spent we set
this field to true instead of deleting the entire utxo from the
database.
This allows us to create txs double-spending txs already in blockchain.
Listunspent won't return spent in mempool utxos, effectively excluding them from the coin selection and balance calculation
Fixes #414

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've updated CHANGELOG.md
I'm linking the issue being fixed by this PR

rajarshimaitra

Initial Concept ACK. The approach looks good to me.

Would like to see a small tests for list_unspent also. Ensuring its working as expected will ensure all other application like coinselection, get_balance are working fine too.

This might also be a breaking change for DB APIs as LocalUtxo struct is modified. So would be better to include in the change log too.

src/types.rs

danielabrozzoni · 2022-02-14T23:08:40Z

Thanks for the review Raj, and sorry it took so long for me to update! I added the rpc code as well, so this PR is now ready :D

I agree with you regarding the test on listunspent, but I'm not sure where I should put it (I still haven't familiarized myself with the test structure...). For now, I've added a check in test_double_spend:

for utxo in wallet.list_unspent().unwrap() {
    // Making sure the TXO we just spent is not returned by list_unspent
    assert!(utxo.outpoint != initial_tx.input[0].previous_output, "wallet displays spent txo in unspents");
}

but let me know if you prefer it to be tested somewhere else as well :)

Edit: Uhm, I did forget to update compact filters code 😭 Nevermind, this is still not ready, sorry...
Edit2: Ready 🎉 I'm not really sure if/where I should document this though...

rajarshimaitra

Hey @danielabrozzoni , sorry it also took me a long time to get back to it..

made a thorough pass.. tACK f3317eb

The code changes looks good to me.. I have also witnessed problem with esplora before.. Not sure this one is us or something upstream.. For now I think its good to go without it..

Below are just few non-blocking nits I have found.. If you happen to touch this again, feel free toi add them..

src/blockchain/rpc.rs

rajarshimaitra

ReACK d39f4a4

afilini · 2022-03-08T15:16:42Z

I know I previously said that we should delete UTXOs once confirmed (to avoid bloating the database), but I'm changing my mind: having them would help me with my work on #549, specifically in the code that tries to figure out if we've already performed the "handshake" with a payment code: I could list the transactions received by the notification address corresponding to that payment code, and if I find a transaction that spends our UTXO (here's where I need to have them even if spent) I know we did.

danielabrozzoni · 2022-03-08T15:52:16Z

Avoid deleting utxos would made the code a bit cleaner 😏 I can modify this PR to never delete utxos, and then in future we can think of how to implement some kind of garbage collector.
What should the LocalUtxo struct look like? I could:

Add a boolean is_spent, which is true if the UTXO is spent (both in mempool and already confirmed)
Add a spender_txid field, which would be None if the UTXO is unspent, a Txid otherwise.

Do you think we would ever need a spender_txid field? If we don't, I'd go with is_spent (it's easier and saves space 😝). If we might, let's just go with the spender_txid.

afilini · 2022-03-08T18:08:29Z

I think we've been having a ton of troubles lately because we try to over-optimize, so in this case I would go for the full spender_txid. We don't need it right now, but I'm sure somebody will in the future.

danielabrozzoni · 2022-03-09T14:37:26Z

Actually when trying to implement it I noticed that the spender_txid solution is not trivial, and this PR has been opened for a while now. Better to go with is_spent now and improve later :)

afilini

ACK 227eede

afilini · 2022-03-10T10:18:08Z

There are conflicts with master, rebase and then I'll re-ack

A `is_spent` field is added to LocalUtxo; when a txo is spent we set this field to true instead of deleting the entire utxo from the database. This allows us to create txs double-spending txs already in blockchain. Listunspent won't return spent utxos, effectively excluding them from the coin selection and balance calculation

afilini · 2022-03-14T15:06:09Z

Re-ACK f2f0efc

LLFourn · 2022-03-15T00:55:08Z

CHANGELOG.md

@@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `verify` flag removed from `TransactionDetails`.
 - Add `get_internal_address` to allow you to get internal addresses just as you get external addresses.
 - added `ensure_addresses_cached` to `Wallet` to let offline wallets load and cache addresses in their database
+- Add `is_spent` field to `LocalUtxo`; when we notice that a utxo has been spent we set `is_spent` field to true instead of deleting it from the db.


post-merge comment: This should probably be called LocalTxo now since they're not necessarily ~~spent~~ unspent.

In my opinion it's fine to just call it "utxo", I think it's pretty common to refer to outputs in general as utxos. I think what we gain in terms of "naming consistency", we lose with confusion because "txo" doesn't sound as familiar as "utxo" to people (or at least it doesn't to myself 😅)

Calling something "unspent" when it isn't unspent seems like it crossing a line...

I think they're usually called "txout"s rather than txos thought.

Yeah I like LocalTxout or LocalTxOut better. "txo", while correct, is a weird term imho, it's not used that much.

danielabrozzoni changed the title ~~WIP~~ WIP: Allow using spent unconfirmed UTXOs in add_utxo Dec 31, 2021

danielabrozzoni force-pushed the 2021_11_10_add_utxo_non_rbf branch 2 times, most recently from e995a87 to 11cca43 Compare December 31, 2021 17:43

rajarshimaitra reviewed Jan 13, 2022

View reviewed changes

src/types.rs Outdated Show resolved Hide resolved

notmandatory assigned rajarshimaitra Feb 1, 2022

danielabrozzoni force-pushed the 2021_11_10_add_utxo_non_rbf branch from 11cca43 to 596c264 Compare February 14, 2022 23:04

danielabrozzoni force-pushed the 2021_11_10_add_utxo_non_rbf branch 2 times, most recently from 754889c to 9a6e04a Compare February 14, 2022 23:32

notmandatory assigned danielabrozzoni Feb 15, 2022

danielabrozzoni force-pushed the 2021_11_10_add_utxo_non_rbf branch from 9a6e04a to f3317eb Compare February 19, 2022 21:52

danielabrozzoni marked this pull request as ready for review February 19, 2022 22:04

danielabrozzoni changed the title ~~WIP: Allow using spent unconfirmed UTXOs in add_utxo~~ Allow using spent unconfirmed UTXOs in add_utxo Feb 19, 2022

danielabrozzoni requested a review from rajarshimaitra February 19, 2022 22:05

rajarshimaitra reviewed Mar 3, 2022

View reviewed changes

src/blockchain/rpc.rs Outdated Show resolved Hide resolved

src/blockchain/rpc.rs Show resolved Hide resolved

src/blockchain/rpc.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the 2021_11_10_add_utxo_non_rbf branch 2 times, most recently from 1a49912 to d39f4a4 Compare March 3, 2022 16:12

rajarshimaitra approved these changes Mar 3, 2022

View reviewed changes

notmandatory added this to the Release 0.18.0 Feature Freeze milestone Mar 3, 2022

afilini mentioned this pull request Mar 8, 2022

Database is getting corrupted on Android bitcoindevkit/bdk-ffi#114

Closed

danielabrozzoni force-pushed the 2021_11_10_add_utxo_non_rbf branch 3 times, most recently from 7c4a35a to 227eede Compare March 9, 2022 15:32

danielabrozzoni changed the title ~~Allow using spent unconfirmed UTXOs in add_utxo~~ Never delete spent utxos from the database Mar 9, 2022

afilini approved these changes Mar 10, 2022

View reviewed changes

danielabrozzoni force-pushed the 2021_11_10_add_utxo_non_rbf branch from 227eede to f2f0efc Compare March 10, 2022 10:59

afilini merged commit 9a6db15 into bitcoindevkit:master Mar 14, 2022

LLFourn reviewed Mar 15, 2022

View reviewed changes

danielabrozzoni mentioned this pull request Mar 18, 2022

Remove old LocalUtxos #573

Open

danielabrozzoni mentioned this pull request Apr 12, 2022

Add waste metric for coin selection #558

Closed

6 tasks

weiwencrypto mentioned this pull request Jun 14, 2022

Never delete spent utxos from the database monacohq/bdk#1

Merged

danielabrozzoni mentioned this pull request Jul 12, 2022

Better names for types #665

Closed

danielabrozzoni deleted the 2021_11_10_add_utxo_non_rbf branch August 16, 2022 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Never delete spent utxos from the database #515

Never delete spent utxos from the database #515

danielabrozzoni commented Dec 31, 2021 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

danielabrozzoni commented Feb 14, 2022 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

rajarshimaitra left a comment

afilini commented Mar 8, 2022

danielabrozzoni commented Mar 8, 2022 •

edited

Loading

afilini commented Mar 8, 2022

danielabrozzoni commented Mar 9, 2022

afilini left a comment

afilini commented Mar 10, 2022

afilini commented Mar 14, 2022

LLFourn Mar 15, 2022 •

edited

Loading

afilini Mar 15, 2022

LLFourn Mar 15, 2022

afilini Mar 16, 2022

Never delete spent utxos from the database #515

Never delete spent utxos from the database #515

Conversation

danielabrozzoni commented Dec 31, 2021 • edited Loading

Description

Checklists

All Submissions:

New Features:

rajarshimaitra left a comment • edited Loading

Choose a reason for hiding this comment

danielabrozzoni commented Feb 14, 2022 • edited Loading

rajarshimaitra left a comment • edited Loading

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

afilini commented Mar 8, 2022

danielabrozzoni commented Mar 8, 2022 • edited Loading

afilini commented Mar 8, 2022

danielabrozzoni commented Mar 9, 2022

afilini left a comment

Choose a reason for hiding this comment

afilini commented Mar 10, 2022

afilini commented Mar 14, 2022

LLFourn Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

afilini Mar 15, 2022

Choose a reason for hiding this comment

LLFourn Mar 15, 2022

Choose a reason for hiding this comment

afilini Mar 16, 2022

Choose a reason for hiding this comment

danielabrozzoni commented Dec 31, 2021 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

danielabrozzoni commented Feb 14, 2022 •

edited

Loading

rajarshimaitra left a comment •

edited

Loading

danielabrozzoni commented Mar 8, 2022 •

edited

Loading

LLFourn Mar 15, 2022 •

edited

Loading