Repository navigation
Conversation
MaidSafe-Hermes
left a comment
There was a problem hiding this comment.
Verdict: Changes requested
Blocker
src/web_rtc.rs:1788–1833 — When the ordinary request-rate buckets are spent, try_admit_frame can charge a frame to the swarm buckets. read_webrtc_request then has several error exits (read_pq_payload_after_first, buffer reservations, PQ open, and parse_request_header) before it reaches the only explicit refund_swarm() call. Those exits retain both the per-source and listener-global swarm token even though the frame was not classified as a valid swarm request. A connected client can trigger this with a malformed encrypted request header or an incomplete/malformed frame. Repeated failures can consume swarm capacity needed by other connections sharing the source IP or listener until tokens refill.
Keep a swarm charge provisional: refund it on every error/cancellation path and disarm the refund only after a valid swarm request is classified. Add a regression test that exhausts the ordinary request budget, triggers a post-admission parse/frame error, and verifies both swarm buckets are restored and a subsequent valid swarm request is admitted.
Coverage: 2/2 review seats (sol-1, gpt-6.1-sol; glm-1, z-ai/glm-5.3) reviewed this targeted accounting/error-path scope at d79b7e998ba479a2b7729bc657a03c26a2c5c610; both confirmed the finding. This is not a claim that the panel covered every file in the PR.
The focused Rust test command did not complete during the review; no test result is claimed.
d79b7e9 to
e5e8648
Compare
Patch ant-protocol, saorsa-core, saorsa-transport and ant-node to their feat/peer-swarm branches, which this change needs: the swarm messages (WithAutonomi/ant-protocol#50), the send on one named channel (WithAutonomi/saorsa-core#183), the plain STUN answers (WithAutonomi/saorsa-transport#231) and the rendezvous on every node (WithAutonomi/ant-node#262). Once those merge, these point at their main branches, or at the released versions at the release train. Cargo.lock starts from main's lock and differs from it only where those branches need it: the swarm's WebRTC and STUN crates, saorsa-transport's main dropping its CLI (and its clap and reqwest dependencies), and ant-node's main adding winapi-util on Windows. iana-time-zone keeps the windows-core 0.62.2 main's lock has.
Clients reading the same file share its records over WebRTC, BitTorrent
style. Every node now introduces them and stays the last seed: a record no
client has comes from the nodes as before. There is no new binary and no
new port. Design: ADR-0026.
The rendezvous (`src/swarm.rs`):
- Rooms are keyed by a file's root DataMap address. Readers announce at the
2 closest nodes to it that host rooms, on the connections they already
have (QUIC for native clients, WebRTC Direct for browsers).
- The node names each member from its connection, hands each offer to a
different member, and passes an answer back only for an offer it
delivered to the answerer, once, before it expires (the protocol's 70 s
signal lifetime).
- An offer or answer is passed on with only the candidates another reader
may try: no public address but its sender's own host, as the node reaches
it on that request, no local address or mDNS name unless the node reaches
the sender at a local address itself, and at most 16 (ant-protocol's
`constrain_swarm_sdp`), so a stranger cannot point readers' connectivity
checks at a third party or at their own home networks. A stored SDP is
charged the memory it holds.
- A native membership continued over a connection from another address
counts against that address from then on, if it has room.
- Native announces may wait up to 20 s for a signal. Browser announces never
wait, so they hold no browser request slot.
- A node hosts rooms only once browsers can reach it: until its browser
listener has an endpoint and a STUN server to name, it refuses announces
as `NotHosting`, so native readers pass it over at once and meet browsers
at the closest nodes both reach.
- Native requests are signed and checked for freshness and replay. A
request naming another node is refused before it touches that history.
- Membership numbers let the node keep each connection's newest
membership. Ended memberships are remembered for as long as a request sent
before they ended can be taken, so a late request cannot end, take over
or revive one.
- Native replies go only on the connection the request came on, through
saorsa-core's `send_message_on_channel`.
- Hard limits with no configuration:
- rooms, members per room and in all, and rooms per connection;
- members, histories, ended memberships and stored SDP bytes per host
(an IPv4 address or an IPv6 /64);
- unanswered offers per member and waiting announces.
- The WebRTC Direct socket answers plain STUN Binding requests for hosts
that announced in the last 60 s, rate limited in saorsa-transport.
Routing: `NativeSwarmRouter` in `src/node.rs` (shared with the devnet)
handles native swarm requests on their own task budget. `src/web_rtc.rs`
handles browser ones before the storage check, so nodes that store no
chunks host rooms too, and advertises `swarm_rendezvous`. Browser swarm
requests run on a request budget of their own (64 a second, 8 per source):
one within it gives the reads' budget back the token it took on arrival,
one arriving when the reads' budget is spent is taken on the swarm's, and
one past the swarm's budget is refused without closing the channel the
reads share, so a room's announces never spend the request budget reads
need, nor lose the channel to it.
Patch ant-protocol, saorsa-core and saorsa-transport to their feat/peer-swarm branches, which this change needs: the swarm messages (WithAutonomi/ant-protocol#50), the send on one named channel (WithAutonomi/saorsa-core#183) and the plain STUN answers (WithAutonomi/saorsa-transport#231). Once those merge, these point at their main branches, or at the released versions at the release train.
cargo tree percent-encodes the slash in a git branch's name and cargo metadata does not, so the notices generator matched no package on such a branch (this PR's dependency branches are feat/peer-swarm): it now compares both decoded. saorsa-transport's main pulls granit-parser 2.0.0, which ships its licence texts as 1.3.0 did; its entry names the new version.
Rust 1.99's clippy (assert_is_empty, which CI's stable toolchain runs) asks for assert_eq when a test checks that a value is empty, so a failure shows the value.
The pending-offer test never let B take its offers, so B's 32-signal inbox stopped delivery whatever A's own cap on unanswered offers did. B now collects every offer without answering, and A's 33rd offer is still refused while B's inbox is empty. The expiry test swept past the 70 s signal deadline, which is also past the 60 s a silent member lasts, so the members left and took their offers with them whether or not signals expired. The members are now kept fresh across that deadline, the offer and its SDP must still be there just before it and gone just after it, and silent members expiring is a test of its own. Each test fails with the limit or expiry it covers removed.
The earlier test answered an offer that did not exist, so it passed with the recipient check removed. Now A's live offer goes to B, a third member of the same room answers it with its real id and is refused, the offer stays pending, and B's answer still reaches A. It fails if any member of the room may answer.
The client now counts each of its own fetches from the nodes, not only reads of a file's data records, when it limits what it sends other readers: opening a file's map, a raw read or an existence check needs its CPU and line first too.
Browsers' swarm requests run on a budget of 64 a second per node and 8 per source, but native clients' only had a cap on tasks running at once. A native client can make identities for free, so one host could announce with wait_ms 0 and fresh memberships as fast as the slots turned over, and every replacement does its work under the rendezvous lock browsers use too. Native swarm requests now take a place from a budget of their own with the same figures, 64 a second in all and 8 from one host (an IPv4 address or an IPv6 /64), before they are decoded or given a task; past it they are dropped and counted (native_over_budget), and the client reads from nodes alone. The node's budget is taken first, so once it is spent a flood costs one check a request. At most 4096 hosts' budgets are remembered, those that have refilled forgotten first, which always leaves room for a new host since the node's own budget keeps all but a few full. Tests: a host's identities and an IPv6 /64 share one budget, many hosts together get the node's and no more, the hosts remembered stay bounded and a forgotten one starts afresh, and a flood through the router from one host is held to its budget while another host gets through. They fail with the router's check or the per-host budget removed. ADR-0026 says so.
…or read The client now tries a waiting room join again when the file is joined again or a read of one of its records starts, not only when a record of it comes in, so a file whose reads the nodes fail still joins once a room has gone unread.
The swarm names the browser endpoint's address as its rooms' STUN server and hosts rooms only while it has one. Publishing a changed endpoint writes the endpoint file first, which can fail and keep the old publication: if that was a gateway mapping that has ended or moved, the swarm went on naming an address the node no longer has. Every refresh of the endpoint now ends by setting the swarm's STUN server from its outcome: the published endpoint's address after a refresh that succeeded, none after one that failed. A refresh that changes or withdraws the endpoint also stops the swarm hosting before its first await (the file write, or the DHT record's withdrawal, which can wait on peers), so the swarm never names the old address meanwhile. This covers an old endpoint published again before a changed one could be, and a first endpoint that cannot be published. The refresh's former body is `publish_browser_endpoint`, unchanged but for that clearing. Four tests block the endpoint file: a moved mapping (the old endpoint stays published, the swarm hosts no rooms, then names the moved mapping once the file can be written), the old mapping coming back before the moved one is published (the swarm names it again), a first publication, and, on Unix, a FIFO that holds the moved mapping's write while the test checks the swarm hosts no rooms. The first two fail if the swarm takes the published endpoint even after a failed refresh, they and main's withdrawal test fail if it is never updated, and the FIFO test fails without the clearing before the write. ADR-0026 says when a node hosts rooms.
…stener `SwarmService::ice_servers` is read only by the browser listener's tests in src/web_rtc.rs, a module that exists only with the `webrtc-direct` feature. Without that feature the accessor went unused, so the CI job that runs `cargo test --lib --no-default-features` under `-D warnings` failed to compile the tests. Gate it on `webrtc-direct` as well as `test`. Without default features, the library's tests build with `-D warnings` and 1190 pass; with default features 1251 pass. The previous gate fails the same build with "method `ice_servers` is never used".
The ant-protocol, saorsa-core and saorsa-transport feat/peer-swarm branches were rebased onto their mains (ant-protocol ffbed67b, saorsa-core e04065db, saorsa-transport 5e6ea135). Cargo.lock follows, starting from main's own lock, so it differs from main's only where those branches need it: saorsa-transport's main dropped its CLI, and with it its clap and reqwest dependencies. winapi-util keeps the windows-sys 0.61.2 main's lock has.
A request frame that arrives once the reads' budget is spent is admitted on the swarm's own budget, since only the opened frame says whether it is a swarm request. The charge was given back only when the opened frame turned out to be some other request. Every earlier exit kept it: a frame that failed to arrive (bytes after its declared end, the channel closing partway), that did not open under the PQ session, whose header did not parse, or whose byte reservation was refused, and a read the connection abandoned partway through a frame. A connected browser could so spend its source's and the listener's swarm tokens with malformed frames, and hold them from the swarm requests of other connections until they refilled. The charge is now provisional: a guard taken at admission gives it back when dropped, on every exit and on cancellation, and only a frame known for a swarm request keeps it. The request reader takes its messages through a small trait, implemented for the DataChannel, so tests can feed it frames. Tests exhaust the reads' budget, then send twice the source's swarm budget of each kind of failing frame, with a real PQ session for those that open, and check that both swarm buckets are full after each and that a swarm request is still admitted on the swarm's budget and keeps its charge there. They fail with the refund removed.
A browser download asked for a file's records in an order picked at random for that download. Two downloads of one file in the same client then asked in different orders, so they no longer overlapped record by record and shared fewer fetches, and main's test that a joined fetch charges both downloads its GETs could fail. The client now picks one order per client (ant-client WithAutonomi#244): readers in different clients still ask in different orders, which is what the order is for, while one client's downloads of a file keep sharing each record's fetch. The ADR says so.
008a76d to
d39c393
Compare
Patch ant-protocol, saorsa-core, saorsa-transport and ant-node to their feat/peer-swarm branches, which this change needs: the swarm messages (WithAutonomi/ant-protocol#50), the send on one named channel (WithAutonomi/saorsa-core#183), the plain STUN answers (WithAutonomi/saorsa-transport#231) and the rendezvous on every node (WithAutonomi/ant-node#262). Once those merge, these point at their main branches, or at the released versions at the release train. Cargo.lock starts from main's lock and differs from it only where those branches need it: the swarm's WebRTC and STUN crates, saorsa-transport's main dropping its CLI (and its clap and reqwest dependencies), and ant-node's main adding winapi-util on Windows. iana-time-zone keeps the windows-core 0.62.2 main's lock has.
Every node with a WebRTC Direct endpoint hosts swarm rooms (one without refuses joins as not hosting, so clients go on to the next): it introduces clients reading the same file to each other, carries their WebRTC offers and answers, answers their STUN on its WebRTC Direct socket, and stays the last seed for records. No new binary or port. Design, bounds and rollout: ADR-0026 (new, Proposed). The branch is rebased onto main as linear commits, so the swarm's wire messages follow ADR-0020's get-or-closer request and the check that routes swarm requests by a message's header moves with them. The commits after the feature point the three dependencies at their matching branches, let the notices generator match git dependencies on a branch whose name has a slash, meet Rust 1.99's clippy, make three swarm tests prove what they claim (the pending-offer cap, signal expiry, and that only an offer's recipient can answer it), state the client's send limit and waiting room joins as the client applies them, hold native clients' swarm requests to a budget of their own (64 a second per node, 8 per host, as browsers' already were), host rooms only under the endpoint each refresh of the browser listener published (none from the start of a UPnP-mapped endpoint's withdrawal, ADR-0027, or of a changed endpoint's publication, and none after a refresh that failed), compile a test-only accessor only with the browser listener, lock the dependencies at their branches' heads, give a request frame's swarm charge back on every early exit (a frame taken on the swarm's budget that fails to arrive, open or parse, whose buffers are refused, or whose read is abandoned, spends none of it), and say in ADR-0026 that a client's downloads of one file share their record order.
One change across six repositories. Merge order:
This PR depends on: protocol, transport and core, as git branch dependencies on their
feat/peer-swarmbranches. Once those merge, the dependency commit moves to theirmainbranches (or the released versions at the release train).Linear issue
Closes V2-1892
Risk tier
Proposed; a human confirms at review.
Compatibility
swarm_rendezvousto browsers and answers plain STUN for hosts that announced in the last 60 s; clients without the swarm see no change.swarmmodule the node and its devnet use.Semver impact
Test evidence
d39c393f, rebased onto main34b17c65(the WebRTC listener summary, pressure-gated retention, the devnet's SIGTERM and run-directory changes, then test nodes on OS-assigned ports with 10 s for DHT requests, a Security Audit that fails only on advisories a pull request introduces, notices generated only when their inputs change, and the release signing key), on this PR's git dependencies at their branches' heads; the last rebase moved this PR's native swarm flood test onto an OS-assigned port, as main's tests now are. As main's CI runs them: lib and bins withtest-utils1307, 10 and 5 passed;cargo test --lib --no-default-features1241 passed; doc tests 1 passed (2 ignored);webrtc_direct_devnet6 passed and its ignored five-node test 1 passed;cargo check --lib --no-default-features --lockedandRUSTDOCFLAGS="-D warnings" cargo doc --all-features --no-depsclean;cargo fmt --checkclean.cargo clippy --all-targets --all-features -- -D warnings(Rust 1.99, macOS) reports only anunnecessary_wrapsin main'sdevnet/run_dir.rs, whose function can fail only on Linux, so CI's Linux lint passes; the library alone (cargo clippy -- -D warnings) also reports main'sunused_selfinpayment/verifier.rs, and without default features main'sunused_selflints inreplication/audit_metrics.rstoo; this PR touches none of these files.src/swarm.rs,src/web_rtc.rs): a request frame admitted on the swarm's budget once the reads' is spent giving that charge back on every early exit (twice the source's swarm budget each of frames with bytes after their end, a channel closing mid-frame, frames that do not open under a real PQ session, headers that do not parse, a chunk read, a read abandoned mid-frame, and frames over the source's byte budget; both swarm buckets full after each, and a swarm request then still admitted on the swarm's budget and keeping its charge; the tests fail with the refund removed), requests naming another node, introductions, answers nobody asked for, long-poll wake-up and shutdown, caps and leaving, expiry, the STUN allowlist, the unanswered-offer cap, browsers never waiting, replayed or stale native requests, histories and members capped per host, ended memberships refused however late, a relayed SDP keeping no public address but its sender's (its IPv4 address, or any in its IPv6 /64; the test relays IPv4), a stored SDP charged the memory it holds, the browser request budgets (swarm requests on a budget of their own, refunded to the reads, taken on the swarm's when the reads' is spent, all-or-nothing across buckets), the native request budget (64 a second per node, 8 per host, an IPv6 /64 sharing one, a flood through the router held to it while another host gets through), only an offer's recipient answering it, the pending-offer cap and signal expiry each on their own, swarm requests told apart by a message's header alone and a get-or-closer request or response never taken for one (the test fails with the swarm's discriminants from before main's get-or-closer pair), and the swarm hosting rooms only under the endpoint a refresh published: none once main's withdrawal removes an endpoint only a gateway mapping reached, none while a changed endpoint's file is being written (a FIFO holds the write, on Unix) or after a refresh that could not write a changed or first endpoint's file, and the old endpoint's again if it comes back before a changed one is published (the moved-mapping and comes-back tests fail if the swarm takes the published endpoint after a failed refresh, they and main's withdrawal test fail if it is never updated, and the FIFO test fails without the clearing before the write).008a76d6, the swarm client ate44ea44and, as the released client, client main26199fe; ant-node and ant-client were rebased again since, onto mains whose new commits change tests, CI and the release signing key, and the PRs' own code is unchanged but for one node test moved to an OS-assigned port):f37429a7, which reads saorsa-transport 0.38.2 from crates.io while these nodes read transport main and its unreleased commits since: these nodes finished 24 of 32 downloads against 15 of 32 with one reader starting every second (two runs each), and 20 of 96 against 38 of 96 with every reader starting within 3 s (six runs each; per pair of runs, 3 fewer of 16 on average, 95% interval 8.6 fewer to 2.6 more). Against node main built on the current core, protocol and transport mains with the third-party versions of these nodes' lockfile, so that only these PRs differ: these nodes finished 45 of 64 against 30 of 64 with every reader starting within 3 s (four runs each; 3.75 more of 16 per pair, interval 1.8 fewer to 9.3 more). The same node binary on the same scenario files finished 20 of 96 in the first two controls and 45 of 64 in the third, so this lab's batches swing by more than these differences, and none of these controls rules out a small loss for released clients on these nodes. Two later batches of these two builds broke off early, when the lab host's disk filled and then its memory ran short; their first runs finished 0 of 16 on these nodes, and the first run of every batch so far did badly whichever build it used (6, 0, 2, 0 and 0 of 16). On the code, a released native client's requests take the same path through these nodes as through node main: these PRs add a look at each chunk message's tag (to send swarm requests elsewhere), a sweep of the swarm's rooms every 15 s and a cleanup when a peer disconnects; decoding now refuses bytes after a message's end, which released clients'postcard::to_stdvecencoding never sends; the transport change is on the browsers' WebRTC Direct socket, not the QUIC one; and saorsa-core only gains a method.039ce7c5, the swarm client at40258e4and, as the released client, client main42b68ff, which reads through ADR-0020's get-or-closer lookup; the later commits change only docs, a test's name and locks):e61446b8, every reader on the released client, three runs), finished 2 of 48 downloads, the nodes sending 22.4 to 25.0 GiB a run (7 of 64 and 8.6 to 16.9 GiB before the merges): the harder collapse comes with the merged mains, ADR-0020's read among them, and the mix with the swarm client finished more (12 of 48 against 2 of 48, p = 0.007) with the nodes sending 22% fewer bytes (18.5 against 23.7 GiB a run on average); its released readers did as well as today's (1 of 24 against 2 of 48). The old nodes logged no panic and no ERROR line, and 31 to 325 decode warnings a run beside these PRs' nodes, the swarm client's room requests.ant file downloadCLI joins the same rooms as the pages (25 s beside four pages, which took 144 MiB from each other). No dev testnet run yet.ant file downloadon its own address with a fifth of a vCPU, one 40 MiB file, a fresh network for every run), the swarm client ata5cb5a2(the client's later commits space a file download's last retry rounds after slow rounds,19715a9, in the rollout runs below, update Cargo.lock: the native devnet's ant-node and a Windows-only dependency resolution, and merge main) against the released one (client main, no swarm), alternating:ae8e997, without the waiting room join on read) had been slower in its first two 8-reader pairs (73 and 78 s against 47 and 55 s, the nodes sending 16 to 25% more) and mixed in the next four.a5cb5a2, or where marked at19715a9, the client's commit that spaces a file download's last two retry rounds by its slowest round so far):19715a9: 16 of 16 swarm-client and 14 of 16 released-client downloads finished (both failures could not fetch the file's DataMap), median 191 s against 205 s.a5cb5a2the swarm client did worse than the released one. Beside these PRs' nodes 4 of 40 swarm-client and 13 of 40 released-client downloads finished (p = 0.03); past the first reader of each client, 0 of 35 against 10 of 35 (p = 0.001), the ten in two runs, at 944 to 1198 s, each time after every swarm reader there had given up (at 618 to 940 s). Beside main's nodes 7 of 64 finished (every reader beside these PRs' nodes, 17 of 80: p = 0.1; their released readers, 13 of 40: p = 0.01), the nodes sending 9.1 to 16.3 GiB against 8.6 to 16.9 GiB a run. Both clients give up on a record after the same four tries. The swarm readers had most records from each other, so their rounds held only the records none of them had, took little time and put all four tries in the worst of the overload: they gave up first (median 729 s against 1014 s), and some released readers finished after every swarm reader there had stopped. Every failed swarm reader lacked a record no other swarm reader held. Five more runs of this mix at19715a9, after the others: the swarm client finished 36 of 40 against 23 of 40 for the released client in the same runs (p = 0.002), past the first reader 33 of 35 against 21 of 35 (p = 0.001); two of its four failures were the first swarm reader, with no reader ahead of it, and the four gave up at 707 to 1015 s. The released readers did no worse beside it (23 of 40 against 13 of 40 besidea5cb5a2, a later batch, p = 0.04), the nodes sending 5.3 to 14.3 GiB a run. Load varied from run to run: one of the five never overloaded and all 16 finished (without it, 28 of 32 against 15 of 32). The readers also met later than with every node upgraded (the first record from another reader at a median of 167 s against 116 s, ata5cb5a2): a node on the old release cannot read a room request and leaves it unanswered, and a joining reader passes such a node over after about 60 s, then waits 10 s before trying the next, twice as long after each further node that never answers (up to 5 minutes). So with19715a9, while half the nodes run the old release and the network is overloaded like this, a reader on the swarm client gave up less often than one on the released client.19715a9: no panic or ERROR line, 0 to 279 such warnings a run.d39c393f: every job passes, Security Audit included: main's audit now fails only on advisories a pull request introduces, and the one vulnerability in this lockfile, RUSTSEC-2026-0387 (crypto-bigint0.5.5, throughelliptic-curve0.13), is in main's too, whose fixes wait on alloy and webrtc-rs releases. The macOS test job failed once before running any test, when GitHub's release download of Foundry returned 504, and passed when rerun.linear-linkandpr-template, which check this description, pass.New dependency
none
ADR
https://github.com/grumbach/ant-node/blob/feat/peer-swarm/docs/adr/ADR-0026-clients-share-records-nodes-introduce-them.md (new in the ant-node PR, status Proposed)
Mitigation / rollback
Revert: clients pass over nodes that host no rooms and read from nodes alone, as today.