core/types: add method to compute transaction intrinsic gas #31109

cedrickah · 2025-02-01T13:03:23Z

Resolves: #31058

Added an IntrinsicGas() method in core/types to compute transaction intrinsic gas.

rjl493456442 · 2025-02-05T07:04:03Z

Please use this method to prevent potential future expansion of the IntrinsicGas function in the core package.

kaber2 · 2025-02-05T20:51:19Z

core/types/transaction.go

+			if byt != 0 {
+				nz++
+			}
+		}


While you're at it, you might as well change the loop to:

z := uint64(bytes.Count(data, []byte{0})) nz := uint64(len(data)) - z

bytes.Count() uses an optimized assembly implementation to count bytes.

cedrickah · 2025-02-06T07:25:51Z

Please use this method to prevent potential future expansion of the IntrinsicGas function in the core package.

Hi @rjl493456442. Please, can you give me more details on what you mean?

fjl · 2025-02-06T17:35:05Z

He means the function core.IntrinsicGas should be removed.

cedrickah · 2025-02-06T17:55:02Z

He means the function core.IntrinsicGas should be removed.

Okay. I will push the changes.

cedrickah · 2025-02-07T19:56:07Z

Please use this method to prevent potential future expansion of the IntrinsicGas function in the core package.

Hi @rjl493456442 Can you review the these changes please?

fjl · 2025-02-08T01:00:33Z

core/bench_test.go

+			Data:     data,
+			GasPrice: gasPrice,
+		})
+		gas, _ := tx0.IntrinsicGas(&rules)


Repeating the tx construction here is not great, and hightlights a shortcoming of having IntrinsicGas as a method of Transaction: you can't compute the intrinsic gas without creating the tx first, but it's often called to fill in the Gas of a new transaction. We need to think a bit harder how to solve this one. I have a hunch IntrinsicGas could be defined over types.TxData. But then we'd need to implement TxData in Transaction.

I think TxData is already in Transaction as the inner field.

OK, I've figured it out. What we need to do is, add a function IntrinsicGas(TxData, *params.Rules) to core/types. Keep the IntrinsicGas method on Transaction, and make it call this new function on tx.inner.

OK. I wonder if the IntrinsicGas function could not have this signature instead: func (data []byte, accessList types.AccessList, authList []types.SetCodeAuthorization, isContractCreation, rules *params.Rules) (uint64, error).
It allows to pass fields in tx.inner instead of tx.inner, we can then do without a variable of type TxData if necessary. However, review the modifications please.

lightclient

Left a few comments. Need to decide how to pass the txdata in state_transition.

Please rebase also when you can.

lightclient · 2025-02-18T01:24:54Z

core/state_transition.go

+		Gas:      msg.GasLimit,
+		Data:     msg.Data,
+		GasPrice: msg.GasPrice,
+	})


I think this isn't great.

By going tx -> msg -> back to tx we require ourselves to convert to msg correct and back correctly. TxData seems like the right thing to feed IntrinsicGas, but forcing stuff into a legacy tx when we don't exactly know the origin (or it isn't defined) isn't correct.

This currently fails to compute the intrinsic gas correctly for SetCode txs, because when converting from msg to a tx, the conversion is for a tx that does not have authorizations. Since SetCode seems like a perfect superset of all possible values which goes into InstrinsicGas, but if a new tx type comes along without auths and needs a new input to the IntrinsicGas computation, we'll have the same problem again.

Seems like there are two paths here:

add new struct like GenericTxData that implements TxData and use this in places where we don't know the tx type

add a new method on Message that infers the tx type and constructs the correct TxData

is probably less code, but adds a weird export. 2) is more code, but still has the problem occasionally in other places outside core like tests where we want to generically compute some intrinsic gas.

I think I lean towards 1) ?

Thanks for the review. I understand the problems posed. I'm going to look into the new GenericTxData structure. But I would also like to know if we should then put the IntrinsicGas method on TxData or leave it on Transaction.

I think it should be on Transaction like you have it.

I also think we can have it on both Transaction and as a standalone function.

Ahh, wait, I see the issue now. So we would need to implement TxData with Message somehow, but that's not possible because it's supposed to be a private interface. One thing we could do is making the intrinsic gas a part of Message as a field. Then we'd compute it earlier, like in ApplyTransaction. Just unsure how much of a ripple effect this has on other code.

Right. I kinda like this idea of precomputing earlier though.

