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: ttValue #961

Merged
merged 6 commits into from
Oct 15, 2021
Merged

TransactionTests Update: ttValue #961

merged 6 commits into from
Oct 15, 2021

Conversation

marioevz
Copy link
Member

Contains updated TransactionTests for category ttValue.
Outstanding issues:

  • The field seems to accept 256bit+ values.

@marioevz
Copy link
Member Author

Hi @holiman, this field also allows values greater than 256bits. I added an state test for this, similar to the one added to the one suggested in #956.

@holiman
Copy link
Contributor

holiman commented Oct 14, 2021

The 'high value' one:

rlpdump -hex 0xf87f800182520894095e7baea6a6c7c4c2dfeb977efac326af552d87a0ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff801ba048b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353a01fffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804
[
  "",
  01,
  5208,
  095e7baea6a6c7c4c2dfeb977efac326af552d87,
  ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff,
  "",
  1b,
  48b55bfa915ac795c431978d8a6a992b628d557da5ff759b307d495a36649353,
  1fffd310ac743f371de3b9f7f9cb56c0b28ad43601b4ab949f53faa07bd2c804,
]

The value is ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff -- exactly 32 bytes/ 256 bits. .
Overflow:

rlpdump -hex f880806482520894d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0a1010000000000000000000000000000000000000000000000000000000000000001801ba0c16787a8e25e941d67691954642876c08f00996163ae7dfadbbfd6cd436f549da06180e5626cae31590f40641fe8f63734316c4bfeb4cdfab6714198c1044d2e28
[
  "",
  "d",
  5208,
  d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0,
  010000000000000000000000000000000000000000000000000000000000000001,
  "",
  1b,
  c16787a8e25e941d67691954642876c08f00996163ae7dfadbbfd6cd436f549d,
  6180e5626cae31590f40641fe8f63734316c4bfeb4cdfab6714198c1044d2e28,
]

Indeed, it's 33 bytes.

So you want t9n to issue an error on these?

@holiman
Copy link
Contributor

holiman commented Oct 14, 2021

The real validation rules work like this:

	// 1. the nonce of the message caller is correct
	// 2. caller has enough balance to cover transaction fee(gaslimit * gasprice)
	// 3. the amount of gas required is available in the block
	// 4. the purchased gas is enough to cover intrinsic usage
	// 5. there is no overflow when calculating intrinsic gas
	// 6. caller has enough balance to cover asset transfer for **topmost** call

Additionally, there are extra constraints in London about maxPriorityFee and maxFee. There is no explicit validity check that the value must be <256 bits, theoretically, but it would fail check 2.

I can add a 'synthetic' helper-check in t9n, but it doesn't actually map to any real error during block processing.

@holiman
Copy link
Contributor

holiman commented Oct 14, 2021

ps. See core/state_transition.go in go-ethereum for more details

@holiman
Copy link
Contributor

holiman commented Oct 14, 2021

ps. please file a ticket in go-ethereum for change requests on t9n

@marioevz
Copy link
Member Author

Thanks for the explanation @holiman, I have filed ethereum/go-ethereum#23740.

to: '0xd0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0d0'
# Value is (2 ** 256) + 1
value:
- '0x:bigint 0x10000000000000000000000000000000000000000000000000000000000000001'
Copy link
Collaborator

@winsvega winsvega Oct 15, 2021

Choose a reason for hiding this comment

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

No, I think it should get rejected. as all values in Yellow paper are 256 bit max.
so the balance check should not occur as the structure itself is invalid

@winsvega winsvega merged commit 5e81b77 into develop Oct 15, 2021
@winsvega winsvega deleted the txUpdateValue branch October 15, 2021 16:53
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