Skip to content

Never delete spent utxos from the database #515

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

@LLFourn LLFourn Mar 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 😅)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


### Sync API change

Expand Down
15 changes: 12 additions & 3 deletions src/blockchain/compact_filters/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,19 @@ impl CompactFiltersBlockchain {
if let Some(previous_output) = database.get_previous_output(&input.previous_output)? {
inputs_sum += previous_output.value;

if database.is_mine(&previous_output.script_pubkey)? {
// this output is ours, we have a path to derive it
if let Some((keychain, _)) =
database.get_path_from_script_pubkey(&previous_output.script_pubkey)?
{
outgoing += previous_output.value;

debug!("{} input #{} is mine, removing from utxo", tx.txid(), i);
updates.del_utxo(&input.previous_output)?;
debug!("{} input #{} is mine, setting utxo as spent", tx.txid(), i);
updates.set_utxo(&LocalUtxo {
outpoint: input.previous_output,
txout: previous_output.clone(),
keychain,
is_spent: true,
})?;
}
}
}
Expand All @@ -185,6 +193,7 @@ impl CompactFiltersBlockchain {
outpoint: OutPoint::new(tx.txid(), i as u32),
txout: output.clone(),
keychain,
is_spent: false,
})?;
incoming += output.value;

Expand Down
17 changes: 10 additions & 7 deletions src/blockchain/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ impl WalletSync for RpcBlockchain {
let mut list_txs_ids = HashSet::new();

for tx_result in list_txs.iter().filter(|t| {
// list_txs returns all conflicting tx we want to
// list_txs returns all conflicting txs, we want to
// filter out replaced tx => unconfirmed and not in the mempool
t.info.confirmations > 0 || self.client.get_mempool_entry(&t.info.txid).is_ok()
}) {
Expand Down Expand Up @@ -332,20 +332,23 @@ impl WalletSync for RpcBlockchain {
value: u.amount.as_sat(),
script_pubkey: u.script_pub_key,
},
is_spent: false,
})),
},
)
.collect::<Result<HashSet<_>, Error>>()?;

