-
Notifications
You must be signed in to change notification settings - Fork 778
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
trie: use Uint8Array as value type in DB #3067
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
Really important here to check as early on as possible on the performance side. I guess this is really only with it if it brings at least a 3-5 % jump in overall block execution. |
I will report exact numbers, but I ran the entire blockchain test suite with and without this change (i.e. store values as Uint8Arrays) and as far as I remember the performance gain was above 10%. (I think 12%) |
Updated this via UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, synced the first 200.000 blocks with this, works like a charm! 🤩
Will merge.
} | ||
result = equalsBytes(this.txTrie.root(), this.header.transactionsTrie) | ||
result = equalsBytes(this.cache.txTrieRoot, this.header.transactionsTrie) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. 🙂👍
default: false, | ||
deprecated: | ||
'Usage of old DBs which uses string-values is temporary. Please sync new instances without this option.', | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No big deal, but I think we should rather use StateDB
as a terminoloy for something like this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good point, so it is not being confused with a blocks DB or anything 😄
In this PR the trie will use values as Uint8Array in the DB.
TODOs
Note: I tested that with the new
--useStringValueTrieDB
flag, client can still run the "old version" of the DB. If this flag is not provided and an old DB version is ran, it fails.This PR also:
new Trie
if we need it (we do not need a Trie when we want the transactions root of 0 transactions, for instance)transactionsTrieIsValid
: calculate the transactionsRoot once and caches this (does not cache trie, just the root)