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

Handle re-orgs #81

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Handle re-orgs #81

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
937e728
electrum: handle re-org transactions
dr-orlovsky Jan 14, 2025
248eed3
esplora: handle re-org transactions
dr-orlovsky Jan 14, 2025
2bd637d
chore: fix clippy lints
dr-orlovsky Jan 14, 2025
aeea5e9
wallet: add cache prune methods
dr-orlovsky Jan 15, 2025
da8c0d7
indexers: add prune argument to update, use default create impl
dr-orlovsky Jan 15, 2025
320eedd
indexers: rename publish into broadcast
dr-orlovsky Jan 15, 2025
affaa20
indexers: move pruning to the end of calls
dr-orlovsky Jan 15, 2025
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
16 changes: 16 additions & 0 deletions src/indexers/electrum.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
// limitations under the License.

use std::collections::BTreeMap;
use std::mem;
use std::num::NonZeroU32;
use std::str::FromStr;

Expand Down Expand Up @@ -78,6 +79,13 @@
) -> MayError<usize, Vec<Self::Error>> {
let mut errors = Vec::<ElectrumError>::new();

// First, we scan all addresses.
// Addresses may be re-used, so known transactions doesn't help here.
// We collect these transactions, which contain the most recent information, into a new
// cache. We remove old transaction, since its data are now updated (for instance, if a
// transaction was re-orged, it may have a different height).

let mut old_cache = mem::take(&mut cache.tx);

Check warning on line 88 in src/indexers/electrum.rs

View check run for this annotation

Codecov / codecov/patch

src/indexers/electrum.rs#L82-L88 

Added lines #L82 - L88 were not covered by tests
let mut address_index = BTreeMap::new();
for keychain in descriptor.keychains() {
let mut empty_count = 0usize;
Expand All @@ -104,6 +112,7 @@

empty_count = 0;

// TODO: Separate as `WalletTx::from_electrum_history` method.

Check warning on line 115 in src/indexers/electrum.rs

View check run for this annotation

Codecov / codecov/patch

src/indexers/electrum.rs#L115 

Added line #L115 was not covered by tests
let mut process_history_entry =
|hr: GetHistoryRes| -> Result<WalletTx, ElectrumError> {
let txid = hr.tx_hash;
Expand Down Expand Up @@ -202,6 +211,7 @@
for hr in hres {
match process_history_entry(hr) {
Ok(tx) => {
old_cache.remove(&tx.txid);

Check warning on line 214 in src/indexers/electrum.rs

View check run for this annotation

Codecov / codecov/patch

src/indexers/electrum.rs#L214 

Added line #L214 was not covered by tests
cache.tx.insert(tx.txid, tx);
}
Err(e) => errors.push(e),
Expand All @@ -213,6 +223,12 @@
}
}

// The remaining transactions are unmined ones.
for (txid, mut tx) in old_cache {
tx.status = TxStatus::Unknown;
cache.tx.insert(txid, tx);
}

Check warning on line 230 in src/indexers/electrum.rs

View check run for this annotation

Codecov / codecov/patch

src/indexers/electrum.rs#L227-L230 

Added lines #L227 - L230 were not covered by tests

Copy link
Contributor

Choose a reason for hiding this comment

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

If a TX disappeared from both the chain and the mempool I don't see why we should keep it in the cache. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two reasons:

First, someone may re-publish it later. And second, in data it may have associated info, like tx or output description, payment purpose etc. If we remove it here, these data will be orphaned.

Copy link
Contributor

Choose a reason for hiding this comment

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

First, someone may re-publish it later.

In that case a call to sync will find it, therefore keeping it or dropping it in/from the cache is unrelated to the fact that someone may re-publish it later.

And second, in data it may have associated info, like tx or output description, payment purpose etc. If we remove it here, these data will be orphaned.

This is an implementation matter, handling a bitcoin reorg where a TX completely disappears should cleanup its associated data. At that point there would be no orphaned data.

Copy link
Member Author

Choose a reason for hiding this comment

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

They can always be removed later through a specific cleaning procedure.

I assume we both agree that this is an opinion-based API, and my opinion is to keep them in indexers and remove elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW this may be also very useful in debugging.

Anyway I have added a dedicated cache prune API in a commit on top: aeea5e9

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think it would be useful in debugging? To me it looks like pollution. The cache prune API is not a solution since it deletes all data, both valid and invalid.

Copy link
Member Author

@dr-orlovsky dr-orlovsky Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Because I need it in a debugging.

Also, this is a very rare case and it won't create any significant amount of pollution in a real-world wallets

Finally, I need my wallet to keep past versions of RBFed transactions for me, and not to remove them until I explicitly ask for ("pruning"). Since the main user of BP wallet library is myself, I will have it this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, I need my wallet to keep past versions of RBFed transactions for me, and not to remove them until I explicitly ask for ("pruning"). Since the main user of BP wallet library is myself, I will have it this way.

Your solution is still incomplete:

  1. the is_unspent method is not ignoring TXs with a TxStatus::Unknown status (causing RGB wallets to think the inexistent UTXO is spendable)
  2. the WalletAddr balance in WalletCache::addr is not recalculated to exclude disappeared TXs

Also, in the future there might be more bp-wallet users with necessities different from yours. I propose to change the sync API in a way that it accepts a boolean to specify whether disappeared TXs should be pruned or not during the update. This would be more performant because in highly-used wallets pruning and re-syncing everything from scratch could be very time-expensive.

Copy link
Member Author

@dr-orlovsky dr-orlovsky Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also, in the future there might be more bp-wallet users with necessities different from yours.

My argument is that as a creator and maintainer of this library I have a right to decide on opinionated questions (i.e. when there is no clear rational argument and they are a matter of people/users liking or disliking some approaches).

I mean if I need some APIs to be this and this, and somebody needs something else - they can fork or use another library. Since if I merge somebody else opinion and not mine (needing different stuff), I will just fork this library and move somewhere else, which is isomorphic to my original claim and statement ("I will do as I like on the opionated topics in this lib").

At the end of the days this is non-consensus code and library, so it is destined to be opinionated and aligned to specific needs.

I propose to change the sync API in a way that it accepts a boolean to specify whether disappeared TXs should be pruned or not during the update.

I am fine with adding argument to the API call to do update and pruning simultaneously. I will do that alongside fixing two remaining issues you mentioned with balance and is_unspent.

This would be more performant because in highly-used wallets pruning and re-syncing everything from scratch could be very time-expensive.

Just as a note, keeping old transactions adds zero load to the procedure. First, they take no network calls. Second, they are very rare - they are just outdated version of RBFed txes, meaning <10 per normal wallet. Maybe few dozens. So no overload.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your solution is still incomplete:

  • the is_unspent method is not ignoring TXs with a TxStatus::Unknown status (causing RGB wallets to think the inexistent UTXO is spendable)
  • the WalletAddr balance in WalletCache::addr is not recalculated to exclude disappeared TXs

No, from what I see, it is not.

Both is_unspent and WalletAddr balance doesn't touch cache.tx, which contains old transactions, and recomputed from scratch using independent indexer queries. So they DO update.

// TODO: Update headers & tip

for (script, (wallet_addr, txids)) in &mut address_index {
Expand Down
19 changes: 18 additions & 1 deletion src/indexers/esplora.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
// limitations under the License.

use std::collections::BTreeMap;
use std::mem;
use std::num::NonZeroU32;
use std::ops::{Deref, DerefMut};

Expand Down Expand Up @@ -208,6 +209,13 @@
) -> MayError<usize, Vec<Self::Error>> {
let mut errors = vec![];

// First, we scan all addresses.
// Addresses may be re-used, so known transactions doesn't help here.
// We collect these transactions, which contain the most recent information, into a new
// cache. We remove old transaction, since its data are now updated (for instance, if a
// transaction was re-orged, it may have a different height).

let mut old_cache = mem::take(&mut cache.tx);

Check warning on line 218 in src/indexers/esplora.rs

View check run for this annotation

Codecov / codecov/patch

src/indexers/esplora.rs#L212-L218 

Added lines #L212 - L218 were not covered by tests
let mut address_index = BTreeMap::new();
for keychain in descriptor.keychains() {
let mut empty_count = 0usize;
Expand All @@ -232,7 +240,10 @@
}
Ok(txes) => {
empty_count = 0;
txids = txes.iter().map(|tx| tx.txid).collect();
txids.extend(txes.iter().map(|tx| tx.txid));
for txid in &txids {
old_cache.remove(txid);
}

Check warning on line 246 in src/indexers/esplora.rs

View check run for this annotation

Codecov / codecov/patch

src/indexers/esplora.rs#L243-L246 

Added lines #L243 - L246 were not covered by tests
cache
.tx
.extend(txes.into_iter().map(WalletTx::from).map(|tx| (tx.txid, tx)));
Expand All @@ -244,6 +255,12 @@
}
}

// The remaining transactions are unmined ones.
for (txid, mut tx) in old_cache {
tx.status = TxStatus::Unknown;
cache.tx.insert(txid, tx);
}

Check warning on line 262 in src/indexers/esplora.rs

View check run for this annotation

Codecov / codecov/patch

src/indexers/esplora.rs#L259-L262 

Added lines #L259 - L262 were not covered by tests

// TODO: Update headers & tip

for (script, (wallet_addr, txids)) in &mut address_index {
Expand Down
Loading