diff --git a/integration_tests/tests/cross_zone_state_machine.rs b/integration_tests/tests/cross_zone_state_machine.rs index 6d0b64e09..119d09cb2 100644 --- a/integration_tests/tests/cross_zone_state_machine.rs +++ b/integration_tests/tests/cross_zone_state_machine.rs @@ -27,8 +27,7 @@ use ping_core::{ReceiverInstruction, ping_record_pda}; const INITIAL_BALANCE: u128 = 100; const LOCK_AMOUNT: u128 = 30; const RECIPIENT: [u8; 32] = [9; 32]; -/// The source block a delivery names. These tests drive the guest directly, so -/// there is no peer block to hash and any fixed value does. +/// These tests drive the guest directly, so any fixed source-block hash does. const SRC_BLOCK_HASH: [u8; 32] = [7; 32]; /// State registering the cross-zone builtins these tests exercise. @@ -540,14 +539,11 @@ fn mint_replay_rejected() { } } -/// A peer that publishes two different blocks claiming one block id gets at most -/// one of them delivered from. +/// A peer publishing two blocks at one block id gets at most one delivered from. /// -/// The shard's address covers the zone and the block id but not which block -/// claimed them, so both resolve to the same account. The first delivery binds -/// it to its own source block and the second cannot execute against it. Failing -/// is the point: were the second merely no-op'd as a replay, a peer could pick -/// which of two messages at one coordinate the target program ever sees. +/// Both resolve to the same shard account; the first binds it. Failing rather +/// than no-opping is the point: a replay no-op would let a peer choose which of +/// two messages at one coordinate the target program ever sees. #[test] fn a_delivery_from_a_second_block_at_the_same_id_is_refused() { let inbox_id = programs::cross_zone_inbox().id(); @@ -561,8 +557,7 @@ fn a_delivery_from_a_second_block_at_the_same_id_is_refused() { let mut state = base_state(); seed_inbox_config(&mut state, self_zone, src_zone, [9_u32; 8], receiver_id); - // The shard as the first delivery left it: bound to SRC_BLOCK_HASH, holding - // that block's transaction 0. + // The shard as the first delivery left it: bound, holding transaction 0. let seen_id = inbox_seen_shard_account_id(inbox_id, &src_zone, src_block_id); let mut shard = SeenShard::default(); shard.insert(SRC_BLOCK_HASH, 0); @@ -613,9 +608,8 @@ fn a_delivery_from_a_second_block_at_the_same_id_is_refused() { "a delivery from a block the shard is not bound to must not execute" ); - // The control, so the refusal above is the binding and not the shape of the - // transaction: the same second delivery, differing only in naming the block - // the shard is bound to, executes and is recorded alongside the first. + // Control: the same delivery naming the bound block executes, so the refusal + // above is the binding and not the transaction's shape. let control_words = risc0_zkvm::serde::to_vec(&ReceiverInstruction::Record { payload: b"from-the-bound-block".to_vec(), }) diff --git a/lez/cross_zone/src/lib.rs b/lez/cross_zone/src/lib.rs index 71212f3b8..7ea3fdf0f 100644 --- a/lez/cross_zone/src/lib.rs +++ b/lez/cross_zone/src/lib.rs @@ -33,12 +33,11 @@ pub struct Emission { /// Where a delivery came from on the peer chain. /// -/// One struct so the watcher and the verifier fill the same field list. They -/// must produce byte-identical dispatch transactions for the same emission, and -/// a field one side sets differently is exactly how that breaks. +/// One struct so the watcher and the verifier fill the same field list: their +/// dispatch transactions for one emission must be byte-identical. /// -/// `src_block_hash` is the block's recomputed hash on both sides, never the -/// `header.hash` it declares, which its signature does not cover. +/// `src_block_hash` is the recomputed hash on both sides, never the declared +/// `header.hash`, which the signature does not cover. pub struct EmissionSource { pub src_zone: ZoneId, pub src_block_id: u64, diff --git a/lez/indexer/core/src/cross_zone_verifier.rs b/lez/indexer/core/src/cross_zone_verifier.rs index d91d0051e..1cc48ee17 100644 --- a/lez/indexer/core/src/cross_zone_verifier.rs +++ b/lez/indexer/core/src/cross_zone_verifier.rs @@ -63,14 +63,9 @@ pub enum CrossZoneVerifyError { }, } -/// What the verifier treats as one delivery for the purpose of skipping -/// re-derivation. -/// -/// The replay key and the source block it came from. The inbox no-ops a replay -/// only when the shard it lands in is bound to that same block, so skipping on -/// the key alone would wave through a dispatch the guest will refuse, and the -/// block would then park and hold ingestion. Both sides have to agree on what a -/// replay is. +/// The replay key plus the source block, which is what the inbox treats as one +/// delivery. Skipping re-derivation on the key alone would wave through a +/// dispatch the guest refuses, parking the block and holding ingestion. type SeenKey = (MessageKey, [u8; 32]); /// One peer zone's cached blocks, plus how far this reader has read them as an @@ -462,10 +457,8 @@ impl CrossZoneVerifier { ))); } - // Recomputed here rather than read from `msg`, which would make the - // field attest to itself. `accept_peer_block` already proved this equals - // the `header.hash` of every cached block, so it costs one hash and the - // fact stays local. + // Recomputed rather than read from `msg`, which would make the field + // attest to itself. Ok(build_dispatch_from_emission( &EmissionSource { src_zone: msg.src_zone, @@ -542,6 +535,13 @@ struct PeerPass { stalled_at: Option, } +fn seen_key(msg: &CrossZoneMessage) -> SeenKey { + ( + message_key(&msg.src_zone, msg.src_block_id, msg.src_tx_index), + msg.src_block_hash, + ) +} + /// Whether a block read off a peer's channel may enter the cache. The channel /// authorizes who may write, not what they may claim. /// @@ -550,14 +550,6 @@ struct PeerPass { /// without recomputing it a peer can assert links it never built. The key check /// applies only when one is pinned, mirroring the watcher; it subsumes the hash /// check, but a peer with no pinned key still gets that one. -/// The delivery `msg` identifies, for the seen set. -fn seen_key(msg: &CrossZoneMessage) -> SeenKey { - ( - message_key(&msg.src_zone, msg.src_block_id, msg.src_tx_index), - msg.src_block_hash, - ) -} - fn accept_peer_block( block: &Block, peer_zone: ZoneId, @@ -876,10 +868,8 @@ mod tests { let verifier = verifier(); cache_chain(&verifier, peer_chain(b"hi")).await; - // Everything else matches the peer's block, so only the claimed source - // hash is wrong. The verifier recomputes it from the block it resolved - // rather than reading the field, which is what makes this detectable at - // all; trusting the message would make the field attest to itself. + // Only the claimed source hash is wrong. Detectable because the verifier + // recomputes it from the resolved block instead of reading the field. let block = produce_dummy_block(9, None, vec![dispatch_naming_block_hash(b"hi", [0xab; 32])]); assert!( @@ -980,11 +970,9 @@ mod tests { let keys = verifier.verify_block(&first).await.expect("first verifies"); verifier.record_seen(keys).await; - // Same zone, block id and transaction index as the delivery just seen, - // but naming a different source block. The inbox refuses this rather - // than no-opping it, since the shard is bound to the other block, so - // skipping re-derivation here would wave through a dispatch that then - // parks the block and holds ingestion. + // Same coordinates as the delivery just seen, different source block. + // The inbox refuses rather than no-ops it, so skipping re-derivation + // would wave through a dispatch that parks the block. let other = produce_dummy_block( 10, None, diff --git a/lez/programs/bridge_lock/src/main.rs b/lez/programs/bridge_lock/src/main.rs index 58d173516..ec4ac7b4a 100644 --- a/lez/programs/bridge_lock/src/main.rs +++ b/lez/programs/bridge_lock/src/main.rs @@ -44,10 +44,9 @@ fn main() { mint_amount, amount, "locked amount must equal the wrapped mint amount" ); - // Refused here rather than on the destination, where the mint would fail - // after this side had already escrowed the balance. Nothing releases an - // escrow, so an amount the destination will not mint has to fail before the - // debit, in the submitter's own transaction where they can see it. + // Before the debit, not on the destination: nothing releases an escrow, so + // an amount the destination will not mint has to fail in the submitter's own + // transaction. assert!( amount <= MAX_MINT_AMOUNT, "locked amount exceeds what the wrapped token will mint" diff --git a/lez/programs/cross_zone_inbox/core/src/lib.rs b/lez/programs/cross_zone_inbox/core/src/lib.rs index de3684a65..c0dcf64b2 100644 --- a/lez/programs/cross_zone_inbox/core/src/lib.rs +++ b/lez/programs/cross_zone_inbox/core/src/lib.rs @@ -9,11 +9,9 @@ use serde::{Deserialize, Serialize}; const MESSAGE_KEY_DOMAIN: [u8; 32] = *b"/LEZ/v0.3/CrossZoneMsgKey/00000/"; const INBOX_CONFIG_SEED: [u8; 32] = *b"/LEZ/v0.3/CrossZoneInboxCfg/000/"; -/// Not `/00/`, which keyed a shard by `src_block_id / 10_000`: an epoch and a -/// block id collide under one domain, so a shard carrying the older layout would -/// land at a new address and decode as nonsense. Belt and braces, since the -/// image id is what actually relocates every PDA here, and it moves with any -/// change to this crate. +/// `/01/` because `/00/` keyed shards by epoch: an epoch and a block id are +/// indistinguishable under one domain. Belt and braces, since the image id +/// already relocates every PDA in this crate whenever the crate changes. const INBOX_SEEN_SEED_DOMAIN: [u8; 32] = *b"/LEZ/v0.3/CrossZoneInboxSeen/01/"; /// Raw 32-byte zone (channel) id; the host maps it to the zone-sdk `ChannelId`. @@ -126,18 +124,13 @@ impl InboxConfig { /// What one peer block has already delivered. /// -/// One shard per peer block, holding transaction indices rather than message -/// keys. The shard's own address binds `(src_zone, src_block_id)`, so hashing -/// those into a 32-byte key and storing it back inside that account records -/// nothing the address does not. +/// Indices, not message keys: the shard's address already binds +/// `(src_zone, src_block_id)`, so a key stored inside it adds nothing. /// -/// Indices rather than keys are what make a per-block shard affordable, not -/// free. A shard costs a new account plus a 36-byte header, so at one delivery -/// per peer block it holds more state per message than one shard shared across -/// ten thousand blocks did, and it breaks even around five. What it buys is that -/// a shard cannot saturate: at 32 bytes per delivery, one peer block's own -/// messages could overflow the account, and the guest's only way to say so is a -/// panic that costs the message. +/// A shard costs an account plus a 36-byte header and breaks even against a +/// shared shard at about five deliveries. What that buys is saturation +/// resistance: at 32 bytes per delivery one peer block could overflow the +/// account, and the guest's only answer is a panic that costs the message. #[derive(Clone, Debug, Default, PartialEq, Eq, BorshSerialize, BorshDeserialize)] pub struct SeenShard { /// Recomputed hash of the peer block this shard records deliveries from. @@ -153,11 +146,10 @@ impl SeenShard { /// Borsh is 32 bytes of hash, a 4-byte count, then 4 bytes per index, so /// this is exactly the 100 KiB an account may carry. /// - /// What keeps it out of reach is the L1 inscription limit rather than - /// anything a peer configures: a whole block is inscribed as one op, capped - /// near 1.75 MiB, and a minimal emitting transaction is about 257 bytes, so - /// one peer block tops out around 7,100 deliveries. Raising that L1 cap past - /// roughly 6.3 MiB would put this back in reach. + /// Out of reach only because of the L1 inscription cap: a block inscribes as + /// one op near 1.75 MiB and a minimal emitting transaction is about 257 + /// bytes, capping a peer block near 7,100 deliveries. Raising that L1 cap + /// past roughly 6.3 MiB puts this back in reach. pub const MAX_DELIVERIES: usize = 25_591; /// Decodes a shard from account data; empty data is an unclaimed shard. @@ -175,10 +167,9 @@ impl SeenShard { /// Whether a delivery from the block with this hash may be recorded here. /// - /// An unclaimed shard binds to whoever claims it first. Unclaimed is the - /// whole value being default rather than the hash being all zero, so a shard - /// that has recorded anything can never read as unclaimed even if a hash - /// somehow were. + /// An unclaimed shard binds to its first claimant. Unclaimed is the whole + /// value being default, not the hash being zero, so a shard holding any + /// delivery can never read as unclaimed. #[must_use] pub fn binds(&self, src_block_hash: &[u8; 32]) -> bool { *self == Self::default() || self.src_block_hash == *src_block_hash @@ -189,13 +180,11 @@ impl SeenShard { self.delivered.contains(&src_tx_index) } - /// Binds the shard if unclaimed and records the delivery; returns true if it - /// was newly recorded. + /// Binds the shard if unclaimed and records the delivery; true if new. /// - /// A hash the shard does not bind records nothing. The guest asserts - /// [`Self::binds`] before reaching this, so that refusal is a backstop - /// against a later caller rebinding a claimed shard and quietly erasing - /// which peer block delivered what. + /// A non-binding hash records nothing. The guest already asserts + /// [`Self::binds`], so this is a backstop against a future caller rebinding + /// a claimed shard and erasing which peer block delivered what. pub fn insert(&mut self, src_block_hash: [u8; 32], src_tx_index: u32) -> bool { if !self.binds(&src_block_hash) { return false; @@ -280,8 +269,8 @@ pub fn inbox_seen_shard_account_id( /// Seed of the seen-shard PDA, exposed so the guest can claim the account. /// -/// One shard per peer block, so what it can hold is bounded by that block, and a -/// peer cannot accumulate deliveries into one account across many of them. +/// One shard per peer block, so a peer cannot accumulate deliveries from many +/// blocks into one account. #[must_use] pub fn inbox_seen_shard_seed(src_zone: &ZoneId, src_block_id: u64) -> PdaSeed { use risc0_zkvm::sha::{Impl, Sha256 as _}; diff --git a/lez/programs/cross_zone_inbox/src/main.rs b/lez/programs/cross_zone_inbox/src/main.rs index 292c4eccf..4d2181c23 100644 --- a/lez/programs/cross_zone_inbox/src/main.rs +++ b/lez/programs/cross_zone_inbox/src/main.rs @@ -97,15 +97,12 @@ fn dispatch( let mut shard = SeenShard::from_bytes(&seen.account.data.clone().into_inner()).expect("seen shard decodes"); - // One block id, one delivering block. The shard's address binds the zone and - // the block id but not which block claimed them, so an equivocating peer's - // two blocks at one id resolve to this same account. The first to deliver - // binds it to its own hash and the second aborts here, rather than the two - // sharing a replay set while being different blocks. + // One block id, one delivering block. The address binds the zone and block + // id but not which block claimed them, so an equivocating peer's two blocks + // at one id land here; the first binds the shard and the second aborts. // - // Before the replay check, not after. A mismatched hash has to fail the - // transaction; reaching the replay branch first would turn a delivery from - // the wrong block into a silent no-op, which is what the indexer's own + // Before the replay check, not after: reaching the replay branch first would + // turn a wrong-block delivery into a silent no-op, which the indexer's // already-seen short circuit would then wave through. assert!( shard.binds(&msg.src_block_hash), diff --git a/lez/programs/wrapped_token/core/src/lib.rs b/lez/programs/wrapped_token/core/src/lib.rs index 8fa525f11..2d5e0d775 100644 --- a/lez/programs/wrapped_token/core/src/lib.rs +++ b/lez/programs/wrapped_token/core/src/lib.rs @@ -10,20 +10,14 @@ use serde::{Deserialize, Serialize}; /// The most one mint may credit. /// -/// The amount is chosen on the peer zone and the balance is a `u128`, so without -/// a bound a single delivery can push a holding to within a hair of the maximum. -/// Every honest mint to that recipient then overflows, and an overflow is a guest -/// panic, so each one fails execution and is eventually given up on. The holding -/// is unusable for inbound transfers for good, at a cost to the attacker of one -/// message. +/// The peer zone chooses the amount and the balance is a `u128`, so unbounded +/// one delivery pins a holding near the maximum, every later honest mint +/// overflows into a guest panic, and the holding is bricked for inbound +/// transfers at a cost of one message. The cap does not remove that ceiling, it +/// makes reaching it cost 2^64 deliveries instead of one. /// -/// A cap does not put the maximum out of reach, it makes reaching it cost 2^64 -/// deliveries rather than one. -/// -/// `u64::MAX` is a bound the bridge imposes rather than one native balances -/// already obey: `Balance` is a `u128` and the faucet is seeded at its maximum, -/// so a larger amount is representable. `bridge_lock` refuses one at the source -/// so it fails in the submitter's transaction rather than after escrowing. +/// `u64::MAX` is the bridge's bound, not one native balances obey. `bridge_lock` +/// refuses a larger amount at the source so it fails before escrowing. pub const MAX_MINT_AMOUNT: u128 = 0xFFFF_FFFF_FFFF_FFFF; const CONFIG_SEED_DOMAIN: [u8; 32] = *b"/LEZ/v0.3/WrappedTokenConfig/00/"; diff --git a/lez/sequencer/core/metrics/src/record.rs b/lez/sequencer/core/metrics/src/record.rs index 39b47a38a..e6f7e8d33 100644 --- a/lez/sequencer/core/metrics/src/record.rs +++ b/lez/sequencer/core/metrics/src/record.rs @@ -180,8 +180,8 @@ pub fn increment_cross_zone_dispatches_retired_total() { cross_zone_dispatches_retired_total_counter().increment(1); } -/// Retained dead letters, which both evict at their cap and drop when a -/// delivery turns out to settle, so this falls as well as rises. +/// Retained dead letters. A gauge, not a counter: eviction and reconciliation +/// make this fall as well as rise. pub fn record_cross_zone_dead_letter_dispatches(count: usize) { gauge!( description: "Given-up-on cross-zone deliveries currently retained for inspection", diff --git a/lez/sequencer/core/src/cross_zone_watcher.rs b/lez/sequencer/core/src/cross_zone_watcher.rs index 3d350a60e..0ddc4b6d3 100644 --- a/lez/sequencer/core/src/cross_zone_watcher.rs +++ b/lez/sequencer/core/src/cross_zone_watcher.rs @@ -1183,9 +1183,8 @@ mod tests { panic!("the recorded transaction is an inbox dispatch"); }; - // The block this delivery came from, which is what the indexer - // recomputes independently when it re-derives the same transaction. - // Naming a different block is what makes the two disagree. + // The indexer recomputes this independently when it re-derives the same + // transaction; a different block here is what makes the two disagree. assert_eq!( msg.src_block_hash, chain_block(1).recompute_hash().0, diff --git a/lez/sequencer/core/src/lib.rs b/lez/sequencer/core/src/lib.rs index 26f0de2c0..b9269e980 100644 --- a/lez/sequencer/core/src/lib.rs +++ b/lez/sequencer/core/src/lib.rs @@ -1082,9 +1082,8 @@ impl SequencerCore { /// A delivery's payload and target accounts are chosen on the peer zone and /// validated by nobody in between, so one can fail for good; but a failure /// can equally be a property of the moment, so give up only after several. - /// Giving up moves the record to the dead letter, which is what keeps a peer - /// from growing the pending list with deliveries that can never execute - /// while still leaving the delivery somewhere an operator can find it. + /// Giving up moves the record to the dead letter: a peer cannot grow the + /// pending list with deliveries that never execute, and it stays findable. fn count_dispatch_failure(&self, tx: &LeeTransaction) { let Some(message) = extract_cross_zone_dispatch(tx) else { return; @@ -1134,12 +1133,11 @@ impl SequencerCore { } } - /// The deliveries this node has given up on, with how many times it has done - /// so, which is the larger number once entries evict or reconcile away. + /// The deliveries this node has given up on, and how many times it has. /// /// Retained is read first so the pair can only skew towards a total that - /// leads its list, which is an ordinary evicted or settled state. The other - /// order would report entries against a total of zero. + /// leads its list, an ordinary evicted or settled state. The other order + /// would report entries against a total of zero. pub fn cross_zone_dead_letters(&self) -> Result<(u64, Vec), DbError> { let dbio = self.store.dbio(); let retained = dbio.get_dead_letter_cross_zone_dispatches()?; @@ -1230,16 +1228,14 @@ fn deposit_already_minted(state: &lee::V03State, deposit_op_id: HashType) -> boo /// Whether a cross-zone delivery is already on the chain we are building on. /// -/// The inbox records each peer block's delivered transaction indices in that -/// block's seen shard and no-ops a replay, so the shard is the same kind of -/// answer the deposit receipt gives: state, not bookkeeping. An orphan reverts -/// the entry with the block, so the next turn re-delivers with nothing of ours -/// to unwind. +/// The inbox records each peer block's delivered indices in that block's seen +/// shard and no-ops a replay, so the shard is the same kind of answer the +/// deposit receipt gives: state, not bookkeeping. An orphan reverts the entry +/// with the block, so the next turn re-delivers with nothing to unwind. /// /// Both halves matter. A shard bound to a different peer block is not this -/// delivery's replay record; it is what will make this delivery abort, and -/// calling that delivered would drop the record instead of retrying it and -/// dead-lettering it where an operator can see it. +/// delivery's replay record, it is what will make it abort, and calling that +/// delivered would drop the record instead of dead-lettering it. fn dispatch_already_delivered(state: &lee::V03State, message: &CrossZoneMessage) -> bool { let shard_id = cross_zone_inbox_core::inbox_seen_shard_account_id( programs::cross_zone_inbox().id(), @@ -1253,6 +1249,22 @@ fn dispatch_already_delivered(state: &lee::V03State, message: &CrossZoneMessage) }) } +/// Publishes how many given-up-on deliveries are retained. +/// +/// Read from the store because the list falls as well as rises (eviction, and +/// reconciliation when a delivery settles elsewhere). Costs a read and a decode, +/// so call it only where one of those can have happened. +fn record_dead_letter_gauge(dbio: &RocksDBIO) { + match dbio.get_dead_letter_cross_zone_dispatches() { + Ok(records) => { + sequencer_core_metrics::record_cross_zone_dead_letter_dispatches(records.len()); + } + Err(err) => { + warn!("Failed to read the cross-zone dead letter for its gauge: {err:#}"); + } + } +} + /// Feed one channel delta into the follow state and mirror it to the store: /// revert orphaned, then apply and persist adopted and finalized blocks. /// Production builds on this same head. Wired to the publisher via @@ -1265,24 +1277,6 @@ fn dispatch_already_delivered(state: &lee::V03State, message: &CrossZoneMessage) /// relies on a valid successor or a restart. `ChainState` never emits /// `AcceptOutcome::RetryableFailure` yet; adding retry parity here is a /// follow-up. -/// Publishes how many given-up-on deliveries are retained. -/// -/// Read from the store rather than tracked in memory because the list falls as -/// well as rises: it evicts at its cap, and an entry is dropped when its -/// delivery turns out to settle on a block another sequencer produced. Called -/// only where one of those can have happened, since it costs a read and a -/// decode. -fn record_dead_letter_gauge(dbio: &RocksDBIO) { - match dbio.get_dead_letter_cross_zone_dispatches() { - Ok(records) => { - sequencer_core_metrics::record_cross_zone_dead_letter_dispatches(records.len()); - } - Err(err) => { - warn!("Failed to read the cross-zone dead letter for its gauge: {err:#}"); - } - } -} - fn apply_follow_update( dbio: &RocksDBIO, chain: &Mutex, @@ -1461,9 +1455,8 @@ fn apply_follow_update( }; sequencer_core_metrics::record_chain_height(head_height); - // This is the runtime path that reconciles: a delivery this node gave up on - // reaches a block another sequencer produced, and finalizing that block - // drops its dead letter. + // The runtime reconcile path: finalizing another sequencer's block drops the + // dead letter of a delivery this node gave up on. record_dead_letter_gauge(dbio); if outcome.accepted_deposits > 0 { diff --git a/lez/sequencer/core/src/tests.rs b/lez/sequencer/core/src/tests.rs index ac8f16d93..751053f85 100644 --- a/lez/sequencer/core/src/tests.rs +++ b/lez/sequencer/core/src/tests.rs @@ -221,10 +221,8 @@ fn dispatch_tx(src_block_id: u64, payload: Vec) -> LeeTransaction { )) } -/// A stand-in for the peer block's recomputed hash, distinct per block id. -/// -/// These records are seeded straight into the store rather than read off a peer -/// channel, so no real block exists to hash. Only its consistency matters. +/// A stand-in for the peer block's recomputed hash, distinct per block id. These +/// records are seeded into the store, so no real block exists to hash. fn peer_block_hash(src_block_id: u64) -> [u8; 32] { let mut hash = [0_u8; 32]; hash[..8].copy_from_slice(&src_block_id.to_le_bytes()); @@ -720,10 +718,8 @@ async fn a_dispatch_that_never_executes_is_given_up_on_after_repeated_failures() "giving up on a delivery must take its record out of the pending list" ); - // A dispatch that fails execution is left out of the block, so the dead - // letter is the only place recording that this happened at all. The origin - // is the point of the record: it is what identifies which message stopped - // being attempted, and this is the only place that builds one. + // The dead letter is the only record that this happened, and the origin is + // what identifies which message stopped being attempted. let dbio = sequencer.store.dbio(); let dead_letters = dbio.get_dead_letter_cross_zone_dispatches().unwrap(); assert_eq!(dead_letters.len(), 1); diff --git a/lez/sequencer/service/protocol/src/lib.rs b/lez/sequencer/service/protocol/src/lib.rs index e608f81aa..37f70415b 100644 --- a/lez/sequencer/service/protocol/src/lib.rs +++ b/lez/sequencer/service/protocol/src/lib.rs @@ -11,11 +11,10 @@ use serde_with::{DeserializeFromStr, SerializeDisplay}; #[derive(Debug, Clone, PartialEq, Eq, Hash, SerializeDisplay, DeserializeFromStr)] pub struct ChannelId(pub [u8; 32]); -/// A cross-zone delivery a sequencer gave up on after repeated execution -/// failures. +/// A cross-zone delivery a sequencer gave up on after repeated failures. /// -/// Identifies the message rather than carrying it: the peer zone, block id and -/// transaction index are what locate it on the peer's channel. +/// Identifies the message rather than carrying it: zone, block id and tx index +/// locate it on the peer's channel. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CrossZoneDeadLetter { pub message_key: HashType, @@ -28,9 +27,8 @@ pub struct CrossZoneDeadLetter { /// What a sequencer has given up delivering. /// -/// `total_retired` counts every give-up; `retained` holds the ones still kept. -/// The two differ once entries evict at the cap, or once a delivery this node -/// abandoned settles on a block another sequencer produced. +/// `total_retired` counts every give-up, `retained` only the ones still kept; +/// they diverge on eviction and on reconciliation. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CrossZoneDeadLetterReport { pub total_retired: u64, diff --git a/lez/storage/src/sequencer/mod.rs b/lez/storage/src/sequencer/mod.rs index c6a2d66a7..90f074145 100644 --- a/lez/storage/src/sequencer/mod.rs +++ b/lez/storage/src/sequencer/mod.rs @@ -83,16 +83,13 @@ pub const MAX_PENDING_CROSS_ZONE_DISPATCHES: usize = 4096; /// How many given-up-on cross-zone deliveries are kept for inspection. /// -/// Retaining them is the point, but a peer chooses how many deliveries fail, so -/// this list cannot be unbounded any more than the pending one can. At the cap -/// the oldest is dropped, which keeps the entries an operator reaching for this -/// after an alert actually wants. Nothing is concealed by that: every -/// retirement is counted separately and that count does not evict. +/// A peer chooses how many deliveries fail, so this cannot be unbounded. The +/// oldest evicts at the cap, and nothing is concealed by that: retirements are +/// counted separately and the count does not evict. /// -/// A count is a real bound here only because a record identifies a delivery -/// instead of carrying it. Each is a fixed 84 bytes, so the whole list is 21 KB -/// at the cap, bounded in bytes as well as in entries. That matters because it -/// is one value rewritten under the lock that block production needs. +/// An entry count bounds bytes only because a record identifies a delivery +/// rather than carrying it. At a fixed 84 bytes each the list is 21 KB, which +/// matters because it is one value rewritten under the block-production lock. pub const MAX_DEAD_LETTER_CROSS_ZONE_DISPATCHES: usize = 256; /// Key base for storing the LEE state. @@ -107,10 +104,8 @@ pub const CF_LEE_STATE_NAME: &str = "cf_lee_state"; /// What counting a failed production attempt did to a delivery's record. /// -/// Three outcomes rather than a bool because the caller reports each -/// differently and only one of them means this node stopped trying. A delivery -/// that has already settled has no pending record, so it is [`Self::Absent`] -/// rather than a give-up. +/// Three outcomes rather than a bool: only one means this node stopped trying, +/// and a settled delivery has no record, so it is [`Self::Absent`]. #[derive(Debug, Clone, PartialEq, Eq)] pub enum DispatchFailure { /// Counted; the delivery is still pending and will be attempted again. @@ -783,18 +778,14 @@ impl RocksDBIO { /// Counts a failed production attempt against a delivery, retiring it once /// it reaches `retire_at`. /// - /// Retiring moves the record out of the pending list and into the dead - /// letter. The pending list has to lose it, or the drain would re-feed a - /// transaction that never executes for ever; the dead letter is what keeps - /// the delivery identifiable, since a dispatch that fails execution is left - /// out of the block and so leaves no trace anywhere else. It is bounded on - /// its own terms, so a peer that can make deliveries fail still cannot grow - /// the store without limit. + /// The pending list has to lose the record, or the drain re-feeds a + /// transaction that never executes for ever. The dead letter keeps the + /// delivery identifiable, since a dispatch that fails execution is left out + /// of the block and leaves no trace elsewhere, and it is bounded separately. /// - /// A delivery with no pending record is reported as [`DispatchFailure::Absent`] - /// rather than as a retirement: there is nothing to count against, and the - /// two are different events. It is the ordinary shape of a delivery that - /// settled and then failed to execute on a later attempt. + /// No pending record gives [`DispatchFailure::Absent`], not a retirement: + /// the ordinary shape of a delivery that settled and then failed a later + /// attempt. pub fn record_dispatch_failure( &self, message_key: [u8; 32], @@ -828,12 +819,10 @@ impl RocksDBIO { transaction_bytes: u32::try_from(retired.transaction.len()).unwrap_or(u32::MAX), }; - // One entry per delivery, not per retirement. The same delivery can be - // recorded again after its record is gone, since a watcher rebuilding a - // peer tip re-reads that channel from the peer's genesis and a delivery - // that never executes never reaches the inbox seen-set to be recognised - // as delivered. Without this, one message that always fails would fill - // the list with copies of itself and evict every other one. + // One entry per delivery, not per retirement. A watcher rebuilding a + // peer tip re-reads from the peer's genesis, and a never-executing + // delivery never reaches the seen-set, so the same one retires again; + // undeduped it would evict every other entry with copies of itself. let mut dead_letters = self.get_dead_letter_cross_zone_dispatches()?; if !dead_letters .iter() @@ -844,17 +833,15 @@ impl RocksDBIO { dead_letters.remove(0); } } - // Counted per retirement even so: it measures how often this node gave - // up, which the retained list cannot, since that both evicts and drops - // entries whose delivery later settles. + // Counted per retirement even so: the retained list evicts and drops + // settled entries, so its length is not how often this node gave up. let count = self .get_dead_letter_cross_zone_dispatch_count()? .saturating_add(1); - // One batch: the record leaving the pending list and arriving in the - // dead letter is one event, and a crash between the two halves would - // either lose the message silently or leave the drain retrying a - // delivery already recorded as given up on. + // One batch: a crash between the two halves either loses the message + // silently or leaves the drain retrying a delivery already recorded as + // given up on. let mut batch = WriteBatch::default(); self.put_pending_cross_zone_dispatches_batch(&records, &mut batch)?; self.put_batch( @@ -907,10 +894,8 @@ impl RocksDBIO { records.retain(|record| !to_remove.contains(&record.message_key)); let removed = before.saturating_sub(records.len()); - // Both lists in one batch, for the same reason the retire path batches: - // a crash between them leaves the pending record gone and a dead letter - // behind saying the delivery was abandoned, and nothing recomputes these - // keys on a later pass to correct it. + // Both lists in one batch, as in `record_dispatch_failure`: nothing + // recomputes these keys on a later pass to fix a torn write. let mut batch = WriteBatch::default(); if removed > 0 { self.put_pending_cross_zone_dispatches_batch(&records, &mut batch)?; @@ -929,14 +914,12 @@ impl RocksDBIO { /// Stages the removal of dead letters whose delivery turned out to settle. /// - /// Giving up is this node's decision and every sequencer makes it alone, - /// against its own head and its own mempool ordering, so a delivery this one - /// stopped attempting can still reach a block another one produced. Left - /// alone, the entry would report that delivery as abandoned for as long as - /// the store lives, and nothing else removes one. + /// Every sequencer gives up alone, against its own head, so a delivery this + /// one abandoned can still reach another's block. Nothing else removes an + /// entry, so without this it reports as abandoned for the store's lifetime. /// - /// The count is deliberately not decremented. It records how often this node - /// gave up, which stays true whatever happened next. + /// The count is deliberately not decremented: it records how often this node + /// gave up, which stays true. fn stage_reconciled_dead_letters( &self, settled: &std::collections::HashSet<&[u8; 32]>, @@ -983,9 +966,8 @@ impl RocksDBIO { self.put_pending_cross_zone_dispatches_batch(&records, batch)?; } - // A settled delivery this node had given up on is not one it abandoned, - // and this is the path that catches the ordinary case: another sequencer - // carries it into a block that then becomes irreversible. + // The ordinary case: another sequencer carried a delivery this node gave + // up on into a block that just became irreversible. self.stage_reconciled_dead_letters(&to_remove, batch)?; Ok(removed) } diff --git a/lez/storage/src/sequencer/sequencer_cells.rs b/lez/storage/src/sequencer/sequencer_cells.rs index 7b066f2b3..8cbfe012d 100644 --- a/lez/storage/src/sequencer/sequencer_cells.rs +++ b/lez/storage/src/sequencer/sequencer_cells.rs @@ -297,9 +297,8 @@ pub struct PendingCrossZoneDispatchRecord { /// validated by nobody in between, so one can fail for good. A failure can /// equally be a property of the moment, so a single one is not enough to /// give up on a delivery. Once too many accumulate the record leaves this - /// list, which the drain re-feeds every turn, and a - /// [`DeadLetterDispatchRecord`] is kept in its place so the delivery this - /// node stopped attempting is still identifiable. + /// list (the drain re-feeds it every turn) for a + /// [`DeadLetterDispatchRecord`], which keeps the delivery identifiable. pub failed_attempts: u32, } @@ -361,29 +360,25 @@ pub struct DispatchOrigin { /// A cross-zone delivery this node has given up on. /// -/// A dispatch that fails execution is left out of the block, so unlike every -/// other failure in the pipeline this one leaves no on-chain trace that it was -/// ever attempted. This record is what makes it observable rather than a log -/// line that scrolls away. +/// A dispatch that fails execution is left out of the block, so nothing on chain +/// records that it was attempted; this is the only durable trace. /// -/// It identifies the message rather than carrying it. The peer block and -/// transaction index are enough to read the message back off the peer channel, -/// and the encoded transaction is chosen by the peer zone and can exceed this -/// node's whole block size limit, so retaining it would bound the list in -/// entries while leaving it unbounded in bytes. +/// It identifies the message rather than carrying it: the peer block and index +/// are enough to read it back off the channel, and the encoded transaction is +/// peer-chosen and can exceed a whole block, which would leave the list bounded +/// in entries but unbounded in bytes. /// -/// Giving up is this node's decision, not the network's: another sequencer may -/// carry the same delivery successfully, and a record here is dropped again if -/// that happens. +/// Giving up is this node's decision, not the network's, so an entry is dropped +/// again if another sequencer carries the same delivery. #[derive(Debug, Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)] pub struct DeadLetterDispatchRecord { pub message_key: [u8; 32], pub origin: DispatchOrigin, - /// Attempts made before giving up, recording the policy that was in force - /// rather than distinguishing one retirement from another. + /// Attempts made before giving up, so the record carries the policy that was + /// in force at the time. pub failed_attempts: u32, - /// Size of the delivery transaction that would not execute, which is the - /// diagnostic for the one failure mode that is about size. + /// Size of the delivery transaction that would not execute, the diagnostic + /// for size-related failures. pub transaction_bytes: u32, } @@ -422,10 +417,9 @@ impl SimpleWritableCell for DeadLetterCrossZoneDispatchesCellRef<'_> { /// Deliveries given up on since this store was created. /// -/// Counted separately from the retained list because that list both evicts at -/// its cap and drops entries whose delivery later settles, so its length is not -/// how many times this node has given up. A node that gave up hundreds of times -/// would otherwise look like one that gave up at the cap. +/// Separate from the retained list, which evicts at its cap and drops settled +/// entries: a node that gave up hundreds of times would otherwise look like one +/// that gave up at the cap. #[derive(BorshSerialize, BorshDeserialize)] pub struct DeadLetterCrossZoneDispatchCountCell(pub u64); diff --git a/lez/storage/src/sequencer/tests.rs b/lez/storage/src/sequencer/tests.rs index 900ca860e..2d4d2ed34 100644 --- a/lez/storage/src/sequencer/tests.rs +++ b/lez/storage/src/sequencer/tests.rs @@ -719,10 +719,7 @@ fn a_dead_letter_is_dropped_once_its_delivery_settles_elsewhere() { 1 ); - // Every sequencer decides to give up alone, against its own head, so a - // delivery this node stopped attempting can still reach a block another one - // produced. Reporting it as abandoned for ever afterwards is the failure - // this guards against. + // A delivery this node gave up on can still reach another sequencer's block. dbio.drop_settled_cross_zone_dispatches(&[key]).unwrap(); assert!( dbio.get_dead_letter_cross_zone_dispatches() @@ -775,11 +772,8 @@ fn one_delivery_that_always_fails_takes_one_dead_letter_slot() { let temp_dir = tempdir().unwrap(); let (dbio, _genesis) = dbio_with_genesis(temp_dir.path()); - // A watcher rebuilding a peer tip re-reads that channel from the peer's - // genesis, and a delivery that never executes never reaches the inbox - // seen-set to be recognised as delivered, so the same one is recorded and - // retired again. Without dedupe it would fill the list with copies of itself - // and evict every other message that was given up on. + // A watcher rebuilding a peer tip re-reads from genesis, so the same + // never-executing delivery retires repeatedly (see `record_dispatch_failure`). let key = key_from_index(1); let other = key_from_index(2); dbio.add_pending_cross_zone_dispatches(vec![PendingCrossZoneDispatchRecord::recorded( diff --git a/tools/dashboard_gen/src/dashboards/sequencer.rs b/tools/dashboard_gen/src/dashboards/sequencer.rs index 2098e876d..b75f120d3 100644 --- a/tools/dashboard_gen/src/dashboards/sequencer.rs +++ b/tools/dashboard_gen/src/dashboards/sequencer.rs @@ -195,9 +195,8 @@ pub fn dashboard() -> Dashboard { .row( 7, [ - // A dispatch that fails execution is left out of the block, so - // nothing on chain records that a delivery was abandoned. These - // are the only signal that it happened. + // A failed dispatch is left out of the block, so nothing on chain + // records it. These panels are the only signal. Panel::stat("Cross-zone deliveries given up on since startup") .width(6) .unit(Unit::Short)