[Merge] implement ExecutionPayload timestamp validation in BlockValidator #4730
Conversation
...atetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java
Outdated
Show resolved
Hide resolved
...ransition/src/test/java/tech/pegasys/teku/statetransition/validation/BlockValidatorTest.java
Outdated
Show resolved
Hide resolved
| SignedBeaconBlock block = spy(signedBlockAndState.getBlock()); | ||
| BeaconBlock beaconBlock = spy(block.getMessage()); | ||
| BeaconBlockBody beaconBlockBody = spy(beaconBlock.getBody()); | ||
| doReturn(beaconBlock).when(block).getMessage(); | ||
| doReturn(beaconBlockBody).when(beaconBlock).getBody(); | ||
|
|
||
| doReturn(Optional.of(specContext.getDataStructureUtil().randomExecutionPayload())) | ||
| .when(beaconBlockBody) | ||
| .getOptionalExecutionPayload(); |
There was a problem hiding this comment.
Using spies to make this work feels pretty ugly. We generally shouldn't need to mock/spy data structure objects like blocks.
I think we'd ideally use the generateBlockAtSlot(nextSlot, <BlockOptions>) method and add the ability to specify a specific ExecutionPayload for the created block. Might need to also specify the resulting state root since you won't actually be able to apply the block.
I know it's a bunch more work here but it gives us the test utilities we need to do more tests like this which I think is a pretty big win.
There was a problem hiding this comment.
I spent some time on that (i implemented the setExecutionPayload) but block processor kicks in even if you are just generating a new block without applying it ant it gets rejected.. so appeared to me that we need a way to skip validations to be able to build "wrong" chains.
There was a problem hiding this comment.
I updated the test. Have a look if the approach makes sense to you.
...atetransition/src/main/java/tech/pegasys/teku/statetransition/validation/BlockValidator.java
Outdated
Show resolved
Hide resolved
tbenr
force-pushed
the
gossip_block_validation_conditions
branch
from
2494138
to
2061ee4
Compare
| .getMessage() | ||
| .getBody() | ||
| .getOptionalExecutionPayload() | ||
| .orElseThrow(); |
There was a problem hiding this comment.
nit: I think this is right because to parse the block we'd have to get a payload and we're just checking if it's the default payload or not. But I think it would be clearer if instead of .orElseThrow we just returned reject("Missing execution payload") if it's not present.
There was a problem hiding this comment.
yeah, will never happen but going to change it
… to generate invalid blocks. apply suggestions in timestamp validation apply suggestions in SpecContext assumptions helpers improve a ValidatorDataProvider test
tbenr
force-pushed
the
gossip_block_validation_conditions
branch
from
4873256
to
6a07b11
Compare
PR Description
implements the validation in
BlockValidatorand migrateBlockValidatorTestto useStorageSystemandChainUpdaterFixed Issue(s)
fixes #4669
Documentation
documentationlabel to this PR if updates are required.Changelog