[Decoupling] Implement BlockFactoryDeneb (part 2)
#7161
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...rc/main/java/tech/pegasys/teku/spec/executionlayer/ExecutionLayerBlockProductionManager.java
Fixed
Show fixed
Hide fixed
...n/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockFactoryDenebTest.java
Show resolved
Hide resolved
| assertThat(block.getBody().getOptionalBlsToExecutionChanges()) | ||
| .isPresent() | ||
| .hasValue(blsToExecutionChanges); | ||
| } else { | ||
| assertThat(block.getBody().getOptionalBlsToExecutionChanges()).isEmpty(); | ||
| } | ||
|
|
||
| return block; | ||
| if (milestone.isGreaterThanOrEqualTo(SpecMilestone.DENEB)) { |
There was a problem hiding this comment.
I worry on broad usage of such complex milestone based assertions in big methods. There is a chance we could assert not the thing we expect in tests. For example here, input block and milestone variable could be from different milestones etc.
Just a feeling
There was a problem hiding this comment.
But the milestone is based on the new block created, so will always align. Changed the test a little bit to mimic the blsExecutionChanges for Capella
...n/validator/src/test/java/tech/pegasys/teku/validator/coordinator/BlockFactoryDenebTest.java
Show resolved
Hide resolved
StefanBratanov
force-pushed
the
block_factory_deneb
branch
from
fc4f2fe to
4856090
Compare
...tor/src/main/java/tech/pegasys/teku/validator/coordinator/BlockOperationSelectorFactory.java
Outdated
Show resolved
Hide resolved
| import tech.pegasys.teku.spec.datastructures.blocks.versions.deneb.BlockContents; | ||
| import tech.pegasys.teku.spec.util.DataStructureUtil; | ||
|
|
||
| public class BlockFactoryDenebTest extends AbstractBlockFactoryTest { |
There was a problem hiding this comment.
I wonder if we could follow the same production class hierarchy so the deneb tests will run phase0 tests
There was a problem hiding this comment.
Tried that, the issue is BlockFactoryDeneb is expected to have SchemaDefinitionsDeneb which are not retrievable from a spec pre-Deneb. And there is no way to create BlockContents without the schema definitions
There was a problem hiding this comment.
I see.. maybe we can generify some checks and move them to the Abstract test so we can call them from deneb too?
There was a problem hiding this comment.
yeah I find it hard to think of a way to generify it though since BlockFactoryDeneb requires some stuff like prepareBlobsBundle() which needs the deneb spec as well
PR Description
Next part of #7129
BlockFactoryDenebwhich createsBlockContentsorBlindedBlockContentsinstead ofBeaconBlock.BlockFactoryDenebis only used when slot is Deneb onegetOptionalBlsToExecutionChangesinBeaconBlockBodyto act in a similar way to other optional fields.Next parts:
BlockContainerinAbstractBlockPublisher(in order to publish and import blobs)BlockProductionDutyFixed Issue(s)
when next parts are done, it will fix couple of tasks in #6822
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog