-
Notifications
You must be signed in to change notification settings - Fork 23
Upgrade go-ethereum to 1.13 #116
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
base: feat/geth-1.13
Are you sure you want to change the base?
Conversation
// use Cosmos-SDK fork to enable Ledger functionality | ||
github.com/cosmos/cosmos-sdk => github.com/cosmos/cosmos-sdk v0.50.13-0.20250319183239-53dea340efc7 | ||
// use Cosmos geth fork | ||
github.com/ethereum/go-ethereum => github.com/cosmos/go-ethereum v1.10.26-evmos-rc4.0.20250402013457-cf9d288f0147 | ||
github.com/ethereum/go-ethereum => github.com/TacBuild/go-ethereum v0.0.0-20250428082551-b4f5a8f8420a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update this to point at https://github.com/cosmos/go-ethereum/releases/tag/v1.13.5-cosmos-rc0
@@ -137,6 +137,12 @@ if [[ $overwrite == "y" || $overwrite == "Y" ]]; then | |||
# Enable precompiles in EVM params | |||
jq '.app_state["evm"]["params"]["active_static_precompiles"]=["0x0000000000000000000000000000000000000100","0x0000000000000000000000000000000000000400","0x0000000000000000000000000000000000000800","0x0000000000000000000000000000000000000801","0x0000000000000000000000000000000000000802","0x0000000000000000000000000000000000000803","0x0000000000000000000000000000000000000804","0x0000000000000000000000000000000000000805"]' "$GENESIS" >"$TMP_GENESIS" && mv "$TMP_GENESIS" "$GENESIS" | |||
|
|||
# Set EVM config | |||
jq '.app_state["evm"]["params"]["chain_config"]["chain_id"]="262144"' "$GENESIS" >"$TMP_GENESIS" && mv "$TMP_GENESIS" "$GENESIS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use the $CHAINID
variable here?
|
||
// check that caller has enough balance to cover asset transfer for **topmost** call | ||
// NOTE: here the gas consumed is from the context with the infinite gas meter | ||
if msg.Value().Sign() > 0 && !evm.Context.CanTransfer(stateDB, msg.From(), msg.Value()) { | ||
if msg.Value.Sign() > 0 && !evm.Context.CanTransfer(stateDB, msg.From, uint256.MustFromBig(msg.Value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it actually safe to panic here? Seems like we want to return an error on failure.
@@ -75,7 +76,7 @@ func (p *Precompile) Transfer( | |||
// NOTE: This ensures that the changes in the bank keeper are correctly mirrored to the EVM stateDB | |||
// when calling the precompile from another smart contract. | |||
// This prevents the stateDB from overwriting the changed balance in the bank keeper when committing the EVM state. | |||
amt := msg.Token.Amount.BigInt() | |||
amt := uint256.MustFromBig(msg.Token.Amount.BigInt()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not sure if we should panic here ever.
@@ -242,7 +243,7 @@ func (p *Precompile) Delegate( | |||
|
|||
// Need to scale the amount to 18 decimals for the EVM balance change entry | |||
scaledAmt := evmtypes.ConvertAmountTo18DecimalsBigInt(msg.Amount.Amount.BigInt()) | |||
p.SetBalanceChangeEntries(cmn.NewBalanceChangeEntry(delHexAddr, scaledAmt, cmn.Sub)) | |||
p.SetBalanceChangeEntries(cmn.NewBalanceChangeEntry(delHexAddr, uint256.MustFromBig(scaledAmt), cmn.Sub)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
cosmosAddr := sdk.AccAddress(addr.Bytes()) | ||
|
||
// Get the balance via bank wrapper to convert it to 18 decimals if needed. | ||
coin := k.bankWrapper.GetBalance(ctx, cosmosAddr, types.GetEVMCoinDenom()) | ||
|
||
return coin.Amount.BigInt() | ||
return uint256.MustFromBig(coin.Amount.BigInt()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
ret, _, leftoverGas, vmErr = evm.Create(sender, msg.Data(), leftoverGas, msg.Value()) | ||
stateDB.SetNonce(sender.Address(), msg.Nonce()+1) | ||
stateDB.SetNonce(sender.Address(), msg.Nonce) | ||
ret, _, leftoverGas, vmErr = evm.Create(sender, msg.Data, leftoverGas, uint256.MustFromBig(msg.Value)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MustFromBig
--same as above
// DefaultAllowUnprotectedTxs rejects all unprotected txs (i.e false) | ||
DefaultAllowUnprotectedTxs = false | ||
// DefaultStaticPrecompiles defines the default active precompiles. | ||
DefaultStaticPrecompiles []string | ||
// DefaultExtraEIPs defines the default extra EIPs to be included. | ||
// On v15, EIP 3855 was enabled | ||
DefaultExtraEIPs = []int64{ | ||
3855, // NOTE: we suggest to enable EIP-3855 on all chains to support new Solidity versions >=v0.8.20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now default enabled so we don't need to add it to extra eips?
DefaultEVMDenom = "atest" | ||
// DefaultEVMChainID is the default value for the evm chain ID | ||
DefaultEVMChainID = "cosmos_262144-1" | ||
// DefaultEVMDecimals is the default value for the evm denom decimal precision | ||
DefaultEVMDecimals uint64 = 18 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this stuff is probably best specified via the chain itself--like evmd could use these variables, but other chains should just get these as defaults.
Hi, I’d like to ask a few questions regarding the upgrade to go-ethereum 1.15.x after this PR:
c.c. @zsystm |
@@ -188,8 +190,8 @@ func (k *Keeper) ApplyTransaction(ctx sdk.Context, tx *ethtypes.Transaction) (*t | |||
evmDenom := types.GetEVMCoinDenom() | |||
|
|||
// refund gas in order to match the Ethereum gas consumption instead of the default SDK one. | |||
if err = k.RefundGas(ctx, msg, msg.Gas()-res.GasUsed, evmDenom); err != nil { | |||
return nil, errorsmod.Wrapf(err, "failed to refund gas leftover gas to sender %s", msg.From()) | |||
if err = k.RefundGas(ctx, *msg, msg.GasLimit-res.GasUsed, evmDenom); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like msg.GasLimit - res.GasUsed
could underflow if GasUsed
> GasLimit
, resulting in a large wrapped uint64 value due to Go's unsigned arithmetic.
Might be safer to guard it like this:
remainingGas := uint64(0)
if msg.GasLimit > res.GasUsed {
remainingGas = msg.GasLimit - res.GasUsed
}
And then pass remainingGas into RefundGas(...).
What do you think?
// MigrateStore migrates the x/evm module state from the consensus version 5 to | ||
// version 6. Specifically, it migrates the geth chain configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// MigrateStore migrates the x/evm module state from the consensus version 5 to | |
// version 6. Specifically, it migrates the geth chain configuration | |
// MigrateStore migrates the x/evm module state from the consensus version 8 to | |
// version 9. Specifically, it migrates the geth chain configuration |
requires: cosmos/go-ethereum#2
closes: #115