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

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

fjl · 2025-02-19T10:25:51Z

core/types/transaction.go

+}
+
+// IntrinsicGas computes the 'intrinsic gas' for a message with the given data.
+func IntrinsicGas(txdata TxData, rules *params.Rules) (uint64, error) {


It's annoying to have this function return an error. The only cases where it could return an error is for very large data, which is highly unlikely in practice. It should panic in these cases instead.

The function IntrinsicGas is supposed to be used when construction a new transaction. If we want to be even more safe, we could keep the error-returning variant as an internal function for use in Transaction.IntrinsicGas, since that one is used to validate transactions received over p2p.

…ated computation bytes count algo

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.

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 added 3 commits February 18, 2025 22:34

core/types: add method to compute transaction intrinsic gas

13d902a

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

ac10896

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, cores/types: added Gas field in Message, precomputed it and upd…

cb3dddc

…ated computation bytes count algo

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

fix transaction tests

f7c61f9

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cedrickah commented Feb 1, 2025

rjl493456442 commented Feb 5, 2025

kaber2 Feb 5, 2025

lightclient Feb 18, 2025

cedrickah commented Feb 6, 2025

fjl commented Feb 6, 2025

cedrickah commented Feb 6, 2025 •

edited

Loading

cedrickah commented Feb 7, 2025

fjl Feb 8, 2025

cedrickah Feb 8, 2025

fjl Feb 8, 2025 •

edited

Loading

cedrickah Feb 8, 2025 •

edited

Loading

lightclient left a comment

lightclient Feb 18, 2025

cedrickah Feb 18, 2025

lightclient Feb 18, 2025

fjl Feb 18, 2025

fjl Feb 18, 2025

lightclient Feb 18, 2025

cedrickah Feb 18, 2025

cedrickah Feb 22, 2025 •

edited

Loading

lightclient Feb 18, 2025

lightclient Feb 18, 2025

fjl Feb 19, 2025

s1na commented Feb 20, 2025

cedrickah commented Feb 20, 2025

cedrickah commented Feb 23, 2025

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

Choose a reason for hiding this comment

s1na commented Feb 20, 2025

cedrickah commented Feb 20, 2025

cedrickah commented Feb 23, 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