Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(evm)!: upgrade geth to v1.11.6 #2005

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

k-yang
Copy link
Member

@k-yang k-yang commented Aug 16, 2024

Purpose / Abstract

Update go-ethereum version to Nibiru Chain's go-ethereum fork v1.11.6. The go-ethereum fork has a patch to enable stateful precompiles.

The patch is similar to the patch on v1.10.26, minus the whitespace and pure style changes.

An upgrade to v1.11 was chosen because an upgrade to v1.14 would be too massive. Following this change, v1.12, v1.13, and v1.14 can be upgraded to in smaller PRs.

Summary by CodeRabbit

  • New Features

    • Upgraded Geth version to v1.11.6, improving EVM functionality.
    • Added new utility functions to enhance state management in blockchain operations.
    • Introduced new fields in the ChainConfig structure for better blockchain parameter representation.
  • Bug Fixes

    • Refined message handling and error reporting in Ethereum transaction processing.
  • Documentation

    • Updated CHANGELOG.md to reflect recent changes and improvements.
  • Refactor

    • Streamlined gas limit handling and message construction logic across various components for better performance and maintainability.
  • Tests

    • Enhanced test assertions to align with updated method signatures and expected behaviors.

Copy link
Contributor

coderabbitai bot commented Aug 16, 2024

Walkthrough

The recent changes enhance the Ethereum Virtual Machine (EVM) functionality by upgrading dependencies, refining code structure, and improving memory efficiency through pointer usage. Key modifications include updates to the Geth version, significant refactoring of message handling in transaction processes, and adjustments to the state management system. These improvements aim to streamline operations, enhance performance, and maintain compatibility with evolving Ethereum standards.

Changes

