consensus/beacon: reenable checking for timestamps #24140
Conversation
MariusVanDerWijden
requested review from
gballet and
karalabe
as code owners
| @@ -192,6 +195,12 @@ func (beacon *Beacon) verifyHeader(chain consensus.ChainHeaderReader, header, pa | |||
| if header.UncleHash != types.EmptyUncleHash { | |||
| return errInvalidUncleHash | |||
| } | |||
| if header.Time > uint64(time.Now().Unix()+allowedFutureBlockTimeSeconds) { | |||
There was a problem hiding this comment.
Why do we need to check the timestamp here? It's passed by CL and we should always assume CL will verify it, like it's not older than parent.
Regarding the future block, it's indeed possible that the given timestamp is in future because CL and EL can in different machine. But does it really matter? And why do we have this 1 second allowance?
There was a problem hiding this comment.
-
Yes that was how the spec used to be. Now it changed a bit and left this underspecified. During
engine_executePayloadwe check that the timestamp matches, however we don't do it for historical blocks passed by sync. -
The 1 second allowance is exactly for that, if two machines are on slightly different clocks. Again, the CL should check it, but I think it is better to not rely on the CL for that
There was a problem hiding this comment.
So what's the side effect for receiving the "invalid timestamp"?
- if the timestamp is even older than parent, then a consensus error is returned and the given block is rejected(looks correct)
- if the timestamp is in the future, the block is supposed to be added into the future queue. But PoS blocks are not allowed caching in the future queue. So eventually the block is ignored silently? (also looks correct). And next time the block sync will fill the gap I think.
There was a problem hiding this comment.
I don't get this change. I'm perfectly fine with
if header.Time <= parent.Time {
return errOlderBlockTime
}
But why do we also require the child block to be within one second from whatever the local time happens to be?
There was a problem hiding this comment.
Also, traditionally, we've always saved ErrFutureBlock for last, so that it basically means "everything checks out, except the timestamp is a bit off". We should not change that logic, so please move it last
There was a problem hiding this comment.
Generally OK for me, the chain behavior is still expected. Just really not sure we should verify the timestamp by ourselves.
The assumption is held that CL should verify all these consensus fields. It's OK to verify more in EL, but the corresponding action is not defined in the spec, like how should we act to future blocks.
| @@ -175,7 +178,7 @@ func (beacon *Beacon) VerifyUncles(chain consensus.ChainReader, block *types.Blo | |||
| // - nonce is expected to be 0 | |||
| // - unclehash is expected to be Hash(emptyHeader) | |||
| // to be the desired constants | |||
| // (b) the timestamp is not verified anymore | |||
| // (b) the timestamp is older than it's parent and not in the future | |||
There was a problem hiding this comment.
the timestamp is not older than it's parent.
| @@ -192,6 +195,12 @@ func (beacon *Beacon) verifyHeader(chain consensus.ChainHeaderReader, header, pa | |||
| if header.UncleHash != types.EmptyUncleHash { | |||
| return errInvalidUncleHash | |||
| } | |||
| if header.Time > uint64(time.Now().Unix()+allowedFutureBlockTimeSeconds) { | |||
There was a problem hiding this comment.
So what's the side effect for receiving the "invalid timestamp"?
- if the timestamp is even older than parent, then a consensus error is returned and the given block is rejected(looks correct)
- if the timestamp is in the future, the block is supposed to be added into the future queue. But PoS blocks are not allowed caching in the future queue. So eventually the block is ignored silently? (also looks correct). And next time the block sync will fill the gap I think.
|
PR #24236 already implemented checking for timestamps |
Reenables checking of timestamps in verifyHeader, thanks to @sunce86 for noticing