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

LWK cache removal: remove dependency on hash tag #102

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Apr 22, 2024

This PR removes the dependency on the hardcoded hash tag

struct DirectoryIdTag = hash_str("LWK-FS-Directory-Id/1.0");

that is used to calculate the hash of the cache directory. Instead, empty_wallet_cache now clears all LWK caches.

The hash tag is pre-pended to a value X when X is hashed. This means the tag directly affects the resulting hash. In our case, X is the cache directory name.

There are two reasons for this PR:

  • first, if this tag changes in a future LWK version, the previous empty_wallet_cache version would not be able to find and remove the existing cache. The tagged hash function of the same directory name will result in a different hash.
  • second, we depend on the implementation of LWK calculates the cache directory hash. If that changes in a future LWK version, we would have to copy-paste the new implementation in our sha256t_hash_newtype!.

@ok300 ok300 requested review from hydra-yse and dangeross April 22, 2024 08:30
@dangeross
Copy link
Collaborator

I imagine changing the hash tag is in case that they want to invalidate the current cache, e.g. for a structure upgrade.

Is there a way to get the current hash tag used by LWK instead of hardcoding it?

I assume calling empty_wallet_cache clears all caches which then get rebuilt when used again. Any other consequences?

@ok300
Copy link
Contributor Author

ok300 commented Apr 22, 2024

Is there a way to get the current hash tag used by LWK instead of hardcoding it?

I didn't find a way, although they could expose it. However, even if they expose it, we'd have to re-implement the hash mechanism (right now it's tagged hashes, but no guarantee it won't change). And even if they export the method by which they generate the hash, it would not be backward compatible for hashes generated with a previous tag.

This is especially tricky if this is used in mobile apps, where space is limited. If we risk ending up in a case where certain cache paths cannot be cleared, that's not good.

I think what this PR does, e.g. bypass the hash logic and clear all caches instead, is the most robust solution.

I assume calling empty_wallet_cache clears all caches which then get rebuilt when used again. Any other consequences?

No other consequences.

Copy link
Collaborator

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@hydra-yse hydra-yse left a comment

Choose a reason for hiding this comment

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

LGTM

@ok300 ok300 merged commit 6f598b7 into main Apr 23, 2024
2 checks passed
@ok300 ok300 deleted the ok300-simplify-empty-cache-wallet branch April 23, 2024 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants