probe-lab / hermes

A Gossipsub listener and tracer.
Other
10 stars 5 forks source link

Hermes updating BeaconStatus for different ForkDigest's than specified ones #12

Closed cortze closed 2 months ago

cortze commented 2 months ago

Description

I just spotted that Hermes deliberately updates its local BeaconStatus when it receives a ForkDigest different to the one specified. Check method SetStatus() for more info:

if r.status != nil && !bytes.Equal(r.status.ForkDigest, status.ForkDigest) {
        slog.Warn("reqresp status updated with different fork digests", "old", hex.EncodeToString(r.status.ForkDigest), "new", hex.EncodeToString(status.ForkDigest))
}

I'm unsure if this is a behaviour that we were looking for (I might be missing something here), but this would make the node set and advertise its ENR and subscribe to a specific ForkDigest and its topics, while then sharing a different one on the BeaconStatus RPC call.

Check example running Hermes with a local Prysm node in the Holesky network:

{"time":"2024-04-08T09:52:33.891074838+02:00","level":"WARN","msg":"reqresp status updated with different fork digests","old":"6a95a1a9","new":"69ae0e99"}

causing:

{"time":"2024-04-08T09:53:16.465105848+02:00","level":"INFO","msg":"Received goodbye message","peer_id":"16Uiu2HAkyqKfXBitW9QWXGkoDrKYydyx8WvhtysYVdHQ2yZXmTyF","msg":"irrelevant network"

Far from ideal, this should heavily impact the connectivity with the connected nodes, as it is misleading for them, and they should drop any connection with a node that belongs to a different network.

Fix

We should ignore any status coming from a node in any other network or with a different ForkDigest:

// if the ForkDigest is not the same, we should drop updating the local status
// TODO: this might be re-checked for hardforks (make the client resilient to them)
if r.status != nil && !bytes.Equal(r.status.ForkDigest, status.ForkDigest) {
       return
}
cortze commented 2 months ago

fixed with #10 . Hermes now will report an error if its ForkDigest doesn't match the one from the trusted node.