initial operator message types - #454
Conversation
91660de to
67e92b4
Compare
| #[derive(Debug, Decode, Encode, Hash)] | ||
| pub struct Demotion { | ||
| /// Originating operator, utf8 bytes | ||
| operator_id: Vec<u8>, |
There was a problem hiding this comment.
this id may be unnecessary if we do straight-forward pubsub on p2p (aka floodsub) rather than gossip, since then the originator id is the sending peer id.
| /// Millisecond UNIX timestamp of the promotion. | ||
| ts_ms: u64, |
| use super::{Operator, OperatorError}; | ||
|
|
||
| #[derive(NetworkBehaviour)] | ||
| struct NetBehaviour { |
There was a problem hiding this comment.
the network is a private network, but connecting to it is open. i'm not too worried about security. fine to keep it like this. if we would want to limit the possibility for random port scanners etc. to connect we could use an allowlist.
#[derive(NetworkBehaviour)]
struct NetBehaviour {
allow_list: allow_block_list::Behaviour<AllowedPeers>,
gossipsub: gossipsub::Behaviour,
ping: ping::Behaviour,
}
// in setup:
let mut allow_list = allow_block_list::Behaviour::default();
for op in &operators {
allow_list.allow_peer(PeerId::from_public_key(&op.pubkey));
}feel free to skip.
| use crate::BlsPublicKeyBytes; | ||
|
|
||
| #[derive(Debug, Decode, Encode)] | ||
| #[ssz(enum_behaviour = "transparent")] |
There was a problem hiding this comment.
interesting agent nit. curious if you guys agree. especially since forwards compatibility of this data type concerns our Ultra Sound implementation too.
#[ssz(enum_behaviour = "transparent")] on OperatorMessage writes no selector byte, and the derived Decode just tries each variant in declaration order and returns the first that parses. This works today only because the three variants happen to have disjoint sizes (Promotion = 64 bytes fixed, BuilderCollateral = 80, Demotion ≥ 100). Any future variant or field addition that collides in size with an existing one will silently decode as the wrong message type — bad for a cross-operator wire format that will need to evolve.
There's also a runtime landmine: the derived is_ssz_fixed_len() for transparent enums asserts that all variants are variable-size, and Promotion/BuilderCollateral are fixed-size. Nothing calls it when encoding/decoding top-level, so it doesn't fire today — but the moment OperatorMessage is nested inside another SSZ container or list (e.g. a signed envelope), it panics.
Suggest #[ssz(enum_behaviour = "union")] instead: one selector byte, unambiguous decoding, safe to nest, and room to add message types later.
| serializer.serialize_str(&hex::encode(&key.to_bytes())) | ||
| } | ||
|
|
||
| /// Deserialize a hex-encoded scp256k1 pubkey. |
| .map(|o| (PeerId::from_public_key(&o.pubkey), o)) | ||
| .collect::<HashMap<_, _>>(); | ||
|
|
||
| for (peer_id, operator) in &peers { |
There was a problem hiding this comment.
this is the only place where we dial. might be worth adding a redial mechanic if an operator disappears.
There was a problem hiding this comment.
if all operators are configured with details of all other operators then I think this is sufficient. Operator A starts, dials all other operators, connects to all those that are up. Operator B goes down, connection dropped. Operator B comes back up, dials all other operators, A and B connected again.
Currently adding a new operator will require a config update and a restart. Will add config reloading and dialling of new operators in a future PR.
| #[derive(Debug, Decode, Encode)] | ||
| #[ssz(enum_behaviour = "transparent")] | ||
| pub enum OperatorMessage { | ||
| Demotion(Demotion), |
There was a problem hiding this comment.
a bigger point is around how we propagate events. i'd be fine with this. i would worry a bit one operator ends up missing a message and therefore doesn't have the correct state.
phase 1, where the messages are purely informative and each still decides their own rule for following the events is no big deal.
phase 2, where the idea is to execute demotion and repromotion such that collateral can be shared, one missed message becomes a big deal. whether an operator is down, or doesn't have a pubkey yet, network issue, a missed message permanently leaves the operator out of sync. worse, imagine a builder has 100 keys, and one message gets lost. hard bug to notice.
i see three options:
- do nothing. just rely on each operator to monitor and notice an active pubkey has been demoted for an extended period.
- on start / subscription / refresh don't just share collateral, share all the promotion state of each pubkey, that way at least restarts / deploys bring operators into consistency again.
- don't propagate events, propagate full state snapshots. so maybe a demotion is still a fast small message, as for demotion milliseconds matter but beyond that you propagate full snapshots of all keys and their status. this way you're always eventually consistent. you could make optimizations like asking a operator for a full snapshot once, then asking only for deltas. this builds on the idea that each operator having state and then having mutation through messages which may fail is hard to keep consistent. since each has own state it may be more natural to have each operator receive the builder pubkey state of every other operator. this way operator C simply makes sure the have a clean picture of the state at operator A and operator B and then combines that with their local state to create their own operator C state. this is obviously a bit more complicated then sending simple messages but a bit easier to keep consistent across operators.
curious what you guys think. i wouldn't go with 1. i think 3. may be worth it but 2. forms nice middleground too. i.e. make sure you get consistent somehow but avoid being v chatty etc.
There was a problem hiding this comment.
why would a message be missed? the idea here is that full state is shared whenever a new connection is made. If a connection is up then no message is lost 'on the wire', quic is a reliable delivery protocol, so either the connection is up and working or it is not and a new connection would be made syncing state.
There was a problem hiding this comment.
also, state I this case is not very big - collateral amount for all builders and any currently demoted builders.
There was a problem hiding this comment.
Perhaps you missed comment on Demotion struct below about sending on connection? and implementation in Event::Subscribed handling
There was a problem hiding this comment.
why would a message be missed?
network failure causing a disconnect? a message that accidentally gets bigger than floodsub allows? an operator is offline when a promotion message is sent?
if the idea is the full state is shared when a new connection is made i think we're already agreeing. just not about what "full state" entails maybe. it should be more than just the current demotions i think. otherwise on start you'd have to copy all demotions from others and disregard your own right? or a you get back to a missed promotion message e.g. because you were offline, being a problem.
There was a problem hiding this comment.
hm, if everything is only in memory. and on start you copy demotions from others and that's all demotions you keep, and locally you only track how much collateral you have. i think your design works. but that still seems a bit brittle. what if all three operators go down? unlikely of course but still, everyone repromoted? or say we notice a bad block but fail to send the demotion and need to restart, simply lost now? i see many reasons why you'd want to have local persistence of the demotions. its also how our system works today. but as soon as you have local persistent + this type of shared writing through propagated events i think you end up in trouble. what do i do when i have a demotion more locally but it came from another operator, but i missed their repromotion message?
i did a couple more iterations on my idea to cover local persistence and reliable propagation, ill paste it below. let me know if you think its worth the added complexity, its pretty small, really just heartbeat snapshots. if not, i think your design is workable as is!
Proposal: per-operator state + heartbeat snapshots
Shared collateral means demotions and repromotions have to reliably make it across. Suggestion for how the messages could work:
Each operator gossips only its own facts. A builder's status and collateral at operator A are only ever written by A. Receivers keep one state slot per operator and derive their own view locally (e.g. demoted at any operator → demoted here).
Nothing ever needs reconciling. If A demotes titan while B still has it promoted, that isn't a conflict to resolve. They're two separate facts, "demoted at A" and "promoted at B", and C stores both and applies its own rule. No merging, no replay, no ordering between operators.
Every fact carries its own version, and ts_ms can be it. The messages already carry it. Receivers store, per (operator, builder pubkey), the fact and its ts_ms, and apply an incoming fact if incoming.ts_ms >= stored.ts_ms. Same rule for deltas and snapshots, so there's no separate sequence number, nothing to persist across restarts, and no coordination needed between the nodes an operator runs.
Two message layers.
- Deltas: the current Demotion / Promotion / BuilderCollateral, sent immediately on change. This is the fast path, and it's what stops bad blocks network-wide.
- Heartbeat snapshot: status and collateral for every builder pubkey the operator holds collateral for or has demoted, pushed on connect and every ~5–10s.
Motivation for the snapshot: today we resend cached demotions when a peer subscribes, but promotions aren't cached, so a peer that misses one keeps that builder demoted indefinitely. The obvious fix is to cache promotions too, except then we're maintaining an event log and need merge rules and ordering to replay it correctly. A snapshot sidesteps that. Rather than replaying what happened, just state what currently holds. The subscribe-time resend could even be dropped entirely, and anything missed for any reason converges within one heartbeat.
Acks piggybacked on the heartbeat (bonus). Each snapshot echoes the highest ts_ms seen from each peer. That confirms peers are actually processing our facts rather than us merely managing to publish them, which is a real concern under shared collateral: a peer can be connected and heartbeating happily while failing to apply what it receives, whether from a decode error or version skew.
There was a problem hiding this comment.
- helix also has persistence of demotions
- I am not sure we need to make a distinction between local and remote demotions / promotions. For example, operator A receives a demotion for builder X from operator B and operator A then goes down and misses the promotion. On restart, operator A sends what it thinks is current demotion state to all other operators, and receives current demotions from other operators. The operators receiving the demotion for builder X from operator A will see that it is a startup / old demotion and does not correlate with their current state (i.e. predates promotion), so can ignore it. Operator A will receive demotion state from other operators and will see that builder X is not demoted according to the other operators and can update current state accordingly.
- The current PR has only demotions being sent on startup. On reflection, this should be the most recent Demotion or Promotion for each builder (if there is one).
- 'Every operator gossips only its own facts' - yes agreed for collateral e.g. the collateral message should only contain the collateral value registered at the local operator. For demotions / promotions this is the case for 'live' promotions and demotions we obviously do not want to rebroadcast received demotions / promotions. But in terms of state broadcast at startup, I think this should be the state of builders as seen by the operator regardless of where the demotion / promotion originated.
- I was thinking that any operator could promote a demoted builder - not just the operator that issued the demotion (after all, demotions may very likely occur concurrently)
- I don't see any need for heartbeat messages containing current state, this seems like pure overhead. Could you describe a scenario where these might be needed?
There was a problem hiding this comment.
The obvious fix is to cache promotions too, except then we're maintaining an event log and need merge rules and ordering to replay it correctly.
not sure I understand this point - could we not just replay the most recent demote / promote event per builder on connection?
There was a problem hiding this comment.
The current PR has only demotions being sent on startup. On reflection, this should be the most recent Demotion or Promotion for each builder (if there is one).
I think this is a bit more nuanced actually: on startup an operator connects to other operators: it only needs to send most recent persisted demotion for each builder.
Running operators receiving a new connection send most recent demotion AND promotion for each builder.
There was a problem hiding this comment.
if we send the latest demotion or promotion per builder pubkey that resolves the main case a worried about 👍.
i could still imagine for whatever reason a message disappears on the app level (good moment to note the floodsub crate seems to have a hardcoded message size limit enforced only on the receiver side, with larger messages silently dropped, may be worth bounding reason_msg as it is the one variable length field i think) but it gets a bit far fetched to worry about those failure scenarios. lets start with the design as you have it then. full demotion / promotion state and all collateral shared on subscribe.
i don't suspect we'll have many issues with lost messages.
36237b3 to
9b692a7
Compare
operator message types for use on p2p
the
operator_idin the messages is justVec<u8>- we should decide what identifiers we want use:-?
i will add the p2p impl next.