Files Change Summary
CHANGELOG.md Added entry for Geth version bump to v1.11.6, indicating a new feature with potential breaking changes.
app/evmante/*.go Simplified access patterns in CanTransferDecorator and modified the NewEVM method to use pointers for improved performance and consistency.
contrib/make/build.mk Adjusted linker flags for non-Darwin systems to enhance static linking and removed macOS-specific flags.
go.mod Updated various dependencies, including github.com/ethereum/go-ethereum to 1.11.6, reflecting ongoing maintenance.
x/evm/*.go Extensive refactoring across multiple files to replace method calls with direct field access, change parameter types to pointers, and streamline message handling.
x/evm/statedb/*.go Introduced new methods for transient state management and improved transaction preparation. Removed the ExtStateDB interface, simplifying state management.
x/evm/precompile/precompile.go Removed mutex for InitPrecompiles, renamed variables for clarity, and improved error handling in OnRunStart.
x/evm/msg.go Changed AsMessage function to return a pointer to core.Message, improving memory management and method clarity.
x/evm/msg_test.go Updated tests to reflect changes in method calls and added a global variable for the coinbase address.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant EVM
    participant Core

    User->>EVM: Send Transaction
    EVM->>Core: Create Message
    Core->>Core: Validate Message
    Core-->>EVM: Return Message
    EVM-->>User: Transaction Processed
Loading

Poem

🐰 Hop along the code, a change so bright,
With Geth upgraded, we take flight!
Pointers now dance, and fields are clear,
In the EVM realm, we've nothing to fear!
Hoppin' through functions, all tidy and neat,
Celebrate the updates, oh what a treat! ✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@k-yang k-yang marked this pull request as ready for review August 16, 2024 21:47
@k-yang k-yang requested a review from a team as a code owner August 16, 2024 21:47
@k-yang k-yang self-assigned this Aug 16, 2024
@k-yang k-yang added the x: evm Relates to Nibiru EVM or the EVM Module label Aug 16, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (2)
x/evm/chain_config.go (1)

12-38: Integration of new configuration fields is incomplete.

The fields ShanghaiTime, CancunTime, and TerminalTotalDifficultyPassed are currently only defined within the EthereumConfig function and do not appear to be used elsewhere in the codebase. This suggests they may not be fully integrated into the system. Further verification or development is needed to ensure these fields are properly utilized and affect blockchain parameter handling as intended.

Analysis chain

Verify integration of new configuration fields.

The EthereumConfig function now includes ShanghaiTime, CancunTime, and TerminalTotalDifficultyPassed. Ensure these fields are correctly integrated and utilized throughout the system, as they may affect blockchain parameter handling.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the integration and usage of new fields in `EthereumConfig`.

# Test: Search for the usage of `ShanghaiTime`, `CancunTime`, and `TerminalTotalDifficultyPassed`. Expect: Proper integration and usage.
rg --type go 'ShanghaiTime|CancunTime|TerminalTotalDifficultyPassed'

Length of output: 258

x/evm/keeper/gas_fees.go (1)

26-28: Ensure msg is not nil before calling RefundGas.

The RefundGas function is called in msg_server.go without checking if msg is nil. To prevent potential runtime errors, ensure that msg is checked for nil before being passed to RefundGas.

  • File: x/evm/keeper/msg_server.go
  • Line: Ensure msg is checked for nil before calling RefundGas.
Analysis chain

Ensure correct handling of msg as a pointer.

Changing msg to a pointer can improve memory efficiency. Ensure that all interactions with msg account for potential nil values to avoid runtime errors.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all usages of the `RefundGas` function handle the `msg` pointer correctly.

# Test: Search for function calls and ensure proper handling of the `msg` pointer.
rg --type go -A 5 $'RefundGas($_, $_, $_, $_)'

Length of output: 47


Script:

#!/bin/bash
# Use ast-grep to find all calls to the RefundGas function and examine how the msg pointer is handled.
ast-grep --lang go --pattern 'RefundGas($_, $_, $_, $_)'

Length of output: 58


Script:

#!/bin/bash
# Use rg to search for the RefundGas function name in the entire codebase.
rg 'RefundGas'

Length of output: 1328


Script:

#!/bin/bash
# Search for the context around the RefundGas function call in msg_server.go to verify nil handling of msg.
rg -A 10 'RefundGas' x/evm/keeper/msg_server.go

Length of output: 492

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ba151df and 042f27e.

Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
Files selected for processing (20)
  • CHANGELOG.md (1 hunks)
  • app/evmante/evmante_can_transfer.go (2 hunks)
  • app/evmante/interfaces.go (1 hunks)
  • contrib/make/build.mk (1 hunks)
  • go.mod (7 hunks)
  • x/evm/chain_config.go (1 hunks)
  • x/evm/json_tx_args.go (5 hunks)
  • x/evm/keeper/erc20.go (2 hunks)
  • x/evm/keeper/gas_fees.go (3 hunks)
  • x/evm/keeper/grpc_query.go (6 hunks)
  • x/evm/keeper/keeper.go (1 hunks)
  • x/evm/keeper/msg_server.go (13 hunks)
  • x/evm/keeper/vm_config.go (1 hunks)
  • x/evm/msg.go (1 hunks)
  • x/evm/msg_test.go (1 hunks)
  • x/evm/precompile/precompile.go (4 hunks)
  • x/evm/statedb/interfaces.go (1 hunks)
  • x/evm/statedb/statedb.go (2 hunks)
  • x/evm/statedb/statedb_test.go (4 hunks)
  • x/evm/vmtracer.go (4 hunks)
Files skipped from review due to trivial changes (1)
  • app/evmante/evmante_can_transfer.go
Additional comments not posted (37)
app/evmante/interfaces.go (1)

22-22: Verify usage of the updated method signature.

The NewEVM method's signature has changed to use a pointer for the msg parameter. Ensure that all invocations of this method are updated accordingly to pass a pointer to core.Message.

Verification successful

Method signature update verified successfully.

The NewEVM method's signature change to use a pointer for the msg parameter has been correctly updated in all instances found in the codebase. No further action is needed.

  • x/evm/keeper/msg_server.go
  • app/evmante/evmante_can_transfer.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all invocations of the `NewEVM` method are updated to pass a pointer to `core.Message`.

# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type go -A 2 $'NewEVM('

Length of output: 94


Script:

#!/bin/bash
# Description: Verify all invocations of the `NewEVM` method are updated to pass a pointer to `core.Message`.

# Corrected Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type go -A 2 'NewEVM\('

Length of output: 1240

x/evm/keeper/vm_config.go (2)

58-61: LGTM! Changes to VMConfig are approved.

The addition of EnablePreimageRecording and removal of the Debug field in vm.Config are appropriate for the new requirements.


Line range hint 51-61:
Verify the impact of changing the receiver from a pointer to a value.

The method signature for VMConfig changed from using a pointer receiver to a value receiver. Ensure this change does not affect the functionality or performance, especially if Keeper holds any state.

Verification successful

No impact from changing the receiver type for VMConfig method

The VMConfig method is not currently invoked anywhere in the codebase. Therefore, changing its receiver from a pointer to a value does not impact the existing functionality.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `VMConfig` to ensure the change from pointer to value receiver is safe.

# Test: Search for the usage of `VMConfig`. Expect: No issues due to receiver change.
ast-grep --lang go --pattern $'$_.$_($$$)' --json

Length of output: 53


Script:

#!/bin/bash
# Description: Specifically search for the `VMConfig` method within the `Keeper` struct to verify the impact of changing the receiver.

# Test: Search for the `VMConfig` method usage in the codebase.
ast-grep --lang go --pattern $'$_.$_($$$)' --json | jq '.[] | select(.matches[].text | test("VMConfig"))'

Length of output: 106


Script:

#!/bin/bash
# Description: Search for explicit calls to the `VMConfig` method on `Keeper` instances.

# Test: Look for the `VMConfig` method being called on `Keeper` instances.
ast-grep --lang go --pattern $'$_ := Keeper{ $$$ }\n$_.$_($$$)' --json | jq '.[] | select(.matches[].text | test("VMConfig"))'

Length of output: 579


Script:

#!/bin/bash
# Description: Find all instances of the `Keeper` struct and calls to the `VMConfig` method.

# Step 1: Find all instances of the `Keeper` struct.
ast-grep --lang go --pattern 'Keeper{ $$$ }' --json

# Step 2: Find calls to the `VMConfig` method.
ast-grep --lang go --pattern '$_ := $_.VMConfig($$$)' --json

Length of output: 12549

x/evm/precompile/precompile.go (2)

90-90: LGTM! Error handling in OnRunStart is improved.

The updated error message in OnRunStart using hex.EncodeToString enhances clarity.


50-57: Verify thread safety after removing the mutex in InitPrecompiles.

The mutex lock removal simplifies the code but raises concerns about thread safety if InitPrecompiles is called concurrently.

Verification successful

Thread safety is not compromised by removing the mutex in InitPrecompiles.

The function is called in a single-threaded context during application setup, which does not indicate any concurrent execution that would require synchronization.

  • app/app.go: InitPrecompiles is called without evidence of concurrent execution.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the thread safety of `InitPrecompiles` after mutex removal.

# Test: Search for concurrent calls to `InitPrecompiles`. Expect: No concurrent calls without external synchronization.
rg --type go 'InitPrecompiles' -A 5

Length of output: 2705

x/evm/vmtracer.go (2)

Line range hint 24-34:
LGTM! Changes in NewTracer are efficient.

The use of a pointer for core.Message in NewTracer improves performance by avoiding unnecessary copies.


89-89: LGTM! CaptureEnd simplification is appropriate.

The removal of the tm parameter in CaptureEnd streamlines the method signature without affecting functionality.

x/evm/keeper/keeper.go (2)

137-137: Enhance the tracer with block time context.

The addition of uint64(ctx.BlockTime().Unix()) provides valuable context for tracing operations, reflecting the block time. Ensure that this change is compatible with the tracer's expected parameters.


135-137: Consider the impact of changing msg to a pointer.

The change from core.Message to *core.Message can improve performance by avoiding unnecessary copying. However, ensure that all usages of this function handle the pointer correctly to avoid nil pointer dereferences.

contrib/make/build.mk (1)

73-73: Review linker flags for non-Darwin systems.

The change to use ifneq for non-Darwin systems adds static linking flags. Verify that these flags are appropriate for the target environments and do not introduce compatibility issues.

x/evm/keeper/gas_fees.go (2)

40-40: Update field access to reflect structural changes.

The shift from msg.From().Bytes() to msg.From.Bytes() suggests structural changes in the core.Message type. Ensure that these changes are consistent across the codebase.


136-136: Review changes to intrinsic gas calculation.

The modification of parameters in core.IntrinsicGas suggests changes in gas calculation logic. Ensure that this aligns with the intended gas handling strategy.

x/evm/json_tx_args.go (3)

13-14: Import alias change improves clarity.

The change from geth to gethcore for github.com/ethereum/go-ethereum/core/types enhances readability by clearly distinguishing core types from other Ethereum-related types.


41-42: AccessList type change aligns with import aliasing.

The change to *gethcore.AccessList for the AccessList field in JsonTxArgs maintains consistency with the updated import aliasing.


Line range hint 161-240: Return type change to pointer is idiomatic.

The change to return a pointer to core.Message and returning nil on error is more idiomatic in Go, improving memory management and error handling.

x/evm/keeper/erc20.go (1)

193-204: Direct struct initialization improves clarity.

The change to use direct struct initialization for evmMsg enhances code readability and maintainability by explicitly setting the fields of the core.Message struct.

go.mod (2)

Line range hint 18-211: Dependency updates enhance compatibility and security.

The updates to dependencies, including go-ethereum, golang-set, go-ole, go-stack, snappy, go-sysconf, and numcpus, improve compatibility, security, and performance by aligning with the latest stable versions.


249-249: Replacement of go-ethereum is appropriate.

The replacement of go-ethereum with NibiruChain/go-ethereum at version v1.11.6-nibiru is necessary for integrating specific patches and enhancements.

x/evm/msg.go (1)

316-317: LGTM! But verify the impact of returning a pointer.

Returning a pointer to core.Message can improve performance by avoiding unnecessary copying. Ensure that this change does not introduce any unintended side effects in the rest of the codebase.

x/evm/statedb/statedb.go (1)

501-526: Enhance transaction preparation with access list handling.

The updates to the Prepare method improve its functionality by comprehensively handling access lists. This ensures all relevant addresses and keys are accounted for before transaction execution.

Verification successful

Verification of Prepare function integration is successful.

The Prepare function is well-integrated into the codebase, with test coverage in x/evm/statedb/statedb_test.go ensuring its functionality. The changes appear to be correctly implemented and tested.

  • Test Coverage: The function is tested in x/evm/statedb/statedb_test.go, verifying address handling in the access list.
  • Separation of Concerns: PrepareAccessList in x/evm/keeper/msg_server.go suggests a clear separation of preparation steps.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `Prepare` in the codebase to ensure the changes are correctly integrated.

# Test: Search for the function usage. Expect: All usages correctly handle the updated access list logic.
rg --type go -A 5 $'Prepare'

Length of output: 2913

x/evm/statedb/statedb_test.go (2)

28-28: Introduce coinbase for enhanced test scenarios.

The addition of the coinbase variable allows tests to simulate scenarios involving a predefined coinbase address effectively.


45-45: Optimize CollectContractStorage with pointer receiver.

Changing the parameter to a pointer improves performance by avoiding unnecessary copying and allows direct modification of the StateDB instance.

x/evm/msg_test.go (1)

70-70: Verify the change from msg.To() to msg.To.

The shift from using a method call to a direct field reference indicates a structural change in the msg object. Ensure that this change aligns with the updated structure and that msg.To is correctly initialized.

x/evm/keeper/grpc_query.go (5)

382-382: Ensure msg.GasLimit is correctly set.

The direct assignment to msg.GasLimit improves clarity, but verify that this assignment is consistent with the rest of the codebase and aligns with the intended logic.


495-495: Check the use of core.TransactionToMessage.

The use of core.TransactionToMessage standardizes message conversion. Ensure that this function is used consistently and correctly throughout the codebase.

Verification successful

The use of core.TransactionToMessage is consistent and correct across the codebase. The function is used with the same parameters (transaction, signer, base fee) in all instances, ensuring standardized message conversion. No inconsistencies or misuse were found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `core.TransactionToMessage` across the codebase.

# Test: Search for the usage of `core.TransactionToMessage` to ensure consistent application.
rg --type go $'core.TransactionToMessage'

Length of output: 408


Script:

#!/bin/bash
# Extract surrounding lines of code for each usage of `core.TransactionToMessage` to understand the context.

# Get context around each usage of `core.TransactionToMessage`
rg --type go --context 3 'core.TransactionToMessage' 

Length of output: 1804


636-636: Review the conversion to core.Message.

The conversion to core.Message in TraceEthTxMsg needs to be verified for correctness. Ensure that the conversion aligns with the expected input for core.TransactionToMessage.


694-694: Confirm gas meter setup.

The setup of the gas meter with eth.NewInfiniteGasMeterWithLimit should be consistent with the intended gas tracking logic. Verify that this setup is correctly implemented.

Verification successful

Gas meter setup is consistent with intended logic.

The use of eth.NewInfiniteGasMeterWithLimit in x/evm/keeper/grpc_query.go aligns with the intended logic of tracking gas usage without enforcing the limit. This setup is appropriate for the context in which it is used.

  • File: x/evm/keeper/grpc_query.go
  • Function Definition: Found in eth/gas_limit.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the setup of the gas meter with `eth.NewInfiniteGasMeterWithLimit`.

# Test: Search for the usage of `eth.NewInfiniteGasMeterWithLimit` to ensure correct implementation.
rg --type go $'eth.NewInfiniteGasMeterWithLimit'

Length of output: 990


Script:

#!/bin/bash
# Search for the definition of `NewInfiniteGasMeterWithLimit` in the `eth` package to understand its implementation.
ast-grep --lang go --pattern 'func NewInfiniteGasMeterWithLimit($_) { $$$ }'

Length of output: 78


Script:

#!/bin/bash
# Use ripgrep to search for any mention of `NewInfiniteGasMeterWithLimit` to find its definition or documentation.
rg 'NewInfiniteGasMeterWithLimit'

Length of output: 1319


664-664: Validate tracer initialization.

Ensure that the tracer is correctly initialized and configured according to the provided traceConfig. This is crucial for accurate transaction tracing.

x/evm/keeper/msg_server.go (8)

32-32: Variable renaming improves clarity.

The renaming of msg to msgEthTx enhances readability. Ensure that all references to this variable have been updated accordingly.


216-216: Pointer usage for core.Message improves performance.

The change to use a pointer for core.Message enhances performance and memory management. Ensure that this change is consistently applied.


358-358: Confirm leftover gas calculation.

The calculation of leftoverGas should be verified to ensure it correctly reflects the gas used during transaction processing.

Verification successful

Leftover Gas Calculation Verified

The calculation of leftoverGas correctly reflects the gas used during transaction processing. It is initialized with msg.GasLimit, adjusted for intrinsic gas, modified during transaction execution, and recalculated with refunds. The logic ensures it does not exceed the initial gas limit. No issues found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the calculation of `leftoverGas`.

# Test: Search for the logic around `leftoverGas` to ensure it is calculated correctly.
rg --type go $'leftoverGas'

Length of output: 1569


182-183: Ensure gas refund logic is correct.

The logic for refunding gas needs to be verified for correctness. Ensure that the refund aligns with the intended gas consumption and refund policies.

Verification successful

Gas refund logic is correctly implemented.

The RefundGas function ensures that only positive leftover gas amounts are refunded to the sender. It handles potential errors and refunds from the fee collector module account. The logic aligns with standard gas refund practices. No issues found with the current implementation.

  • x/evm/keeper/gas_fees.go: RefundGas function handles refunds based on leftover gas and ensures no negative refunds occur.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the gas refund logic.

# Test: Search for the implementation of `RefundGas` to ensure it aligns with gas refund policies.
rg --type go $'RefundGas'

Length of output: 1338


Script:

#!/bin/bash
# Extract the implementation of the RefundGas function to verify its logic.
ast-grep --lang go --pattern $'func (k *Keeper) RefundGas(ctx sdk.Context, msg *core.Message, leftoverGas uint64, denom string) error {
  $$$
}' x/evm/keeper/gas_fees.go

Length of output: 2000


442-456: Check gas usage and refund calculations.

The calculations for gas usage and refunds should be verified for accuracy. Ensure that they align with Ethereum's gas policies.

Verification successful

Gas Usage and Refund Calculations Verified

The GasToRefund function implements a standard gas refund mechanism by dividing consumed gas by a refund quotient and capping it at the available refund. Its usage in msg_server.go correctly updates the gas values, ensuring accurate calculations. No issues were found with the current implementation.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the calculations for gas usage and refunds.

# Test: Search for the logic around gas usage and refunds to ensure accuracy.
rg --type go $'GasToRefund'

Length of output: 357


Script:

#!/bin/bash
# Search for the implementation of the GasToRefund function in gas_fees.go.
ast-grep --lang go --pattern 'func GasToRefund($_, $_, $_) $_ { $$$ }'

# Search for the usage of GasToRefund in msg_server.go to understand its application context.
rg --type go 'GasToRefund' -A 5

Length of output: 1069


110-110: Verify message conversion to core.Message.

The conversion of tx to core.Message should be checked for correctness. Ensure that the conversion logic aligns with the expected input for core.TransactionToMessage.


344-344: Review ApplyEvmMsg logic.

The logic within ApplyEvmMsg should be thoroughly reviewed to ensure it aligns with the intended transaction processing flow.


138-139: Check contract address creation logic.

The logic for creating a contract address should be verified to ensure it aligns with Ethereum standards.

CHANGELOG.md (1)

110-110: Changelog entry is well-structured and consistent.

The entry for the Geth version upgrade to v1.11.6 is correctly formatted and aligns with the PR objectives. The use of the "feat" label and the exclamation mark for a breaking change is appropriate.

Comment on lines +492 to +494
func (s *StateDB) GetTransientState(addr common.Address, key common.Hash) common.Hash {
// TODO(k-yang): implement
return common.Hash{}
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 16, 2024

Choose a reason for hiding this comment

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

Implement GetTransientState.

This method is currently a placeholder. Implementing it will be crucial for managing transient state effectively.

Would you like me to help with the implementation or open a GitHub issue to track this task?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please help me with the implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the search results, it appears that transient state management is a concept used across various parts of the codebase, particularly in the context of gas calculation and temporary storage for Ethereum transactions. However, there is no specific implementation detail for transient storage directly within the StateDB class.

To implement the GetTransientState function, we need to consider how transient state is intended to be stored and accessed. Since the StateDB class currently lacks a defined structure for transient storage, we can propose a basic implementation that uses a map to simulate transient state storage. This will allow retrieval of state values associated with a given address and key.

Here is a proposed implementation for the GetTransientState function:

func (s *StateDB) GetTransientState(addr common.Address, key common.Hash) common.Hash {
	// Assuming transientStorage is a map[common.Address]map[common.Hash]common.Hash
	if s.transientStorage == nil {
		return common.Hash{}
	}
	if state, exists := s.transientStorage[addr]; exists {
		if value, found := state[key]; found {
			return value
		}
	}
	return common.Hash{}
}

This implementation assumes the existence of a transientStorage map within the StateDB struct. If such a map does not exist, it would need to be added to the StateDB struct to support transient state operations.

Let me know if you would like to proceed with this implementation or if there's anything else you'd like to adjust.

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for references to transient state management in the codebase.
rg "transient" -A 5

Length of output: 6471

Comment on lines +497 to +499
func (s *StateDB) SetTransientState(addr common.Address, key, value common.Hash) {
// TODO(k-yang): implement
}
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 16, 2024

Choose a reason for hiding this comment

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

Implement SetTransientState.

This method is currently a placeholder. Implementing it will be crucial for managing transient state effectively.

Would you like me to help with the implementation or open a GitHub issue to track this task?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please help me with the implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

@k-yang, to implement the SetTransientState function, we can add a transient storage mechanism to the StateDB struct. Here's how you can do it:

  1. Add a Transient Storage Map: Add a map to StateDB to store transient states.

  2. Implement SetTransientState: Update the function to set values in the transient storage map.

Here's an example implementation:

// Add this field to the StateDB struct
transientStorage map[common.Address]map[common.Hash]common.Hash

// Initialize the transient storage in the New function
func New(ctx sdk.Context, keeper Keeper, txConfig TxConfig) *StateDB {
	return &StateDB{
		keeper:           keeper,
		ctx:              ctx,
		stateObjects:     make(map[common.Address]*stateObject),
		journal:          newJournal(),
		accessList:       newAccessList(),
		txConfig:         txConfig,
		transientStorage: make(map[common.Address]map[common.Hash]common.Hash),
	}
}

// Implement the SetTransientState function
func (s *StateDB) SetTransientState(addr common.Address, key, value common.Hash) {
	if _, exists := s.transientStorage[addr]; !exists {
		s.transientStorage[addr] = make(map[common.Hash]common.Hash)
	}
	s.transientStorage[addr][key] = value
}

This implementation adds a transientStorage map to the StateDB struct, which maps addresses to another map of key-value pairs. The SetTransientState function initializes the map for an address if it doesn't exist and sets the value for the given key.

If you need further assistance or adjustments, feel free to ask!

go.mod Outdated Show resolved Hide resolved
x/evm/keeper/erc20.go Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Not an issue from your PR, but I think we should add more specific tests for RefundGas to make sure it's not over-sending with the remaining value. It's not clear to me whether that value's meant to be in wei like msg.Value or if it's guaranteed to be micronibi.

Co-authored-by: Unique Divine <[email protected]>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 042f27e and 3fe15a4.

Files selected for processing (1)
  • go.mod (7 hunks)
Files skipped from review as they are similar to previous changes (1)
  • go.mod

@k-yang k-yang marked this pull request as draft August 19, 2024 17:48
@k-yang
Copy link
Member Author

k-yang commented Aug 19, 2024

Put on hold due to dependency issue outlined in this Slack thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x: evm Relates to Nibiru EVM or the EVM Module
Projects
Status: 🧱 Blocked
Development

Successfully merging this pull request may close these issues.

2 participants