It will then remove the need for GenericTxData struct.

Hello @fjl @lightclient. The effect is that the gas must be computed in Message before calling ApplyMessage wherever it is executed. Not just at the ApplyTransaction level. A method SetGas(tx types.TxData, rules *params.Rules) for Message can be useful for that.

lightclient · 2025-02-18T01:28:08Z

core/types/transaction.go

+			if byt != 0 {
+				nz++
+			}
+		}


lightclient · 2025-02-18T01:29:21Z

core/types/transaction.go

+		data               = txdata.data()
+		accessList         = txdata.accessList()
+		authList           = txdata.setCodeAuthorizations()


I think these can be removed and use the txdata getter directly

core/types/transaction.go

s1na · 2025-02-20T15:31:19Z

@cedrickah You force-pushed over my commit. Git must have warned about un-synced commits which you probably ignored. Please double check before force pushing.

I have now pushed it again. It fixes the TransactionTest in tests/. I meant to write a comment so you're aware but somehow it got away from me.

Btw after the latest refactor now many more tests are failing. Please look into them. You can see them on appveyor: https://ci.appveyor.com/project/ethereum/go-ethereum/builds/51553192 (it started another run now so wait a bit).

cedrickah · 2025-02-20T19:25:11Z

@cedrickah You force-pushed over my commit. Git must have warned about un-synced commits which you probably ignored. Please double check before force pushing.

I have now pushed it again. It fixes the TransactionTest in tests/. I meant to write a comment so you're aware but somehow it got away from me.

Btw after the latest refactor now many more tests are failing. Please look into them. You can see them on appveyor: https://ci.appveyor.com/project/ethereum/go-ethereum/builds/51553192 (it started another run now so wait a bit).

Ooh sorry. I'll be more careful. I'm leaning towards tests.

cedrickah · 2025-02-23T05:23:05Z

@cedrickah You force-pushed over my commit. Git must have warned about un-synced commits which you probably ignored. Please double check before force pushing.

I have now pushed it again. It fixes the TransactionTest in tests/. I meant to write a comment so you're aware but somehow it got away from me.

Btw after the latest refactor now many more tests are failing. Please look into them. You can see them on appveyor: https://ci.appveyor.com/project/ethereum/go-ethereum/builds/51553192 (it started another run now so wait a bit).

Hello @s1na . I saw the logs. As we are trying to computer gas in Message, the errors occured because ApplyMessage is called before the gas is precomputed in Mesage at locations. I'll take care of it.

lightclient · 2025-03-12T20:42:40Z

I moved the computation of the intrinsic gas from ApplyTransactionWithEVM(..) to TransactionToMessage(..). This makes it clearer that the tx -> msg computation is fork aware (added Rules to the parameter list) and avoids situations where the caller forgets to set the IntrinsicGas value of the Message.

There are some places in the code base like the RPC and some tests where there is the concept of untyped "generic" transaction. I considered adding an intrinsic gas calculator where the caller can directly pass in the relevant fields like to, data, etc, but decided to not have a publicly exported function that we know will continue to be unstable. So I am forcing the data into either a dynamic fee tx type or a set code tx type then running the computation.

lightclient

Pushed some changes, LGTM.

lightclient · 2025-03-12T21:01:19Z

Will wait for @fjl to approve this since I know he cares about API implications of this PR.

cedrickah · 2025-03-12T21:15:03Z

I moved the computation of the intrinsic gas from ApplyTransactionWithEVM(..) to TransactionToMessage(..). This makes it clearer that the tx -> msg computation is fork aware (added Rules to the parameter list) and avoids situations where the caller forgets to set the IntrinsicGas value of the Message.

There are some places in the code base like the RPC and some tests where there is the concept of untyped "generic" transaction. I considered adding an intrinsic gas calculator where the caller can directly pass in the relevant fields like to, data, etc, but decided to not have a publicly exported function that we know will continue to be unstable. So I am forcing the data into either a dynamic fee tx type or a set code tx type then running the computation.

The effect of pre-computation was quite significant, I felt it was necessary to get your opinion on some points on the best approach for you to avoid situations where the caller forgets to set the IntrinsicGas value of the message.

core/state_transition.go

core/types/transaction.go

MariusVanDerWijden · 2025-03-13T09:41:31Z

eth/tracers/api.go

