diff --git a/CHANGELOG.md b/CHANGELOG.md index 30c99d1..36e8004 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,154 @@ ## [Unreleased] +### 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, + `__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, 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**, 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 + +- **`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 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 + 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 + +- **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/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 diff --git a/rust/src/client_api/client_events.rs b/rust/src/client_api/client_events.rs index 1364fae..f5c40ed 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( @@ -3923,3 +3934,384 @@ 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, WrappedState}; + 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() { + #[allow(dead_code)] // `payload` exists to occupy wire space, not to be read + #[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() { + // 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: 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_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 + // would be pinning a type nobody sends. + 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." + ); + + // 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. + /// + /// `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`. + #[allow(dead_code)] // decoded into, never read — the decode is the test + #[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" + ); + } +} diff --git a/rust/src/delegate_host.rs b/rust/src/delegate_host.rs index d11b1e5..2148461 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 //! @@ -108,6 +122,22 @@ 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; + +// 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) // ============================================================================ @@ -181,6 +211,25 @@ 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, 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; } #[cfg(target_family = "wasm")] @@ -223,7 +272,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 +649,48 @@ 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, 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 /// - /// Returns `true` on success, `false` if the contract is unknown or on error. + /// 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; + /// subscribing does not fetch it. pub fn subscribe_contract(&mut self, instance_id: &[u8; 32]) -> bool { #[cfg(target_family = "wasm")] { @@ -618,6 +705,122 @@ 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, + /// 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. + /// + /// # 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 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 + /// + /// 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 + /// 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 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". + // + // 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() }; + let capacity = validate_list_len(len)?; + if capacity == 0 { + return Ok(Vec::new()); + } + + let mut buf = vec![0u8; capacity]; + let written = unsafe { + __frnt__delegate__list_subscriptions(buf.as_mut_ptr() as i64, buf.len() as i64) + }; + 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"))] + { + 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 +887,117 @@ 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) +// ============================================================================ + +/// 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 +1082,161 @@ 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}; + + /// 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]]; + 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" + ); + } +} + +#[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 ae6558f..15ffa19 100644 --- a/rust/src/delegate_interface.rs +++ b/rust/src/delegate_interface.rs @@ -513,14 +513,46 @@ 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 — 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. +/// +/// 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> { @@ -532,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<'_> { @@ -555,6 +590,9 @@ impl InboundDelegateMsg<'_> { InboundDelegateMsg::ContractNotification(r) } InboundDelegateMsg::DelegateMessage(r) => InboundDelegateMsg::DelegateMessage(r), + InboundDelegateMsg::UnsubscribeContractResponse(r) => { + InboundDelegateMsg::UnsubscribeContractResponse(r) + } } } @@ -563,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) } @@ -580,7 +622,14 @@ impl InboundDelegateMsg<'_> { Some(context) } InboundDelegateMsg::DelegateMessage(DelegateMessage { context, .. }) => Some(context), - _ => None, + InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + context, + .. + }) => Some(context), + // 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. } } @@ -589,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) } @@ -606,7 +659,14 @@ impl InboundDelegateMsg<'_> { Some(context) } InboundDelegateMsg::DelegateMessage(DelegateMessage { context, .. }) => Some(context), - _ => None, + InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + context, + .. + }) => Some(context), + // 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. } } } @@ -697,6 +757,86 @@ 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 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 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 +/// 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: 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, 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 +/// 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 +/// 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 @@ -712,6 +852,12 @@ 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 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), } impl From for OutboundDelegateMsg { @@ -744,6 +890,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) @@ -766,6 +918,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, @@ -790,6 +943,10 @@ impl OutboundDelegateMsg { context, .. }) => Some(context), + OutboundDelegateMsg::UnsubscribeContractRequest(UnsubscribeContractRequest { + context, + .. + }) => Some(context), OutboundDelegateMsg::SendDelegateMessage(DelegateMessage { context, .. }) => { Some(context) } @@ -815,6 +972,10 @@ impl OutboundDelegateMsg { context, .. }) => Some(context), + OutboundDelegateMsg::UnsubscribeContractRequest(UnsubscribeContractRequest { + context, + .. + }) => Some(context), OutboundDelegateMsg::SendDelegateMessage(DelegateMessage { context, .. }) => { Some(context) } @@ -969,6 +1130,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 @@ -1249,3 +1463,568 @@ 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 = 9; + const OUTBOUND_VARIANT_COUNT: u32 = 9; + + 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, + InboundDelegateMsg::UnsubscribeContractResponse(_) => 8, + } + } + + /// 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, + OutboundDelegateMsg::UnsubscribeContractRequest(_) => 8, + } + } + + /// 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], + )), + InboundDelegateMsg::UnsubscribeContractResponse(UnsubscribeContractResponse { + contract_id: id, + result: Ok(()), + context: ctx.clone(), + }), + ] + } + + /// 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], + )), + OutboundDelegateMsg::UnsubscribeContractRequest(UnsubscribeContractRequest::new(id)), + ] + } + + /// 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() { + // 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 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 = 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 + // 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 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", + ); + } + + /// Direction 1 of the append rule: **old sender to new receiver works.** + /// + /// 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 + /// 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 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]; + 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. + // 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, + 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" + ); + } + + /// 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"); + 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 + // 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:?}"), + } + } + + /// 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" + ); + } + } +} 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::*; 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;