Skip to content

Conversation

@alarso16
Copy link
Contributor

@alarso16 alarso16 commented Dec 15, 2025

Why this should be merged

This is about a 30% performance improvement for bootstrapping nodes.

How this works

See ava-labs/libevm#240 for comments from an initial review. All proposals created at hash time are stored in a map, and some shutdown recovery is added for block hashes and heights.

How this was tested

UT and re-execution of state.

Need to be documented in RELEASES.md?

No

@alarso16 alarso16 self-assigned this Dec 15, 2025
@alarso16 alarso16 added evm Related to EVM functionality coreth Related to the former coreth standalone repository subnet-evm Related to the former subnet-evm standalone repository labels Dec 15, 2025
@alarso16 alarso16 linked an issue Dec 15, 2025 that may be closed by this pull request
@alarso16 alarso16 force-pushed the alarso16/shared-firewood branch from 6b31642 to 9ab772f Compare December 15, 2025 20:42
@alarso16 alarso16 force-pushed the alarso16/firewood-perf-propose branch 3 times, most recently from 201573a to e7a87c3 Compare December 16, 2025 19:59
@alarso16 alarso16 changed the base branch from alarso16/shared-firewood to alarso16/firewood-v0.0.17 December 16, 2025 20:00
@alarso16 alarso16 force-pushed the alarso16/firewood-perf-propose branch from e7a87c3 to 1395fce Compare December 16, 2025 20:05
@alarso16 alarso16 force-pushed the alarso16/firewood-v0.0.17 branch from e7ee927 to 51e8ad4 Compare December 16, 2025 20:07
@alarso16 alarso16 force-pushed the alarso16/firewood-perf-propose branch from 1395fce to de5a6aa Compare December 16, 2025 20:48
Base automatically changed from alarso16/firewood-v0.0.17 to master December 16, 2025 21:01
@alarso16 alarso16 force-pushed the alarso16/firewood-perf-propose branch from de5a6aa to 9961f98 Compare December 18, 2025 19:12
@alarso16 alarso16 marked this pull request as ready for review December 18, 2025 22:07
@alarso16 alarso16 requested a review from a team as a code owner December 18, 2025 22:07
Copilot AI review requested due to automatic review settings December 18, 2025 22:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the Firewood database integration to improve proposal management and enable a ~30% performance improvement for bootstrapping nodes. The key optimization is sharing proposals between hashing and commit operations, eliminating redundant proposal creation. Additionally, the refactoring includes better recovery handling through persistent storage of block hashes and heights.

Key changes:

  • Proposals are now created during the Hash() operation and reused during Commit(), rather than being created and dropped during hashing
  • Recovery information (committed block hashes and heights) is now persisted to disk
  • Configuration structure and naming conventions have been improved for clarity

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
graft/evm/firewood/triedb.go Major refactoring to track proposals created during hashing for later commit, added recovery state persistence, renamed config fields
graft/evm/firewood/account_trie.go Updated to use new proposal creation API, improved documentation
graft/evm/firewood/storage_trie.go Enhanced documentation for storage trie behavior
graft/evm/firewood/recovery.go New file implementing recovery functions for persisting/reading committed block hashes and heights
graft/evm/firewood/hash_test.go Updated to use new config API
graft/subnet-evm/core/genesis.go Added Firewood-specific handling for empty genesis blocks, added helper function
graft/subnet-evm/core/genesis_test.go Updated to use new config API
graft/subnet-evm/core/blockchain.go Updated config field names to match new API
graft/subnet-evm/tests/state_test_util.go Updated to use new config API
graft/coreth/core/genesis.go Added Firewood-specific handling for empty genesis blocks, added helper function
graft/coreth/core/genesis_test.go Updated to use new config API
graft/coreth/core/blockchain.go Updated config field names to match new API
graft/coreth/tests/state_test_util.go Updated to use new config API

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

Copy link
Member

@JonathanOppenheimer JonathanOppenheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments - you definitely know this code better than I do

@Elvis339
Copy link
Contributor

Elvis339 commented Jan 2, 2026

Hey Austin

Found a bug while running C-Chain re-execution benchmarks to measure the performance impact of your proposal-sharing changes.

The PR CI passes because it runs with config: "default" (hashdb) using a hashdb snapshot. It never exercises the Firewood code path.

When running with config: "firewood" starting from a restored Firewood snapshot at block 10M, it fails immediately on the first block https://github.com/ava-labs/avalanchego/actions/runs/20661797719/job/59326295398#step:4:831:

failed to verify block at height 10000001: no proposal found for block 10000001,
root 0x5f7138331cc9dd9d47cd78fb5bf632b0961a28d29d394f19e6d32f2a9e1f4266,
hash 0x90649defd375c9784a95bb01e752c2a06341213c59e25dedfe8aee7a7e8b8d3d

The proposal-sharing logic expects Hash() to populate t.possible before Update() is called. When loading from a Firewood snapshot and executing the first block, something in the initialization path skips Hash() so t.possible is empty when Update() runs.

To reproduce:

gh workflow run "C-Chain Re-Execution Benchmark w/ Container" \
    --repo ava-labs/avalanchego \
    --ref es/candidate-firewood \
    -f runner="avalanche-avalanchego-runner-2ti" \
    -f block-dir-src="cchain-mainnet-blocks-10m-20m-ldb" \
    -f current-state-dir-src="cchain-current-state-firewood-10m" \
    -f config="firewood" \
    -f start-block="10000001" \
    -f end-block="10050000"

To save you time digging through git history: es/candidate-firewood is your PR merged on top of es/baseline-firewood, which contains master plus benchmarking infrastructure (pprof, FFI metrics, cross-stack profiling). The baseline branch with the same Firewood config and snapshot works fine. https://github.com/ava-labs/avalanchego/actions/runs/20661268516/job/59324164005

Let me know if you need help reproducing or testing a fix.

@alarso16
Copy link
Contributor Author

After some investigation, I've discovered a couple ideas

I made a draft showing how number 3 could be implemented: #4835. It's definitely messier, but wouldn't break state. However, I do think that we will be making a breaking change to firewood state before the v0.1.0 release

@github-project-automation github-project-automation bot moved this to In Progress 🏗️ in avalanchego Jan 15, 2026
@JonathanOppenheimer
Copy link
Member

I'm not sure about test coverage for this -- I would assume this refactor would introduce new things to test -- some ideas include -- like if Hash() gets called concurrently, or if any of the functions (e.g. Update()) are called in correctly.

@alarso16
Copy link
Contributor Author

These tests are not complete, but I do have an outstanding issue for guaranteeing minimum behavior. This just ensure that the basics are laced together correctly, and the specific multiple hashing thing you suggested

@JonathanOppenheimer
Copy link
Member

These tests are not complete, but I do have an outstanding issue for guaranteeing minimum behavior. This just ensure that the basics are laced together correctly, and the specific multiple hashing thing you suggested

fire - they can be built upon!!

@maru-ava maru-ava requested a review from a team as a code owner January 21, 2026 15:23
@alarso16 alarso16 force-pushed the alarso16/firewood-perf-propose branch from 6cba567 to 015675b Compare January 21, 2026 15:31
panic(fmt.Sprintf("unable to commit genesis block to statedb: %v", err))
}

// Firewood requires `Update` and `Commit`, even if the state is empty.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we provide a bit more for why it requires?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deleted it. Now it's abstracted in the narrowest special case to get the empty block tests to pass

@ceyonur ceyonur added this pull request to the merge queue Jan 21, 2026
Merged via the queue into master with commit 9deeb71 Jan 21, 2026
53 checks passed
@ceyonur ceyonur deleted the alarso16/firewood-perf-propose branch January 21, 2026 21:50
@github-project-automation github-project-automation bot moved this from In Progress 🏗️ to Done 🎉 in avalanchego Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coreth Related to the former coreth standalone repository evm Related to EVM functionality subnet-evm Related to the former subnet-evm standalone repository

Projects

Status: Done 🎉

Development

Successfully merging this pull request may close these issues.

perf: Use persisted proposal in Firewood

7 participants