Skip to content

Comments

Fix multiple bugs in FakeEVMChain simulator#21254

Open
wentzeld wants to merge 6 commits intodevelopfrom
fix/fake-evm-chain-additional-bugs
Open

Fix multiple bugs in FakeEVMChain simulator#21254
wentzeld wants to merge 6 commits intodevelopfrom
fix/fake-evm-chain-additional-bugs

Conversation

@wentzeld
Copy link
Contributor

Summary

  • Fix nil pointer dereference in FilterLogs when converting log topics to protobuf
  • Fix FilterLogs ignoring topics from filter query, and ignoring sign on FromBlock/ToBlock
  • Fix nil pointer in GetTransactionByHash when To() is nil (contract creation txs)
  • Fix GetTransactionReceipt log conversion missing Data and Topics fields
  • Add nil input checks to FilterLogs, EstimateGas, GetTransactionByHash, GetTransactionReceipt (matching existing BalanceAt pattern)

Test plan

  • Verified go build ./core/capabilities/fakes/... compiles cleanly
  • Rebuilt CRE CLI with patched dependency and ran EVM client test workflow exercising all 7 read capabilities

@github-actions
Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

  • #added For any new functionality added.
  • #breaking_change For any functionality that requires manual action for the node to boot.
  • #bugfix For bug fixes.
  • #changed For any change to the existing functionality.
  • #db_update For any feature that introduces updates to database schema.
  • #deprecation_notice For any upcoming deprecation functionality.
  • #internal For changesets that need to be excluded from the final changelog.
  • #nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
  • #removed For any functionality/config that is removed.
  • #updated For any functionality that is updated.
  • #wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

@trunk-io
Copy link

trunk-io bot commented Feb 20, 2026

Static BadgeStatic BadgeStatic BadgeStatic Badge

View Full Report ↗︎Docs

@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2026

✅ No conflicts with other open PRs targeting develop

tarcisiozf
tarcisiozf previously approved these changes Feb 20, 2026
tarcisiozf
tarcisiozf previously approved these changes Feb 20, 2026
@wentzeld wentzeld marked this pull request as ready for review February 20, 2026 19:38
@wentzeld wentzeld requested review from a team as code owners February 20, 2026 19:38
Copilot AI review requested due to automatic review settings February 20, 2026 19:38
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 fixes multiple critical bugs in the FakeEVMChain simulator related to nil pointer dereferences and incorrect data handling. The fixes ensure proper conversion between protobuf and go-ethereum types, handle edge cases like contract creation transactions, and add defensive nil checks following the established pattern in the codebase.

Changes:

  • Fixed nil pointer dereference in FilterLogs when converting log topics to protobuf
  • Fixed FilterLogs ignoring topics filter and sign information in FromBlock/ToBlock
  • Fixed nil pointer crash in GetTransactionByHash for contract creation transactions (nil To address)
  • Fixed missing Data and Topics fields in GetTransactionReceipt log conversion
  • Added nil input validation to FilterLogs, EstimateGas, GetTransactionByHash, and GetTransactionReceipt

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

Copy link
Contributor

@nolag nolag left a comment

Choose a reason for hiding this comment

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

Tests?

Comment on lines +274 to +276
if filterQueryPb == nil {
return nil, caperrors.NewPublicSystemError(errors.New("FilterQuery is nil"), caperrors.Unknown)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an error, do I need to filter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Verify against the real capability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the real capability >
nil input: ConvertFilterFromProto returns ErrEmptyFilter
nil filterQuery: Same — returns ErrEmptyFilter

in new fake
nil input: Returns error "FilterLogsRequest is nil"
nil FilterQuery: Returns error "FilterQuery is nil"

DylanTinianov
DylanTinianov previously approved these changes Feb 20, 2026
  - Add validateBlockNumber helper matching real capability behavior
  - Validate fromBlock/toBlock in FilterLogs (reject 0, bad negatives, non-int64)
  - Reject reversed block ranges (toBlock < fromBlock)
  - Populate EventSig field in FilterLogs log conversion
  - Populate EventSig field in GetTransactionReceipt log conversion
  - Add missing log fields in GetTransactionReceipt (BlockNumber, BlockHash, TxHash, Index, Removed)
@wentzeld wentzeld dismissed stale reviews from DylanTinianov and tarcisiozf via ec5a7fd February 21, 2026 18:30
@cl-sonarqube-production
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants