Handle re-orgs

[dr-orlovsky]

Closes #80

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 59 lines in your changes missing coverage. Please review.

Project coverage is 4.9%. Comparing base (2e58208) to head (affaa20).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/indexers/esplora.rs	0.0%	19 Missing ⚠️
src/indexers/electrum.rs	0.0%	17 Missing ⚠️
src/indexers/any.rs	0.0%	8 Missing ⚠️
src/wallet.rs	0.0%	6 Missing ⚠️
src/indexers/mod.rs	0.0%	4 Missing ⚠️
src/cli/args.rs	0.0%	3 Missing ⚠️
src/cli/command.rs	0.0%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #81     +/-   ##
=======================================
- Coverage     4.9%   4.9%   -0.0%     
=======================================
  Files          24     25      +1     
  Lines        2315   2323      +8     
=======================================
  Hits          113    113             
- Misses       2202   2210      +8     

Flag	Coverage Δ
rust	4.9% <0.0%> (-<0.1%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[zoedberg]

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?

[dr-orlovsky]

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.

[zoedberg]

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.

[dr-orlovsky]

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.

[dr-orlovsky]

BTW this may be also very useful in debugging.

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

[zoedberg]

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.

[dr-orlovsky]

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.

[zoedberg]

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.

[dr-orlovsky]

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.

[dr-orlovsky]

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.

[dr-orlovsky]

I have added prune argument to the call, as well as improved few other things (among them renamed publish into broadcast as you have asked some time ago).

[zoedberg]

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.

I was referring to pruning everything and re-syncing from scratch, that would have performance issues in highly-used wallets.

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.

They do not filter out transactions in the TxStatus::Unknown status (causing incorrect balances), you can check this yourself via rgb-tests.

I have added prune argument to the call

Updating with the prune bool set to true leads to a panic saying cache data inconsistency (from bp-wallet/src/wallet.rs:398:50). You can see it yourself by checking out https://github.com/zoedberg/rgb-tests/tree/fix/80 and running cargo test --test transfers revert_genesis -- --include-ignored