From 7121b7f8d74ef0824b27e1eb46b98513e594c74b Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 13:08:33 -0500 Subject: [PATCH 01/13] feat(delegate): subscription introspection, and pin every wire tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `DelegateCtx::list_subscriptions` so a delegate can ask the node what it is subscribed to. Its subscription set lives in the node, not in the delegate: the WASM is instantiated per invocation and dropped afterwards, and the node replays subscriptions across a restart without running the delegate at all, so a delegate had no way to learn its own state (freenet-core#5467). It returns a `Result`, not a bare `Vec` — an empty list and a failed enumeration mean opposite things to a caller deciding whether to re-subscribe. Delivered as a V2 host function rather than a message variant. Host functions resolve by name at instantiation, so this is additive for every existing delegate and fails at load time with a named missing-import error on a node too old to provide it, instead of mid-protocol on a decode. Pins the bincode tag of EVERY variant of both delegate message enums. The previous pin covered `InboundDelegateMsg`'s variant 0 alone, so any reorder that left `ApplicationMessage` first went undetected — including swapping `UserResponse` and `GetContractResponse`, which reassigns two tags and makes deployed delegate WASM read each as the other, silently. That exact swap was written during the work that produced this pin. The guard fails closed both ways: the tag map is an exhaustive match, so a new variant is a compile error until pinned, and a probe asserts the next tag along does not decode. Also asserts the compatibility rules rather than only stating them. Appending an enum variant and appending a struct field break in opposite directions, and a struct field is the more dangerous: bincode is positional with no field tags, so an old payload fails outright on a new receiver, and a new field is skipped cleanly only when the struct is terminal in its message. `#[serde(default)]` does not make a bincode field optional; it protects the serde_json path only. Corrects two false doc comments: `InboundDelegateMsg` claimed `OutboundDelegateMsg` was `#[non_exhaustive]` (it never has been, and it is deliberately staying un-marked so the host cannot gain a variant without a handler), and `subscribe_contract` claimed notification delivery was a follow-up while saying nothing about the fact that a delegate subscription registers no demand in the network (freenet-core#4669). No wire change: no variant is added, removed or reordered. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- CHANGELOG.md | 90 ++++++ rust/src/client_api/client_events.rs | 224 ++++++++++++++ rust/src/delegate_host.rs | 210 ++++++++++++- rust/src/delegate_interface.rs | 439 ++++++++++++++++++++++++++- rust/src/lib.rs | 3 +- 5 files changed, 951 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 30c99d1..ff57df4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,96 @@ ## [Unreleased] +### Added + +- **`DelegateCtx::list_subscriptions`** — a delegate can now ask the node which + contracts it is subscribed to. Backed by two new V2 host functions in the + `freenet_delegate_contracts` import namespace, + `__frnt__delegate__list_subscriptions_len` and + `__frnt__delegate__list_subscriptions`, with + `encode_contract_id_list` / `decode_contract_id_list` as the shared codec. + + A delegate's subscription set lives in the node: the WASM is instantiated per + invocation and dropped afterwards, and the node replays subscriptions across a + restart without running the delegate at all. Until now a delegate had no way + to learn its own state after a restart — it could only keep a parallel record + in its secrets, which drifts from the node's exactly when it matters, or + re-subscribe to everything on every wake. See freenet-core#5467. + + It returns `Result, i64>`, not a bare `Vec`. An empty list and a + failed enumeration mean opposite things to a caller deciding whether to + re-subscribe, so they must not share a representation. + + **This is additive for every existing delegate.** Host functions resolve by + name at module instantiation, so a delegate that does not import it is + unaffected, and one that does fails to *load* on a node too old to provide it + — a named missing-import error, rather than a silent failure mid-protocol. + **Requires a node whose freenet-core registers these imports**; calling it + against an older node is a load-time failure, by design. + +### Fixed + +- **`InboundDelegateMsg`'s doc comment claimed `OutboundDelegateMsg` was + `#[non_exhaustive]`. It never has been.** The comment is corrected, and the + asymmetry is now documented as the deliberate choice it is, on both enums. + + `OutboundDelegateMsg` is **staying** un-marked, and should not be "fixed" by + marking it. Every variant is a request the host must act on, and freenet-core + dispatches them in exhaustive matches with no wildcard (`contract.rs`, in the + request loop and in the app-message filter). Marking the enum would force + those to grow `_ =>` arms, and a newly added variant would then compile + against the host with no handler — the delegate's request silently swallowed, + the call reporting success. That is the failure mode a delegate + `SubscribeContractRequest` has today, and the compile error is what prevents + the next one. `InboundDelegateMsg` keeps the attribute because its consumers + are third-party delegate WASM, which can reasonably ignore an unknown variant. + +- **`DelegateCtx::subscribe_contract`'s doc comment** said notification delivery + was "a follow-up" (it works), and said nothing about the fact that a delegate + subscription **registers no demand in the network** — it is a local + notification hook that does not pin the contract, enter the renewal set, or + exempt it from eviction, so a delegate only sees remote updates while some + other route keeps the node subscribed. The call succeeds either way, and + nothing distinguishes the two, which is why it is now documented at the call + site rather than left to be rediscovered. Tracked in freenet-core#4669. + +### Internal — wire-format guards + +- **Every variant of both delegate message enums now has its bincode tag + pinned** (`delegate_msg_variant_tags_are_pinned`). The previous pin covered + `InboundDelegateMsg`'s variant 0 alone, so any reorder that left + `ApplicationMessage` first went undetected — including swapping + `UserResponse` and `GetContractResponse`, which reassigns two tags and makes + deployed delegate WASM decode each as the other, silently. That exact swap was + written during the work that produced this pin, which is the argument for it. + + The guard fails closed in both directions: the tag map is an exhaustive + `match`, so a new variant is a compile error until it is pinned, and a probe + asserts that the next tag along does not decode, so a variant cannot be added + without the count constants noticing. + +- **The compatibility rules are now asserted, not just documented** + (`delegate_wire_compat` in `delegate_interface.rs`, `struct_field_wire_compat` + in `client_events.rs`). Worth reading before changing anything on the wire, + because appending an enum variant and appending a struct field break in + **opposite** directions: + + | change | old sender → new receiver | new sender → old receiver | + |---|---|---| + | append an enum variant | fine, old tags unchanged | hard error, unknown tag | + | append a struct field | **hard error**, unexpected end of input | silently ignored *if the struct is terminal in its message*; **silent corruption** if it is not | + + So a struct field is the more dangerous of the two: bincode is positional and + carries no field tags, so there is nothing for a decoder to skip, and + `#[serde(default)]` does not help — it is a self-describing-format feature and + protects the `serde_json` path only. The practical rule is to prefer a new + enum variant over a new field on an existing wire struct, since a variant is + only seen by a peer that asked for it. + + `NodeDiagnosticsResponse` happens to be terminal in its message, which is the + only reason a field could be appended to it without corrupting anything after + it. That property is now pinned rather than assumed. + ### TypeScript SDK — Breaking (npm package `@freenetorg/freenet-stdlib`; next release must be 0.4.0, not a patch) The npm package is versioned separately from the Rust crate. This release diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index 1364fae..0fddac3 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -3923,3 +3923,227 @@ mod fbs_decode_hardening { ); } } + +/// Executable evidence for what happens on the bincode wire when a **field** is +/// appended to a struct. +/// +/// This is a different question from appending a variant to an enum, and the +/// answers are not merely different — they break in the **opposite direction**. +/// Getting that backwards is easy and expensive, so the behaviour is pinned +/// here rather than reasoned about in a review comment. +/// +/// Summary of what these tests establish, for `bincode::serialize` / +/// `bincode::deserialize` as this crate uses them: +/// +/// | change | old sender to new receiver | new sender to old receiver | +/// |---|---|---| +/// | append an **enum variant** | fine (old tags unchanged) | hard error, unknown tag | +/// | append a **struct field** | **hard error**, unexpected end of input | silently ignored *if the struct is terminal*, **silent corruption** if it is not | +/// +/// So a struct field is the more dangerous of the two. It has no tag, so there +/// is nothing for a decoder to skip; bincode is positional and not +/// self-describing. `#[serde(default)]` does not help — see +/// `serde_default_does_not_rescue_a_missing_bincode_field`. +/// +/// The practical rule that follows: **prefer a new enum variant over a new +/// field on an existing wire struct.** A variant is only seen by a peer that +/// asked for it; a field changes what every existing peer decodes. +#[cfg(test)] +mod struct_field_wire_compat { + use super::{HostResponse, NodeDiagnosticsResponse, QueryResponse}; + use serde::{Deserialize, Serialize}; + + /// A wire struct before a field was appended. + #[derive(Serialize, Deserialize, Debug, PartialEq)] + struct OldShape { + first: u32, + second: String, + } + + /// The same struct after appending `added`, the way a new node would emit it. + #[derive(Serialize, Deserialize, Debug, PartialEq)] + struct NewShape { + first: u32, + second: String, + added: Vec, + } + + /// The same again, but with `#[serde(default)]` on the new field — the + /// annotation people reach for expecting it to make the change compatible. + #[derive(Serialize, Deserialize, Debug, PartialEq)] + struct NewShapeWithDefault { + first: u32, + second: String, + #[serde(default)] + added: Vec, + } + + fn old_value() -> OldShape { + OldShape { + first: 7, + second: "diagnostics".to_string(), + } + } + + fn new_value() -> NewShape { + NewShape { + first: 7, + second: "diagnostics".to_string(), + added: vec![0xDE, 0xAD], + } + } + + /// **New sender to old receiver, struct terminal: silently succeeds, and + /// the new field is dropped.** + /// + /// `bincode::deserialize` is configured `allow_trailing_bytes()` + /// (bincode-1.3.3 `src/lib.rs`), so the appended field's bytes are simply + /// left unread. No error, no warning — the old peer just never learns the + /// field exists. + /// + /// This is the benign case, and it is benign *only* because nothing follows + /// the struct in the encoding. See + /// `an_appended_field_corrupts_whatever_follows_it`. + #[test] + fn a_new_field_is_silently_ignored_by_an_old_receiver() { + let bytes = bincode::serialize(&new_value()).expect("new value must serialize"); + let decoded: OldShape = + bincode::deserialize(&bytes).expect("trailing bytes are allowed, so this succeeds"); + assert_eq!(decoded, old_value(), "the shared prefix decodes unchanged"); + } + + /// **Old sender to new receiver: hard decode error.** + /// + /// This is the direction that bites, and it is the reverse of the enum + /// case. A new client reading an old node's response runs off the end of + /// the input looking for a field the old node never wrote, and the whole + /// message fails — not just the new field. + /// + /// Concretely: appending a field to a response struct means a freshly-built + /// client cannot decode that response from **any** node not yet upgraded. + /// During a staged fleet rollout that is most of the fleet, and the tool + /// you would use to watch the rollout is the one that breaks. + #[test] + fn an_old_payload_fails_to_decode_once_a_field_is_appended() { + let bytes = bincode::serialize(&old_value()).expect("old value must serialize"); + let decoded = bincode::deserialize::(&bytes); + assert!( + decoded.is_err(), + "an old payload must NOT decode into a struct with an appended field; \ + if this ever passes, the compatibility table on this module is wrong" + ); + } + + /// `#[serde(default)]` does **not** rescue the case above. + /// + /// It is a self-describing-format feature: it fills a field whose *name* + /// was absent from the input. bincode carries no names and no field count, + /// so there is no "absent" to detect — the decoder just reads past the end + /// of the buffer and fails. `#[serde(default)]` on a bincode struct field + /// is therefore JSON-only protection, and reading it as wire compatibility + /// is a mistake worth naming explicitly. + /// + /// (`ContractState::size_bytes` in this file carries exactly this + /// annotation. It protects the `serde_json` report path, not the bincode + /// client path.) + #[test] + fn serde_default_does_not_rescue_a_missing_bincode_field() { + let bytes = bincode::serialize(&old_value()).expect("old value must serialize"); + assert!( + bincode::deserialize::(&bytes).is_err(), + "#[serde(default)] must not be mistaken for bincode wire compatibility" + ); + + // The same annotation genuinely does work for JSON, which is why it is + // easy to believe it works everywhere. + let json = serde_json::to_string(&old_value()).expect("old value must serialize to JSON"); + let from_json: NewShapeWithDefault = + serde_json::from_str(&json).expect("serde(default) fills the missing field in JSON"); + assert!(from_json.added.is_empty()); + } + + /// **The dangerous case: an appended field that is not terminal corrupts + /// whatever follows it, silently.** + /// + /// If the struct has siblings after it in the enclosing encoding, the extra + /// bytes are not trailing — they shift every subsequent field. An old + /// receiver then reads the new field's bytes *as* the next field and gets a + /// plausible, wrong value with no error at all. + /// + /// This is why "adding a field is fine, we checked" is not a conclusion you + /// can carry from one struct to another: whether it is safe depends on + /// where the struct sits in the message, not on the struct. + #[test] + fn an_appended_field_corrupts_whatever_follows_it() { + #[derive(Serialize, Deserialize, Debug)] + struct OldEnvelope { + payload: OldShape, + trailer: u32, + } + #[derive(Serialize, Deserialize, Debug)] + struct NewEnvelope { + payload: NewShape, + trailer: u32, + } + + let bytes = bincode::serialize(&NewEnvelope { + payload: new_value(), + trailer: 0xABCD_EF01, + }) + .expect("new envelope must serialize"); + + match bincode::deserialize::(&bytes) { + Ok(decoded) => assert_ne!( + decoded.trailer, 0xABCD_EF01, + "if this ever holds, bincode grew field framing and this whole module \ + needs revisiting" + ), + Err(_) => { + // Also an acceptable outcome, and the better one: the shifted + // bytes happened not to form a decodable value. The point of + // the test is that the field is NOT skipped cleanly, and both + // arms show that. + } + } + } + + /// `NodeDiagnosticsResponse` is **terminal** in its enclosing message, and + /// this pins that property. + /// + /// It is the only reason appending a field to it is survivable for old + /// clients at all (`a_new_field_is_silently_ignored_by_an_old_receiver`). + /// The property is invisible at the definition site — nothing next to + /// `NodeDiagnosticsResponse` says "must stay last" — so if a field is ever + /// added *after* the payload in `QueryResponse::NodeDiagnostics` or + /// `HostResponse::QueryResponse`, this fails and says why. + /// + /// Note what this does NOT license: appending to `NodeDiagnosticsResponse` + /// still breaks a new client talking to an old node, per + /// `an_old_payload_fails_to_decode_once_a_field_is_appended`. Terminality + /// buys one direction, not both. + #[test] + fn node_diagnostics_response_is_terminal_in_its_message() { + let response = NodeDiagnosticsResponse { + node_info: None, + network_info: None, + subscriptions: vec![], + contract_states: Default::default(), + system_metrics: None, + connected_peers_detailed: vec![], + }; + + let inner = bincode::serialize(&response).expect("response must serialize"); + let whole = bincode::serialize(&HostResponse::>::QueryResponse( + QueryResponse::NodeDiagnostics(response), + )) + .expect("host response must serialize"); + + assert!( + whole.ends_with(&inner), + "NodeDiagnosticsResponse must remain the LAST thing in its encoding. \ + Something now follows it, so appending a field to it would no longer be \ + trailing-byte-safe for older clients — it would silently corrupt whatever \ + was added after it." + ); + } +} diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index d11b1e5..6e64535 100644 --- a/rust/src/delegate_host.rs +++ b/rust/src/delegate_host.rs @@ -47,8 +47,22 @@ //! Survives across all delegate invocations. Use for private keys, tokens, etc. //! //! - **Contracts** (`get_contract_state`/`put_contract_state`/`update_contract_state`/ -//! `subscribe_contract`): V2 host functions for direct contract state access. -//! Synchronous local reads/writes — no request/response round-trips. +//! `subscribe_contract`/`list_subscriptions`): V2 host functions for direct +//! contract state access. Synchronous local reads/writes — no +//! request/response round-trips. +//! +//! # Adding a host function is the additive way to extend this API +//! +//! Host functions are resolved **by name at module instantiation**. A delegate +//! that imports one an older node does not provide fails to load, with a named +//! missing-import error; a delegate that does not import it is unaffected. So +//! adding a host function is additive for every existing delegate, and its +//! failure mode for a too-old node is loud and diagnosable at load time. +//! +//! Contrast the message API (`OutboundDelegateMsg`): a new variant sent to an +//! older host fails mid-protocol at bincode decode, with no way for the +//! delegate to have detected the host's version first. Where a capability can +//! be expressed either way, prefer the host function. //! //! # Error Codes //! @@ -181,6 +195,16 @@ extern "C" { ) -> i64; /// Subscribe to contract updates. Returns 0 on success, or negative error code (i64). fn __frnt__delegate__subscribe_contract(id_ptr: i64, id_len: i32) -> i64; + /// Byte length of this delegate's serialized subscription list — always a + /// multiple of 32. Returns the count to allocate, or a negative error code + /// (i64). Zero means "subscribed to nothing", which is distinct from an + /// error and must stay so. + fn __frnt__delegate__list_subscriptions_len() -> i64; + /// Enumerate this delegate's current contract subscriptions: writes the raw + /// 32-byte instance ids back to back into `out_ptr` (at most `out_len` + /// bytes) and returns the number of bytes written, or a negative error code + /// (i64). + fn __frnt__delegate__list_subscriptions(out_ptr: i64, out_len: i64) -> i64; } #[cfg(target_family = "wasm")] @@ -223,7 +247,8 @@ extern "C" { /// - [`get_contract_state`](Self::get_contract_state), /// [`put_contract_state`](Self::put_contract_state), /// [`update_contract_state`](Self::update_contract_state), -/// [`subscribe_contract`](Self::subscribe_contract) +/// [`subscribe_contract`](Self::subscribe_contract), +/// [`list_subscriptions`](Self::list_subscriptions) /// /// # Delegate Management Methods (V2) /// - [`create_delegate`](Self::create_delegate) @@ -599,11 +624,30 @@ impl DelegateCtx { /// Subscribe to contract updates by instance ID. /// - /// Registers interest in receiving notifications when the contract's state - /// changes. Currently validates that the contract is known and returns success; - /// actual notification delivery is a follow-up. + /// Registers interest in receiving `ContractNotification` when the + /// contract's state changes. Notification delivery does work — an earlier + /// version of this comment said it was "a follow-up", which is no longer + /// true. + /// + /// # What this does not do, as of 0.9.0 /// - /// Returns `true` on success, `false` if the contract is unknown or on error. + /// **A delegate subscription does not register demand in the network.** It + /// is a local notification hook: it does not mark the contract as in use, + /// does not enter the renewal set, and does not exempt the contract from + /// eviction. So a delegate sees a remote update only while the node happens + /// to be subscribed to that contract by some *other* route — typically a UI + /// client that is open. Close the tab and the notifications stop, without + /// any error being reported anywhere. + /// + /// This is tracked as freenet-core#4669 (phase 1 of the freenet-core#5467 + /// epic) and is being fixed. It is documented here rather than left to be + /// rediscovered because the call *succeeds*: nothing in the return value, + /// the logs, or the delegate's own view distinguishes a subscription that + /// pinned the contract from one that did nothing. + /// + /// Returns `true` on success, `false` if the contract is unknown or on + /// error. Note that the contract must already be in the node's local store; + /// subscribing does not fetch it. pub fn subscribe_contract(&mut self, instance_id: &[u8; 32]) -> bool { #[cfg(target_family = "wasm")] { @@ -618,6 +662,68 @@ impl DelegateCtx { } } + /// List the contract instance ids this delegate is currently subscribed to. + /// + /// A delegate's subscription set lives in the node, not in the delegate. + /// The WASM is instantiated per invocation and dropped immediately after, + /// and the node replays subscriptions across a restart without running the + /// delegate at all. So without this call a delegate has no way to learn + /// what it is already subscribed to: it can only keep a parallel record in + /// its own secrets, which drifts from the node's exactly in the cases that + /// matter, or re-subscribe to everything on every wake. This is the gap + /// freenet-core#5467 names for restart-replay. + /// + /// Order is unspecified; do not depend on it. + /// + /// # Why this returns a `Result` + /// + /// An empty list and a failed enumeration mean opposite things to the + /// caller — "you hold no subscriptions, take them out again" versus "I + /// could not tell you" — so they must not be represented by the same + /// value. Collapsing them into an empty `Vec` is how a delegate ends up + /// concluding its user's content is unpinned because a host call failed. + /// The error is the raw host code (see [`error_codes`]). + /// + /// Off-WASM this is `Err(ERR_NOT_IN_PROCESS)` rather than an empty list, + /// for the same reason: a host-side unit test must not be able to read + /// "no subscriptions" out of a stub that never had any. + /// + /// **Host version floor:** this import is provided by nodes built against + /// freenet-stdlib 0.9.0 or later. A delegate that calls it fails to + /// instantiate on an older node with a named missing-import error — loud + /// and diagnosable at load time, rather than silently mid-protocol. + pub fn list_subscriptions(&self) -> Result, i64> { + #[cfg(target_family = "wasm")] + { + // Step 1: how many bytes to allocate. Zero is a valid answer and + // means "subscribed to nothing". + let len = unsafe { __frnt__delegate__list_subscriptions_len() }; + if len < 0 { + return Err(len); + } + if len == 0 { + return Ok(Vec::new()); + } + + // Step 2: read the ids. The host may write fewer bytes than it + // reported if the set shrank between the two calls, so the return + // value, not `len`, is authoritative. + let mut buf = vec![0u8; len as usize]; + let written = unsafe { + __frnt__delegate__list_subscriptions(buf.as_mut_ptr() as i64, buf.len() as i64) + }; + if written < 0 { + return Err(written); + } + buf.truncate(written as usize); + decode_contract_id_list(&buf).ok_or(error_codes::ERR_STORE_ERROR as i64) + } + #[cfg(not(target_family = "wasm"))] + { + Err(error_codes::ERR_NOT_IN_PROCESS as i64) + } + } + /// Create a new child delegate from WASM bytecode and parameters. /// /// This V2 host function allows a delegate to spawn new delegates at runtime. @@ -684,6 +790,55 @@ impl std::fmt::Debug for DelegateCtx { } } +// ============================================================================ +// Contract-id-list wire codec (shared host↔delegate contract for +// list_subscriptions) +// ============================================================================ + +/// Serialize contract instance ids for [`DelegateCtx::list_subscriptions`]: the +/// raw 32 bytes of each id, back to back, with no framing. +/// +/// Ids are fixed width, so unlike the secret-key list below they need no length +/// prefix. The encoding lives here rather than only in the host so that both +/// sides and the round-trip tests share one authoritative definition — a codec +/// written twice is a codec that will disagree with itself eventually. +pub fn encode_contract_id_list<'a, I>(ids: I) -> Vec +where + I: IntoIterator, +{ + let mut out = Vec::new(); + for id in ids { + out.extend_from_slice(id); + } + out +} + +/// Decode the format written by [`encode_contract_id_list`], or `None` if the +/// buffer is not a whole number of ids. +/// +/// This is deliberately stricter than [`decode_secret_key_list`], which +/// tolerates a truncated trailing record. Ids are fixed width, so a length that +/// is not a multiple of 32 cannot be a short read of a valid list — it is a +/// host-side bug or a corrupted buffer, and the only honest answer is that the +/// enumeration failed. Silently dropping a partial id would hand the delegate +/// a list that looks complete and is not, which is precisely the class of +/// failure this API exists to remove. +pub fn decode_contract_id_list(buf: &[u8]) -> Option> { + let chunks = buf.chunks_exact(32); + if !chunks.remainder().is_empty() { + return None; + } + Some( + chunks + .map(|chunk| { + let mut id = [0u8; 32]; + id.copy_from_slice(chunk); + id + }) + .collect(), + ) +} + // ============================================================================ // Secret-key-list wire codec (shared host↔delegate contract for list_secrets) // ============================================================================ @@ -768,3 +923,44 @@ mod secret_key_list_codec_tests { assert_eq!(decode_secret_key_list(&encoded), vec![b"abc".to_vec()]); } } + +#[cfg(test)] +mod contract_id_list_codec_tests { + use super::{decode_contract_id_list, encode_contract_id_list}; + + #[test] + fn round_trips_multiple_ids() { + let ids = [[0x01u8; 32], [0xFEu8; 32], [0x00u8; 32]]; + let encoded = encode_contract_id_list(ids.iter()); + assert_eq!( + encoded.len(), + 96, + "ids are fixed width, so the encoding carries no framing" + ); + assert_eq!(decode_contract_id_list(&encoded), Some(ids.to_vec())); + } + + #[test] + fn round_trips_the_empty_list() { + let encoded = encode_contract_id_list(std::iter::empty::<&[u8; 32]>()); + assert!(encoded.is_empty()); + assert_eq!( + decode_contract_id_list(&encoded), + Some(vec![]), + "an empty buffer is a successful enumeration of nothing, NOT an error — \ + a delegate must be able to tell those apart" + ); + } + + #[test] + fn rejects_a_truncated_trailing_id() { + let mut encoded = encode_contract_id_list([&[0xAAu8; 32], &[0xBBu8; 32]]); + encoded.truncate(encoded.len() - 1); + assert_eq!( + decode_contract_id_list(&encoded), + None, + "a partial id means the buffer is wrong; returning the ids that did parse \ + would hand the caller a list that looks complete and is not" + ); + } +} diff --git a/rust/src/delegate_interface.rs b/rust/src/delegate_interface.rs index ae6558f..573514d 100644 --- a/rust/src/delegate_interface.rs +++ b/rust/src/delegate_interface.rs @@ -513,14 +513,42 @@ impl AsRef<[u8]> for DelegateContext { /// Messages delivered **into** a delegate's `process()` function. /// /// This is the inbound counterpart of [`OutboundDelegateMsg`] and sits on the -/// host↔delegate wire boundary. Marked `#[non_exhaustive]` so future variants -/// can be added without a source-level break; downstream `match` sites must -/// include a wildcard arm. This matches the pre-existing `#[non_exhaustive]` -/// on `OutboundDelegateMsg`. +/// host↔delegate wire boundary. /// -/// Wire format: bincode with variant index 0..=N in declaration order. The -/// `inbound_delegate_msg_wire_format_is_stable` test pins the bytes for -/// `ApplicationMessage(..)` so that refactors cannot silently shift the tag. +/// Marked `#[non_exhaustive]` so future variants can be added without a +/// source-level break; downstream `match` sites must include a wildcard arm. +/// [`OutboundDelegateMsg`] is deliberately **not** marked, and the asymmetry is +/// the point — see the rationale on that enum. (An earlier version of this +/// comment asserted that `OutboundDelegateMsg` already carried the attribute. +/// It never has.) +/// +/// # Wire format and compatibility +/// +/// bincode, variant index 0..=N in **declaration order**. Two rules follow, and +/// the compiler enforces neither: +/// +/// - **Never insert or reorder a variant.** That silently reassigns every later +/// tag, so delegate WASM compiled against an older stdlib decodes the same +/// bytes into a *different* variant — no error, just a message quietly +/// reinterpreted as another one. `delegate_msg_variant_tags_are_pinned` pins +/// the tag of every variant of both enums so a reorder fails CI instead. +/// - **Appending is compatible in exactly one direction.** An old sender's old +/// variant always decodes on a new receiver. A **new** sender's **new** +/// variant does **not** decode on an old receiver: bincode rejects the +/// unknown tag with `ErrorKind::InvalidTagEncoding`. `#[non_exhaustive]` does +/// not change this — it is a source-level attribute with no effect on the +/// encoding, and serde has no unknown-variant fallback to fall back to. +/// +/// For this enum the incompatible direction is a **new host → old delegate**, +/// and it is mostly unreachable in practice: the host emits a response variant +/// only in reply to the matching request variant, so a delegate that never +/// emits a request added in stdlib version X never receives the response added +/// in X. Deployed delegate WASM therefore keeps working against an upgraded +/// node. The genuinely constrained direction is delegate → host; see +/// [`OutboundDelegateMsg`]. +/// +/// The compatibility claims above are asserted, not merely asserted-in-prose, +/// by the `delegate_wire_compat` test module at the bottom of this file. #[non_exhaustive] #[derive(Serialize, Deserialize, Debug, Clone)] pub enum InboundDelegateMsg<'a> { @@ -697,6 +725,65 @@ impl UserInputResponse<'_> { } } +/// Messages emitted **out of** a delegate's `process()` function. +/// +/// This is the outbound counterpart of [`InboundDelegateMsg`] and sits on the +/// same host↔delegate wire boundary. +/// +/// # Deliberately not `#[non_exhaustive]` +/// +/// Adding a variant here is a source-level break for any downstream crate that +/// matches on it exhaustively. That is the intended behaviour and it should not +/// be "fixed" by marking the enum. +/// +/// Every variant of this enum is a **request the host must act on**. There is +/// one host — freenet-core — and it dispatches these in exhaustive matches with +/// no wildcard (`crates/core/src/contract.rs`, in the request loop and again in +/// the app-message filter). Marking this enum `#[non_exhaustive]` would force +/// those matches to grow `_ =>` arms, and a newly added variant would then +/// compile against the host with **no handler**: the delegate's request would be +/// silently swallowed, the call would appear to succeed, and nothing anywhere +/// would report that it did nothing. That failure mode is not hypothetical — it +/// is what a delegate `SubscribeContractRequest` does today, and the reason this +/// workstream exists. +/// +/// The compile error is the mechanism that stops it. Keep it. +/// +/// [`InboundDelegateMsg`] carries the opposite trade-off, and is marked: its +/// consumers are third-party delegate WASM, which can reasonably ignore a +/// variant it does not know about. +/// +/// # Wire format and compatibility +/// +/// bincode, variant index 0..=N in **declaration order**. Never insert or +/// reorder a variant: that silently reassigns every later tag, and deployed +/// delegate WASM built against an older stdlib would encode into what the host +/// now reads as a different variant. `delegate_msg_variant_tags_are_pinned` +/// pins every tag so a reorder fails CI rather than production. +/// +/// Appending is compatible in one direction only, and this enum is the +/// direction that bites: +/// +/// - **Old delegate → new host: always fine.** The host understands every tag +/// an older delegate can emit, so deployed delegate WASM keeps working +/// against an upgraded node with no rebuild. +/// - **New delegate → old host: fails, and fails loudly.** bincode rejects the +/// unknown variant tag with `ErrorKind::InvalidTagEncoding`, so the host +/// surfaces a decode error on that message rather than misreading it. +/// +/// There is deliberately **no feature-detection handshake**. A delegate cannot +/// ask the host which variants it understands, and adding a probe would itself +/// be a wire change with the same bootstrapping problem. The rule is therefore +/// the blunt one: **a delegate that emits a variant introduced in stdlib +/// version X requires a host built against stdlib >= X.** +/// +/// A delegate that must work against older hosts has one good alternative: the +/// V2 host-function API (the `freenet_delegate_contracts` import namespace). +/// Host functions are resolved **by name at module instantiation**, so an +/// import an old host does not provide fails at load time with a named +/// missing-import error, instead of mid-protocol on a decode. That is the +/// better failure mode, and it is why new capabilities should prefer a host +/// function over a new variant where there is a choice. #[derive(Serialize, Deserialize, Debug, Clone)] pub enum OutboundDelegateMsg { // for the apps @@ -1249,3 +1336,341 @@ mod message_origin_tests { assert!(matches!(decoded, InboundDelegateMsg::ApplicationMessage(_))); } } + +/// Executable evidence for the wire-compatibility rules documented on +/// [`InboundDelegateMsg`] and [`OutboundDelegateMsg`]. +/// +/// The claims those doc comments make about bincode's behaviour are asserted +/// here rather than believed, because every one of them is the kind of claim +/// that is easy to state, easy to get backwards, and impossible to notice being +/// wrong until deployed delegate WASM misreads a message in production. +#[cfg(test)] +mod delegate_wire_compat { + use super::*; + use crate::contract_interface::WrappedContract; + use crate::prelude::ContractCode; + use crate::versioning::ContractWasmAPIVersion; + use std::sync::Arc; + + /// The number of variants each enum has **today**. These are not free + /// parameters: see `an_unpinned_variant_fails_this_test`, which is what + /// makes them fail closed rather than drift. + const INBOUND_VARIANT_COUNT: u32 = 8; + const OUTBOUND_VARIANT_COUNT: u32 = 8; + + fn instance_id() -> ContractInstanceId { + ContractInstanceId::new([0x5Au8; 32]) + } + + fn delegate_key() -> DelegateKey { + DelegateKey::new([0x11u8; 32], CodeHash::new([0x22u8; 32])) + } + + fn contract_container() -> ContractContainer { + ContractContainer::Wasm(ContractWasmAPIVersion::V1(WrappedContract::new( + Arc::new(ContractCode::from(vec![1u8, 2, 3])), + Parameters::from(vec![9u8, 8, 7]), + ))) + } + + /// The bincode variant tag actually on the wire: a 4-byte little-endian + /// u32 prefix (this workspace's bincode config uses fixint encoding). + fn wire_tag(encoded: &[u8]) -> u32 { + u32::from_le_bytes( + encoded[..4] + .try_into() + .expect("a bincode enum encoding starts with a 4-byte tag"), + ) + } + + /// The tag each [`InboundDelegateMsg`] variant is frozen at, forever. + /// + /// This match is **exhaustive on purpose**. `#[non_exhaustive]` has no + /// effect inside the crate that defines the enum, so adding a variant + /// without adding an arm here is a **compile error** — which is the point. + /// A new variant cannot slip in unpinned. + /// + /// If you are here because you added a variant: give it the next unused + /// number, append it at the END of the enum, add it to `every_inbound` + /// below, and bump `INBOUND_VARIANT_COUNT`. Do not renumber anything. + fn pinned_inbound_tag(msg: &InboundDelegateMsg<'_>) -> u32 { + match msg { + InboundDelegateMsg::ApplicationMessage(_) => 0, + InboundDelegateMsg::UserResponse(_) => 1, + InboundDelegateMsg::GetContractResponse(_) => 2, + InboundDelegateMsg::PutContractResponse(_) => 3, + InboundDelegateMsg::UpdateContractResponse(_) => 4, + InboundDelegateMsg::SubscribeContractResponse(_) => 5, + InboundDelegateMsg::ContractNotification(_) => 6, + InboundDelegateMsg::DelegateMessage(_) => 7, + } + } + + /// The tag each [`OutboundDelegateMsg`] variant is frozen at, forever. + /// Exhaustive for the same reason as [`pinned_inbound_tag`]. + fn pinned_outbound_tag(msg: &OutboundDelegateMsg) -> u32 { + match msg { + OutboundDelegateMsg::ApplicationMessage(_) => 0, + OutboundDelegateMsg::RequestUserInput(_) => 1, + OutboundDelegateMsg::ContextUpdated(_) => 2, + OutboundDelegateMsg::GetContractRequest(_) => 3, + OutboundDelegateMsg::PutContractRequest(_) => 4, + OutboundDelegateMsg::UpdateContractRequest(_) => 5, + OutboundDelegateMsg::SubscribeContractRequest(_) => 6, + OutboundDelegateMsg::SendDelegateMessage(_) => 7, + } + } + + /// One value of every [`InboundDelegateMsg`] variant. + fn every_inbound() -> Vec> { + let id = instance_id(); + let ctx = DelegateContext::default(); + vec![ + InboundDelegateMsg::ApplicationMessage(ApplicationMessage::new(vec![0xCC])), + InboundDelegateMsg::UserResponse(UserInputResponse { + request_id: 7, + response: ClientResponse::new(vec![0x01]), + context: ctx.clone(), + }), + InboundDelegateMsg::GetContractResponse(GetContractResponse { + contract_id: id, + state: None, + context: ctx.clone(), + }), + InboundDelegateMsg::PutContractResponse(PutContractResponse { + contract_id: id, + result: Ok(()), + context: ctx.clone(), + }), + InboundDelegateMsg::UpdateContractResponse(UpdateContractResponse { + contract_id: id, + result: Ok(()), + context: ctx.clone(), + }), + InboundDelegateMsg::SubscribeContractResponse(SubscribeContractResponse { + contract_id: id, + result: Ok(()), + context: ctx.clone(), + }), + InboundDelegateMsg::ContractNotification(ContractNotification { + contract_id: id, + new_state: WrappedState::new(vec![0xAB]), + context: ctx.clone(), + }), + InboundDelegateMsg::DelegateMessage(DelegateMessage::new( + delegate_key(), + delegate_key(), + vec![0xEE], + )), + ] + } + + /// One value of every [`OutboundDelegateMsg`] variant. + /// + /// Every variant is covered, `PutContractRequest` included: building a + /// `ContractContainer` is four lines (see `contract_container`), and a pin + /// test with a hole in it is exactly the shape of guard that reads as + /// coverage while providing none. + fn every_outbound() -> Vec { + let id = instance_id(); + vec![ + OutboundDelegateMsg::ApplicationMessage(ApplicationMessage::new(vec![0xCC])), + OutboundDelegateMsg::RequestUserInput(UserInputRequest { + request_id: 7, + message: NotificationMessage(Cow::Owned(vec![0x02])), + responses: vec![], + }), + OutboundDelegateMsg::ContextUpdated(DelegateContext::default()), + OutboundDelegateMsg::GetContractRequest(GetContractRequest::new(id)), + OutboundDelegateMsg::PutContractRequest(PutContractRequest::new( + contract_container(), + WrappedState::new(vec![0xAB]), + RelatedContracts::default(), + )), + OutboundDelegateMsg::UpdateContractRequest(UpdateContractRequest::new( + id, + UpdateData::State(vec![0xAB].into()), + )), + OutboundDelegateMsg::SubscribeContractRequest(SubscribeContractRequest::new(id)), + OutboundDelegateMsg::SendDelegateMessage(DelegateMessage::new( + delegate_key(), + delegate_key(), + vec![0xEE], + )), + ] + } + + /// Pins the bincode variant tag of **every** variant of both delegate + /// message enums. + /// + /// The pin this replaces covered `InboundDelegateMsg`'s variant 0 alone, so + /// any reorder that happened to leave `ApplicationMessage` first — swapping + /// `UserResponse` and `GetContractResponse`, say — went undetected. That is + /// not a theoretical gap: exactly that swap was written, and staged, during + /// the work that produced this test. + /// + /// A reorder is the dangerous edit precisely because it is silent. The + /// bytes still decode. They decode into the wrong variant, and the failure + /// surfaces as a delegate acting on a message it was never sent. + /// + /// **If this test fails, do not update the expected numbers.** Either a + /// variant was inserted or reordered (revert it; append instead), or one + /// was removed — which reassigns every later tag and is a wire break + /// needing a deliberate release decision. See the + /// `RegisterDelegateWithPredecessors` removal in 0.9.0 for the shape of + /// that decision: it was appended last specifically so that removing it + /// renumbered nothing. + #[test] + fn delegate_msg_variant_tags_are_pinned() { + for msg in every_inbound() { + let expected = pinned_inbound_tag(&msg); + let encoded = bincode::serialize(&msg).expect("inbound must serialize"); + assert_eq!( + wire_tag(&encoded), + expected, + "InboundDelegateMsg::{msg:?} moved off wire tag {expected}; inserting, \ + reordering or removing variants breaks deployed delegate WASM" + ); + } + + for msg in every_outbound() { + let expected = pinned_outbound_tag(&msg); + let encoded = bincode::serialize(&msg).expect("outbound must serialize"); + assert_eq!( + wire_tag(&encoded), + expected, + "OutboundDelegateMsg::{msg:?} moved off wire tag {expected}; inserting, \ + reordering or removing variants breaks deployed delegate WASM" + ); + } + } + + /// Every variant is actually exercised by the pin above. + /// + /// [`pinned_inbound_tag`] is exhaustive, so a new variant cannot be left + /// unpinned without a compile error — but it *could* be left out of + /// `every_inbound`, and then the pin would silently stop covering it. + /// Asserting that the sampled tags are exactly `0..COUNT`, with no gaps and + /// no repeats, closes that. + #[test] + fn every_variant_is_covered_by_the_pin() { + let mut inbound: Vec = every_inbound().iter().map(pinned_inbound_tag).collect(); + inbound.sort_unstable(); + assert_eq!( + inbound, + (0..INBOUND_VARIANT_COUNT).collect::>(), + "every_inbound must contain each InboundDelegateMsg variant exactly once" + ); + + let mut outbound: Vec = every_outbound().iter().map(pinned_outbound_tag).collect(); + outbound.sort_unstable(); + assert_eq!( + outbound, + (0..OUTBOUND_VARIANT_COUNT).collect::>(), + "every_outbound must contain each OutboundDelegateMsg variant exactly once" + ); + } + + /// The count constants above cannot be allowed to drift, so this probes the + /// enums themselves: a payload whose tag is one past the last known variant + /// must fail to decode. + /// + /// This is the test that fails **closed**. Add a variant and forget + /// everything else here, and the tag that was previously undecodable + /// becomes decodable, and this fails. Without it, `INBOUND_VARIANT_COUNT` + /// would be a number asserted only against a list written by the same hand + /// in the same commit — which is not a check, it is a restatement. + /// + /// The payload is a run of zero bytes after the tag, which decodes as + /// empty vectors, `None`, `Ok`, `false` and zeroed arrays, so it satisfies + /// essentially any variant shape a new variant is likely to have. Trailing + /// bytes are ignored: `bincode::deserialize` configures + /// `allow_trailing_bytes()` (bincode-1.3.3 `src/lib.rs`), which is also why + /// a fixed-size probe is safe here. + #[test] + fn an_unpinned_variant_fails_this_test() { + let mut probe = INBOUND_VARIANT_COUNT.to_le_bytes().to_vec(); + probe.extend_from_slice(&[0u8; 256]); + let decoded = bincode::deserialize::>(&probe); + assert!( + decoded.is_err(), + "tag {INBOUND_VARIANT_COUNT} decoded as an InboundDelegateMsg, so a variant was \ + added without updating INBOUND_VARIANT_COUNT, pinned_inbound_tag and every_inbound" + ); + + let mut probe = OUTBOUND_VARIANT_COUNT.to_le_bytes().to_vec(); + probe.extend_from_slice(&[0u8; 256]); + let decoded = bincode::deserialize::(&probe); + assert!( + decoded.is_err(), + "tag {OUTBOUND_VARIANT_COUNT} decoded as an OutboundDelegateMsg, so a variant was \ + added without updating OUTBOUND_VARIANT_COUNT, pinned_outbound_tag and \ + every_outbound" + ); + } + + /// Direction 1 of the append rule: **old sender to new receiver always + /// works.** Bytes produced before a variant was appended still decode into + /// the variant they always meant. + /// + /// The payload here is hand-built rather than produced by this crate's own + /// encoder, so it stands in for bytes emitted by a delegate compiled + /// against an older stdlib. An encoder-produced value would only prove the + /// code agrees with itself. + #[test] + fn an_old_payload_still_decodes_after_appending_a_variant() { + // InboundDelegateMsg tag 6 = ContractNotification { contract_id, + // new_state: WrappedState (empty), context: DelegateContext (empty) }. + let mut old_payload = vec![6u8, 0, 0, 0]; + old_payload.extend_from_slice(&[0x5Au8; 32]); + old_payload.extend_from_slice(&0u64.to_le_bytes()); // new_state: len 0 + old_payload.extend_from_slice(&0u64.to_le_bytes()); // context: len 0 + + let decoded: InboundDelegateMsg<'_> = bincode::deserialize(&old_payload) + .expect("a payload predating any appended variant must still decode"); + match decoded { + InboundDelegateMsg::ContractNotification(n) => { + assert_eq!(n.contract_id, instance_id()); + } + other => panic!("an old ContractNotification decoded as {other:?}"), + } + } + + /// Direction 2 of the append rule: **new sender to old receiver fails, and + /// fails loudly.** This is the direction the docs warn about, so it is + /// asserted rather than assumed. + /// + /// An old receiver is modelled by an enum with a truncated tag space, + /// which is exactly what an older stdlib's version of these types is. The + /// point is that the failure is an `Err` — not a silent mis-decode into + /// whatever variant happens to sit at that index. + #[test] + fn a_new_variant_does_not_decode_on_an_old_receiver() { + // An "old" OutboundDelegateMsg that knows tags 0..=6 only, i.e. one + // built before `SendDelegateMessage` was appended at 7. + #[derive(serde::Deserialize, Debug)] + enum OldOutboundTagSpace { + V0, + V1, + V2, + V3, + V4, + V5, + V6, + } + + let new_msg = bincode::serialize(&OutboundDelegateMsg::SendDelegateMessage( + DelegateMessage::new(delegate_key(), delegate_key(), vec![0xEE]), + )) + .expect("outbound must serialize"); + assert_eq!(wire_tag(&new_msg), 7); + + let decoded = bincode::deserialize::(&new_msg); + assert!( + decoded.is_err(), + "a receiver that predates a variant must REJECT it, not mis-decode it; \ + if this ever passes, the compatibility rule documented on \ + OutboundDelegateMsg is wrong and delegates are silently misreading messages" + ); + } +} diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 40b2c18..15e45d0 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -39,7 +39,8 @@ pub mod prelude { pub use crate::contract_interface::wasm_interface::ContractInterfaceResult; pub use crate::contract_interface::*; pub use crate::delegate_host::{ - decode_secret_key_list, encode_secret_key_list, error_codes, DelegateCtx, + decode_contract_id_list, decode_secret_key_list, encode_contract_id_list, + encode_secret_key_list, error_codes, DelegateCtx, }; pub use crate::delegate_interface::wasm_interface::DelegateInterfaceResult; pub use crate::delegate_interface::*; From d856e90bae72bdd5e1eed0f2f193ce4c8ec4504a Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 13:13:57 -0500 Subject: [PATCH 02/13] chore(delegate): silence dead_code on the deliberately-unconstructed test enums The old-tag-space and envelope types in the compat tests exist to occupy wire space and to be deserialized into, never to be constructed, which trips dead_code under CI's -D warnings. Also drops a doc comment's reference to what a previous version of that same comment said, which is of no use to a reader of the published API. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/client_api/client_events.rs | 1 + rust/src/delegate_host.rs | 5 ++--- rust/src/delegate_interface.rs | 3 +++ 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index 0fddac3..cd81fbc 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -4075,6 +4075,7 @@ mod struct_field_wire_compat { /// where the struct sits in the message, not on the struct. #[test] fn an_appended_field_corrupts_whatever_follows_it() { + #[allow(dead_code)] // `payload` exists to occupy wire space, not to be read #[derive(Serialize, Deserialize, Debug)] struct OldEnvelope { payload: OldShape, diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index 6e64535..49320d5 100644 --- a/rust/src/delegate_host.rs +++ b/rust/src/delegate_host.rs @@ -625,9 +625,8 @@ impl DelegateCtx { /// Subscribe to contract updates by instance ID. /// /// Registers interest in receiving `ContractNotification` when the - /// contract's state changes. Notification delivery does work — an earlier - /// version of this comment said it was "a follow-up", which is no longer - /// true. + /// contract's state changes. Delivery works, and covers state committed + /// locally as well as state arriving from the network. /// /// # What this does not do, as of 0.9.0 /// diff --git a/rust/src/delegate_interface.rs b/rust/src/delegate_interface.rs index 573514d..b817436 100644 --- a/rust/src/delegate_interface.rs +++ b/rust/src/delegate_interface.rs @@ -1648,6 +1648,9 @@ mod delegate_wire_compat { fn a_new_variant_does_not_decode_on_an_old_receiver() { // An "old" OutboundDelegateMsg that knows tags 0..=6 only, i.e. one // built before `SendDelegateMessage` was appended at 7. + // Variants are only ever produced by deserialization, never + // constructed here — which is the whole point of the test. + #[allow(dead_code)] #[derive(serde::Deserialize, Debug)] enum OldOutboundTagSpace { V0, From e48d1c44b53623b61f7402b76e8607842f6f75a0 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 13:18:31 -0500 Subject: [PATCH 03/13] test(wire): pin the shipped size_bytes append as a real instance of the rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ContractState::size_bytes was appended in #52 (2026-02-18, crate 0.1.36) and ships in every tag from rust-v0.8.0. ContractState is a HashMap VALUE inside NodeDiagnosticsResponse with two more fields after the map, so the appended u64 is not trailing — it shifts what follows, and an older reader does not get a response missing one field, it gets no response at all. Isolates the single variable by using today's String map key, so it measures the appended field rather than the later key change in #70. Scope, stated so the finding is not read as larger than it is: the only external consumer of this query is fdev diagnostics, which ships from core's own tree and is version-matched in practice; River touches NodeDiagnostics only in tests and pins stdlib 0.8.5. The exposure is an fdev built before 2026-02-18 pointed at a newer node. The value here is the rule with a real instance attached, not the instance. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/client_api/client_events.rs | 99 ++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index cd81fbc..e75721a 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -4147,4 +4147,103 @@ mod struct_field_wire_compat { was added after it." ); } + + /// The rule above, in code that has already shipped. + /// + /// `ContractState::size_bytes` was appended in #52 (2026-02-18, crate + /// version 0.1.36) and is present in every released tag from `rust-v0.8.0` + /// onward. `ContractState` is a `HashMap` **value** inside + /// `NodeDiagnosticsResponse`, and two more fields follow the map — so the + /// appended `u64` is not trailing. It shifts everything after it. + /// + /// A client built before that commit, querying a node built after it, does + /// not get a diagnostics response with one field missing. It gets **no + /// diagnostics response at all**: the decoder reads the appended `u64` as + /// the next value in sequence and fails, or worse, does not. + /// + /// This test isolates that single variable — it uses today's `String` map + /// key, so it measures the effect of the appended field and not of the + /// later key change in #70. + /// + /// Nothing here is fixable after the fact; the released bytes are the + /// released bytes. It is pinned as the concrete instance of why the table + /// on this module matters, and because a rule with a real example attached + /// is the one people believe. + #[test] + fn the_shipped_size_bytes_append_is_an_instance_of_this() { + use super::{ + ConnectedPeerInfo, ContractState, NetworkInfo, NodeInfo, SubscriptionInfo, + SystemMetrics, + }; + use crate::contract_interface::ContractInstanceId; + use std::collections::HashMap; + + /// `ContractState` as it was before #52 appended `size_bytes`. + #[derive(Serialize, Deserialize, Debug)] + struct OldContractState { + subscribers: u32, + subscriber_peer_ids: Vec, + } + + /// `NodeDiagnosticsResponse` as an older client sees it: identical in + /// every respect except the map's value type. + #[allow(dead_code)] + #[derive(Serialize, Deserialize, Debug)] + struct OldNodeDiagnosticsResponse { + node_info: Option, + network_info: Option, + subscriptions: Vec, + contract_states: HashMap, + system_metrics: Option, + connected_peers_detailed: Vec, + } + + let mut contract_states = HashMap::new(); + contract_states.insert( + "6kVs66bKaQAC6ohr8b43SvJ95r36tc2hnG7HezmaJHF9".to_string(), + ContractState { + subscribers: 3, + subscriber_peer_ids: vec!["peer-a".to_string()], + size_bytes: 1024, + }, + ); + + let new_node_response = NodeDiagnosticsResponse { + node_info: None, + network_info: None, + subscriptions: vec![SubscriptionInfo { + contract_key: ContractInstanceId::new([7u8; 32]), + client_id: 42, + }], + contract_states, + system_metrics: Some(SystemMetrics { + active_connections: 1, + hosting_contracts: 1, + }), + connected_peers_detailed: vec![ConnectedPeerInfo { + peer_id: "peer-x".to_string(), + address: "10.0.0.1:31337".to_string(), + }], + }; + + let bytes = bincode::serialize(&new_node_response).expect("a new node's response"); + + let faithfully_decoded = match bincode::deserialize::(&bytes) { + // The common outcome, and the honest one: the shifted bytes do not + // form a decodable value and the client sees an error. + Err(_) => false, + // The quieter outcome: it decodes into something, and that + // something is wrong. + Ok(decoded) => { + decoded.system_metrics.is_some() && decoded.connected_peers_detailed.len() == 1 + } + }; + + assert!( + !faithfully_decoded, + "an appended field on a non-terminal struct must not decode cleanly on an older \ + reader; if this ever passes, bincode gained field framing and the compatibility \ + table on this module needs rewriting" + ); + } } From bc322771f9d9ca8f17968fce9f35476d59efd489 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 13:21:24 -0500 Subject: [PATCH 04/13] chore(wire): allow dead_code on the pre-#52 ContractState mirror Its fields are decoded into and never read, which is the test: the decode either fails or produces something wrong. CI denies warnings. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/client_api/client_events.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index e75721a..649680b 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -4179,6 +4179,7 @@ mod struct_field_wire_compat { use std::collections::HashMap; /// `ContractState` as it was before #52 appended `size_bytes`. + #[allow(dead_code)] // decoded into, never read — the decode is the test #[derive(Serialize, Deserialize, Debug)] struct OldContractState { subscribers: u32, From 72763bad23f17f63b7bb97733b18142905344eed Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 13:25:57 -0500 Subject: [PATCH 05/13] test(wire): pin terminality against the HostResponse actually sent HostResponse defaults its type parameter to WrappedState, which is what goes over the wire. The terminality pin was instantiating it at Vec, so it was pinning the layout of a type nobody sends. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/client_api/client_events.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index 649680b..ece4ec7 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -3950,7 +3950,7 @@ mod fbs_decode_hardening { /// asked for it; a field changes what every existing peer decodes. #[cfg(test)] mod struct_field_wire_compat { - use super::{HostResponse, NodeDiagnosticsResponse, QueryResponse}; + use super::{HostResponse, NodeDiagnosticsResponse, QueryResponse, WrappedState}; use serde::{Deserialize, Serialize}; /// A wire struct before a field was appended. @@ -4134,7 +4134,10 @@ mod struct_field_wire_compat { }; let inner = bincode::serialize(&response).expect("response must serialize"); - let whole = bincode::serialize(&HostResponse::>::QueryResponse( + // The default type parameter, i.e. the `HostResponse` that is actually + // on the wire. Pinning terminality against some other instantiation + // would be pinning a type nobody sends. + let whole = bincode::serialize(&HostResponse::::QueryResponse( QueryResponse::NodeDiagnostics(response), )) .expect("host response must serialize"); From 9d97f3d5e63a3d0e8c7d462715f237cdc7348a1e Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 13:52:46 -0500 Subject: [PATCH 06/13] docs(delegate): demand registration is a node property, not an API property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit net-wiring's freenet-core#4669 work changes whether a delegate subscribe registers demand, so stating 'registers no demand' as a fact about this API would be wrong the moment their PR merges — the same doc rot in the other direction. Reframed as node behaviour with a tracking reference: pre-#4669 nodes register no demand at all; post-#4669 nodes register it when hosting the contract, and still do not when they can resolve but are not hosting, since a pin on an unheld contract could be neither renewed nor reclaimed. The delegate cannot detect which node it has, and the call reports success in every case. Also documents list_subscriptions' real cost: the node keys delegate subscriptions contract -> delegates, so this is a scan across every contract with any delegate subscription, not O(this delegate's). Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- CHANGELOG.md | 24 +++++++++++++------ rust/src/delegate_host.rs | 50 +++++++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff57df4..009b652 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,13 +47,23 @@ are third-party delegate WASM, which can reasonably ignore an unknown variant. - **`DelegateCtx::subscribe_contract`'s doc comment** said notification delivery - was "a follow-up" (it works), and said nothing about the fact that a delegate - subscription **registers no demand in the network** — it is a local - notification hook that does not pin the contract, enter the renewal set, or - exempt it from eviction, so a delegate only sees remote updates while some - other route keeps the node subscribed. The call succeeds either way, and - nothing distinguishes the two, which is why it is now documented at the call - site rather than left to be rediscovered. Tracked in freenet-core#4669. + was "a follow-up" (it works), and said nothing about whether a delegate + subscription registers demand in the network. + + It now says that **whether it does is a property of the node, not of this + library, and a delegate cannot detect which it has**. On nodes predating + freenet-core#4669 a subscribe registers no demand at all — no pin, no renewal + set, no eviction exemption — so the delegate sees remote updates only while + something else keeps the node subscribed, typically an open UI client. Once + #4669 lands it registers demand when the node is hosting the contract, and + still does not when the node can resolve but is not hosting it, since a pin on + a contract the node does not hold could be neither renewed nor reclaimed. + + The call reports success in every one of those cases and nothing + distinguishes them, which is why this belongs at the call site rather than in + an issue. It is deliberately phrased as current node behaviour with a tracking + reference rather than as a property of the API, so that freenet-core#4669 + landing makes it incomplete rather than wrong. ### Internal — wire-format guards diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index 49320d5..96bf093 100644 --- a/rust/src/delegate_host.rs +++ b/rust/src/delegate_host.rs @@ -628,21 +628,33 @@ impl DelegateCtx { /// contract's state changes. Delivery works, and covers state committed /// locally as well as state arriving from the network. /// - /// # What this does not do, as of 0.9.0 - /// - /// **A delegate subscription does not register demand in the network.** It - /// is a local notification hook: it does not mark the contract as in use, - /// does not enter the renewal set, and does not exempt the contract from - /// eviction. So a delegate sees a remote update only while the node happens - /// to be subscribed to that contract by some *other* route — typically a UI - /// client that is open. Close the tab and the notifications stop, without - /// any error being reported anywhere. - /// - /// This is tracked as freenet-core#4669 (phase 1 of the freenet-core#5467 - /// epic) and is being fixed. It is documented here rather than left to be - /// rediscovered because the call *succeeds*: nothing in the return value, - /// the logs, or the delegate's own view distinguishes a subscription that - /// pinned the contract from one that did nothing. + /// # Whether this registers demand is a property of the NODE, not of this library + /// + /// Subscribing always installs a local notification hook. Whether it *also* + /// registers demand — keeping the contract in the update mesh and + /// protecting it from eviction — depends on the freenet-core the delegate + /// happens to be running on, and **a delegate cannot detect which it has**. + /// The call reports success either way. + /// + /// - **Nodes predating freenet-core#4669** register no demand at all: + /// `contract_in_use` has no delegate term, so the contract does not enter + /// the renewal set and is not exempt from eviction. Such a delegate sees + /// remote updates only while something else keeps the node subscribed to + /// that contract — typically an open UI client. Close the tab and the + /// notifications stop, with no error reported anywhere. + /// - **Once #4669 lands**, a subscribe registers demand *when the node is + /// hosting the contract*. If the node can resolve the contract but is not + /// hosting it, the subscribe still succeeds and notifications still work, + /// but no demand is registered — registering demand for a contract the + /// node does not hold would create a pin that can be neither renewed nor + /// reclaimed. Closing that remaining gap needs a subscribe that can + /// bootstrap an unheld contract over the network. + /// + /// So do not write a delegate that assumes its subscription pins anything. + /// This is documented at the call site rather than left in an issue + /// precisely because nothing in the return value, the logs, or the + /// delegate's own view distinguishes the cases. Tracked in + /// freenet-core#4669, phase 1 of the freenet-core#5467 epic. /// /// Returns `true` on success, `false` if the contract is unknown or on /// error. Note that the contract must already be in the node's local store; @@ -674,6 +686,14 @@ impl DelegateCtx { /// /// Order is unspecified; do not depend on it. /// + /// # Cost + /// + /// The node holds delegate subscriptions keyed contract → delegates, so + /// answering this is a scan across every contract carrying any delegate + /// subscription, filtered to the caller — **O(all such contracts), not + /// O(this delegate's subscriptions)**. Call it on wake or after a restart, + /// which is what it is for; do not call it in a loop or per message. + /// /// # Why this returns a `Result` /// /// An empty list and a failed enumeration mean opposite things to the From a0865c56ccf2fe3f44f5116e4d66f7c029d124c6 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 13:57:30 -0500 Subject: [PATCH 07/13] docs(delegate): list_subscriptions promises notification, not pinning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After freenet-core#4669 a delegate subscription is two records — the notification hook and the demand registration — and they can separate. An eviction that sheds a still-in-use contract clears the demand and leaves the hook, so a list read from the hook alone reports a contract the delegate is no longer pinning. That 'looks subscribed, is not pinned' state is what #5467 exists to make visible, and reproducing it inside the introspection API meant to reveal it would be the same defect one layer up. A delegate replaying this list after a restart would also re-subscribe to things it holds no demand for and believe it had recovered. Promises the narrower meaning deliberately, so tightening the host's answer to the cross-checked set later is a bug fix rather than a breaking change. Reported by net-wiring from the core side, where the divergence is visible and from stdlib it is not. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/delegate_host.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index 96bf093..bc41a09 100644 --- a/rust/src/delegate_host.rs +++ b/rust/src/delegate_host.rs @@ -686,6 +686,30 @@ impl DelegateCtx { /// /// Order is unspecified; do not depend on it. /// + /// # What this list means + /// + /// It answers **"which contracts will notify me"** — not "which contracts + /// am I keeping alive". Those coincide today, and coincide in the common + /// case once freenet-core#4669 lands, but they are not the same thing by + /// construction. + /// + /// A delegate subscription is two records on the node: the notification + /// hook, and (after #4669) the demand registration that actually pins the + /// contract. They are written and torn down together on the ordinary paths, + /// but not on all of them — an eviction that sheds a still-in-use contract + /// clears the demand and leaves the hook standing, and the delegate is told + /// nothing. A list sourced from the hook alone would therefore report a + /// contract the delegate is no longer pinning. + /// + /// That "looks subscribed, is not pinned" state is exactly what + /// freenet-core#5467 exists to make visible, so this call must not + /// reproduce it in the API meant to reveal it. The host is expected to + /// answer from records it can cross-check rather than from the hook alone. + /// This documentation deliberately promises the narrower meaning, so + /// tightening the host's answer later is a bug fix and not a breaking + /// change. Both records become one, with one owner, in #4669 part 3's + /// durable delegate-subscription store. + /// /// # Cost /// /// The node holds delegate subscriptions keyed contract → delegates, so From 13d6aa4168cff60f6f7dd365efc4fe636f7ef2f6 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 15:16:00 -0500 Subject: [PATCH 08/13] docs(delegate): link freenet-core#5487 from list_subscriptions Filed by net-wiring, covering both subscription desyncs and why the two obvious fixes are wrong. Points the reader at the mechanism instead of my summary of it, and records that introspection built against the two-record shape wants rewriting when #4669 part 3's single store lands. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/delegate_host.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index bc41a09..98b1f1d 100644 --- a/rust/src/delegate_host.rs +++ b/rust/src/delegate_host.rs @@ -707,8 +707,13 @@ impl DelegateCtx { /// answer from records it can cross-check rather than from the hook alone. /// This documentation deliberately promises the narrower meaning, so /// tightening the host's answer later is a bug fix and not a breaking - /// change. Both records become one, with one owner, in #4669 part 3's - /// durable delegate-subscription store. + /// change. + /// + /// Both divergences, and why the two obvious fixes are wrong, are tracked + /// in freenet-core#5487. They close together at #4669 part 3's durable + /// delegate-subscription store, where the two records become one with one + /// owner — so introspection built against the two-record shape will want + /// rewriting when that lands. /// /// # Cost /// From eea3c0a2b9977f618fc8a3bb00c12a132b0e5988 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 15:16:12 -0500 Subject: [PATCH 09/13] fix(memory): restore from_ptr's safety contract, which had moved to a getter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit StreamingBuffer::from_ptr is a pub unsafe fn whose entire doc comment, including its '# Safety' section, was attached to total_remaining — a safe getter two lines above that takes no pointer. So the getter was documented as 'Create a streaming reader from a buffer pointer' with a safety contract about a ptr it does not have, and the unsafe constructor, whose callers must uphold that invariant, had no documentation at all. A method had been inserted into the middle of another method's doc block. Splitting them back apart also clears clippy::missing_safety_doc. The other error in this file is a false positive: the non-WASM stub must keep the mangled __frnt__fill_buffer name to match the WASM import it stands in for, so it gets a narrow allow with the reason. Neither is reachable by CI's lint gate, because ci.yml:99 passes no --features: the clippy matrix covers both wasm32 and x86_64, but 'contract' is off in both legs, and both of these live behind #[cfg(feature = "contract")]. Filed as issue #100; not fixed here, because CI is a shared Full-tier surface. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/memory/buf.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/rust/src/memory/buf.rs b/rust/src/memory/buf.rs index 2c5576b..fa1c431 100644 --- a/rust/src/memory/buf.rs +++ b/rust/src/memory/buf.rs @@ -366,6 +366,8 @@ extern "C" { // Stub for non-WASM builds (native tests, host-side compilation). // Returns 0 (EOF) since there is no WASM host to refill from. +// Name must match the WASM import above exactly, so it cannot be snake_cased. +#[allow(non_snake_case)] #[cfg(all(feature = "contract", not(target_family = "wasm")))] unsafe extern "C" fn __frnt__fill_buffer(_id: i64, _buf_ptr: i64) -> u32 { 0 @@ -385,6 +387,11 @@ pub struct StreamingBuffer { #[cfg(feature = "contract")] impl StreamingBuffer { + /// Returns the total number of payload bytes remaining to be read. + pub fn total_remaining(&self) -> usize { + self.total_remaining + } + /// Create a streaming reader from a buffer pointer. /// /// Reads the `[total_len: u32]` header and prepares for streaming. @@ -392,11 +399,6 @@ impl StreamingBuffer { /// # Safety /// `ptr` must point to a valid `BufferBuilder` in WASM linear memory /// whose first 4 bytes of data contain the total payload length as LE u32. - /// Returns the total number of payload bytes remaining to be read. - pub fn total_remaining(&self) -> usize { - self.total_remaining - } - pub unsafe fn from_ptr(ptr: i64) -> Self { let buf_ptr = ptr as *mut BufferBuilder; let builder = &*buf_ptr; From 42ecc979ac9bce4e1ba80e4b27f94baaf55e570d Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 15:21:24 -0500 Subject: [PATCH 10/13] docs(contributing): warn that building rewrites the generated FlatBuffers files A build regenerates rust/src/generated/ with whatever local flatc is present, which is not the one that produced the checked-in files, so any build leaves thousands of lines of unrelated churn in the working tree. Staging explicit paths is what keeps it out; a single 'git add -A' puts a toolchain downgrade into a PR where nobody is looking for one. Found while preparing this branch: the diff against main showed ~4000 lines of generated churn that the committed diff did not contain. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- CONTRIBUTING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1d62852..16d7fbc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,6 +14,7 @@ We welcome contributions to Freenet! Here's what you need to know. - PRs should explain **why**, not just what. See [AGENTS.md](AGENTS.md) for description structure. - Bug fixes should include a regression test that fails without the fix. - Run `cargo fmt`, `cargo clippy --all-targets`, and `cargo test` before pushing. +- **Discard `rust/src/generated/` before committing, unless changing it is the point of your PR.** Building regenerates those FlatBuffers files with whatever `flatc` you have locally, which is usually not the one that produced the checked-in versions — so a build leaves thousands of lines of unrelated churn in your working tree. `git checkout -- rust/src/generated/` clears it. Stage explicit paths rather than `git add -A`, or a toolchain-version downgrade rides into your PR unnoticed and unreviewed. - Keep PRs focused — one logical change per PR. ## AI-Assisted Contributions From be638f3362153558ca6646e5380ac59ad728a95f Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 15:41:01 -0500 Subject: [PATCH 11/13] fix(review): close the vacuous pins and correct four false claims Multi-lens review found the terminality pin vacuous and three doc claims wrong. Each was verified against source before being changed. Vacuous test, found independently by three reviewers: the terminality pin built NodeDiagnosticsResponse from all defaults, which encodes as 27 ZERO bytes, so ends_with(inner) asserted only that the message ends in zeros. That stays true after appending any field that encodes as zeros, which is exactly the mutation it claims to catch. Now uses a distinctive non-zero fixture, asserts the fixture is not all-zero, and adds a length equality so an appended sibling cannot hide even if its bytes coincide. The unknown-tag probe asserted only is_err(), so a future variant whose payload rejects zeros (a DateTime, a NonZero, a validating deserialize_with) would fail for the wrong reason and leave the count constants drifting undetected. It now asserts the error names an invalid variant index, plus a control that the last known tag still decodes from the same payload, so it cannot go vacuous either way. Wrong claim 1: bincode does NOT reject an unknown enum tag with InvalidTagEncoding. That is produced only for a bad Option discriminant (de/mod.rs:340); deserialize_enum hands the index to serde's derived visitor, producing ErrorKind::Custom naming an invalid variant index. Stated in two doc comments; corrected, and now pinned by the probe above. Wrong claim 2: the node replays subscriptions across a restart is false, and it was the motivating premise for list_subscriptions. DELEGATE_SUBSCRIPTIONS is an in-memory LazyLock DashMap with no persistence, so a restart LOSES them. The API is still right to add, but it is the read side of a capability that needs #4669 part 3's durable store; scoped accordingly. Wrong claim 3: the non_exhaustive rationale cited SubscribeContractRequest as a variant compiled with no handler. It IS handled (contract.rs:916-940); its defect is that it registers no demand. Different bug. The argument stands on its own and now states two honest limits: the compile error forces an arm to exist, not a working handler, and this crate's own encoder has arms that log and drop. Wrong claim 4: the version floor named a stdlib version. Host functions are registered by name and reference no stdlib type, so the stdlib a node was built against guarantees nothing. Says so, and that no released node provides these imports yet. Also hardens the FFI, which reviewers found could silently under-report. Validate that the host length is a multiple of 32 and within a new MAX_SUBSCRIPTION_LIST_BYTES before allocating: usize is 32-bit on wasm32, so an unchecked i64 cast truncates to 0 and surfaces as an empty list, the exact conflation the Result return type exists to prevent. Reject written > len, which truncate would otherwise ignore, leaving zero-filled tail bytes to decode as valid-looking all-zero ids. Re-check on an exactly-full buffer so a set that GREW between the two calls returns ERR_BUFFER_TOO_SMALL rather than a short list that looks complete. The import contract now specifies that requirement, since the host half is written against it. Adds the off-WASM test that list_subscriptions returns Err rather than an empty list; documents that notification delivery is best-effort and lossy; qualifies the old-delegate-to-new-host claim, which holds for appended variants but not for fields appended to their payload structs; and renames a test to what it actually pins. The additive claim is now evidence: River's shipped chat_delegate.wasm builds against stdlib 0.8.5, which declares the five freenet_delegate_contracts externs, and imports none of them. A first attempt to show this with a synthetic delegate was itself vacuous, since that harness exported no entry point, so nothing could import and an empty module reports zero the same way. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- CHANGELOG.md | 59 ++++++++---- rust/src/client_api/client_events.rs | 62 ++++++++++-- rust/src/delegate_host.rs | 137 +++++++++++++++++++++++---- rust/src/delegate_interface.rs | 118 +++++++++++++++++------ 4 files changed, 304 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 009b652..8f6356e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,22 +12,42 @@ `encode_contract_id_list` / `decode_contract_id_list` as the shared codec. A delegate's subscription set lives in the node: the WASM is instantiated per - invocation and dropped afterwards, and the node replays subscriptions across a - restart without running the delegate at all. Until now a delegate had no way - to learn its own state after a restart — it could only keep a parallel record - in its secrets, which drifts from the node's exactly when it matters, or - re-subscribe to everything on every wake. See freenet-core#5467. + invocation and dropped afterwards, so between invocations the delegate has no + view of it. Until now it could only keep a parallel record in its secrets, + which drifts from the node's exactly when it matters, or re-subscribe to + everything on every wake. + + **Scoped honestly: this does not by itself deliver the restart-replay + freenet-core#5467 asks for.** The node's delegate-subscription registry is + in-memory, so a restart *loses* those subscriptions rather than replaying + them, and after one this call correctly returns an empty list. It is the read + side of that capability, and becomes load-bearing when freenet-core#4669 + part 3's durable store lands and there is something persistent to read back. + Within a single node lifetime it is useful today. It returns `Result, i64>`, not a bare `Vec`. An empty list and a failed enumeration mean opposite things to a caller deciding whether to re-subscribe, so they must not share a representation. - **This is additive for every existing delegate.** Host functions resolve by - name at module instantiation, so a delegate that does not import it is - unaffected, and one that does fails to *load* on a node too old to provide it - — a named missing-import error, rather than a silent failure mid-protocol. - **Requires a node whose freenet-core registers these imports**; calling it - against an older node is a load-time failure, by design. + **This is additive for every existing delegate**, and that was checked against + a shipped artifact rather than assumed. River's deployed `chat_delegate.wasm` + is built against stdlib 0.8.5, which already declares the five + `freenet_delegate_contracts` externs — and `wasm-objdump -x` shows it imports + **none** of them, only the four secrets functions and one logger it actually + calls. The linker drops unreferenced externs, so declaring two more changes + nothing for a delegate that does not call them. Had that not held, every + delegate merely *rebuilt* against 0.9.0 would have acquired imports no + deployed node provides and failed to instantiate everywhere. + + A delegate that *does* call it fails to **load** on a node that does not + provide the imports — a named missing-import error at instantiation, rather + than a silent failure mid-protocol. That is the reason for choosing a host + function over a message variant. + + **Requires a node whose freenet-core registers these imports, and no released + node does yet.** Host functions are registered by name and reference no stdlib + type, so the stdlib version a node was built against guarantees nothing here; + the core half is tracked separately. ### Fixed @@ -40,11 +60,18 @@ dispatches them in exhaustive matches with no wildcard (`contract.rs`, in the request loop and in the app-message filter). Marking the enum would force those to grow `_ =>` arms, and a newly added variant would then compile - against the host with no handler — the delegate's request silently swallowed, - the call reporting success. That is the failure mode a delegate - `SubscribeContractRequest` has today, and the compile error is what prevents - the next one. `InboundDelegateMsg` keeps the attribute because its consumers - are third-party delegate WASM, which can reasonably ignore an unknown variant. + against the host with no arm of its own — the delegate's request falling into + the wildcard, the call reporting success. The compile error is what prevents + that, and it is the only thing that does. `InboundDelegateMsg` keeps the + attribute because its consumers are third-party delegate WASM, which can + reasonably ignore an unknown variant. + + The doc states two limits on that argument rather than overselling it: the + compile error forces an *arm* to exist, not a working handler (this crate's + own FlatBuffers encoder has explicit arms that log and drop), and it is **not** + the bug behind this workstream — a delegate `SubscribeContractRequest` *is* + handled today; its defect is that it registers no demand + (freenet-core#4669), which is a different failure with a different fix. - **`DelegateCtx::subscribe_contract`'s doc comment** said notification delivery was "a follow-up" (it works), and said nothing about whether a delegate diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index ece4ec7..a4a311e 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -4124,16 +4124,55 @@ mod struct_field_wire_compat { /// buys one direction, not both. #[test] fn node_diagnostics_response_is_terminal_in_its_message() { + // Every field carries a DISTINCTIVE, NON-ZERO value on purpose. + // + // An all-default response encodes as 27 zero bytes, and `ends_with` + // against a run of zeros degenerates into "the message ends in zeros" — + // which stays true after appending any field that encodes as zeros + // (`None`, an empty `Vec`, `0u64`, `false`). That is satisfiable under + // exactly the mutation this test exists to catch, so the assertion + // would have passed while terminality was broken. + let mut contract_states = std::collections::HashMap::new(); + contract_states.insert( + "6kVs66bKaQAC6ohr8b43SvJ95r36tc2hnG7HezmaJHF9".to_string(), + super::ContractState { + subscribers: 0xAB, + subscriber_peer_ids: vec!["peer-a".to_string()], + size_bytes: 0xCDEF, + }, + ); let response = NodeDiagnosticsResponse { - node_info: None, - network_info: None, - subscriptions: vec![], - contract_states: Default::default(), - system_metrics: None, - connected_peers_detailed: vec![], + node_info: Some(super::NodeInfo { + peer_id: "peer-self".to_string(), + is_gateway: true, + location: Some("0.5".to_string()), + listening_address: Some("0.0.0.0:31337".to_string()), + uptime_seconds: 0x1234, + }), + network_info: Some(super::NetworkInfo { + connected_peers: vec![("peer-x".to_string(), "10.0.0.1:31337".to_string())], + active_connections: 7, + }), + subscriptions: vec![super::SubscriptionInfo { + contract_key: crate::contract_interface::ContractInstanceId::new([7u8; 32]), + client_id: 42, + }], + contract_states, + system_metrics: Some(super::SystemMetrics { + active_connections: 0x5678, + hosting_contracts: 0x9A, + }), + connected_peers_detailed: vec![super::ConnectedPeerInfo { + peer_id: "peer-x".to_string(), + address: "10.0.0.1:31337".to_string(), + }], }; let inner = bincode::serialize(&response).expect("response must serialize"); + assert!( + inner.iter().any(|b| *b != 0), + "the fixture must not be all zeros, or the ends_with below proves nothing" + ); // The default type parameter, i.e. the `HostResponse` that is actually // on the wire. Pinning terminality against some other instantiation // would be pinning a type nobody sends. @@ -4149,6 +4188,17 @@ mod struct_field_wire_compat { trailing-byte-safe for older clients — it would silently corrupt whatever \ was added after it." ); + + // Belt to the ends_with brace: the payload must account for everything + // after the two 4-byte enum tags (HostResponse::QueryResponse, then + // QueryResponse::NodeDiagnostics) and nothing else. This catches an + // appended sibling field even in the case ends_with cannot — one whose + // encoding happens to match the payload's own trailing bytes. + assert_eq!( + whole.len(), + 8 + inner.len(), + "exactly two enum tags may precede the payload and nothing may follow it" + ); } /// The rule above, in code that has already shipped. diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index 98b1f1d..69b97fd 100644 --- a/rust/src/delegate_host.rs +++ b/rust/src/delegate_host.rs @@ -122,6 +122,17 @@ pub mod error_codes { pub const ERR_STORE_FAILED: i32 = -24; } +/// Upper bound on the serialized subscription list a host may report from +/// `__frnt__delegate__list_subscriptions_len`, in bytes — 32 KiB, i.e. 1024 +/// contract ids. +/// +/// This exists because the delegate allocates on the strength of that number. +/// An allocation failure inside a delegate is an abort, not a value we can +/// return, so an implausible length has to be refused before `vec![0u8; len]` +/// rather than survived afterwards. A delegate holding a thousand subscriptions +/// is already well outside the intended shape. +pub const MAX_SUBSCRIPTION_LIST_BYTES: i64 = 32 * 1024; + // ============================================================================ // Host function declarations (WASM only) // ============================================================================ @@ -196,14 +207,23 @@ extern "C" { /// Subscribe to contract updates. Returns 0 on success, or negative error code (i64). fn __frnt__delegate__subscribe_contract(id_ptr: i64, id_len: i32) -> i64; /// Byte length of this delegate's serialized subscription list — always a - /// multiple of 32. Returns the count to allocate, or a negative error code - /// (i64). Zero means "subscribed to nothing", which is distinct from an - /// error and must stay so. + /// multiple of 32, and never more than [`MAX_SUBSCRIPTION_LIST_BYTES`]. + /// Returns the count to allocate, or a negative error code (i64). Zero + /// means "subscribed to nothing", which is distinct from an error and must + /// stay so. fn __frnt__delegate__list_subscriptions_len() -> i64; /// Enumerate this delegate's current contract subscriptions: writes the raw /// 32-byte instance ids back to back into `out_ptr` (at most `out_len` /// bytes) and returns the number of bytes written, or a negative error code /// (i64). + /// + /// **The host MUST return `ERR_BUFFER_TOO_SMALL` (-6) rather than a partial + /// list if the set no longer fits in `out_len`.** The set can change + /// between the length call and this one, and a buffer filled exactly to + /// `out_len` is indistinguishable from one the host wanted to overflow — so + /// a silently truncated list would look complete to the delegate, which is + /// the failure this API exists to remove. Writing more than `out_len` bytes + /// is a contract violation in either direction. fn __frnt__delegate__list_subscriptions(out_ptr: i64, out_len: i64) -> i64; } @@ -625,8 +645,15 @@ impl DelegateCtx { /// Subscribe to contract updates by instance ID. /// /// Registers interest in receiving `ContractNotification` when the - /// contract's state changes. Delivery works, and covers state committed - /// locally as well as state arriving from the network. + /// contract's state changes, covering state committed locally as well as + /// state arriving from the network. + /// + /// **Delivery is best-effort and lossy.** The node drops notifications + /// rather than blocking a state commit when the delivery channel is full, + /// and if that channel is closed it removes the contract's subscription + /// entry outright — silently, for every delegate subscribed to it. A + /// delegate that needs to be sure should poll contract state as a fallback + /// rather than treat a notification as guaranteed. /// /// # Whether this registers demand is a property of the NODE, not of this library /// @@ -675,14 +702,28 @@ impl DelegateCtx { /// List the contract instance ids this delegate is currently subscribed to. /// - /// A delegate's subscription set lives in the node, not in the delegate. - /// The WASM is instantiated per invocation and dropped immediately after, - /// and the node replays subscriptions across a restart without running the - /// delegate at all. So without this call a delegate has no way to learn - /// what it is already subscribed to: it can only keep a parallel record in - /// its own secrets, which drifts from the node's exactly in the cases that - /// matter, or re-subscribe to everything on every wake. This is the gap - /// freenet-core#5467 names for restart-replay. + /// A delegate's subscription set lives in the node, not in the delegate: + /// the WASM is instantiated per invocation and dropped immediately after, + /// so between invocations the delegate has no view of it at all. Without + /// this call it can only keep a parallel record in its own secrets, which + /// drifts from the node's exactly in the cases that matter, or re-subscribe + /// to everything on every wake. + /// + /// # What this does not yet solve + /// + /// **Today the node does not survive a restart with its delegate + /// subscriptions intact — it loses them.** `DELEGATE_SUBSCRIPTIONS` is an + /// in-memory map (freenet-core `wasm_runtime/native_api.rs`), so after a + /// restart this call correctly returns `Ok(vec![])`, and a delegate should + /// read that as "the node is holding nothing for me", not as "my + /// subscriptions are gone but recoverable from somewhere else". + /// + /// So this call does **not**, on its own, deliver the restart-replay that + /// freenet-core#5467 asks for. It is the read side of that capability, and + /// it becomes load-bearing when #4669 part 3's durable + /// delegate-subscription store lands and there is finally something + /// persistent to read back. Until then its value is within a single node + /// lifetime: learning what the node currently holds, without guessing. /// /// Order is unspecified; do not depend on it. /// @@ -736,15 +777,27 @@ impl DelegateCtx { /// for the same reason: a host-side unit test must not be able to read /// "no subscriptions" out of a stub that never had any. /// - /// **Host version floor:** this import is provided by nodes built against - /// freenet-stdlib 0.9.0 or later. A delegate that calls it fails to - /// instantiate on an older node with a named missing-import error — loud - /// and diagnosable at load time, rather than silently mid-protocol. + /// **Host requirement:** the node must register these imports in its + /// wasmtime linker. That is a freenet-core change, not a stdlib one — host + /// functions are registered by name and reference no stdlib type, so the + /// stdlib version a node was built against guarantees nothing here. **No + /// released node provides them yet.** A delegate that calls this against a + /// node that does not fails to instantiate, with a named missing-import + /// error — loud and diagnosable at load time rather than silently + /// mid-protocol, which is the reason for choosing a host function over a + /// message variant. pub fn list_subscriptions(&self) -> Result, i64> { #[cfg(target_family = "wasm")] { // Step 1: how many bytes to allocate. Zero is a valid answer and // means "subscribed to nothing". + // + // Every host return is validated before it is used as a length. + // `usize` is 32 bits on wasm32, so an `as usize` cast on an + // unchecked i64 truncates silently: a bogus 2^32 would become 0 and + // surface as `Ok(vec![])` — the "you hold no subscriptions" answer + // this API's whole return type exists to keep distinguishable from + // a failure. Reject rather than cast. let len = unsafe { __frnt__delegate__list_subscriptions_len() }; if len < 0 { return Err(len); @@ -752,10 +805,15 @@ impl DelegateCtx { if len == 0 { return Ok(Vec::new()); } + if len % 32 != 0 || len > MAX_SUBSCRIPTION_LIST_BYTES { + // The import contract promises a multiple of 32 within the + // cap. A host that breaks it is malfunctioning, and allocating + // on its say-so risks an allocation failure, which in a + // delegate is an abort rather than an error we can return. + return Err(error_codes::ERR_STORE_ERROR as i64); + } - // Step 2: read the ids. The host may write fewer bytes than it - // reported if the set shrank between the two calls, so the return - // value, not `len`, is authoritative. + // Step 2: read the ids. let mut buf = vec![0u8; len as usize]; let written = unsafe { __frnt__delegate__list_subscriptions(buf.as_mut_ptr() as i64, buf.len() as i64) @@ -763,6 +821,23 @@ impl DelegateCtx { if written < 0 { return Err(written); } + if written > len { + // Writing past the buffer we advertised is a host bug. Left + // unchecked, `truncate` would be a no-op and the zero-filled + // tail would decode as valid-looking all-zero contract ids. + return Err(error_codes::ERR_STORE_ERROR as i64); + } + if written == len { + // Ambiguous: an exactly-full buffer cannot be distinguished + // from one the host wanted to overflow, so a set that GREW + // between the two calls would silently come back one entry + // short and look complete. Re-ask instead of guessing; the + // host reports ERR_BUFFER_TOO_SMALL if it still does not fit. + let recheck = unsafe { __frnt__delegate__list_subscriptions_len() }; + if recheck > len { + return Err(error_codes::ERR_BUFFER_TOO_SMALL as i64); + } + } buf.truncate(written as usize); decode_contract_id_list(&buf).ok_or(error_codes::ERR_STORE_ERROR as i64) } @@ -976,6 +1051,28 @@ mod secret_key_list_codec_tests { mod contract_id_list_codec_tests { use super::{decode_contract_id_list, encode_contract_id_list}; + /// Off-WASM, `list_subscriptions` must report an error rather than an + /// empty list. + /// + /// The whole argument for its `Result` return type is that "you hold no + /// subscriptions" and "I could not tell you" must not share a + /// representation. A stub that returned `Ok(vec![])` would let a host-side + /// test read "no subscriptions" out of something that never had any — the + /// exact conflation the type exists to prevent, reintroduced at the one + /// place nobody looks. + #[test] + #[cfg(not(target_family = "wasm"))] + fn list_subscriptions_off_wasm_is_an_error_not_an_empty_list() { + // SAFETY: `__new` builds a zero-sized handle; off-WASM every method on + // it takes the non-WASM branch and touches no host state. + let ctx = unsafe { super::DelegateCtx::__new() }; + assert_eq!( + ctx.list_subscriptions(), + Err(super::error_codes::ERR_NOT_IN_PROCESS as i64), + "off-WASM must be distinguishable from a successful empty enumeration" + ); + } + #[test] fn round_trips_multiple_ids() { let ids = [[0x01u8; 32], [0xFEu8; 32], [0x00u8; 32]]; diff --git a/rust/src/delegate_interface.rs b/rust/src/delegate_interface.rs index b817436..5b3cad7 100644 --- a/rust/src/delegate_interface.rs +++ b/rust/src/delegate_interface.rs @@ -535,7 +535,11 @@ impl AsRef<[u8]> for DelegateContext { /// - **Appending is compatible in exactly one direction.** An old sender's old /// variant always decodes on a new receiver. A **new** sender's **new** /// variant does **not** decode on an old receiver: bincode rejects the -/// unknown tag with `ErrorKind::InvalidTagEncoding`. `#[non_exhaustive]` does +/// unknown tag — as `ErrorKind::Custom("invalid value: integer `N`, expected +/// variant index 0 <= i < M")`, since bincode hands the index to serde's +/// derived visitor rather than validating it itself. (Not +/// `InvalidTagEncoding`, which bincode only ever produces for a bad `Option` +/// discriminant.) `#[non_exhaustive]` does /// not change this — it is a source-level attribute with no effect on the /// encoding, and serde has no unknown-variant fallback to fall back to. /// @@ -741,13 +745,27 @@ impl UserInputResponse<'_> { /// no wildcard (`crates/core/src/contract.rs`, in the request loop and again in /// the app-message filter). Marking this enum `#[non_exhaustive]` would force /// those matches to grow `_ =>` arms, and a newly added variant would then -/// compile against the host with **no handler**: the delegate's request would be -/// silently swallowed, the call would appear to succeed, and nothing anywhere -/// would report that it did nothing. That failure mode is not hypothetical — it -/// is what a delegate `SubscribeContractRequest` does today, and the reason this -/// workstream exists. +/// compile against the host with **no arm of its own**: the delegate's request +/// would fall into the wildcard, the call would appear to succeed, and nothing +/// would report that it did nothing. /// -/// The compile error is the mechanism that stops it. Keep it. +/// The compile error is what stops that, and it is the only mechanism that +/// does. Keep it. +/// +/// Two honest limits on this argument, because it is easy to claim more: +/// +/// - **It forces an arm to exist, not a handler to be correct.** This crate's +/// own FlatBuffers encoder (`client_api::client_events`) has explicit arms +/// for five outbound variants that log an error and drop the message. The +/// compile error made someone write those arms deliberately; it could not +/// make them do anything useful. +/// - **It is not the bug behind this workstream.** A delegate +/// `SubscribeContractRequest` *is* handled by the host today. Its defect is +/// different and subtler: it registers no demand in the network, so the +/// subscription does not pin the contract (freenet-core#4669). Do not read +/// the compile-error argument as a fix for that; it is a guard against a +/// different failure that has not happened yet, which is the point of a +/// guard. /// /// [`InboundDelegateMsg`] carries the opposite trade-off, and is marked: its /// consumers are third-party delegate WASM, which can reasonably ignore a @@ -764,12 +782,18 @@ impl UserInputResponse<'_> { /// Appending is compatible in one direction only, and this enum is the /// direction that bites: /// -/// - **Old delegate → new host: always fine.** The host understands every tag -/// an older delegate can emit, so deployed delegate WASM keeps working -/// against an upgraded node with no rebuild. +/// - **Old delegate → new host: fine, for appended VARIANTS.** The host +/// understands every tag an older delegate can emit, so deployed delegate +/// WASM keeps working against an upgraded node with no rebuild. This does +/// **not** extend to appending a FIELD to an existing variant's payload +/// struct — several of them are `#[non_exhaustive]`, which invites exactly +/// that — because a field breaks in the opposite direction. See +/// `struct_field_wire_compat` in `client_api::client_events`. /// - **New delegate → old host: fails, and fails loudly.** bincode rejects the -/// unknown variant tag with `ErrorKind::InvalidTagEncoding`, so the host -/// surfaces a decode error on that message rather than misreading it. +/// unknown variant tag — as `ErrorKind::Custom("invalid value: integer `N`, +/// expected variant index 0 <= i < M")`, since it hands the index to serde's +/// derived visitor rather than validating it itself — so the host surfaces a +/// decode error on that message rather than misreading it. /// /// There is deliberately **no feature-detection handshake**. A delegate cannot /// ask the host which variants it understands, and adding a probe would itself @@ -1589,36 +1613,70 @@ mod delegate_wire_compat { /// a fixed-size probe is safe here. #[test] fn an_unpinned_variant_fails_this_test() { + // The probe must fail because the TAG is unknown, not because a + // payload of zeros happened not to parse. Asserting only `is_err()` + // would let a new variant whose first field rejects zeros (a + // `DateTime`, a `NonZero*`, a validating `deserialize_with`) go + // undetected: the tag would be valid, the decode would still fail, and + // this test would stay green while the counts drifted. + // + // bincode hands an out-of-range variant index to serde's derived + // visitor, which rejects it as `invalid value: integer `N`, expected + // variant index 0 <= i < M` — an `ErrorKind::Custom`. Match on that + // wording rather than on `InvalidTagEncoding`, which bincode produces + // only for a bad `Option` discriminant. + fn assert_rejected_as_unknown_variant(err: &bincode::Error, tag: u32, which: &str) { + let msg = err.to_string(); + assert!( + msg.contains("variant index"), + "tag {tag} on {which} failed for the wrong reason ({msg}); the tag itself must \ + still be unknown, otherwise a variant was added without updating the count, \ + the pinned_*_tag match and the every_* list" + ); + } + let mut probe = INBOUND_VARIANT_COUNT.to_le_bytes().to_vec(); probe.extend_from_slice(&[0u8; 256]); - let decoded = bincode::deserialize::>(&probe); - assert!( - decoded.is_err(), - "tag {INBOUND_VARIANT_COUNT} decoded as an InboundDelegateMsg, so a variant was \ - added without updating INBOUND_VARIANT_COUNT, pinned_inbound_tag and every_inbound" - ); + let err = bincode::deserialize::>(&probe) + .expect_err("tag {INBOUND_VARIANT_COUNT} must not decode as an InboundDelegateMsg"); + assert_rejected_as_unknown_variant(&err, INBOUND_VARIANT_COUNT, "InboundDelegateMsg"); let mut probe = OUTBOUND_VARIANT_COUNT.to_le_bytes().to_vec(); probe.extend_from_slice(&[0u8; 256]); - let decoded = bincode::deserialize::(&probe); - assert!( - decoded.is_err(), - "tag {OUTBOUND_VARIANT_COUNT} decoded as an OutboundDelegateMsg, so a variant was \ - added without updating OUTBOUND_VARIANT_COUNT, pinned_outbound_tag and \ - every_outbound" + let err = bincode::deserialize::(&probe) + .expect_err("tag {OUTBOUND_VARIANT_COUNT} must not decode as an OutboundDelegateMsg"); + assert_rejected_as_unknown_variant(&err, OUTBOUND_VARIANT_COUNT, "OutboundDelegateMsg"); + + // Control, so the probe cannot pass vacuously from the other end: the + // LAST known tag must still decode from the same all-zero payload. If + // this ever fails, the zero payload has stopped being a valid encoding + // for the final variant, and the probes above are no longer testing + // what they claim. + let mut control = (INBOUND_VARIANT_COUNT - 1).to_le_bytes().to_vec(); + control.extend_from_slice(&[0u8; 256]); + bincode::deserialize::>(&control).expect( + "the last known inbound tag must decode from zeros, or the unknown-tag probe above \ + is no longer distinguishing an unknown tag from an unparseable payload", ); } - /// Direction 1 of the append rule: **old sender to new receiver always - /// works.** Bytes produced before a variant was appended still decode into - /// the variant they always meant. + /// Direction 1 of the append rule: **old sender to new receiver works.** /// - /// The payload here is hand-built rather than produced by this crate's own + /// The payload is hand-built rather than produced by this crate's own /// encoder, so it stands in for bytes emitted by a delegate compiled - /// against an older stdlib. An encoder-produced value would only prove the + /// against an older stdlib; an encoder-produced value would only prove the /// code agrees with itself. + /// + /// Named for what it actually pins. Nothing here appends a variant — the + /// test cannot fail *because of* an append, only because a tag moved or a + /// payload layout changed, which `delegate_msg_variant_tags_are_pinned` + /// also covers. Its distinct value is that the expected bytes are written + /// out by hand, so a change to `ContractNotification`'s field order or to + /// the bincode config fails here with a concrete byte string to compare + /// against. Direction 2, which genuinely models an old receiver, is + /// `a_new_variant_does_not_decode_on_an_old_receiver` below. #[test] - fn an_old_payload_still_decodes_after_appending_a_variant() { + fn a_hand_built_old_encoder_payload_decodes_into_the_same_variant() { // InboundDelegateMsg tag 6 = ContractNotification { contract_id, // new_state: WrappedState (empty), context: DelegateContext (empty) }. let mut old_payload = vec![6u8, 0, 0, 0]; From d09c862343d8bd907fbb97f6d081eacf13bfafb0 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 15:47:00 -0500 Subject: [PATCH 12/13] feat(delegate): add the unsubscribe pair at wire tag 8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit freenet-core#2830 specified subscribe and unsubscribe together; only subscribe was built, and core has carried the TODO(#2830) since. Until now the only way a delegate's subscription was released was the implicit cleanup when the delegate itself was unregistered, so a delegate that had finished with a contract kept holding interest for as long as it existed. Appends UnsubscribeContractRequest to OutboundDelegateMsg and UnsubscribeContractResponse to InboundDelegateMsg, both at tag 8. Every existing tag is unchanged, so deployed delegate WASM is unaffected. Tag 8 is Ian's call, coordinated with freenet-stdlib#82, which appends ScheduleWakeup/WakeupFired and now takes tag 9; recorded as a comment on that PR so the decision does not live only in a working session. Unsubscribing a contract the delegate is not subscribed to reports Ok(()). Not a convenience: the host's teardown already treats an absent client id as a no-op, so an error return would have it inventing a failure it did not have. That reasoning is net-wiring's, from the side that implements it, and it survives someone later deciding convenience was not a good enough justification. The pin was made to fail before it was made to pass. Adding the two variants with nothing else changed produced five compile errors, all E0004 non-exhaustive-pattern: three in production code, including the FlatBuffers encoder in client_api/client_events.rs that the OutboundDelegateMsg doc cites as the reason the enum is deliberately NOT non_exhaustive, and two in the tag pin itself (pinned_inbound_tag, pinned_outbound_tag). So the guard is demonstrated rather than asserted: a variant cannot be appended without both the host dispatch site and the wire pin refusing to compile. Adds a round-trip test for the pair that also asserts a hand-built pre-0.9.0 ContractNotification still decodes unchanged, so the append is shown not to disturb anything older. The host half is freenet-core's and is owned by net-wiring, who has the exact field layout. This does not ship until they confirm they are landing it — shipping a variant no host handles would be the same defect this workstream exists to remove. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- CHANGELOG.md | 21 ++++ rust/src/client_api/client_events.rs | 11 ++ rust/src/delegate_interface.rs | 148 ++++++++++++++++++++++++++- 3 files changed, 178 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f6356e..36e8004 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,27 @@ ### Added +- **`OutboundDelegateMsg::UnsubscribeContractRequest` and + `InboundDelegateMsg::UnsubscribeContractResponse`**, both appended at bincode + **tag 8** of their enum. + + freenet-core#2830 specified subscribe and unsubscribe together; only subscribe + was built, and `crates/core/src/contract.rs` has carried the + `TODO(#2830)` since. Until now the only way a delegate's subscription was + released was the implicit cleanup when the delegate itself was unregistered, + so a delegate that had finished with a contract kept holding interest in it + for as long as the delegate existed. + + **Unsubscribing a contract the delegate is not subscribed to reports + `Ok(())`.** Not a convenience: it is what the host does. Teardown goes through + a removal path that is already a no-op for a client id that is not present, so + an error return would have the host inventing a failure it did not have. + + **Appended, never inserted** — every existing tag is exactly where it was, so + deployed delegate WASM is unaffected. Tag 8 was chosen deliberately in + coordination with freenet-stdlib#82, which appends `ScheduleWakeup` / + `WakeupFired` and now takes **tag 9**. + - **`DelegateCtx::list_subscriptions`** — a delegate can now ask the node which contracts it is subscribed to. Backed by two new V2 host functions in the `freenet_delegate_contracts` import namespace, diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index a4a311e..2a8312d 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -1633,11 +1633,22 @@ impl HostResponse { "SubscribeContractRequest reached client serialization - this is a bug" ); } + OutboundDelegateMsg::UnsubscribeContractRequest(_) => { + tracing::error!( + "UnsubscribeContractRequest reached client serialization - this is a bug" + ); + } OutboundDelegateMsg::SendDelegateMessage(_) => { tracing::error!( "SendDelegateMessage reached client serialization - this is a bug" ); } + // Deliberately exhaustive, no wildcard. `#[non_exhaustive]` + // does not apply inside the defining crate, so a new + // outbound variant is a compile error here until someone + // decides whether it has a FlatBuffers union member or is + // executor-only like the contract requests above. Adding + // the unsubscribe pair is what proved this fires. }); let messages_offset = builder.create_vector(&messages); let delegate_response_offset = FbsDelegateResponse::create( diff --git a/rust/src/delegate_interface.rs b/rust/src/delegate_interface.rs index 5b3cad7..67b2676 100644 --- a/rust/src/delegate_interface.rs +++ b/rust/src/delegate_interface.rs @@ -564,6 +564,9 @@ pub enum InboundDelegateMsg<'a> { SubscribeContractResponse(SubscribeContractResponse), ContractNotification(ContractNotification), DelegateMessage(DelegateMessage), + // Appended in 0.9.0 at tag 8. New variants go at the END, never inserted — + // see the wire-format note on this enum. + UnsubscribeContractResponse(UnsubscribeContractResponse), } impl InboundDelegateMsg<'_> { @@ -587,6 +590,9 @@ impl InboundDelegateMsg<'_> { InboundDelegateMsg::ContractNotification(r) } InboundDelegateMsg::DelegateMessage(r) => InboundDelegateMsg::DelegateMessage(r), + InboundDelegateMsg::UnsubscribeContractResponse(r) => { + InboundDelegateMsg::UnsubscribeContractResponse(r) + } } } @@ -612,6 +618,10 @@ impl InboundDelegateMsg<'_> { Some(context) } InboundDelegateMsg::DelegateMessage(DelegateMessage { context, .. }) => Some(context), + InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + context, + .. + }) => Some(context), _ => None, } } @@ -638,6 +648,10 @@ impl InboundDelegateMsg<'_> { Some(context) } InboundDelegateMsg::DelegateMessage(DelegateMessage { context, .. }) => Some(context), + InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + context, + .. + }) => Some(context), _ => None, } } @@ -823,6 +837,10 @@ pub enum OutboundDelegateMsg { UpdateContractRequest(UpdateContractRequest), SubscribeContractRequest(SubscribeContractRequest), SendDelegateMessage(DelegateMessage), + // Appended in 0.9.0 at tag 8. New variants go at the END, never inserted — + // see the wire-format note on this enum. freenet-stdlib#82 appends + // ScheduleWakeup after this, at tag 9. + UnsubscribeContractRequest(UnsubscribeContractRequest), } impl From for OutboundDelegateMsg { @@ -855,6 +873,12 @@ impl From for OutboundDelegateMsg { } } +impl From for OutboundDelegateMsg { + fn from(req: UnsubscribeContractRequest) -> Self { + Self::UnsubscribeContractRequest(req) + } +} + impl From for OutboundDelegateMsg { fn from(msg: DelegateMessage) -> Self { Self::SendDelegateMessage(msg) @@ -877,6 +901,7 @@ impl OutboundDelegateMsg { OutboundDelegateMsg::PutContractRequest(msg) => msg.processed, OutboundDelegateMsg::UpdateContractRequest(msg) => msg.processed, OutboundDelegateMsg::SubscribeContractRequest(msg) => msg.processed, + OutboundDelegateMsg::UnsubscribeContractRequest(msg) => msg.processed, OutboundDelegateMsg::SendDelegateMessage(msg) => msg.processed, OutboundDelegateMsg::RequestUserInput(_) => true, OutboundDelegateMsg::ContextUpdated(_) => true, @@ -901,6 +926,10 @@ impl OutboundDelegateMsg { context, .. }) => Some(context), + OutboundDelegateMsg::UnsubscribeContractRequest(UnsubscribeContractRequest { + context, + .. + }) => Some(context), OutboundDelegateMsg::SendDelegateMessage(DelegateMessage { context, .. }) => { Some(context) } @@ -926,6 +955,10 @@ impl OutboundDelegateMsg { context, .. }) => Some(context), + OutboundDelegateMsg::UnsubscribeContractRequest(UnsubscribeContractRequest { + context, + .. + }) => Some(context), OutboundDelegateMsg::SendDelegateMessage(DelegateMessage { context, .. }) => { Some(context) } @@ -1080,6 +1113,59 @@ pub struct SubscribeContractResponse { pub context: DelegateContext, } +/// Request to stop receiving a contract's state changes, from within a delegate. +/// +/// The counterpart of [`SubscribeContractRequest`]. Before 0.9.0 a delegate had +/// no way to drop a subscription it had taken: the only release path was the +/// implicit cleanup when the delegate itself was unregistered, so a delegate +/// that had finished with a contract went on holding interest in it for as long +/// as the delegate existed. Specified in freenet-core#2830 alongside subscribe; +/// only subscribe was built. +/// +/// Answered with [`InboundDelegateMsg::UnsubscribeContractResponse`]. +/// +/// Field order is the wire format. Do not reorder. +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct UnsubscribeContractRequest { + /// The contract to stop receiving notifications for. + pub contract_id: ContractInstanceId, + /// Context for the delegate. + pub context: DelegateContext, + /// Whether this request has been processed. + pub processed: bool, +} + +impl UnsubscribeContractRequest { + pub fn new(contract_id: ContractInstanceId) -> Self { + Self { + contract_id, + context: Default::default(), + processed: false, + } + } +} + +/// Response after attempting to unsubscribe from a contract from a delegate. +/// +/// **Unsubscribing a contract the delegate is not subscribed to reports +/// `Ok(())`, not an error.** That is not a convenience: it is what the host +/// actually does. Teardown goes through the same removal path that a +/// no-longer-present client id already takes as a no-op, so returning an error +/// would have the host inventing a failure it did not have. It also matches the +/// subscribe side, where a repeat subscribe is a set insert. +/// +/// Field order is the wire format. Do not reorder. +#[derive(Serialize, Deserialize, Debug, Clone)] +pub struct UnsubscribeContractResponse { + /// The contract unsubscribed from. + pub contract_id: ContractInstanceId, + /// Success (Ok) or error message (Err). Unsubscribing a contract the + /// delegate was not subscribed to reports `Ok(())`. + pub result: Result<(), String>, + /// Context for the delegate. + pub context: DelegateContext, +} + /// A message sent from one delegate to another. /// /// Delegates can communicate with each other by emitting @@ -1379,8 +1465,8 @@ mod delegate_wire_compat { /// The number of variants each enum has **today**. These are not free /// parameters: see `an_unpinned_variant_fails_this_test`, which is what /// makes them fail closed rather than drift. - const INBOUND_VARIANT_COUNT: u32 = 8; - const OUTBOUND_VARIANT_COUNT: u32 = 8; + const INBOUND_VARIANT_COUNT: u32 = 9; + const OUTBOUND_VARIANT_COUNT: u32 = 9; fn instance_id() -> ContractInstanceId { ContractInstanceId::new([0x5Au8; 32]) @@ -1427,6 +1513,7 @@ mod delegate_wire_compat { InboundDelegateMsg::SubscribeContractResponse(_) => 5, InboundDelegateMsg::ContractNotification(_) => 6, InboundDelegateMsg::DelegateMessage(_) => 7, + InboundDelegateMsg::UnsubscribeContractResponse(_) => 8, } } @@ -1442,6 +1529,7 @@ mod delegate_wire_compat { OutboundDelegateMsg::UpdateContractRequest(_) => 5, OutboundDelegateMsg::SubscribeContractRequest(_) => 6, OutboundDelegateMsg::SendDelegateMessage(_) => 7, + OutboundDelegateMsg::UnsubscribeContractRequest(_) => 8, } } @@ -1486,6 +1574,11 @@ mod delegate_wire_compat { delegate_key(), vec![0xEE], )), + InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + contract_id: id, + result: Ok(()), + context: ctx.clone(), + }), ] } @@ -1521,6 +1614,7 @@ mod delegate_wire_compat { delegate_key(), vec![0xEE], )), + OutboundDelegateMsg::UnsubscribeContractRequest(UnsubscribeContractRequest::new(id)), ] } @@ -1734,4 +1828,54 @@ mod delegate_wire_compat { OutboundDelegateMsg is wrong and delegates are silently misreading messages" ); } + + /// The unsubscribe pair added in 0.9.0 round-trips, and adding it did not + /// disturb any payload that predates it. + /// + /// The pre-0.9.0 byte string is hand-built rather than produced by this + /// crate, so it stands in for bytes from a delegate compiled before the + /// pair existed. Both halves matter: the new variant must work, and the old + /// ones must be untouched by its arrival. + #[test] + fn the_unsubscribe_pair_round_trips_and_disturbs_nothing_older() { + let id = instance_id(); + + let req = + OutboundDelegateMsg::UnsubscribeContractRequest(UnsubscribeContractRequest::new(id)); + let encoded = bincode::serialize(&req).expect("request must serialize"); + assert_eq!(wire_tag(&encoded), 8, "unsubscribe request is frozen at 8"); + match bincode::deserialize::(&encoded).expect("must round-trip") { + OutboundDelegateMsg::UnsubscribeContractRequest(r) => { + assert_eq!(r.contract_id, id); + assert!(!r.processed); + } + other => panic!("round-tripped into {other:?}"), + } + + let resp = InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + contract_id: id, + result: Ok(()), + context: DelegateContext::default(), + }); + let encoded = bincode::serialize(&resp).expect("response must serialize"); + assert_eq!(wire_tag(&encoded), 8, "unsubscribe response is frozen at 8"); + assert!(matches!( + bincode::deserialize::>(&encoded).expect("must round-trip"), + InboundDelegateMsg::UnsubscribeContractResponse(_) + )); + + // A ContractNotification encoded before 0.9.0 existed: tag 6, the 32 + // raw id bytes, an empty state and an empty context. Appending at 8 + // must leave it decoding exactly as it always did. + let mut pre_0_9_0 = vec![6u8, 0, 0, 0]; + pre_0_9_0.extend_from_slice(&[0x5Au8; 32]); + pre_0_9_0.extend_from_slice(&0u64.to_le_bytes()); + pre_0_9_0.extend_from_slice(&0u64.to_le_bytes()); + match bincode::deserialize::>(&pre_0_9_0) + .expect("a pre-0.9.0 payload must still decode") + { + InboundDelegateMsg::ContractNotification(n) => assert_eq!(n.contract_id, id), + other => panic!("a pre-0.9.0 ContractNotification decoded as {other:?}"), + } + } } From d4ad47e7c079c41a6bded5a95cd4de73ea70eebd Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Sun, 30 Aug 2026 16:01:52 -0500 Subject: [PATCH 13/13] fix(review): address the re-review, including a design error of my own MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the unsubscribe commit found one real design mistake, the oracle problem in my own new test, and a false claim I had just reintroduced. The exactly-full re-check was wrong twice over. It re-called the length function whenever the read came back exactly filling the buffer, meaning to catch a set that had grown. But an exactly-full buffer is the NORMAL result, not an edge case: len is derived from the same set the read serialises. So it doubled a scan the docs describe as O(all contracts with any delegate subscription) on every non-empty call, and it could fail a correct read by reporting ERR_BUFFER_TOO_SMALL when a subscription happened to arrive in between. It was not even sound: grow-then-shrink passes it. Removed. The import contract already requires the host to return ERR_BUFFER_TOO_SMALL rather than truncate, so a short write means the set shrank, and completeness rests on that contract, which is why the contract is stated on the import rather than implied. The decisions now live in validate_list_len and resolve_written, which are pure and compiled on every target. That matters because CI runs cargo test on the host only; the wasm32 matrix entries build and lint but execute nothing, so everything previously inside cfg(target_family = "wasm") was type-checked and never run. Ten table-driven tests now cover the branches, including the wasm32 truncation case (1 << 32 as usize is 0 there, which would have surfaced as an empty list). My round-trip test for the new unsubscribe pair proved only that the code agrees with itself. Both structs' docs say the field ORDER is the wire format, and a round-trip through this crate's own encoder cannot establish that — swapping contract_id and result would round-trip just as happily. Both layouts are now frozen as hand-written bytes, the inbound half asserts the VALUES rather than just the variant, and the Err(String) path is exercised since it has a different bincode shape from Ok. Reintroduced false claim, the same defect class as this PR's headline fix: the doc said "several of them are #[non_exhaustive]" of the payload structs. Exactly one is, ApplicationMessage. Corrected and named. Also: the #82 note asserted that PR takes tag 9, which it does not yet — it still declares 8, so the text now says it must move and that the pin will catch whichever lands second. The unknown-tag probe gained an outbound control to match the inbound one. The terminality fixture's non-zero guard asserted some byte was non-zero when what ends_with relies on is a non-zero TAIL. Fixes a pre-existing bug found in review: get_context and get_mut_context returned None for UserResponse, which carries a context, because a `_ => None` wildcard swallowed the missing arm and nothing in the crate called either accessor. Arm added, both accessors are now exhaustive with no wildcard, and a table-driven test drives them off every_inbound/every_outbound so the next omission is a compile error rather than a silent None. Filed #101 for four sibling sites with the same unvalidated-length shape. One of these fixes was itself wrong first: replacing expect_err with unwrap_or_else to make a panic message interpolate inverted the test, since unwrap_or_else unwraps Ok and runs the closure on Err. The compiler caught it. Claude-Session: https://claude.ai/code/session_014tq59dRUCNsHR1GuUkguHw --- rust/src/client_api/client_events.rs | 9 +- rust/src/delegate_host.rs | 211 ++++++++++++++++++++++----- rust/src/delegate_interface.rs | 181 +++++++++++++++++++++-- 3 files changed, 342 insertions(+), 59 deletions(-) diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index 2a8312d..f5c40ed 100644 --- a/rust/src/client_api/client_events.rs +++ b/rust/src/client_api/client_events.rs @@ -4180,9 +4180,12 @@ mod struct_field_wire_compat { }; let inner = bincode::serialize(&response).expect("response must serialize"); - assert!( - inner.iter().any(|b| *b != 0), - "the fixture must not be all zeros, or the ends_with below proves nothing" + assert_ne!( + *inner.last().expect("the fixture encodes to something"), + 0, + "ends_with needs a non-zero TAIL, not merely a non-zero byte somewhere. If the last \ + field ever becomes empty again, ends_with silently degenerates into 'ends in zeros' \ + and stops catching the append it exists to catch" ); // The default type parameter, i.e. the `HostResponse` that is actually // on the wire. Pinning terminality against some other instantiation diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index 69b97fd..2148461 100644 --- a/rust/src/delegate_host.rs +++ b/rust/src/delegate_host.rs @@ -133,6 +133,11 @@ pub mod error_codes { /// is already well outside the intended shape. pub const MAX_SUBSCRIPTION_LIST_BYTES: i64 = 32 * 1024; +// The cap is compared against a length that must also be a multiple of 32. If +// it ever stops being one, the upper bound becomes unreachable and the "1024 +// contract ids" above becomes a lie, silently. +const _: () = assert!(MAX_SUBSCRIPTION_LIST_BYTES % 32 == 0); + // ============================================================================ // Host function declarations (WASM only) // ============================================================================ @@ -792,53 +797,22 @@ impl DelegateCtx { // Step 1: how many bytes to allocate. Zero is a valid answer and // means "subscribed to nothing". // - // Every host return is validated before it is used as a length. - // `usize` is 32 bits on wasm32, so an `as usize` cast on an - // unchecked i64 truncates silently: a bogus 2^32 would become 0 and - // surface as `Ok(vec![])` — the "you hold no subscriptions" answer - // this API's whole return type exists to keep distinguishable from - // a failure. Reject rather than cast. + // The decisions live in `validate_list_len` and `resolve_written`, + // which are pure and compiled on every target, so they have + // host-side tests. Only the two `unsafe` host calls are wasm-only — + // otherwise these branches would be reachable by nothing but a + // wasm32 runtime CI does not run. let len = unsafe { __frnt__delegate__list_subscriptions_len() }; - if len < 0 { - return Err(len); - } - if len == 0 { + let capacity = validate_list_len(len)?; + if capacity == 0 { return Ok(Vec::new()); } - if len % 32 != 0 || len > MAX_SUBSCRIPTION_LIST_BYTES { - // The import contract promises a multiple of 32 within the - // cap. A host that breaks it is malfunctioning, and allocating - // on its say-so risks an allocation failure, which in a - // delegate is an abort rather than an error we can return. - return Err(error_codes::ERR_STORE_ERROR as i64); - } - // Step 2: read the ids. - let mut buf = vec![0u8; len as usize]; + let mut buf = vec![0u8; capacity]; let written = unsafe { __frnt__delegate__list_subscriptions(buf.as_mut_ptr() as i64, buf.len() as i64) }; - if written < 0 { - return Err(written); - } - if written > len { - // Writing past the buffer we advertised is a host bug. Left - // unchecked, `truncate` would be a no-op and the zero-filled - // tail would decode as valid-looking all-zero contract ids. - return Err(error_codes::ERR_STORE_ERROR as i64); - } - if written == len { - // Ambiguous: an exactly-full buffer cannot be distinguished - // from one the host wanted to overflow, so a set that GREW - // between the two calls would silently come back one entry - // short and look complete. Re-ask instead of guessing; the - // host reports ERR_BUFFER_TOO_SMALL if it still does not fit. - let recheck = unsafe { __frnt__delegate__list_subscriptions_len() }; - if recheck > len { - return Err(error_codes::ERR_BUFFER_TOO_SMALL as i64); - } - } - buf.truncate(written as usize); + buf.truncate(resolve_written(len, written)?); decode_contract_id_list(&buf).ok_or(error_codes::ERR_STORE_ERROR as i64) } #[cfg(not(target_family = "wasm"))] @@ -913,6 +887,68 @@ impl std::fmt::Debug for DelegateCtx { } } +// ============================================================================ +// list_subscriptions host-return validation +// +// Split out from the wasm-only body on purpose. CI runs `cargo test` on the +// host target only — the wasm32 matrix entries build and lint but execute +// nothing — so logic left inside `#[cfg(target_family = "wasm")]` is +// type-checked and never run. These are the branches most worth running. +// ============================================================================ + +/// Validate the byte length the host reports before allocating on it. +/// +/// Returns the capacity to allocate, or the error to hand back. +/// +/// `usize` is 32 bits on wasm32, so an `as usize` cast on an unvalidated `i64` +/// truncates silently: a bogus `2^32` becomes `0` and surfaces as `Ok(vec![])`, +/// the "you hold no subscriptions" answer that this API's whole return type +/// exists to keep distinguishable from a failure. So the value is refused +/// rather than cast. The cap additionally keeps an implausible length away from +/// `vec![0u8; len]`, since an allocation failure inside a delegate is an abort +/// rather than an error anyone can return. +#[cfg_attr(not(target_family = "wasm"), allow(dead_code))] +fn validate_list_len(len: i64) -> Result { + if len < 0 { + return Err(len); + } + if len % 32 != 0 || len > MAX_SUBSCRIPTION_LIST_BYTES { + return Err(error_codes::ERR_STORE_ERROR as i64); + } + Ok(len as usize) +} + +/// Decide how much of the buffer to keep, given what the host reports writing. +/// +/// `written < len` is accepted as a complete list: the import contract requires +/// the host to return `ERR_BUFFER_TOO_SMALL` rather than truncate, so a short +/// write means the set shrank between the two calls, not that it was cut off. +/// Completeness therefore rests on the host honouring that contract, which is +/// why the contract is stated on the import rather than left implied. +/// +/// An earlier version re-called the length function whenever the buffer came +/// back exactly full, meaning to catch a set that had grown. That was wrong +/// twice over: an exactly-full buffer is the *normal* result, not an edge case, +/// so it doubled a scan documented as expensive on every non-empty call — and +/// it could fail a correct read, by reporting `ERR_BUFFER_TOO_SMALL` for a +/// complete list when a subscription happened to arrive in between. +#[cfg_attr(not(target_family = "wasm"), allow(dead_code))] +fn resolve_written(len: i64, written: i64) -> Result { + if written < 0 { + return Err(written); + } + if written > len { + // Writing past the buffer we advertised is a host bug. Left unchecked, + // `truncate` is a no-op and the zero-filled tail decodes as + // valid-looking all-zero contract ids. + return Err(error_codes::ERR_STORE_ERROR as i64); + } + if written % 32 != 0 { + return Err(error_codes::ERR_STORE_ERROR as i64); + } + Ok(written as usize) +} + // ============================================================================ // Contract-id-list wire codec (shared host↔delegate contract for // list_subscriptions) @@ -1109,3 +1145,98 @@ mod contract_id_list_codec_tests { ); } } + +#[cfg(test)] +mod list_subscriptions_guard_tests { + use super::{error_codes, resolve_written, validate_list_len, MAX_SUBSCRIPTION_LIST_BYTES}; + + const STORE_ERR: i64 = error_codes::ERR_STORE_ERROR as i64; + + #[test] + fn a_negative_length_is_passed_through_as_the_host_error() { + assert_eq!( + validate_list_len(error_codes::ERR_NOT_IN_PROCESS as i64), + Err(error_codes::ERR_NOT_IN_PROCESS as i64), + "a host error code must reach the caller unchanged, not be reshaped" + ); + } + + #[test] + fn zero_is_a_successful_empty_enumeration() { + assert_eq!( + validate_list_len(0), + Ok(0), + "zero means 'subscribed to nothing' and must NOT be an error" + ); + } + + #[test] + fn a_length_that_is_not_a_whole_number_of_ids_is_refused() { + assert_eq!(validate_list_len(33), Err(STORE_ERR)); + assert_eq!(validate_list_len(31), Err(STORE_ERR)); + } + + #[test] + fn an_implausible_length_is_refused_before_it_reaches_an_allocation() { + assert_eq!( + validate_list_len(MAX_SUBSCRIPTION_LIST_BYTES + 32), + Err(STORE_ERR) + ); + assert_eq!( + validate_list_len(MAX_SUBSCRIPTION_LIST_BYTES), + Ok(MAX_SUBSCRIPTION_LIST_BYTES as usize), + "the cap itself is allowed" + ); + } + + /// The wasm32 truncation this guard exists for: `usize` is 32 bits there, + /// so `2^32 as usize` is `0` and would have surfaced as an empty list. + #[test] + fn a_length_that_would_truncate_to_zero_on_wasm32_is_refused() { + assert_eq!(validate_list_len(1i64 << 32), Err(STORE_ERR)); + } + + #[test] + fn a_negative_write_is_passed_through_as_the_host_error() { + assert_eq!( + resolve_written(320, error_codes::ERR_STORE_ERROR as i64), + Err(STORE_ERR) + ); + } + + #[test] + fn writing_past_the_advertised_buffer_is_refused() { + assert_eq!( + resolve_written(320, 352), + Err(STORE_ERR), + "truncate would be a no-op, leaving a zero-filled tail to decode as \ + valid-looking all-zero contract ids" + ); + } + + #[test] + fn a_partial_id_written_is_refused() { + assert_eq!(resolve_written(320, 300), Err(STORE_ERR)); + } + + #[test] + fn an_exactly_full_buffer_is_the_normal_case_and_is_accepted() { + assert_eq!( + resolve_written(320, 320), + Ok(320), + "len comes from the same set the read serialises, so exactly-full is \ + the ordinary outcome — treating it as suspicious cost a second scan \ + on every call and could fail a correct read" + ); + } + + #[test] + fn a_short_write_is_accepted_as_a_set_that_shrank() { + assert_eq!( + resolve_written(320, 288), + Ok(288), + "the import contract requires ERR_BUFFER_TOO_SMALL rather than \ + truncation, so a short write means the set shrank" + ); + } +} diff --git a/rust/src/delegate_interface.rs b/rust/src/delegate_interface.rs index 67b2676..15ffa19 100644 --- a/rust/src/delegate_interface.rs +++ b/rust/src/delegate_interface.rs @@ -601,6 +601,10 @@ impl InboundDelegateMsg<'_> { InboundDelegateMsg::ApplicationMessage(ApplicationMessage { context, .. }) => { Some(context) } + // UserResponse carries a context too. It was missing from both + // accessors, so this returned None for it — the `_ => None` + // wildcard below swallowed the omission silently. Found in review. + InboundDelegateMsg::UserResponse(UserInputResponse { context, .. }) => Some(context), InboundDelegateMsg::GetContractResponse(GetContractResponse { context, .. }) => { Some(context) } @@ -622,7 +626,10 @@ impl InboundDelegateMsg<'_> { context, .. }) => Some(context), - _ => None, + // No wildcard, deliberately. Every variant carries a context, and + // the `_ => None` that used to sit here is what let UserResponse go + // unhandled and silently report "no context". Exhaustive means a + // new variant is a compile error here instead. } } @@ -631,6 +638,10 @@ impl InboundDelegateMsg<'_> { InboundDelegateMsg::ApplicationMessage(ApplicationMessage { context, .. }) => { Some(context) } + // UserResponse carries a context too. It was missing from both + // accessors, so this returned None for it — the `_ => None` + // wildcard below swallowed the omission silently. Found in review. + InboundDelegateMsg::UserResponse(UserInputResponse { context, .. }) => Some(context), InboundDelegateMsg::GetContractResponse(GetContractResponse { context, .. }) => { Some(context) } @@ -652,7 +663,10 @@ impl InboundDelegateMsg<'_> { context, .. }) => Some(context), - _ => None, + // No wildcard, deliberately. Every variant carries a context, and + // the `_ => None` that used to sit here is what let UserResponse go + // unhandled and silently report "no context". Exhaustive means a + // new variant is a compile error here instead. } } } @@ -800,9 +814,10 @@ impl UserInputResponse<'_> { /// understands every tag an older delegate can emit, so deployed delegate /// WASM keeps working against an upgraded node with no rebuild. This does /// **not** extend to appending a FIELD to an existing variant's payload -/// struct — several of them are `#[non_exhaustive]`, which invites exactly -/// that — because a field breaks in the opposite direction. See +/// struct, because a field breaks in the opposite direction. See /// `struct_field_wire_compat` in `client_api::client_events`. +/// (`ApplicationMessage` is `#[non_exhaustive]`, which invites precisely that +/// edit. It is the only payload struct here that is.) /// - **New delegate → old host: fails, and fails loudly.** bincode rejects the /// unknown variant tag — as `ErrorKind::Custom("invalid value: integer `N`, /// expected variant index 0 <= i < M")`, since it hands the index to serde's @@ -838,8 +853,10 @@ pub enum OutboundDelegateMsg { SubscribeContractRequest(SubscribeContractRequest), SendDelegateMessage(DelegateMessage), // Appended in 0.9.0 at tag 8. New variants go at the END, never inserted — - // see the wire-format note on this enum. freenet-stdlib#82 appends - // ScheduleWakeup after this, at tag 9. + // see the wire-format note on this enum. freenet-stdlib#82 also appends + // here (ScheduleWakeup) and must therefore move to tag 9; at the time of + // writing that PR still declares tag 8, so whichever lands second will trip + // the pin, which is the intended outcome rather than a surprise. UnsubscribeContractRequest(UnsubscribeContractRequest), } @@ -1731,14 +1748,22 @@ mod delegate_wire_compat { let mut probe = INBOUND_VARIANT_COUNT.to_le_bytes().to_vec(); probe.extend_from_slice(&[0u8; 256]); - let err = bincode::deserialize::>(&probe) - .expect_err("tag {INBOUND_VARIANT_COUNT} must not decode as an InboundDelegateMsg"); + let err = match bincode::deserialize::>(&probe) { + Ok(v) => panic!( + "tag {INBOUND_VARIANT_COUNT} must not decode as an InboundDelegateMsg, got {v:?}" + ), + Err(e) => e, + }; assert_rejected_as_unknown_variant(&err, INBOUND_VARIANT_COUNT, "InboundDelegateMsg"); let mut probe = OUTBOUND_VARIANT_COUNT.to_le_bytes().to_vec(); probe.extend_from_slice(&[0u8; 256]); - let err = bincode::deserialize::(&probe) - .expect_err("tag {OUTBOUND_VARIANT_COUNT} must not decode as an OutboundDelegateMsg"); + let err = match bincode::deserialize::(&probe) { + Ok(v) => panic!( + "tag {OUTBOUND_VARIANT_COUNT} must not decode as an OutboundDelegateMsg, got {v:?}" + ), + Err(e) => e, + }; assert_rejected_as_unknown_variant(&err, OUTBOUND_VARIANT_COUNT, "OutboundDelegateMsg"); // Control, so the probe cannot pass vacuously from the other end: the @@ -1749,8 +1774,17 @@ mod delegate_wire_compat { let mut control = (INBOUND_VARIANT_COUNT - 1).to_le_bytes().to_vec(); control.extend_from_slice(&[0u8; 256]); bincode::deserialize::>(&control).expect( - "the last known inbound tag must decode from zeros, or the unknown-tag probe above \ - is no longer distinguishing an unknown tag from an unparseable payload", + "the LAST inbound variant's payload must be decodable from zeros, or this probe can \ + no longer tell an unknown tag from an unparseable payload. If a variant whose \ + payload rejects zeros was just appended, do not delete this — point the control at \ + a variant that still decodes from zeros", + ); + + let mut control = (OUTBOUND_VARIANT_COUNT - 1).to_le_bytes().to_vec(); + control.extend_from_slice(&[0u8; 256]); + bincode::deserialize::(&control).expect( + "the LAST outbound variant's payload must be decodable from zeros — see the inbound \ + control above for what to do if that stops being true", ); } @@ -1859,10 +1893,62 @@ mod delegate_wire_compat { }); let encoded = bincode::serialize(&resp).expect("response must serialize"); assert_eq!(wire_tag(&encoded), 8, "unsubscribe response is frozen at 8"); - assert!(matches!( - bincode::deserialize::>(&encoded).expect("must round-trip"), - InboundDelegateMsg::UnsubscribeContractResponse(_) - )); + match bincode::deserialize::>(&encoded).expect("must round-trip") { + InboundDelegateMsg::UnsubscribeContractResponse(r) => { + // Assert the VALUES, not merely the variant. Checking only + // `matches!` is what lets a field reorder through: the encoder + // and decoder would still agree with each other. + assert_eq!(r.contract_id, id); + assert!(r.result.is_ok()); + } + other => panic!("round-tripped into {other:?}"), + } + + // Both structs' doc comments say the field ORDER is the wire format. + // A round-trip through this crate's own encoder cannot establish that — + // it proves the code agrees with itself, and a swap of `contract_id` + // and `result` would round-trip just as happily. So the layout is + // frozen as hand-written bytes, the same way ContractNotification is. + let mut expected_resp = vec![8u8, 0, 0, 0]; + expected_resp.extend_from_slice(&[0x5Au8; 32]); // contract_id + expected_resp.extend_from_slice(&0u32.to_le_bytes()); // result: Ok variant tag + expected_resp.extend_from_slice(&0u64.to_le_bytes()); // context: empty + assert_eq!( + encoded, expected_resp, + "UnsubscribeContractResponse layout is frozen: tag, contract_id, result, context" + ); + + let expected_req = { + let mut v = vec![8u8, 0, 0, 0]; + v.extend_from_slice(&[0x5Au8; 32]); // contract_id + v.extend_from_slice(&0u64.to_le_bytes()); // context: empty + v.push(0u8); // processed: false + v + }; + assert_eq!( + bincode::serialize(&req).expect("request must serialize"), + expected_req, + "UnsubscribeContractRequest layout is frozen: tag, contract_id, context, processed" + ); + + // The error path has a different bincode shape from Ok and is part of + // the same frozen layout, so it is exercised rather than assumed. + let err_resp = + InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + contract_id: id, + result: Err("nope".to_string()), + context: DelegateContext::default(), + }); + match bincode::deserialize::>( + &bincode::serialize(&err_resp).expect("must serialize"), + ) + .expect("must round-trip") + { + InboundDelegateMsg::UnsubscribeContractResponse(r) => { + assert_eq!(r.result.unwrap_err(), "nope"); + } + other => panic!("error response round-tripped into {other:?}"), + } // A ContractNotification encoded before 0.9.0 existed: tag 6, the 32 // raw id bytes, an empty state and an empty context. Appending at 8 @@ -1878,4 +1964,67 @@ mod delegate_wire_compat { other => panic!("a pre-0.9.0 ContractNotification decoded as {other:?}"), } } + + /// Every inbound variant whose payload carries a `context` must return it. + /// + /// Both `get_context` and `get_mut_context` end in `_ => None`, so a + /// missing arm is not a compile error — it silently reports "no context". + /// That wildcard had already swallowed one: `UserResponse` carries a + /// context and returned `None` for it, undetected, because nothing in the + /// crate called either accessor. + /// + /// Driven off `every_inbound`, so a newly appended variant is covered the + /// moment it is added to that list — which the tag pin already forces. + #[test] + fn every_inbound_variant_with_a_context_exposes_it() { + for mut msg in every_inbound() { + let carries_context = !matches!(msg, InboundDelegateMsg::ApplicationMessage(_)); + let tag = pinned_inbound_tag(&msg); + + // ApplicationMessage has a context field too, so in fact every + // variant present today should expose one. Asserted uniformly + // rather than by an allow-list, so the question a new variant + // raises is "does it have a context", not "is it in the list". + let _ = carries_context; + + assert!( + msg.get_context().is_some(), + "InboundDelegateMsg tag {tag} has a context field but get_context returned None; \ + the `_ => None` wildcard hides a missing arm" + ); + assert!( + msg.get_mut_context().is_some(), + "InboundDelegateMsg tag {tag} has a context field but get_mut_context returned \ + None; the two accessors must agree" + ); + } + } + + /// The same, for the outbound side. + /// + /// `RequestUserInput` and `ContextUpdated` genuinely have no context field + /// to return, so they are the two exceptions and are named explicitly + /// rather than skipped by a wildcard. + #[test] + fn every_outbound_variant_with_a_context_exposes_it() { + for mut msg in every_outbound() { + let tag = pinned_outbound_tag(&msg); + let has_no_context = matches!( + msg, + OutboundDelegateMsg::RequestUserInput(_) | OutboundDelegateMsg::ContextUpdated(_) + ); + if has_no_context { + continue; + } + assert!( + msg.get_context().is_some(), + "OutboundDelegateMsg tag {tag} has a context field but get_context returned None" + ); + assert!( + msg.get_mut_context().is_some(), + "OutboundDelegateMsg tag {tag} has a context field but get_mut_context returned \ + None; the two accessors must agree" + ); + } + } }