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

Check if setting nurkel behind flag is possible #832

Open
nodech opened this issue Aug 1, 2023 · 1 comment
Open

Check if setting nurkel behind flag is possible #832

nodech opened this issue Aug 1, 2023 · 1 comment
Milestone

Comments

@nodech
Copy link
Contributor

nodech commented Aug 1, 2023

nurkel based hsd and urkel based hsd are storing the data exactly the same way, giving us ability to switch between those seamlessly. Only issue is the API is slightly different, because TX/Batch both need open/close API calls in the nurkel.

This will allow us to put the #804 behind the flag and let users test it.

We need to make sure that nurkel and urkel both have same API even if it's mock (for urkel open/close). nurkel currently has wrapper around urkel for the memory version: https://github.com/nodech/nurkel/blob/master/lib/memory.js
Instead of modifying urkel itself, it could also just be dependency but be a different variation of the tree for the general API.

There are some differences between urkel/nurkel when it comes to caching options:

Side note: maybe fork liburkel to under handshake-org to apply changes, currently nurkel depends on the patches.

@nodech nodech added this to the hsd 7.0.0 milestone Aug 1, 2023
@nodech
Copy link
Contributor Author

nodech commented May 15, 2024

Okay, my thoughts on nurkel upgrade (other than cleanup bug, that needs some work to fix).

  • Fixing API on urkel side sounds like a bad idea, first we can't ensure feature parity in the long run, if at some point urkel gets some new features, nurkel wont be able to support them so we can't assume APIs will stay compatible, so having wrapper on the nurkel side for the urkel is safer bet.
  • Also, some behaviour for async rootHash/tx.rootHash - is different. It may not be relevant to the code right now, but wrapper needs to deal with the differences. nurkel has locks around most operations, e.g. tx.insert will lock the tx, so you can't get the tx.root until it's done - It's not true for the urkel, where it will return old tx_root when insert is in flight. This would require rewrite of the urkel to have locks similar to liburkel.
  • txn/snapshot.open could have a proper place in urkel (even w/o locks), but that is breaking change (well, it can be done in minor way as well). .open method if the root cache is not recovered, may be a really slow operation. nurkel will do those checks on open, while urkel will do on the first read or write.

So, my conclusions currently are: Having urkel behind flag as a separate library is not viable, instead nurkel could expose 3 variations for creation (similar to BlockStore) - nurkel, urkel(memory), urkel(disk) and both urkels be behind a wrapper.

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

No branches or pull requests

1 participant