Skip to content

Conversation

@mmrozek
Copy link
Contributor

@mmrozek mmrozek commented Aug 28, 2020

@mmrozek mmrozek requested a review from KonradStaniec August 28, 2020 09:27
@mmrozek mmrozek changed the base branch from master to phase/etc_forks August 28, 2020 09:28
@mmrozek mmrozek force-pushed the etcm-22-structured-definitions-for-net-gas-metering branch from 6e3b2d3 to 8029fef Compare August 28, 2020 09:32
("60016000556002600055", 1, 5812, 0),
("60016000556001600055", 1, 1612, 0),
("600160005560006000556001600055", 0, 40818, 19200),
("600060005560016000556000600055", 1, 10818, 19200)
Copy link
Contributor

Choose a reason for hiding this comment

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

I found out that geth has two additional test cases : https://github.com/ethereum/go-ethereum/blob/b9df7ecdc3d3685180ceb29665bab59e9f614da5/core/vm/gas_table_test.go#L76.

	{1, 2306, "0x6001600055", 2306, 0, ErrOutOfGas},                            // 1 -> 1 (2300 sentry + 2xPUSH)
	{1, 2307, "0x6001600055", 806, 0, nil},                                     // 1 -> 1 (2301 sentry + 2xPUSH)

Most probably testing some edge cases. Imo we should try to add them, as all this gass couting makes me nervous,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

val eip1283Enabled = isEip1283Enabled(ethFork)

if(eip2200Enabled && state.gas <= state.config.feeSchedule.G_callstipend){
state.withError(OutOfGas)
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually OutOfGas errors zeros caller remaining gas like state.copy(gas = 0).withError(OutOfGas). Do you think we should try to do the same here ? (maybe it worth checking parity of geth)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests from previous comment check that

@mmrozek mmrozek force-pushed the etcm-22-structured-definitions-for-net-gas-metering branch from 8029fef to 648855d Compare August 28, 2020 12:29
@mmrozek mmrozek merged commit 845350b into phase/etc_forks Aug 28, 2020
@mmrozek mmrozek deleted the etcm-22-structured-definitions-for-net-gas-metering branch August 28, 2020 14:03
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.

3 participants