Add firewood wrapper #4661
Add firewood wrapper #4661
Conversation
Signed-off-by: Joshua Kim <[email protected]>
joshua-kim
changed the base branch from
master
to
x-chain-merkle-db-shared-memory
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
| return ids.ID{}, err | ||
| } | ||
|
|
||
| return ids.ID(root[:]), nil |
There was a problem hiding this comment.
| return ids.ID(root[:]), nil | |
| return ids.ID(root), nil |
Why isn't this just a typecast?
| appPrefix = []byte("app") | ||
| ) | ||
|
|
||
| type changes struct { |
There was a problem hiding this comment.
I also use this sort of structure in graft/coreth/triedb/firewood/account_trie.go. I have an issue open at ava-labs/firewood#1433, so if you think it should be, will you contribute any technical requirements/suggestions there?
There was a problem hiding this comment.
Yeah this branch was made prior to the monorepo so I need to do a pass of the evm wrapper and see what we can share here
|
|
||
| db.pending.Put(db.heightKey, database.PackUInt64(db.height)) | ||
|
|
||
| p, err := db.db.Propose(db.pending.Keys, db.pending.Vals) |
There was a problem hiding this comment.
If you will always be proposing and immediately committing, then there is an operation db.Update which combines these into a single FFI
| puts []put | ||
| }{ | ||
| { | ||
| name: "no puts", |
There was a problem hiding this comment.
This test case is unintuitive to me (I would have expected that the root doesn't change). If this is just x-chain knowledge that would be expected, that's ok but might be worth noting
There was a problem hiding this comment.
We are writing to two different dbs - one for the merkle-ized chain state (analagous to evm's state trie) and another for non-canonical chain data (like indicies) and I believe all VMs will have some problem similar to this.
Since we have a variation of a two-db write problem, we need to be able to handle the case where the dbs fall out of sync, and to detect that we need to know the height that firewood is at so we can re-apply blocks to get us back-in-sync with the other database.
There's two ways of solving this:
- one is to include height in the firewood db (which seems wrong - because height is conceptually not a part of canonical chain state, so it feels like we're breaking an abstraction boundary). I went with this approach and the inclusion of height is why the root is different.
- The other idea is to maintain indices for firewood roots -> the height they are created at - but this is complex because now firewood can fall out of sync with the index that that tracks it.
There was a problem hiding this comment.
The EVM also has this problem of course. It elected to use two databases, with a "rollback" mechanism if they end up out of sync. Meaning:
startHeight := MostRecentHeight()
for i :=0; i < maxRollback; i++ {
stateRoot := GetRoot(startHeight - i)
if knownRoot == stateRoot {
break
}
}
It is kind of gross that you violate the abstraction, but it's certainly simpler than what I wrote above. It also guarantees that each block has a unique state root, which is super nice as well. Maybe just document the reservedPrefix and when it should be used, and if it's guaranteed to not be clobbered by external data
|
|
||
| require.NoError(t, db.Close(t.Context())) | ||
|
|
||
| db, err = New(filepath.Join(dir, "firewood.test")) |
There was a problem hiding this comment.
Maybe to avoid errors, move the filepath to a var
There was a problem hiding this comment.
This location is weird to me, for a lot of reasons.
- I think Firewood is eventually joining the monorepo, and this is definitely the logical spot to put it
- This won't be the only firewood implementation (since there's also the C-Chain). Should that go here as well? I would expect not, but maybe.
- There's a whole
database/folder, which seems to currently be only key-value databases. Either way, this does seem like a database
There was a problem hiding this comment.
I left it here because I wasn't sure what I wanted to do with this... we need to figure out what to do before merging because this is in an admittedly weird spot right now. Some thoughts:
- If firewood is joining the monorepo then this needs to move
databaseseems like a good spot to put this at first, but the problem is that this type doesn't implement the database interface. We could implement the interface if we wanted to... but it would also mean that we can do wrapping with the other database types (likeprefixdb) and I don't think we want to use those wrappers (e.gprefixdbhashes prefixes, leading to unpredictable prefixes). Against my point however - we are usingdatabase.ErrNotFoundto avoid unintended behavior changes which makes it seem like we are adatabase.Database- so we need to pick a lane.
I need to review the EVM wrapper over firewood to see if there's a way we can share test coverage + code and do some thinking over where this lives and its relationship to database.
Why this should be merged
Adds a wrapper for VMS to integrate with firewood
How this works
Firewood does not support mutable proposals - so this wrapper buffers all changes until
Flushis called, which copies data into a proposal to flush to firewood. A height counter is also tracked as part of state so that VMs can repair their databases if they fall out-of-sync with each other by replaying blocks until they are at the same height.How this was tested
Added tests
Need to be documented in RELEASES.md?
No