let spent: HashSet<_> = known_utxos.difference(&current_utxos).collect();
for s in spent {
debug!("removing utxo: {:?}", s);
db.del_utxo(&s.outpoint)?;
for utxo in spent {
debug!("setting as spent utxo: {:?}", utxo);
let mut spent_utxo = utxo.clone();
spent_utxo.is_spent = true;
db.set_utxo(&spent_utxo)?;
}
let received: HashSet<_> = current_utxos.difference(&known_utxos).collect();
for s in received {
debug!("adding utxo: {:?}", s);
db.set_utxo(s)?;
for utxo in received {
debug!("adding utxo: {:?}", utxo);
db.set_utxo(utxo)?;
}

for (keykind, index) in indexes {
Expand Down
44 changes: 26 additions & 18 deletions src/blockchain/script_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,23 @@ impl<'a, D: BatchDatabase> State<'a, D> {
batch.del_tx(txid, true)?;
}

// Set every tx we observed
let mut spent_utxos = HashSet::new();

// track all the spent utxos
for finished_tx in &finished_txs {
let tx = finished_tx
.transaction
.as_ref()
.expect("transaction will always be present here");
for input in &tx.input {
spent_utxos.insert(&input.previous_output);
}
}

// set every utxo we observed, unless it's already spent
// we don't do this in the loop above as we want to know all the spent outputs before
// adding the non-spent to the batch in case there are new tranasactions
// that spend form each other.
for finished_tx in &finished_txs {
let tx = finished_tx
.transaction
Expand All @@ -343,30 +359,22 @@ impl<'a, D: BatchDatabase> State<'a, D> {
self.db.get_path_from_script_pubkey(&output.script_pubkey)?
{
// add utxos we own from the new transactions we've seen.
let outpoint = OutPoint {
txid: finished_tx.txid,
vout: i as u32,
};

batch.set_utxo(&LocalUtxo {
outpoint: OutPoint {
txid: finished_tx.txid,
vout: i as u32,
},
outpoint,
txout: output.clone(),
keychain,
// Is this UTXO in the spent_utxos set?
is_spent: spent_utxos.get(&outpoint).is_some(),
})?;
}
}
batch.set_tx(finished_tx)?;
}

// we don't do this in the loop above since we may want to delete some of the utxos we
// just added in case there are new tranasactions that spend form each other.
for finished_tx in &finished_txs {
let tx = finished_tx
.transaction
.as_ref()
.expect("transaction will always be present here");
for input in &tx.input {
// Delete any spent utxos
batch.del_utxo(&input.previous_output)?;
}
batch.set_tx(finished_tx)?;
}

for (keychain, last_active_index) in self.last_active_index {
Expand Down
14 changes: 13 additions & 1 deletion src/database/keyvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ macro_rules! impl_batch_operations {
let value = json!({
"t": utxo.txout,
"i": utxo.keychain,
"s": utxo.is_spent,
});
self.insert(key, serde_json::to_vec(&value)?)$($after_insert)*;

Expand Down Expand Up @@ -125,8 +126,9 @@ macro_rules! impl_batch_operations {
let mut val: serde_json::Value = serde_json::from_slice(&b)?;
let txout = serde_json::from_value(val["t"].take())?;
let keychain = serde_json::from_value(val["i"].take())?;
let is_spent = val.get_mut("s").and_then(|s| s.take().as_bool()).unwrap_or(false);

Ok(Some(LocalUtxo { outpoint: outpoint.clone(), txout, keychain }))
Ok(Some(LocalUtxo { outpoint: outpoint.clone(), txout, keychain, is_spent, }))
}
}
}
Expand Down Expand Up @@ -246,11 +248,16 @@ impl Database for Tree {
let mut val: serde_json::Value = serde_json::from_slice(&v)?;
let txout = serde_json::from_value(val["t"].take())?;
let keychain = serde_json::from_value(val["i"].take())?;
let is_spent = val
.get_mut("s")
.and_then(|s| s.take().as_bool())
.unwrap_or(false);

Ok(LocalUtxo {
outpoint,
txout,
keychain,
is_spent,
})
})
.collect()
Expand Down Expand Up @@ -314,11 +321,16 @@ impl Database for Tree {
let mut val: serde_json::Value = serde_json::from_slice(&b)?;
let txout = serde_json::from_value(val["t"].take())?;
let keychain = serde_json::from_value(val["i"].take())?;
let is_spent = val
.get_mut("s")
.and_then(|s| s.take().as_bool())
.unwrap_or(false);

Ok(LocalUtxo {
outpoint: *outpoint,
txout,
keychain,
is_spent,
})
})
.transpose()
Expand Down
16 changes: 11 additions & 5 deletions src/database/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,10 @@ impl BatchOperations for MemoryDatabase {

fn set_utxo(&mut self, utxo: &LocalUtxo) -> Result<(), Error> {
let key = MapKey::Utxo(Some(&utxo.outpoint)).as_map_key();
self.map
.insert(key, Box::new((utxo.txout.clone(), utxo.keychain)));
self.map.insert(
key,
Box::new((utxo.txout.clone(), utxo.keychain, utxo.is_spent)),
);

Ok(())
}
Expand Down Expand Up @@ -228,11 +230,12 @@ impl BatchOperations for MemoryDatabase {
match res {
None => Ok(None),
Some(b) => {
let (txout, keychain) = b.downcast_ref().cloned().unwrap();
let (txout, keychain, is_spent) = b.downcast_ref().cloned().unwrap();
Ok(Some(LocalUtxo {
outpoint: *outpoint,
txout,
keychain,
is_spent,
}))
}
}
Expand Down Expand Up @@ -326,11 +329,12 @@ impl Database for MemoryDatabase {
.range::<Vec<u8>, _>((Included(&key), Excluded(&after(&key))))
.map(|(k, v)| {
let outpoint = deserialize(&k[1..]).unwrap();
let (txout, keychain) = v.downcast_ref().cloned().unwrap();
let (txout, keychain, is_spent) = v.downcast_ref().cloned().unwrap();
Ok(LocalUtxo {
outpoint,
txout,
keychain,
is_spent,
})
})
.collect()
Expand Down Expand Up @@ -389,11 +393,12 @@ impl Database for MemoryDatabase {
fn get_utxo(&self, outpoint: &OutPoint) -> Result<Option<LocalUtxo>, Error> {
let key = MapKey::Utxo(Some(outpoint)).as_map_key();
Ok(self.map.get(&key).map(|b| {
let (txout, keychain) = b.downcast_ref().cloned().unwrap();
let (txout, keychain, is_spent) = b.downcast_ref().cloned().unwrap();
LocalUtxo {
outpoint: *outpoint,
txout,
keychain,
is_spent,
}
}))
}
Expand Down Expand Up @@ -526,6 +531,7 @@ macro_rules! populate_test_db {
vout: vout as u32,
},
keychain: $crate::KeychainKind::External,
is_spent: false,
})
.unwrap();
}
Expand Down
1 change: 1 addition & 0 deletions src/database/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ pub mod test {
txout,
outpoint,
keychain: KeychainKind::External,
is_spent: true,
};

tree.set_utxo(&utxo).unwrap();
Expand Down
Loading