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.

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

fusion: Filter labels based on tx heuristics #2346

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jonas-lundqvist
Copy link

Instead of parsing the label text use the heuristics for history
filtering. This way fused transactions could be filtered from recovered
wallets without the need for LabelSyncing.

Instead of parsing the label text use the heuristics for history
filtering. This way fused transactions could be filtered from recovered
wallets without the need for LabelSyncing.
@cculianu
Copy link
Collaborator

Hmm. This will be called.. a lot. And it seems somewhat heavy, no? I wonder if the result should be at least cached.... somehow.

@cculianu cculianu added Cash Fusion Cash Fusion enhancements and features UX and usability labels Sep 29, 2021
@jonas-lundqvist
Copy link
Author

Hmm. This will be called.. a lot. And it seems somewhat heavy, no? I wonder if the result should be at least cached.... somehow.

I added a simple dict for keeping track of all txids -> fusion statuses.

Comment on lines +626 to +629
if not txid in self._is_fuz_tx:
self._is_fuz_tx[txid] = FusionPlugin.check_is_fuz_tx(history_list.wallet, txid)

return self._is_fuz_tx[txid]
Copy link
Collaborator

@cculianu cculianu Sep 29, 2021

Choose a reason for hiding this comment

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

Well that check_is_fuz_tx function can return None which signifies that it couldn't (yet) find the tx for some reason in the wallet. It's a theoretical corner case due to the API contract of the function, so it should be handled since it is a branch that exists. As such, we shouldn't blindly store whatever it returns, but only store if not None.

The below also avoids double-lookup in the dict in the happy case where there is a cache hit.

Suggested change
if not txid in self._is_fuz_tx:
self._is_fuz_tx[txid] = FusionPlugin.check_is_fuz_tx(history_list.wallet, txid)
return self._is_fuz_tx[txid]
retval = self._is_fuz_tx.get(txid)
if retval is None:
retval = FusionPlugin.check_is_fuz_tx(history_list.wallet, txid)
if retval is not None:
self._is_fuz_tx[txid] = retval
return retval

@cculianu
Copy link
Collaborator

Hmm. I am wondering if this dict shouldn't actually live in the wallet as an ad-hoc attribute we add. Reason being it's really wallet-specific data.. and if you close the wallet it's nice to have the cache for that wallet be deleted...

Also didn't we add another cache for this elsewhere? I need to check the code again...

@cculianu
Copy link
Collaborator

cculianu commented Sep 29, 2021

Yeah man we already have a cache for this. We added that wallet._cashfusion_is_fuz_txid_cache thing already, and it contains ints (and maybe None)... So, in light of that, what we are doing here I think this duplicates the same functionality needlessly... no?

Hmm. Can you look into whether or not you can use that dict and do away with the cache that was added? More specifically: can you find out if the dict always has "answers" even if "spend only fused coins" is turned off? In the happy case where that cache always has an "answer", it may be possible to:

  • not have the refactor and keep the check_is_fuz_tx function private/nested
  • not even need to call any function, just check the cache that already exists.

@cculianu
Copy link
Collaborator

cculianu commented Sep 29, 2021

Update: yeah that wallet._cashfusion_is_fuz_txid_cache is not always "live". It only really has useful data if "spend only fused coins" facility is enabled. Darn.

Grr. This is annoying. We almost have all the data there... but one can't easily call is_fuzed_coin on historical txs since they have no more "coins".

Grr...

I wish there was a way we could leverage the same code and the same cache for this... and not create a redundant cache.

@jonas-lundqvist jonas-lundqvist marked this pull request as draft September 21, 2022 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cash Fusion Cash Fusion enhancements and features UX and usability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants