Skip to content

Add Firewood state dump tooling for targeted block-height debugging#5226

Open
rkuris wants to merge 1 commit intomasterfrom
rkuris/firewood-block-height-debugging
Open

Add Firewood state dump tooling for targeted block-height debugging#5226
rkuris wants to merge 1 commit intomasterfrom
rkuris/firewood-block-height-debugging

Conversation

@rkuris
Copy link
Copy Markdown
Contributor

@rkuris rkuris commented Apr 11, 2026

Why this should be merged

  • add DUMP_ON_BLOCK_HEIGHT env var in coreth block validation to dump firewood state at a chosen height
  • add firewood TrieDB.DumpProposal with fallback chain (byStateRoot -> possible -> tree -> revision -> database)
  • make merkle-root dberr formatting nil-safe in coreth and subnet-evm validators
  • run reexecute benchmark via local package path (go run ./tests/...)

How this works

  • DumpProposal releases the proposals lock before Firewood FFI calls (Revision/Dump) to avoid blocking proposal operations during dump
  • Error/warning paths use structured log.Warn instead of fmt.Fprintf to stderr; the successful dump output stays on stderr as a delimited forensic blob
  • Each DumpProposal lookup path is annotated with a # source= prefix for traceability

How this was tested

Targeted debugging sessions with firewood hashing changes

Need to be documented in RELEASES.md?

No

- add DUMP_ON_BLOCK_HEIGHT env var in coreth block validation to dump
  firewood state at a chosen height
- add firewood TrieDB.DumpProposal with fallback chain
  (byStateRoot -> possible -> tree -> revision -> database)
- make merkle-root dberr formatting nil-safe in coreth and subnet-evm
  validators
- run reexecute benchmark via local package path (go run ./tests/...)

Review notes:
- DumpProposal releases the proposals lock before Firewood FFI calls
  (Revision/Dump) to avoid blocking proposal operations during dump
- Error/warning paths use structured log.Warn instead of fmt.Fprintf
  to stderr; the successful dump output stays on stderr as a delimited
  forensic blob
- Each DumpProposal lookup path is annotated with a # source= prefix
  for traceability
@rkuris rkuris self-assigned this Apr 11, 2026
@rkuris rkuris requested review from a team as code owners April 11, 2026 23:00
Copilot AI review requested due to automatic review settings April 11, 2026 23:00
Copy link
Copy Markdown
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

Adds targeted Firewood state/proposal dump tooling to support forensic debugging at a specific block height during block validation, plus a small benchmark script adjustment.

Changes:

  • Add DUMP_ON_BLOCK_HEIGHT support in Coreth block validation to dump Firewood proposal/state info at a chosen height.
  • Introduce TrieDB.DumpProposal(root) with multiple lookup fallbacks (tracked proposals → revision → full DB dump).
  • Update the C-Chain reexecute benchmark script to run the local ./tests/reexecute/c package path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
scripts/benchmark_cchain_range.sh Switches go run to use a local package path for reexecute benchmarking.
graft/evm/firewood/triedb.go Adds DumpProposal method to dump tracked/persisted Firewood proposal/revision/DB state with source annotations.
graft/coreth/core/block_validator.go Parses DUMP_ON_BLOCK_HEIGHT and emits Firewood dump output on stderr at the configured height during ValidateState.

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

Comment on lines 287 to 289
else
go run github.com/ava-labs/avalanchego/tests/reexecute/c \
go run ./tests/reexecute/c \
--block-dir="${BLOCK_DIR}" \
Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

go run ./tests/reexecute/c is resolved relative to the caller’s current working directory, not the script location. If this script is invoked from outside the repo root (e.g., via an absolute path), this will fail to find ./tests/.... Consider cd-ing to the repo root early in the script or using a path derived from SCRIPT_DIR so the benchmark is runnable from any CWD.

Copilot uses AI. Check for mistakes.
Comment on lines +423 to +460
proposals, ok := t.proposals.byStateRoot[root]
if ok && len(proposals) > 0 {
for _, p := range proposals {
if p.handle != nil {
dump, err := p.handle.Dump()
t.proposals.Unlock()
if err != nil {
return "", err
}
return fmt.Sprintf("# source=byStateRoot root=%s\n%s", root.Hex(), dump), nil
}
}
}

