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.

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

TransactionTests Update: ttGasPrice #956

Merged
merged 5 commits into from
Oct 15, 2021
Merged

TransactionTests Update: ttGasPrice #956

merged 5 commits into from
Oct 15, 2021

Conversation

marioevz
Copy link
Member

Contains updated TransactionTests for category ttGasPrice.
Outstanding issues:

  • The field seems to accept 256bit+ values.

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.

there must be an exception

"TransactionWithGasPriceOverflow" :
{
"expectException" :
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should trigger an exception as field is >256bits

@marioevz
Copy link
Member Author

Hi @holiman , it seems like this field accepts 256bit+ values, although it is impossible to make a valid transaction with such values, should t8ntool raise an exception?

@holiman
Copy link
Contributor

holiman commented Oct 13, 2021

it seems like this field accepts 256bit+ values, although it is impossible to make a valid transaction with such values, should t8ntool raise an exception?

Interesting. Yeah, whether t9n gives an exception or not is up for debate. I can go whichever way, really. t9n is mostly a tool to aid in test generation, not the actual test executor.

Having >256bit gasprice values is interesting -- e.g make a tx where the gas (21K) multiplied by gasPrice becomes something like 0x1000.....0030000, where the uppermost digit is outside of the 256 (or 64) bit boundary.

I think too high gasprices are not intrinsically invalid, just in that 'ghost area' on semi-validness which happens due to balances being capped at 256 bits.

@marioevz
Copy link
Member Author

Having >256bit gasprice values is interesting -- e.g make a tx where the gas (21K) multiplied by gasPrice becomes something like 0x1000.....0030000, where the uppermost digit is outside of the 256 (or 64) bit boundary.

I just tested this scenario and the transaction is correctly rejected with 'insufficient funds' exception, so it seems ok at least for this client.
I will include this state test case in this PR.

@winsvega
Copy link
Collaborator

Question here, @holiman also
Perhaps we should extend transaction test suite to also export the amount of gas such transaction is expected to eat (among with sender, and hash)

Any of the 256bit overflow in tr fields should trigger an exception as this is a transaction structure unit tests many devs are using for testing

@holiman
Copy link
Contributor

holiman commented Oct 14, 2021

Perhaps we should extend transaction test suite to also export the amount of gas such transaction is expected to eat (among with sender, and hash)

So you're saying that t9n should export something like gasUsage from seeing a transaction? I don't see how that could possibly work, since t9n doesn't actually execute anything on a prestate. It's just the basic tx validations. In order to know how much gas is spent, it needs to be actually executed.

Any of the 256bit overflow in tr fields should trigger an exception as this is a transaction structure unit tests many devs are using for testing

Sure, can do that

@holiman
Copy link
Contributor

holiman commented Oct 15, 2021

Which tx has gasprice exceeding 256 bits? The one in HighGasPrice:

$ rlpdump -hex 0xf87e809f031eea408f8e1799cb883da2927b1336521d73c2c14accfebb70d5c5af006c82520894d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d001801ca039b081ab7094dff1b3ac79cbf8e381adc9f7a4e16290d7abc42dd006e5e062c5a033af00e26e5eb4431dcad601b2c8bf12d51eef2bd037d14681f22692ffab1ccd
[
  "",
  031eea408f8e1799cb883da2927b1336521d73c2c14accfebb70d5c5af006c,

That's "only" 31 bytes.

@holiman
Copy link
Contributor

holiman commented Oct 15, 2021

( and TransactionWithGasPriceOverflow.json was deleted in this PR)

@marioevz
Copy link
Member Author

TransactionWithGasPriceOverflow.json has been added again. I deleted it the first time because, since it was expecting an error, it could not be filled. Now it passes with ethereum/go-ethereum#23743

@winsvega winsvega merged commit 1a5b3d2 into develop Oct 15, 2021
@winsvega winsvega deleted the txUpdateGasPrice branch October 15, 2021 16:48
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