-
Notifications
You must be signed in to change notification settings - Fork 701
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
merkledb
-- dynamic root
#2177
merkledb
-- dynamic root
#2177
Conversation
@@ -51,11 +51,11 @@ var ( | |||
intermediateNodePrefix = []byte{2} | |||
|
|||
cleanShutdownKey = []byte(string(metadataPrefix) + "cleanShutdown") | |||
rootDBKey = []byte(string(metadataPrefix) + "root") |
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.
realized belatedly that you don't actually need this since the root should always be the smallest key present in your nodes db. You should be able to just grab the first key on the iterator, right?
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.
We could do that, but I feel like this is a bit cleaner than having to check both the value and intermediate node databases
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.
The current code still checks both dbs
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.
I still have reservations about this change. This almost doubles the code length of trieview.insert and trieview.remove for almost no functional change. I think the empty trie/proof changes do add something, but those could be separated out.
@@ -249,6 +254,13 @@ func (th *trieHistory) getChangesToGetToRoot(rootID ids.ID, start maybe.Maybe[[] | |||
for i := mostRecentChangeIndex; i > lastRootChangeIndex; i-- { | |||
changes, _ := th.history.Index(i) | |||
|
|||
if i == mostRecentChangeIndex { |
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.
should these just be outside the loop?
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.
They could be, but then we'd end up having to get it from th.history
again anyway, so I don't think that'd really buy us anything
5a316b7
to
473a264
Compare
5023fb8
to
7963a0b
Compare
Why this should be merged
The
root
field of atrieView
or the database should be the actualroot
.How this works
root
is now amaybe.Maybe[*node]
which isNothing
iff the trie is empty.ids.Empty
root ID iff the trie is empty.changeSummary
now explicitly tracks therootChange
.trieview
theroot
is now the actual root of the trie.getPathTo
may now return an empty path.How this was tested
Update existing and add new UT.