Fix envelope proposal

[ahshum]

Fix as per

• https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.2/specs/gloas/builder.md#constructing-the-signedexecutionpayloadenvelope
• https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.5/specs/gloas/builder.md#constructing-the-signedexecutionpayloadenvelope
• https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.7/specs/gloas/builder.md#constructing-the-signedexecutionpayloadenvelope

[github-actions]

Unit Test Results

       12 files  ±0    2 832 suites  ±0   1h 27m 53s ⏱️ - 3m 50s
15 921 tests ±0  14 356 ✔️ ±0  1 565 💤 ±0  0 ❌ ±0 
76 692 runs  ±0  74 952 ✔️ ±0  1 740 💤 ±0  0 ❌ ±0 

Results for commit efc5cb1. ± Comparison against base commit 7b6487d.

♻️ This comment has been updated with latest results.

[agnxsh]

why is this a separate method? can it not be a part of routeSignedBeaconBlock? i'm just trying to understand

[ahshum]

This is one of the duties of builder (https://github.com/ethereum/consensus-specs/blob/v1.7.0-alpha.2/specs/gloas/builder.md) now.

[tersec]

Updating to an unstable release incorporating

• update to Nim v2.2.8 #8010

looks likely to fix the jenkins/nimbus-eth2/linux/x86_64/nix CI failure.

[tersec]

clearanceState is a ForkedHashedBeaconState, which means one shouldn't have to recompute the hash; it's cached:

nimbus-eth2/beacon_chain/spec/forks.nim

Lines 80 to 89 in 82bb3f4

	ForkedHashedBeaconState* = object
	case kind*: ConsensusFork
	of ConsensusFork.Phase0: phase0Data*: phase0.HashedBeaconState
	of ConsensusFork.Altair: altairData*: altair.HashedBeaconState
	of ConsensusFork.Bellatrix: bellatrixData*: bellatrix.HashedBeaconState
	of ConsensusFork.Capella: capellaData*: capella.HashedBeaconState
	of ConsensusFork.Deneb: denebData*: deneb.HashedBeaconState
	of ConsensusFork.Electra: electraData*: electra.HashedBeaconState
	of ConsensusFork.Fulu: fuluData*: fulu.HashedBeaconState
	of ConsensusFork.Gloas: gloasData*: gloas.HashedBeaconState

and

nimbus-eth2/beacon_chain/spec/datatypes/gloas.nim

Lines 392 to 394 in 82bb3f4

	HashedBeaconState* = object
	data*: BeaconState
	root*: Eth2Digest # hash_tree_root(data)

[ahshum]

The main reason is with verify = false, state.root wouldn't be updated due to

nimbus-eth2/beacon_chain/spec/state_transition_block.nim

Lines 1259 to 1262 in 82bb3f4

	if verify:
	state.root = hash_tree_root(state.data)
	if envelope.state_root != state.root:
	return err("process_execution_payload: state root mismatch")

And follows the spec, envelope.state_root is created by using process_execution_payload with verify=false so I believe I have to manually compute the state root after that.

[tersec]

This seems a bit odd to verify here, in general, because this whole handleProposal()/proposeBlock()/proposeBlockAux() callstack only triggers for BN-controlled validators, while as much as possible should be architected to support (even though, yes, Nimbus doesn't have the Beacon API support in place yet, it will have to by hoodi/sepolia testnet shipping) the VC/Beacon REST API approach as symmetrically/identically as possible, so they're guaranteed the same logic and have as much as feasible the same codepaths.

This is why routeFoo() combined with block_processor centralizes things: it means that both beacon_validators and the rpc/rest_foo.nim can call routeFoo() and get the same checks, the same codepaths. As is, to the extent this process_execution_payload() call is important, it's not reached at all here a REST version of Gloas block proposal.

I'd note that similarly the code above delegates to routeSignedBeaconBlock() to handle everything involved e.g.,

nimbus-eth2/beacon_chain/validators/message_router.nim

Lines 293 to 313 in 82bb3f4

	proc routeSignedBeaconBlock*(
	router: ref MessageRouter,
	blck: ForkySignedBeaconBlock,
	someSidecarsOpt: SomeSidecarsToRoute,
	checkValidator: bool
	): Future[RouteBlockResult] {.async: (raises: [CancelledError]).} =
	# 1. Validate
	? router.validateRouteBlock(blck, checkValidator)
	# 2. Publish block
	await router.publishRouteBlock(blck)
	# 3. Publish sidecars
	when someSidecarsOpt is NoSidecarsAtFork:
	const finalSidecars = noSidecars
	else:
	let finalSidecars = await publishSidecars(router, blck, someSidecarsOpt)
	# 4. Add block to DAG
	return await router.addRoutedBlock(blck, finalSidecars)

and then all these callers (aside from the exported definition) run the same validation:

$ rg routeSignedBeaconBlock
beacon_chain/validators/message_router_mev.nim
133:      (await node.router.routeSignedBeaconBlock(
136:        return err("routeSignedBeaconBlock error") # Errors logged in router

beacon_chain/validators/message_router.nim
293:proc routeSignedBeaconBlock*(

beacon_chain/validators/beacon_validators.nim
593:      node.router.routeSignedBeaconBlock(signedBlock, sidecarsOpt,

beacon_chain/rpc/rest_beacon_api.nim
1052:            await node.router.routeSignedBeaconBlock(
1059:            await node.router.routeSignedBeaconBlock(
1064:            await node.router.routeSignedBeaconBlock(
1069:            await node.router.routeSignedBeaconBlock(
1207:            await node.router.routeSignedBeaconBlock(

with no code duplication and no possibility for different validation variations to drift apart, because they all live in that one routeSignedBeaconBlock.

So whatever validation needs to happen should ideally be part of routeExecutionPayloadEnvelope().

[ahshum]

Good shout. Will address this.

[ahshum]

Instead of message router, I move the logic to makeExecutionPayloadEnvelope which I think it would also help on testing.

[arnetheduck]

Suggested change

return newBlockRef.get()

this looks redundant?