@@ -269,10 +269,11 @@ func (api *API) traceChain(start, end *types.Block, config *TraceConfig, closed
 				var (
 					signer   = types.MakeSigner(api.backend.ChainConfig(), task.block.Number(), task.block.Time())
 					blockCtx = core.NewEVMBlockContext(task.block.Header(), api.chainContext(ctx), nil)
+					rules    = api.backend.ChainConfig().Rules(task.block.Number(), task.block.Difficulty().BitLen() == 0, task.block.Time())


I think we should do something like this. Everywhere else in the codebase we use random != nil to determine the post-merge blocks

Suggested change

rules = api.backend.ChainConfig().Rules(task.block.Number(), task.block.Difficulty().BitLen() == 0, task.block.Time())

rules = api.backend.ChainConfig().Rules(blockCtx.BlockNumber, blockCtx.Random != nil, blockCtx.Time)

I think we can change to Random != nil where it makes sense, but there are some cases where it is not easy to e.g. traceBlockParallel where the block context is not safe for concurrent access and gets redefined each iteration. Yeah we can usually construct a block context that is only used for this purpose, but under the hood it also is just checking if Difficultly is zero under the hood.

Maybe it makes sense to create another function RulesFromHeader or a helper IsPostMerge. But I think creating block contexts just to check if it is post merge is unnecessary.

You can see that we do check Difficulty.Sign() != 0 in a few places already with:

grep -r "Difficulty\.Sign" .

MariusVanDerWijden · 2025-03-13T09:47:52Z

eth/tracers/api.go

@@ -652,6 +657,7 @@ func (api *API) traceBlockParallel(ctx context.Context, block *types.Block, stat
 		signer    = types.MakeSigner(api.backend.ChainConfig(), block.Number(), block.Time())
 		results   = make([]*txTraceResult, len(txs))
 		pend      sync.WaitGroup
+		rules     = api.backend.ChainConfig().Rules(block.Number(), block.Difficulty().BitLen() == 0, block.Time())


Same here, we should stay consistent and use blockCtx.Random != nil (and move blockCtx up here)

MariusVanDerWijden · 2025-03-13T09:50:03Z

eth/tracers/api.go

@@ -883,7 +892,8 @@ func (api *API) TraceTransaction(ctx context.Context, hash common.Hash, config *
 		return nil, err
 	}
 	defer release()
-	msg, err := core.TransactionToMessage(tx, types.MakeSigner(api.backend.ChainConfig(), block.Number(), block.Time()), block.BaseFee())
+	rules := api.backend.ChainConfig().Rules(block.Number(), block.Difficulty().BitLen() == 0, block.Time())


here we can use vmctx.Random != nil

MariusVanDerWijden · 2025-03-13T09:50:44Z

eth/tracers/api.go

@@ -962,7 +972,8 @@ func (api *API) TraceCall(ctx context.Context, args ethapi.TransactionArgs, bloc
 		return nil, err
 	}
 	var (
-		msg         = args.ToMessage(vmctx.BaseFee, true, true)
+		rules       = api.backend.ChainConfig().Rules(block.Number(), block.Difficulty().BitLen() == 0, block.Time())


Here vmctx.Random != nil

MariusVanDerWijden · 2025-03-13T09:51:30Z

internal/ethapi/api.go

@@ -693,7 +693,8 @@ func applyMessage(ctx context.Context, b Backend, args TransactionArgs, state *s
 	if err := args.CallDefaults(gp.Gas(), blockContext.BaseFee, b.ChainConfig().ChainID); err != nil {
 		return nil, err
 	}
-	msg := args.ToMessage(header.BaseFee, skipChecks, skipChecks)
+	rules := b.ChainConfig().Rules(header.Number, header.Difficulty.BitLen() == 0, header.Time)


blockContext.Random != nil

MariusVanDerWijden · 2025-03-13T09:56:03Z

internal/ethapi/api.go

@@ -837,7 +838,8 @@ func DoEstimateGas(ctx context.Context, b Backend, args TransactionArgs, blockNr
 	if err := args.CallDefaults(gasCap, header.BaseFee, b.ChainConfig().ChainID); err != nil {
 		return 0, err
 	}
-	call := args.ToMessage(header.BaseFee, true, true)
+	rules := b.ChainConfig().Rules(header.Number, header.Difficulty.BitLen() == 0, header.Time)


Maybe something like

evmContext = core.NewEVMBlockContext(opts.Header, opts.Chain, nil) rules := b.ChainConfig().Rules(header.Number, evmContext.Random != nil, header.Time= ```

pick 9798c84 core/types: add method to compute transaction intrinsic gas pick 2c620ab core: replace core.IntrinsicGas() with types.IntrinsicGas() fixup 0aef175 removed unused commented code pick 989d560 core/types: add IntrinsicGas() function to compute tx intrinsic gas

…ated computation bytes count algo

lightclient · 2025-03-21T05:05:57Z

rebased

cedrickah requested review from holiman and rjl493456442 as code owners February 1, 2025 13:03

kaber2 reviewed Feb 5, 2025

View reviewed changes

cedrickah requested review from MariusVanDerWijden and lightclient as code owners February 7, 2025 19:51

fjl reviewed Feb 8, 2025

View reviewed changes

fjl added this to the 1.15.1 milestone Feb 11, 2025

fjl assigned fjl and lightclient Feb 11, 2025

fjl modified the milestones: 1.15.1, 1.15.2, 1.15.3 Feb 13, 2025

lightclient requested changes Feb 18, 2025

View reviewed changes

cedrickah force-pushed the feature/add-compute-tx-intrinsic-gas-api-in-core-types branch from 97a96f3 to ac10896 Compare February 18, 2025 22:01

fjl reviewed Feb 19, 2025

View reviewed changes

core/types/transaction.go Outdated Show resolved Hide resolved

cedrickah requested review from lightclient and fjl February 19, 2025 11:23

fjl modified the milestones: 1.15.3, 1.15.4, 1.15.5 Feb 25, 2025

fjl modified the milestones: 1.15.5, 1.15.6 Mar 5, 2025

lightclient force-pushed the feature/add-compute-tx-intrinsic-gas-api-in-core-types branch from f7c61f9 to 16e7133 Compare March 12, 2025 20:36

lightclient requested a review from s1na as a code owner March 12, 2025 20:36

lightclient previously approved these changes Mar 12, 2025

View reviewed changes

MariusVanDerWijden reviewed Mar 13, 2025

View reviewed changes

lightclient dismissed their stale review via bd93294 March 13, 2025 15:24

sambacha mentioned this pull request Mar 14, 2025

eth_estimateGas returns wrong gas, off by 1.5% #29726

Open

fjl removed this from the 1.15.6 milestone Mar 20, 2025

cedrickah and others added 9 commits March 20, 2025 19:36

core/types: add method to compute transaction intrinsic gas

90c234b

core/types: add IntrinsicGas() function to compute tx intrinsic gas

45c2c0b

core, cores/types: added Gas field in Message, precomputed it and upd…

47daf83

…ated computation bytes count algo

fix transaction tests

7dc8043

core: compute intrinsic gas in TransactionToMessage(..)

f1ded86

all: rename Gas to IntrinsicGas in message, fixup uses of Message

c0ca97f

core: few final cleanups

93de27d

all: review feedback from marius

0f28070

lightclient force-pushed the feature/add-compute-tx-intrinsic-gas-api-in-core-types branch from bd93294 to 0f28070 Compare March 21, 2025 05:05

tests: remove getRules

81d5d79

	rules = api.backend.ChainConfig().Rules(task.block.Number(), task.block.Difficulty().BitLen() == 0, task.block.Time())
	rules = api.backend.ChainConfig().Rules(blockCtx.BlockNumber, blockCtx.Random != nil, blockCtx.Time)

core/types: add method to compute transaction intrinsic gas #31109

Are you sure you want to change the base?

core/types: add method to compute transaction intrinsic gas #31109

Conversation

cedrickah commented Feb 1, 2025

rjl493456442 commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedrickah commented Feb 6, 2025

fjl commented Feb 6, 2025

cedrickah commented Feb 6, 2025 • edited Loading

cedrickah commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

cedrickah Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

lightclient left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedrickah Feb 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1na commented Feb 20, 2025

cedrickah commented Feb 20, 2025

cedrickah commented Feb 23, 2025

lightclient commented Mar 12, 2025

lightclient left a comment

Choose a reason for hiding this comment

lightclient commented Mar 12, 2025

cedrickah commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightclient commented Mar 21, 2025

cedrickah commented Feb 6, 2025 •

edited

Loading

fjl Feb 8, 2025 •

edited

Loading

cedrickah Feb 8, 2025 •

edited

Loading

cedrickah Feb 22, 2025 •

edited

Loading