Safe block hash uses unrealized justification

[michaelsproul]

Describe the bug

According to the spec, the safe block hash sent in fcU should be set to the block hash of the justified block: https://github.com/ethereum/consensus-specs/blob/dev/fork_choice/safe-block.md#get_safe_beacon_block_root

Prysm uses the unrealized justified block:

prysm/beacon-chain/blockchain/execution_engine.go

Lines 68 to 74 in 45a2746

	finalizedHash := s.cfg.ForkChoiceStore.FinalizedPayloadBlockHash()
	justifiedHash := s.cfg.ForkChoiceStore.UnrealizedJustifiedPayloadBlockHash()
	fcs := &enginev1.ForkchoiceState{
	HeadBlockHash: headPayload.BlockHash(),
	SafeBlockHash: justifiedHash[:],
	FinalizedBlockHash: finalizedHash[:],
	}

I noticed this while working on Eleel's fcU matching algorithm and didn't think anything of it, but @parithosh and @barnabasbusa recently raised this as a potential client interop issue. In some sense it would be nice to be able to poll a collection of nodes and get a consistent answer as to what the safe head is.

Personally I think Prysm's behaviour is actually better than the spec's, because it gives a more recent view of justification that is almost-as-good. I'm opening this issue to track it on Prysm's side, but if you would prefer we change the spec I would be supportive. Changing the definition of "safe" may interact with confirmation rule work (ethereum/consensus-specs#3339), but I get the feeling that most clients aren't close to having that implemented, and that PR doesn't currently change the definition of safe used by fcU anyway.

Has this worked before in a previous version?

Prior to the introduction of unrealized justification.

🔬 Minimal Reproduction

Run Prysm and observe fcU compared to another client (e.g. Lighthouse).

Error

No response

Platform(s)

Linux (x86), Linux (ARM)

What version of Prysm are you running? (Which release)

v4.4.1

Anything else relevant (validator index / public key)?

No response

[parithosh]

cc @bsamuels453 (found the initial discrepancy), in case you wanna track the progress here :)

[potuz]

Discussed it in Discord R&D for a while (mostly with @mkhalinin) and it seemed to have no opposition and be a clearly better option. We didn't really advertise it since unrealized justification wasn't public at the time

[potuz]

For completeness, the consensus spec says that it's a stopgag to use justification, and the engine spec explicitly is lose in regards to what one is allowed to send:

safeBlockHash: DATA, 32 Bytes - the "safe" block hash of the canonical chain under certain synchrony and honesty assumptions. This value MUST be either equal to or an ancestor of headBlockHash

I'll open the CL spec PR when I get home

[michaelsproul]

Linking to latest discussion on Eth R&D to keep everyone in the loop. Current thinking suggests we don't update the spec to require unrealized justification, and either allow clients to continue doing different things, or define new (explicit) justified/confirmed labels in place of "safe".

https://discord.com/channels/595666850260713488/598292067260825641/1184977086214053939

[prestonvanloon]

Closing old issues. It sounds like this one resolved in agreement without any action items to complete. If that isn't the case, please reply here or create a new issue.