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.

Sign up for GitHub

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

Jump to bottom

TransactionTests Update: ttGasLimit #955

Merged
merged 4 commits into from
Oct 15, 2021
Merged

TransactionTests Update: ttGasLimit #955

merged 4 commits into from
Oct 15, 2021

Conversation

marioevz
Copy link
Member

Contains updated TransactionTests for category ttGasLimit.
Outstanding issues:

  • Tests TransactionWithHighGas and TransactionWithHihghGasLimit63m1 expect the transaction to be rejected with GasLimit 0x7fffffffffffffff, but they pass, and this seems expected since the uint64 should allow for this value. Please confirm and I can change the expected result to pass.

winsvega
winsvega reviewed Oct 12, 2021
View reviewed changes
Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

I have some questions
and need a change

src/TransactionTestsFiller/ttGasLimit/TransactionWithHihghGasLimit63m1Filler.json
"expect" : [
{
"network" : ["EIP158", "Byzantium", ">=Constantinople"],
"result" : "valid"
Copy link
Collaborator

Choose a reason for hiding this comment

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

was gasLimit cap a retroactive change ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure why the original test contained selective forks, the uint64 limit applies to all forks it seems.

src/TransactionTestsFiller/ttGasLimit/TransactionWithHighGasFiller.json Outdated
"transaction" :
{
"data" : "",
"gasLimit" : "0x7fffffffffffffff",
"gasLimit" : "0x7fffffffffffffff",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test is identical to TransactionWithHihghGasLimit63m1Filler.json
better change to x, x-1, x+1(if x not fails) where x is gasLimit cap (pow(2,63)-1)

src/TransactionTestsFiller/ttGasLimit/TransactionWithGasLimitxPriceOverflowFiller.json
"transaction" :
{
"data" : "",
"gasLimit" : "115792089237316195423570985008687907853269984665640564039457584007913129639935",
Copy link
Collaborator

Choose a reason for hiding this comment

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

the goal of this test is to make gasLimit * gasPrice > 2**256-1, need to change. is there a cap on gasPrice?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am changing this test to be gasLimit=0xffffffffffffffff, gasPrice=0x1000000000000000100000000000000010000000000000002, where gasLimit is the highest uint64 value, and gasPrice is the lowest value such as the product is equal/higher than a 256bit value.
The test still does not produce an exception.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well the transaction technically valid.
Just that irl no account with enough balance exists.

Is int64max a limit of gasLimit field?

Copy link
Member Author

@marioevz marioevz Oct 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it's unsigned so uint64 and 0xffffffffffffffff is the max.
I agree that the transaction is valid, I think is a good test to have just to see that no client does something funny when receiving the transaction.
Should I add a state test similar to the one I added for gasPrice in the other PR ?
I just checked the test case and is exactly this scenario, so no need to add another state test...

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the exception? GasLimitPriceProductOverflow

src/TransactionTestsFiller/ttGasLimit/TransactionWithGasLimitOverflowZeros64Filler.json Show resolved Hide resolved
src/TransactionTestsFiller/ttGasLimit/TransactionWithGasLimitOverflow63Filler.json Outdated
"transaction" :
{
"data" : "",
"gasLimit" : "0xffffffffffffffff",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not 8000000 ?
why it passes, it overflows ?

@winsvega winsvega marked this pull request as draft October 12, 2021 20:22
winsvega
winsvega requested changes Oct 12, 2021
View reviewed changes
Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

some exceptions are strange need to double chek

@winsvega winsvega marked this pull request as ready for review October 12, 2021 20:24
@marioevz
Copy link
Member Author

marioevz commented Oct 13, 2021 •
edited
Loading

The limit is uint64, I will be renaming all tests which do not overflow (< 2^64) to HighGas instead of Overflow, to make it easier to identify them.
I am not sure why the tests originally expected 2^63 to overflow, but this should not be the case.
I am also removing repeated tests, TransactionWithHihghGasLimit63m1 for example only fails because V=37, but I am not sure what was the original goal of this specific test case.
Will upload a new commit soon.

winsvega
winsvega requested changes Oct 15, 2021
View reviewed changes
Copy link
Collaborator

@winsvega winsvega left a comment

Choose a reason for hiding this comment

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

naming is wrong. did you change the names or is it a github issue?

src/TransactionTestsFiller/ttGasLimit/TransactionWithHighGasLimit65Minus1Filler.json Outdated
"transaction" :
{
"data" : "",
"gasLimit" : "0xffffffffffffffff",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is 64-1

src/TransactionTestsFiller/ttGasLimit/TransactionWithHighGasLimit64Plus1Filler.json Outdated
"transaction" :
{
"data" : "",
"gasLimit" : "0x8000000000000001",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is 63+1

src/TransactionTestsFiller/ttGasLimit/TransactionWithHighGasLimit63Filler.json Outdated
"transaction" :
{
"data" : "",
"gasLimit" : "0x7fffffffffffffff",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is 63-1

src/TransactionTestsFiller/ttGasLimit/TransactionWithGasLimitxPriceOverflowFiller.json
"transaction" :
{
"data" : "",
"gasLimit" : "115792089237316195423570985008687907853269984665640564039457584007913129639935",
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the exception? GasLimitPriceProductOverflow

src/TransactionTestsFiller/ttGasLimit/TransactionWithGasLimitOverflow65Filler.json Outdated
"transaction" :
{
"data" : "",
"gasLimit" : "0x10000000000000000",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is 64

@marioevz
Copy link
Member Author

@winsvega I changed the naming to reference the number of bits instead of powers of two. It's a mistake, I will change it back.

@marioevz
Copy link
Member Author

Fixed naming and TransactionWithGasLimitxPriceOverflow test now correctly fails with GasLimitPriceProductOverflow (gas * gasPrice exceeds 256 bits).

@winsvega winsvega merged commit 0e95f0f into develop Oct 15, 2021
@winsvega winsvega deleted the txUpdateGasLimit branch October 15, 2021 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holiman holiman holiman left review comments

@winsvega winsvega winsvega requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marioevz @holiman @winsvega