for key, p := range t.possible {
if key.root != root || p == nil || p.handle == nil {
continue
}
dump, err := p.handle.Dump()
t.proposals.Unlock()
if err != nil {
return "", fmt.Errorf("dumping possible proposal for root %s (parent block %s): %w", root.Hex(), key.parentBlockHash.Hex(), err)
}
return fmt.Sprintf("# source=possible parent_block=%s\n%s", key.parentBlockHash.Hex(), dump), nil
}

if t.proposals.tree != nil && t.proposals.tree.handle != nil {
treeRoot := t.proposals.tree.root
dump, err := t.proposals.tree.handle.Dump()
t.proposals.Unlock()
if err != nil {
return "", fmt.Errorf("dumping current tree proposal while root %s was missing: %w", root.Hex(), err)
}
return fmt.Sprintf("# source=tree requested_root=%s current_tree_root=%s\n%s", root.Hex(), treeRoot.Hex(), dump), nil
}

t.proposals.Unlock()

Copy link

Copilot AI Apr 11, 2026

Choose a reason for hiding this comment

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

DumpProposal calls p.handle.Dump() (Firewood FFI) while holding t.proposals mutex. If Dump() is slow, this blocks commits/proposals/updates that also need the same lock, undermining the stated goal of not blocking proposal operations during dumps. Consider selecting the handle + any metadata under the lock, releasing the lock, then performing the FFI dump; if concurrent commit/drop is a concern, add a dedicated handle-level lock/refcount so Dump() and Commit()/Drop() can’t race.

Suggested change
proposals, ok := t.proposals.byStateRoot[root]
if ok && len(proposals) > 0 {
for _, p := range proposals {
if p.handle != nil {
dump, err := p.handle.Dump()
t.proposals.Unlock()
if err != nil {
return "", err
}
return fmt.Sprintf("# source=byStateRoot root=%s\n%s", root.Hex(), dump), nil
}
}
}
for key, p := range t.possible {
if key.root != root || p == nil || p.handle == nil {
continue
}
dump, err := p.handle.Dump()
t.proposals.Unlock()
if err != nil {
return "", fmt.Errorf("dumping possible proposal for root %s (parent block %s): %w", root.Hex(), key.parentBlockHash.Hex(), err)
}
return fmt.Sprintf("# source=possible parent_block=%s\n%s", key.parentBlockHash.Hex(), dump), nil
}
if t.proposals.tree != nil && t.proposals.tree.handle != nil {
treeRoot := t.proposals.tree.root
dump, err := t.proposals.tree.handle.Dump()
t.proposals.Unlock()
if err != nil {
return "", fmt.Errorf("dumping current tree proposal while root %s was missing: %w", root.Hex(), err)
}
return fmt.Sprintf("# source=tree requested_root=%s current_tree_root=%s\n%s", root.Hex(), treeRoot.Hex(), dump), nil
}
t.proposals.Unlock()
var (
selectedHandle interface{ Dump() (string, error) }
selectedSource string
selectedParentBlock common.Hash
selectedTreeRoot common.Hash
)
proposals, ok := t.proposals.byStateRoot[root]
if ok && len(proposals) > 0 {
for _, p := range proposals {
if p.handle != nil {
selectedHandle = p.handle
selectedSource = "byStateRoot"
break
}
}
}
if selectedHandle == nil {
for key, p := range t.possible {
if key.root != root || p == nil || p.handle == nil {
continue
}
selectedHandle = p.handle
selectedSource = "possible"
selectedParentBlock = key.parentBlockHash
break
}
}
if selectedHandle == nil && t.proposals.tree != nil && t.proposals.tree.handle != nil {
selectedHandle = t.proposals.tree.handle
selectedSource = "tree"
selectedTreeRoot = t.proposals.tree.root
}
t.proposals.Unlock()
if selectedHandle != nil {
dump, err := selectedHandle.Dump()
if err != nil {
switch selectedSource {
case "possible":
return "", fmt.Errorf("dumping possible proposal for root %s (parent block %s): %w", root.Hex(), selectedParentBlock.Hex(), err)
case "tree":
return "", fmt.Errorf("dumping current tree proposal while root %s was missing: %w", root.Hex(), err)
default:
return "", err
}
}
switch selectedSource {
case "possible":
return fmt.Sprintf("# source=possible parent_block=%s\n%s", selectedParentBlock.Hex(), dump), nil
case "tree":
return fmt.Sprintf("# source=tree requested_root=%s current_tree_root=%s\n%s", root.Hex(), selectedTreeRoot.Hex(), dump), nil
default:
return fmt.Sprintf("# source=byStateRoot root=%s\n%s", root.Hex(), dump), nil
}
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants