refactor(block)!: remove GetLastState from components

[julienrbrt]

Overview

If GetLastState() got somehow called on the component prior that it is started you could get an empty state.
First I thought let's fallback to the store 60c6c0e when empty. However when I checked the usage of GetLastState from the components I couldn't find anything.
I think deleting this public api makes more sense then.

[github-actions]

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	⏩ skipped	✅ passed	✅ passed	Dec 12, 2025, 11:16 AM

[claude]

Claude finished @julienrbrt's task in 2m 9s —— View job

---

Comprehensive Code Review

I've reviewed this PR which removes the public GetLastState() API from the block components. Here's my analysis:

---

✅ Summary

The PR correctly removes unused public API methods and simplifies the codebase. The refactoring is sound and improves the architecture.

What Changed

1. Removed public GetLastState() from Components, Executor, and Syncer
2. Made getLastState() private in Executor and Syncer
3. Removed the fallback-to-store logic that was briefly added in commit 60c6c0e
4. Removed defensive copying of AppHash and LastHeaderHash in the syncer
5. Updated all tests to use private methods where needed (via same-package access)

---

🎯 Code Quality & Architecture

✅ Strengths

1. Proper encapsulation: Making getLastState() private is the right choice since external callers shouldn't access internal component state directly
2. Clean API surface: Removing unused public methods reduces maintenance burden and prevents misuse
3. Breaking change properly marked: The ! in the PR title correctly signals this is a breaking change
4. Well-tested: All affected tests were updated appropriately

📋 Observations

1. State initialization is now stricter: Previously GetLastState() could fall back to the store if called before Start(). Now it returns an empty state. This is actually correct behavior - components should only be queried after they're started.

2. Removed defensive cloning in syncer: The old GetLastState() in syncer cloned AppHash and LastHeaderHash. Since the method is now private and internal callers use it correctly (no mutations), this defensive copying was unnecessary overhead.

3. Atomic pointer pattern is sound: Both components use atomic.Pointer[types.State] which provides thread-safe reads/writes without locks.

---

🔒 Concurrency & Thread Safety

✅ Correct Usage

The getLastState() pattern is thread-safe:

• Uses atomic.Pointer[types.State] for lock-free reads
• Returns a value copy (*state), not a pointer
• Callers receive their own copy, preventing shared mutation

Note on the Removed Fallback

The gemini-code-assist bot flagged a variable shadowing bug in the fallback code (commit 60c6c0e), but this is moot since:

1. That fallback code was removed in commit 1c48c16
2. The current code correctly returns an empty state if lastState is nil
3. Components should never be queried before Start() initializes state anyway

---

🧪 Testing

✅ Test Coverage

All tests were properly updated:

• block/components_test.go: Removed test for deleted API
• block/internal/syncing/syncer_test.go: Updated to use private getLastState() (6 occurrences)
• block/internal/syncing/syncer_forced_inclusion_test.go: Updated to use private getLastState() (8 occurrences)
• node/single_sequencer_integration_test.go: Removed usage of GetLastState()

The test changes confirm that:

1. No external packages were using GetLastState()
2. Only internal (same-package) tests needed the state accessor
3. Test-only usage via same-package access is acceptable

---

🛡️ Error Handling & Resilience

✅ Proper Initialization Contract

The removal of the store fallback strengthens the contract:

• Before: Components could be queried before Start(), returning stale store data
• After: Components return empty state if not started (fail-fast behavior)
• Correct: Callers should always call Start() before using components

Both Executor and Syncer properly initialize lastState in their Start() methods via initializeState():

• executor.go:180-230 - loads or creates initial state
• syncer.go:245-292 - loads or creates initial state

---

🚨 Security & Safety

✅ No Security Issues

1. No secret exposure: State doesn't contain sensitive data
2. No race conditions: Atomic operations are correctly used
3. No unintended mutations: Value copies prevent shared state bugs
4. Proper error handling: Both components handle state load failures in initializeState()

---

📊 Performance

✅ Minor Performance Improvement

Removing defensive cloning in syncer's getLastState():

// OLD (unnecessary defensive copy)
stateCopy := *state
stateCopy.AppHash = bytes.Clone(state.AppHash)
stateCopy.LastHeaderHash = bytes.Clone(state.LastHeaderHash)
return stateCopy

// NEW (simple value copy, no extra allocations)
return *state

Impact: Eliminates 2 heap allocations per call. Since this is called frequently in the hot path (trySyncNextBlock, processHeightEvent), this is a measurable improvement.

---

🎨 Code Style & Conventions

✅ Follows Go Conventions

1. ✅ Unexported methods use lowercase names (getLastState)
2. ✅ Consistent atomic pointer usage across both components
3. ✅ Proper error wrapping with context
4. ✅ Clear documentation comments

Minor Suggestion (Non-blocking)

Consider adding a comment to getLastState() explaining the empty state return:

// getLastState returns the current state.
// Returns an empty state if called before Start() initializes the component.
// getLastState should never directly mutate the returned state.
func (e *Executor) getLastState() types.State {

---

🔍 Verification

I verified that:

• ✅ No external packages reference GetLastState() (grep found 0 matches)
• ✅ All internal callers updated correctly
• ✅ Breaking change properly marked with ! in commit message
• ✅ State initialization happens in Start() before any usage
• ✅ Thread safety maintained via atomic operations

---

✅ Final Recommendation

APPROVE - This is a clean refactoring that:

1. ✅ Removes unused public API
2. ✅ Improves encapsulation
3. ✅ Maintains correctness
4. ✅ Slightly improves performance
5. ✅ Has proper test coverage
6. ✅ Properly marks breaking changes

The PR correctly identifies that GetLastState() wasn't being used externally and removes it. The decision to make the method private instead of adding a store fallback is architecturally sound - components should be started before use.

---

[gemini-code-assist]

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refactors the state retrieval logic within the Executor and Syncer components to introduce a crucial fallback mechanism. The primary goal is to enhance the system's resilience by ensuring that if a component's in-memory state is accessed before it's initialized, the system will attempt to fetch the current state from its persistent storage. This prevents potential issues from uninitialized state access and improves overall stability.

Highlights

• State Fallback Mechanism: Implemented a robust fallback mechanism for state getters in Executor and Syncer components. If the in-memory lastState is not yet loaded, the system now attempts to retrieve the state from the persistent store.
• Error Handling and Logging: Added error handling and logging for the state retrieval process from the store. If fetching the state from the store fails, a warning is logged, and an empty state is returned to prevent panics or unexpected behavior.
• State Caching: Successfully retrieved state from the store is now cached in the lastState atomic value, ensuring subsequent calls can access the state without hitting the persistent store again.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to add a fallback to fetch state from the store in GetLastState if it's not present in memory. While the intention is good, the current implementation introduces a critical bug in both modified files. A variable shadowing issue will cause a nil pointer dereference, leading to a panic. I've added comments with suggested fixes to address this critical issue.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.65%. Comparing base (e867b3f) to head (d1be450).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2923      +/-   ##
==========================================
+ Coverage   63.61%   63.65%   +0.03%     
==========================================
  Files          90       90              
  Lines        8584     8571      -13     
==========================================
- Hits         5461     5456       -5     
+ Misses       2540     2534       -6     
+ Partials      583      581       -2     

Flag	Coverage Δ
combined	63.65% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.