Skip to content

EIP1559 blockchain tests#837

Merged
winsvega merged 1 commit into
developfrom
eip1559-low-med
May 25, 2021
Merged

EIP1559 blockchain tests#837
winsvega merged 1 commit into
developfrom
eip1559-low-med

Conversation

@qbzzt

@qbzzt qbzzt commented May 6, 2021

Copy link
Copy Markdown
Collaborator

InvalidBlocks

  1. badBlocks, checks for blocks with invalid values in the header
  2. feeCap, checks that the fee cap is implemented correctly
  3. gasLimit20m and 4. gasLimit40m, tests cases from the geth tests https://github.com/ethereum/go-ethereum/blob/c9116a7c19301d6906c1aff87085c2fa345a84bf/consensus/misc/eip1559_test.go#L78

ValidBlocks

  1. lowDemand, checks that when gasUsed << gasTarget baseFee <- baseFee*(7/8)
  2. medDemand, checks that when gasUsed = gasTarget baseFee does not change
  3. highDemand, checks that when gasUsed = 2gasTarget baseFee <- baseFee(9/8)
  4. checkGasLimit, check that transactions that cause gasUsed to exceed gasLimit are invalid
  5. tips, check that the tipped amount given to the miner is correct
  6. transTypes, check that all three transaction types (original, type 1 (EIP 2930), and type 2 (EIP 1559)) work.
  7. baseFee, check the behavior of basefee and selfbalance when called in multiple transactions in multiple blocks.
  8. burnVerify, verify the correct amount of wei is burned

Note: The tests needed are at https://hackmd.io/NNforJS7SMmjJ1t6jA8pgQ

@qbzzt qbzzt added ready-to-merge PR is ready to merge and not WIP and removed ready-to-merge PR is ready to merge and not WIP labels May 6, 2021
@qbzzt

This comment has been minimized.

@winsvega winsvega added this to the London milestone May 7, 2021
@qbzzt qbzzt added ready-to-merge PR is ready to merge and not WIP and removed in progress labels May 7, 2021
@winsvega

winsvega commented May 7, 2021

Copy link
Copy Markdown

I think we can remove the python sanitizer scripts and json schemas from here [in a separate PR]. as retesteth checks structures validity upon test generation. and also checks that tests are updated in automatition.

@winsvega winsvega left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

very cool!
lets see if we have to rename gasTarget in our structures too. this will need some modification

Comment thread src/BlockchainTestsFiller/InvalidBlocks/bcEIP1559/badBlocksFiller.yml Outdated
Comment thread src/BlockchainTestsFiller/InvalidBlocks/bcEIP1559/badBlocksFiller.yml Outdated
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/feeCapFiller.yml
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/feeCapFiller.yml Outdated
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/highDemandFiller.yml Outdated
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/lowDemandFiller.yml
@qbzzt qbzzt added ready-to-merge PR is ready to merge and not WIP and removed ready-to-merge PR is ready to merge and not WIP labels May 8, 2021
@qbzzt qbzzt force-pushed the eip1559-low-med branch from 1b63753 to 325da8a Compare May 9, 2021 23:37
@qbzzt qbzzt added ready-to-merge PR is ready to merge and not WIP and removed ready-to-merge PR is ready to merge and not WIP labels May 9, 2021
@winsvega winsvega removed the ready-to-merge PR is ready to merge and not WIP label May 12, 2021
@qbzzt qbzzt added the ready-to-merge PR is ready to merge and not WIP label May 15, 2021
@winsvega

Copy link
Copy Markdown

need to review

@winsvega winsvega self-requested a review May 19, 2021 21:33
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/transTypeFiller.yml Outdated
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/tipsFiller.yml
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/medDemandFiller.yml
Comment thread src/BlockchainTestsFiller/ValidBlocks/bcEIP1559/checkGasLimitFiller.yml Outdated
Comment thread src/BlockchainTestsFiller/InvalidBlocks/bcEIP1559/badBlocksFiller.yml Outdated

@winsvega winsvega left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

please make sure that geth and retesteth are at latest
fix the exception names.
address the comments
and regenerate the tests

@winsvega winsvega removed the ready-to-merge PR is ready to merge and not WIP label May 24, 2021
@qbzzt qbzzt added the ready-to-merge PR is ready to merge and not WIP label May 25, 2021
@winsvega

Copy link
Copy Markdown

what happened with the branch. why changes from another branch here ?

@winsvega winsvega removed the ready-to-merge PR is ready to merge and not WIP label May 25, 2021
@qbzzt qbzzt closed this May 25, 2021
@qbzzt qbzzt force-pushed the eip1559-low-med branch from 73afc86 to 9ffbc3c Compare May 25, 2021 19:24
@qbzzt qbzzt reopened this May 25, 2021
@qbzzt qbzzt added the ready-to-merge PR is ready to merge and not WIP label May 25, 2021
@winsvega winsvega merged commit 4d539de into develop May 25, 2021
@winsvega winsvega deleted the eip1559-low-med branch May 25, 2021 20:41
@holgerd77

Copy link
Copy Markdown
Contributor

Congrats to the merge here! 🎉 😀

@holgerd77

Copy link
Copy Markdown
Contributor

Hi there,
I am just integrating these tests here: ethereumjs/ethereumjs-monorepo#1269

Just noted that the base fee is still called baseFee here in the tests, the agreement here was to use baseFeePerGas, at least for the JSON RPC.

Is this due to an outdated Geth version and will be updated?

//cc @holiman

@holiman

holiman commented May 26, 2021

Copy link
Copy Markdown
Contributor

Yeah, we have several PRs in the pipeline, and I think rpc naming is that last one, so we're definitely not up to spec at the moment

@holgerd77

Copy link
Copy Markdown
Contributor

@holiman ah ok, thanks, I've temporarily added a hot fix on our side.

@holgerd77

Copy link
Copy Markdown
Contributor

We've just run the tests from here, current failures on our side (see ethereumjs/ethereumjs-monorepo#1269):

State Tests

2 failures (total 150)

  • basefeeExample
  • eip1559

Blockchain Tests

10 failures (total 1275)

  • badBlocks
  • gasLimit20m
  • gasLimit40m

Haven't gone into any analysis, so totally open if (more likely) things are wrong on our side or if there might be some test bugs, just dropping here for reference and for others to compare if they also stumble upon things.

@ryanio

ryanio commented May 27, 2021

Copy link
Copy Markdown

Update from EthereumJS: state tests are now passing (it was within our own code)

For the Blockchain Tests, I think these failures are due to outdated versions of the spec that test >= and <= rather than > and <? (PR)

The test comments for badBlocks still reference the outdated spec:

  # From the EIP:
  #
  # assert block.gas_target >= parent_gas_target - parent_gas_target // 1024, 'invalid block: gas target decreased too much'
  ... 
  (this block:)
  - blockHeader:
      gasLimit: 1072693248    # 1024*1024*1023 (1023/1024 of the previous gasLimit)
    transactions: []

Block gasLimit: 1072693248
Max gasLimit: 1072693248


For gasLimit20m, it is on the same boundary:

 - blockHeader:
      gasLimit: 20019531
    transactions: []

Block gasLimit: 20019531
Max gasLimit: 20019531


Same with gasLimit40m:

  - blockHeader:
      gasLimit: 39960938
    transactions: []

Block gasLimit: 39960938
Min gasLimit: 39960938

@qbzzt

qbzzt commented May 27, 2021

Copy link
Copy Markdown
Collaborator Author

Update from EthereumJS: state tests are now passing (it was within our own code)

Good. That means the tests were useful.

For the Blockchain Tests, I think these failures are due to outdated versions of the spec that test >= and <= rather than > and <? (PR)

The tests look at exactly the border line between permitted and not permitted (for example, fail at ...247, succeed at ...248):

  - blockHeader:
      gasLimit: 1072693247
    expectException:
      '>=London': 1559BlockImportImpossible_TargetGasLow
    transactions: []
  - blockHeader:
      gasLimit: 1072693248    # 1024*1024*1023 (1023/1024 of the previous gasLimit)
    transactions: []

If the borderline was different, even by one gas to either direction, the InvalidBlocks/bcEIP1559/badBlocks would have failed on geth.

To verify I downloaded the latest geth and tried badBlocks on it. It still works.

Can you run the test with verbose output, and see on which block it has an error?

Thanks,
Ori

@ryanio

ryanio commented May 27, 2021

Copy link
Copy Markdown

yes they look good!

I quoted the suspect blocks in my above comment, let me know if there is a better way to identify them

@qbzzt

qbzzt commented May 27, 2021

Copy link
Copy Markdown
Collaborator Author

yes they look good!

I quoted the suspect blocks in my above comment, let me know if there is a better way to identify them

OK, sorry I didn't realize that.

   - blockHeader:
      gasLimit: 1072693248    # 1024*1024*1023 (1023/1024 of the previous gasLimit)
    transactions: []

This block is supposed to be accepted (that's why it doesn't have an expectException: field). Does it fail? If so, what is the exception? If not, does the block with a gas limit of 1072693247 fail? If so, what exception is reported to retesteth?

It's possible that the only problem is that retesteth doesn't recognize the exception.

@ryanio

ryanio commented May 27, 2021

Copy link
Copy Markdown

np, yes it's failing with our internal validation of invalid gas limit as it can't be exactly equal (ethereum/EIPs#3566)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge PR is ready to merge and not WIP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants