refactor: Firewood Revisions

[alarso16]

Why this should be merged

Now that using Revision is concurrent safe and guaranteed to not cause undefined behavior, it should be used a more canonical way of accessing state.

How this works

Uses Firewood's Revision instead of relying on a one-Revision cache internal to Rust. The Revision has a finalizer attached to release all related memory when there are no more references.

How this was tested

CI and re-execution tests

Need to be documented in RELEASES.md?

No

[Copilot]

Pull request overview

This PR refactors the Firewood integration to use Revision objects directly for accessing state, improving concurrency safety and memory management. The change eliminates reliance on an internal one-Revision cache in favor of Firewood's native revision system with automatic memory cleanup via finalizers.

• Replaces GetFromRoot calls with Revision objects that have built-in memory management
• Updates the reader struct to hold a Revision instead of database reference and root hash
• Adds explicit cleanup of state references in the legacy transaction pool

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
graft/coreth/triedb/firewood/database.go	Refactors Reader to use Firewood Revision objects instead of root hash lookups
graft/coreth/core/txpool/legacypool/legacypool.go	Adds explicit cleanup of state references during pool closure

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[AminR443]

LGTM. One comment on dropping:

The Revision has a finalizer attached to release all related memory when there are no more references.

Revisions hanging around will prevent the revision manager from reaping the nodestores, and this may not be good from a memory perspective. Last I checked, finalizers were considered expensive and could run at an arbitrary time after becoming unreachable. What's preventing us from explicitly dropping the revision?

Also, if we're going with finalizers, is there any particular reason we're not using runtime.AddCleanup?

[alarso16]

Revisions hanging around will prevent the revision manager from reaping the nodestores, and this may not be good from a memory perspective. Last I checked, finalizers were considered expensive and could run at an arbitrary time after becoming unreachable. What's preventing us from explicitly dropping the revision?

We don't know when it will no longer be used. There's very strict interfaces to adhere to, and that includes that we cannot assume there will be any behavior to indicate the Reader is no longer used.

Also, if we're going with finalizers, is there any particular reason we're not using runtime.AddCleanup?

No particular reason! In our case, they should be equivalent. We know that they are properly being cleaned because closing the database doesn't hang. We could use runtime.AddCleanup in Firewood, and that's worth thinking about

[AminR443]

Sounds good to me. I think we should consider using runtime.AddCleanup, since Go’s docs suggest it as the new go-to option:

New Go code should consider using [AddCleanup](https://pkg.go.dev/runtime#AddCleanup) instead, which is much less error-prone than SetFinalizer.

I'll create an issue for that in firewood repo.

[alarso16]

These keep around a StateDB, which holds onto the *firewood.accountTrie, so the database closure will hang unless we remove these references. Another (worse) option is that on state.Trie.Copy(), we don't copy the Reader, and instead use GetFromRoot then. This seems too messy, so I have this small line change instead.