feat: Add Firewood implementation #240
Conversation
alarso16
added
Status: 🔴 DO NOT MERGE
alarso16
force-pushed
the
alarso16/firewood-implementation
branch
from
9d0eff0 to
bdb53d6
Compare
libevm/triedb/firewood/proposals.go
Outdated
| // we form a tree of proposals, the `proposal.Proposal` field may be the only | ||
| // reference to a given proposal. To ensure that all proposals in the tree | ||
| // can be freed in a finalizer, this cannot be included in the tree structure. |
There was a problem hiding this comment.
I understand the beginning of the comment, but not this part. Don't worry about re-writing the comment, please just explain to me (as verbosely as you need to be) and then we can work on clarifying it later.
There was a problem hiding this comment.
It is unclear. To avoid explicitly dropping a Proposal (which you suggest may not be ideal anyway), the ffi.Proposal cannot be included in the tree, because each link is bidirectional. You can get a cyclic dependency between elements in the tree, so nothing ever gets finalized. We do of course need the actual proposal eventually, but that's only at commit time, where we know that this proposal will be the only one of a given height
| // possibleProposals temporarily holds proposals created during a trie update. | ||
| // This is cleared after the update is complete and the proposals have been sent to the database. |
There was a problem hiding this comment.
Why do they need to be scoped to the Database? It may become clearer when I read the code, but this comment suggests that they're local to a trie update.
There was a problem hiding this comment.
There's only ever one outstanding set of proposals, and I don't know where to put them. It seems that once we create this proposals struct, it belongs there
| return 0, 0 | ||
| } | ||
|
|
||
| // This isn't called anywhere in coreth |
There was a problem hiding this comment.
libevm is agnostic to coreth and subnet-evm (and all consumers for that matter).
| // This isn't called anywhere in coreth | |
| // Reference is a no-op because... |
Why doesn't Firewood need referencing? Because of the finalizer semantics we're working on?
Is Reference typically used as a keepalive signal?
There was a problem hiding this comment.
Oops, yeah that's outdated. From triedb docs:
// Reference adds a new reference from a parent node to a child node. This function
// is used to add reference between internal trie node and external node(e.g. storage
// trie root), all internal trie nodes are referenced together by database itself.
It's only used for HashDB anyway. But yes, it's only exported as a keep-alive signal
| // This isn't called anywhere in coreth | ||
| func (*Database) Reference(common.Hash, common.Hash) {} | ||
|
|
||
| // Dereference drops a proposal from the database. |
There was a problem hiding this comment.
It doesn't though. Let's work on the comment for Reference first and then we can probably just say here that it's a no-op and see [Database.Reference] for rationale.
| // Thus, we recognize the single root C as the only proposal, and dereference it. | ||
| func (*Database) Dereference(common.Hash) {} | ||
|
|
||
| // Firewood does not support this. |
There was a problem hiding this comment.
| // Firewood does not support this. | |
| // Cap is a no-op because it isn't supported by Firewood. |
libevm/triedb/firewood/firewood.go
Outdated
| // All remaining proposals must be dereferenced. | ||
| db.possibleProposals = nil | ||
| db.proposalMap = nil | ||
| db.proposalTree = nil |
There was a problem hiding this comment.
Should we not just explicitly drop them? The finalizer semantics were intended as a fail-safe just in case we "lose" one in the libevm plumbing, but in this case it's all under our control.
There was a problem hiding this comment.
We could. A bigger change but I'll get it done soon
| // We must provide a context since it may hang while waiting for the finalizers to complete. | ||
| return db.Firewood.Close(context.Background()) | ||
| } | ||
|
|
There was a problem hiding this comment.
(note to self) Review below here.
## Why this should be merged Simplify existing implementation. ## How this works 1. Abstract code to reduce nesting 2. Restructure `Database` contents to logically group similar components 3. Embed `blockInfo` inside `proposal` as all fields effectively belong to the `proposal` anyway ## How this was tested N/A as base branch is still to be tested.
Why this should be merged
This is the first step in integrating Firewood into libevm for shared use in coreth and subnet-evm. This PR includes the
triedb.DBOverrideimplementation, as well as thestate.Trieimplementations necessary to inject intostate.Database. This is essentially a re-write of the current implementation in coreth, so feel free to suggest any structural changes. Let's get it right this time.Note this cannot be merged until the next Firewood release. The automatic proposal cleanup and some bug fixes are necessary.
The most notable difference from coreth is a performance difference (see ava-labs/coreth#1238)
How this works
All the logic is essentially the same as before, with the exception of proposal creation and cleanup. Proposals are now created at hash time, and then rather than being dropped, are passed to the triedb for potential commit.
How this was tested
Sorry, there's no unit tests. I'm thinking PR 2 will be creating the
state.Databaseinjections and fixing the fuzz test in coreth. However, you can test this by checking outalarso16/firewood-libevmin coreth, using somego mod editstatements, and building firewood locally. It passes all coreth tests.Alternatively, there's a DONOTMERGE PR in coreth running UT: ava-labs/coreth#1399