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

cmd/evm: make t8ntool handle transaction decoding errors better #28397

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Oct 22, 2023

This PR closes #27730 . By using an iterator instead of a slice of transactions, we can better handle the case when an individual transaction (within an otherwise well-formed RLP-list) cannot be decoded.

For reviewers:

  • First commit just moves existing code into separate file (no change)
  • Second commit just moves existing functionality from slice-form into iterator-form (no functional change)
  • Third commit introduces rlp-iterator with individual handling of rlp-errors (functional change) + adds tests

cc @winsvega

@winsvega
Copy link
Contributor

winsvega commented Oct 23, 2023

https://github.com/ethereum/tests/blob/develop/GeneralStateTests/Cancun/stEIP4844-[blobtransactions/blobhashListBounds7.json]

Error: Client didn't reject transaction: (0xdb696fdd98aad26d3444d688df1ca50dd5947ebd11d5d8f3ef6c70fac0f8334f) 
0x03f901b101800285012a05f200833d090094095e7baea6a6c7c4c2dfeb977efac326af552d87830186a000f85bf85994095e7baea6a6c7c4c2dfeb977efac326af552d87f842a00000000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000010af8e7a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0001a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0002a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0003a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0004a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0005a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0006a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f000701a04c6a25969991d996e7f40964ba872e8dc6e8acdc422af055d9f404180dd2746ca013b66a111e61fb3ad575d456a1d976d221436f403f342ae8e78c19b177705607
Test Expected: TR_BLOBLIST_OVERSIZE

good, now I confirm t8n rejecting invalid blob transactions but this one still got through.
maximum allowed blobs.

  transaction:
    data:
        - data: :raw 0x00
          accessList:
          - address: 0x095e7baea6a6c7c4c2dfeb977efac326af552d87
            storageKeys:
            - 0x00
            - 0x01

    maxFeePerGas: '0x12A05F200'
    maxPriorityFeePerGas: '2'
    gasLimit:
    - '4000000'
    nonce: '0'
    to: 0x095e7baea6a6c7c4c2dfeb977efac326af552d87
    value:
    - '100000'
    secretKey: "45a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8"
    maxFeePerBlobGas: '10'
    blobVersionedHashes:
        - "0x01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0001"
        - "0x01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0002"
        - "0x01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0003"
        - "0x01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0004"
        - "0x01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0005"
        - "0x01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0006"
        - "0x01a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065f0007"

@holiman
Copy link
Contributor Author

holiman commented Oct 23, 2023

@winsvega if that is indeed a bug, then it deserves it's own issue in the tracker. But where is the max blobs per transaction defined? I don't see it in https://eips.ethereum.org/EIPS/eip-4844

@holiman holiman added this to the 1.13.5 milestone Oct 23, 2023
@winsvega
Copy link
Contributor

winsvega commented Oct 23, 2023

it is not defined expicitly but by calculating the blob price. max blobs defined per block. so one tx can have 2, another 3.
but the whole block no more than 6 because of header value calculations

MAX_BLOB_GAS_PER_BLOCK / GAS_PER_BLOB

this might not be a bug, need hive confirmation, as block logic might not be checked in t8n

@holiman
Copy link
Contributor Author

holiman commented Oct 23, 2023

Ok, pushed a fix, PTAL

@holiman holiman force-pushed the t8n_txs_iterato branch 2 times, most recently from d20c8c2 to 2981bae Compare October 23, 2023 13:55
@winsvega
Copy link
Contributor

winsvega commented Oct 24, 2023

Error: Client reject transaction expected to be valid: (0x6f26856255f46b27b31d06e00750d8d75067fd8a28e15e8f5557a33fba288cb5) 
0x03f9010c01800285012a05f200833d090094095e7baea6a6c7c4c2dfeb977efac326af552d87830186a000f85bf85994095e7baea6a6c7c4c2dfeb977efac326af552d87f842a00000000000000000000000000000000000000000000000000000000000000000a000000000000000000000000000000000000000000000000000000000000000010af842a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d8a001a915e4d060149eb4365960e6a7a45f334393093061116b197e3240065ff2d880a0bf751ed5c37bd65d3ace5b73a1c62f7388b203a82ce366392e7b76fd2de12cb1a06f2b5344e5b997d35b3a0768006196a65c4eff8ed3acad5201a105c2b59b4e8c
Reason: blob gas (262144) would exceed maximum allowance 786432

false rejections allover the correct blob transactions. (correct by nimbus)
262144 <> 786432 ?

@winsvega
Copy link
Contributor

now works!

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

lgtm

@holiman holiman merged commit 300df87 into ethereum:master Oct 25, 2023
1 of 2 checks passed
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…reum#28397)

This change closes ethereum#27730 . By using an iterator instead of a slice of transactions, we can better handle the case when an individual transaction (within an otherwise well-formed RLP-list) cannot be decoded.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Dergarcon pushed a commit to specialmechanisms/mev-geth-0x2mev that referenced this pull request Jan 31, 2024
…reum#28397)

This change closes ethereum#27730 . By using an iterator instead of a slice of transactions, we can better handle the case when an individual transaction (within an otherwise well-formed RLP-list) cannot be decoded.
colinlyguo pushed a commit to scroll-tech/go-ethereum that referenced this pull request Oct 31, 2024
…reum#28397)

This change closes ethereum#27730 . By using an iterator instead of a slice of transactions, we can better handle the case when an individual transaction (within an otherwise well-formed RLP-list) cannot be decoded.
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.

t8n crashes on invalid blob tx
3 participants