Conversation
(PFS) Drop redeemed_at/redeemed_ts and the "unredeemed" status
The backend renamed the out-of-band grant provider from "rangeproof" to
"stf", reflecting that such issuances come from the Session Technology
Foundation, not the inactive Rangeproof dev house. Follow the wire/slug
value ("stf") and the C/C++ constant names to match.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(PFS) Rename payment provider rangeproof -> stf
Forward-port of #109 (dev commits c3f6951..b08630b) onto pfs. session_id_matches_blinded_id() read blinded_id[1] before checking the value's length, and never checked that it was hex. It also used the predicate `blinded_id[1] != '5' && (blinded_id[0] != '1' || blinded_id[0] != '2')`, whose right-hand side is always true (a char cannot differ from both '1' and '2' only when it equals one of them, so the || is tautological), leaving the check as just `blinded_id[1] != '5'` -- so any X5-prefixed value, including a plain 05 session id, was accepted as a blinded id. Validate length and hex before indexing, and replace the prefix tests with starts_with("15")/starts_with("25"). Applies to pfs unmodified: line-for-line identical to the dev change.
Records dev up to 4a47113 as merged, keeping pfs's tree unchanged (-s ours). Everything in the range is already applied to pfs: - #104 (refund-requested-config) via #106 refund-requested-config-pfs - #107 (drop-redeemed-at) via #108 drop-redeemed-at-pfs - #110 (rename-rp-to-stf) via #111 rename-rp-to-stf-pfs - #109 (fix-blinded-id-validation) forward-ported in the preceding commit The first three were rewritten for pfs and so have different patch-ids, which is why `git cherry` still reports their commits as absent; the content was verified present by comparing the identifiers each PR added or removed across both branches.
The backend is dropping this field and libsession follows. It carried no actionable information: a single count of "some number of backend errors happened at some unspecified past time", with no what, no when, and nothing a client could do in response. Parsing it only added a way for the response to fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the manual skip_until + consume_string_view + explicit size checks with require<>/require_span<>/maybe<>, which fold presence and size/type validation into the accessor. Build the verification buffer with reserve+insert instead of resize+memcpy (identical bytes; also drops the spurious -Wstringop-overflow warning on the memcpy).
(PFS) Drop error_report from get_pro_status
Records dev up to 947d72a as merged, keeping pfs's tree unchanged (-s ours). The only PR in the range, #112 (drop-error-report), is already applied to pfs via #113 drop-error-report-pfs; nothing needed forward-porting. #113 is a faithful adaptation rather than a straight pick, so its commits have different patch-ids: it carries the same six files, differing only in pfs's unsigned char -> std::byte types, the resulting drop of an ed_pk.first<32>() now that require_span is fixed-extent, and each branch removing its own pre-existing form of the two size-error messages. Verified by outcome: error_report is absent from both branches, both parse decrypt_group_message through the bt_dict_consumer require/maybe helpers, and seed_payment.py is identical on both.
The proof/status/payment/revocation parsers threaded a std::vector<std::string> of errors through every helper and made the caller check it afterwards -- C-style error handling in C++. Replace it with a `parse_error` exception (new, public in pro_backend.hpp): the JSON helpers and each parser throw on a malformed reply, while a well-formed backend *failure* (envelope status fail/error) is still returned normally with status/error_code set. The C entry points catch once and translate to the invalid_response header, now carrying the real diagnostic rather than a fixed "out-of-memory" string. Along the way: - json_require<double> accepts any JSON number (is_number(), since an integer is a valid float value) rather than is_number_float(); the now-redundant json_require_number helper is removed. - json_require<integral> uses is_number_integer() so a fractional wire value is rejected rather than silently truncated by get_to. - rename json_require_fixed_bytes_from_hex -> json_require_hex. - C-layer allocations use make_unique + release() rather than a raw new with a manual delete in the catch; this also closes a leak in the error-translation path where a throw between the new and the pointer assignment orphaned the object. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pro_renewal_target returned nullopt ("never (re)fetch") whenever there was
no proof credential unless a prepaid purchase marker was set. But `s` (the
credential) and `E` (the access expiry) are independent config keys, so an
account can be genuinely entitled -- `E` still in the future -- while
holding no proof (e.g. `s` was dropped or merge-lost). That state should
fetch a proof, not sit idle forever. Return `now` when the access expiry is
still in the future and there's no proof.
Also rename the local `pro` -> `pro_config`: it is the full ProConfig
credential (rotating key + proof), not a boolean "are we pro", and the old
name misled at least one reader into misdiagnosing this very path.
Relies on the client keeping `E` synced to the backend's reported horizon
and clearing it on not_subscribed (E does not self-age); communicated
separately to the clients.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(PFS) pro_backend: throw on parse errors instead of an error vector
(PFS) user_profile: fetch a proof when entitled but holding none
Forward-port of #120 (dev commit b63b4b0) onto pfs; applies unmodified. GetProRevocationsCResponse / GetPaymentDetailsCResponse are aggregates built by the *_parse functions via std::make_unique from a parsed base response. make_unique initializes with parentheses, which only aggregate-initializes under P0960R3 (C++20): GCC implements it, the Apple Clang on the macOS CI runners does not, so it looks for a constructor and finds none. Give each an explicit base-slice constructor; call sites are unchanged. pfs has the same structs and the same make_unique call sites, so it has the same latent macOS break.
Forward-port of #114 (dev commit 89c6acb) onto pfs. Most of #114 is already here: the byte refactor (7662405) had independently given the binary API fixed-extent spans, so sign/verify/pubkey already reject wrong sizes at compile time, the string overloads already validate, and the blinding / group-keys call sites already pass sized views. Two things were missing. First, pfs's verify(string_view) returned false for a wrong-sized signature or pubkey where #114 throws std::invalid_argument, so it conflated "you passed a malformed argument" with "this signature does not verify". Align with dev: letting pfs keep return-false would silently revert #114's behaviour when pfs eventually lands on dev. Nothing in the tree calls the string overloads -- every call site uses the span overloads -- and the return-false came in incidentally with 7662405 rather than as a considered choice. Second, port #114's regression test for the rejected sizes. The three string overloads all validated a length and then narrowed to a fixed span; that is now one require_bytes<N> helper, which also gives the arguments #114's exception wording.
Records dev up to 2b27d27 as merged, keeping pfs's tree unchanged (-s ours). All four PRs in the range are now applied to pfs: - #116 (this-is-not-c) via #115 this-is-not-c-pfs - #118 (renewal-target-no-proof-fetch) via #119 renewal-target-no-proof-fetch-pfs - #120 (macOS C response holders) forward-ported in e682e6b - #114 (fixed-size XEd25519 spans) forward-ported in b03709d Verified for the two that were already applied: #119's user_profile.cpp change is line-identical to #118's, and for #116/#115 the `errs` error-vector is gone from both branches with identical parse_error usage.
Clients sometimes need to know whether a Pro subscription is terminal or auto-renewing (e.g. "renews on X" vs "expires on X"). Store the backend's `auto_renewing` (from get_pro_status) as a presence-only config flag `A`: 1 when auto-renewing, absent otherwise (terminal / unknown / not Pro). Deliberately not tri-state: unlike blinded_msgreqs `M`, this is backend- derived fact, not a defaulted client preference, so there's no upgrade- default edge case that a distinct "unset" would guard. And no t/T bump -- it's synced pro state like E/I/R, not a user-initiated profile edit. Exposes get_/set_pro_auto_renewing (C++ bool; C 0/1) with unit + C-API coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Forward-port of #125 (dev commit d9406eb) onto pfs, where the same code was present verbatim. encrypt_for_multiple_simple's decoy padding was sized from the plaintext rather than the ciphertext, so a padding entry was encrypt_multiple_message_overhead bytes shorter than every real entry and could be picked out by length alone -- defeating the point of padding the list to a fixed count. The `if (int pad_size = ...)` form also made an empty first message pad to zero, i.e. falsy, which skipped the padding loop entirely: a list of empty messages got no decoys at all. Test both, the sizes and the count.
pfs has broken a fair bit of the API (the std::byte span refactor, the crypto header reorganization, the C API changes), and together with core and the coming Client code this is a substantially different release, so it takes a major bump rather than following dev's 1.x line. This deliberately diverges from dev, which is at 1.8.0 (#128): pfs has been on its own version line since before 1.7.0, and the successive `-s ours` merges have been quietly masking dev's bumps. Setting 2.0.0 makes pfs's version unambiguously ahead, so a future merge into dev carries the version forwards rather than dragging it back.
I kept dev's line wrapping when porting #125, but pfs's encryption::XCHACHA20_ABYTES is shorter than dev's crypto_aead_xchacha20poly1305_ietf_ABYTES, so the CHECK now fits on one line and clang-format joins it. Missed because I did not run utils/format.sh before committing.
Forward-port of #126 (dev commit 4143882) onto pfs; the five files dev touches are line-for-line identical to dev's change. A 421 means the account we asked about is not in the queried node's swarm, so recovery requires re-resolving *that account's* swarm. The old code used failed_node.swarm_pubkey(), which was compute_x25519_pubkey(remote_pubkey) -- the X25519 pubkey of the node we had just been rejected by, not of the account -- so it re-resolved the wrong swarm entirely. Requests now carry the account they address in Request::swarm_pubkey (swarm_pubkey_hex in the C params), the 421 path uses that, and a request without one is failed rather than misdirected again. service_node::swarm_pubkey() goes with it: its only caller was that wrong line, and its name invited exactly this mistake. Beyond dev's diff: dev only populates the field in the C API, but pfs has three internal swarm-addressed request sites in core.cpp (the namespace batch poll, the PFS-key prefetch, and the DM store) which dev does not. Left unset those would now fail on their first 421 instead of recovering, so all three go through a swarm_request() helper that carries the pubkey alongside the node -- one place to get right, and a fourth site cannot silently omit it.
Forward-port of #127 (dev commits 724939a..4b582a5) onto pfs. A refresh can succeed and still yield too few nodes -- most easily when the multi-request intersection finds little in common -- and the result was written over the existing cache unconditionally, leaving nothing to route through until the next refresh landed. An empty result wiped it entirely. Treat an under-sized result as a failed refresh instead: keep the cache we have and retry after the usual backoff. The retry path already existed for an unparseable response, so it is lifted out of the catch block into a discard_and_retry lambda holding a weak self ref and reused for both cases. Also carried along from the same branch: seed_payment.py inlines the day-rounding it needs, since the backend moved base.round_datetime_to_next_day into its own test scaffolding. pfs adaptations: `_hexbytes` -> `_hex_b`; the new test helper is static, as pfs builds with -Werror=missing-declarations; and the byte cast when copying the remote key is dropped, view_remote_key() already returning std::byte here.
Forward-port of dev commit 0a23f70; applies unmodified. Aligns the revocationTag column and corrects expiryUnixTs's comment, which claimed milliseconds where the field carries seconds.
…-renewing flag Forward-port of #124 (dev commits 332b3e6..059e529) plus dev commit 7ef8385 onto pfs. pfs already had the auto-renewing half of this (config key `A`, ported as jagerman/pro-auto-renewing-config-pfs), so what lands here is the rest: - The grace period, config key `G`: how much longer the account is served past `E`, so any linked device can compute when coverage actually ends at `E + G`. Deliberately a plain duration rather than an optional -- the backend sends zero when the subscription is not auto-renewing, so "unset" and "zero" describe the same account. - Clearing `E` now clears `G` and `A` with it. A grace that outlived its expiry would silently pair with the *next* `E` write, and that write is typically a proof outcome, which carries no grace to correct it with. - The backend's grace and renewal flags are read off the proof response and off a `subscription_expired` failure. An absent field means "not applicable" and clears any stale cached value rather than preserving it. - The parse helpers move out of pro_backend.cpp: parse_error grows a hierarchy (parse_error_key/_missing/_type) in the new public session/parse_error.hpp, and the JSON readers move to session::detail in the new src/json_parser.hpp, gaining json_maybe and a wire_seconds concept so call sites stop writing sys_seconds{seconds{get<int64_t>()}}. json_require_hex moves there too, per 7ef8385. pfs adaptations: json_require_hex takes std::span<std::byte>; pro_backend.hpp keeps pfs's clock/ed25519 includes and does not gain dev's `using namespace oxenc::literals` (pfs uses session's `_hex_b` here); and the auto-renewing accessors dev's diff re-adds are dropped as pfs already has them.
pfs-only; not going upstream to dev. Every function in the newly-imported json_parser.hpp carried a `json_` prefix -- C-style namespacing by naming convention, in a codebase that has namespaces. Put them in `session::json` and drop the prefix, so call sites read `json::require<T>(obj, "key")`, `json::maybe<T>(...)`, `json::parse(...)`, `json::require_hex(...)`, and internally `json::is<T>` / `json::extract<T>`. `session::detail` is left to what it already held (ProFlags in config/internal.hpp); these helpers were only put there by dev because that is where the extraction landed, not because they belong beside it. One consequence: a parameter named `json` shadows the new namespace, breaking `json::` lookup inside such a function. That only affects definitions, so the four `parse_*` definitions and `read_envelope` take `json_in`, while the public declarations keep `json` -- there the name is doing real work, saying the value is JSON rather than generic input. The C wrappers keep `const char* json` throughout: they only forward it and never qualify with `json::`.
pfs-only, on top of the session::json rename. json_require_hex was misnamed and under-specified: what these fields hold is a binary value -- a pubkey, a signature, a tag -- and hex is merely one encoding the backend may send it in. Renamed to require_binary and taught to accept base64 as well, padded or unpadded. The encoding is chosen by the encoded length, not by inspecting the alphabet, which cannot work: every hex string is also valid base64. That makes the destination length load-bearing, so the buffer must be fixed-size and at least 5 bytes, both enforced by static_assert via CTAD on the span. Below 5 the encodings collide -- 1 byte is 2 chars either way, 2 and 4 bytes are 4 and 8 chars as hex or as padded base64 -- and from 5 up 2N exceeds both 4*ceil(N/3) and ceil(4N/3), so the lengths never coincide again. Dropped the 0x/0X prefix stripping. These are binary values rather than numbers, so the prefix means nothing here, and worse, `0`, `x` and `X` are all valid base64 characters: the strip would have silently corrupted any base64 value beginning that way. Sizes now come from oxenc::to_hex_size and to_base64_size rather than being recomputed inline. Tested through parse_pro_proof: hex, padded base64 and unpadded base64 all decode to the same bytes, and a length matching no encoding is rejected as a parse_error_key naming the field.
Records dev up to 7ef8385 as merged, keeping pfs's tree unchanged (-s ours). Everything in the range is now applied to pfs: - #125 (multi-encrypt padding size) -> 0763aff - #126 (re-resolve swarm after 421) -> 3699813 - #127 (undersized snode refresh) -> ec91a11 - 0a23f70 (proto formatting/comment) -> f8ffbd3 - #124 (pro grace period + tristate) and 7ef8385 (json_require_hex move) -> 85483fe #128 (dev's bump to 1.8.0) is deliberately NOT taken: pfs went to 2.0.0 in db19773 instead, for the API breaks it already carries. Taking dev's 1.8.0 would move pfs's version backwards. Two follow-ups on top are pfs-only and are not going to dev: 4003a13 moves the JSON helpers to session::json without the json_ prefix, and 3706900 reworks json_require_hex into require_binary, which accepts hex or base64.
The elements of a braced-init-list are evaluated in order, so testing `it->second` in the second element was testing a moved-from Info. It gave the right answer only because `state` is an enum and a member-wise move leaves scalars intact, so it would have started lying the moment that field gained a destructive move.
set_poll_interval assigned _poll_interval, stopped the ticker and built a new one on the caller's thread, while the loop that owns both ran. Creating or stopping a libevent event off the loop races it -- libquic's own call_later goes out of its way to avoid exactly that -- so the doc's "safe to call at any time" was true only for a caller that happened to call before a network was attached. Marshalled through Loop::call, which runs inline when already on the loop, and the ticker rebuild handed to _update_polling rather than written out a second time. Documents what the interval trades away, and that nothing serialises polls: an interval short enough to overlap gets duplicate requests, which is wasteful but not incorrect.
Two comments described a Client that no longer exists. The constructor's said core::callbacks could be passed and would be forwarded "as for a bare Core", which the static_assert below it forbids outright. An application cannot supply any, so a Core event with no client::callbacks equivalent is a gap to fill here rather than something a caller can reach past Client for. The accessors' said reading off-thread "deadlocks rather than merely racing". It does not: the pool hands each thread its own connection and WAL lets a reader run beside the loop's writer. The reasons to go through the loop are cost and consistency -- a second encrypted connection per reading thread, contention for the single write lock, and a snapshot with no defined relationship to the callbacks already delivered -- so say those instead of a hazard that sends a reader hunting for a deadlock that cannot happen.
last_message was a std::string, so a row could not tell "no messages" from "the latest carries only attachments" -- both arrived empty, and a conversation that had just moved to the top of the list appeared to say nothing. Replaced by an optional MessagePreview: unset means there is nothing to preview, so an empty body on one that is set means the message has no text. Body and attachment count are independent fields rather than alternatives, because a message can carry both and a row wants both halves -- which is why there is no kind enum to switch on. Attachments are summarised to a count plus whether they are a voice message and whether they are all images: enough vocabulary for a row, without the filenames and content types that are a message view's business. Costs two queries for a list of any size: the body and sender come from a join on one index seek per conversation, and the attachment aggregate is batched over every previewed message at once, as a page of history already does.
A device's state never goes on the wire -- it is inferred from which message the record arrived in -- so a state change moves no field that the record's seqno versions. upsert_device_info guarded on `excluded.seqno > seqno`, which therefore discarded exactly the transitions it was there to decide: an applicant is stored Pending at seqno 1, the accepting device pushes the identical record as Registered at seqno 1, every device compares 1 > 1 and stays Pending. Registration could not complete. State values are renumbered least to most authoritative so the stored integer is a rank, and the guard compares (state, seqno) as a row value. That subsumes the special cases: equal rank falls back to the seqno, an acceptance outranks a newer link request, and a kick outranks everything. Rank only increases and the order is total, so a merged result is the maximum over everything received regardless of arrival order. Kicked becomes a state of its own rather than a second job for Unregistered, which had been covering both "removed from the group" and "never in it" -- two answers that the merge and the payload writer each had to separate by a different ad-hoc test. A schema CHECK ties it to kicked_timestamp so the coupling is enforced rather than remembered, and the ungated kick update becomes an upsert: an update alone matched nothing for a device that joined after the removal, silently storing no tombstone and leaving that device free to accept the removed one back. Also drops kicked_timestamp from upsert_device_info, where it only ever wrote the NULL it was inserted with; the rank guard is now what keeps a record from overwriting a tombstone. No migration: these tables have no rows anywhere yet.
Picks up session-sqlite's bind_each, which is what a runtime-sized parameter list needs to go through bind_oneshot at all, and covers it with tests here since session-sqlite has no suite of its own. The tests that matter are the composing ones: a sequence between two ordinary values, and two sequences in one call. A single sequence on its own passes whether or not the parameter counter advances correctly, so it cannot tell a working implementation from one that numbers parameters by argument position.
A row saying "invoice.pdf" is worth considerably more than one saying "1 file", and the count comes along for free as the vector's length. One entry per attachment, in the order the sender listed them, so the entries line up with Attachment::index and the length is an exact count. An entry is empty where the sender omitted the name, which it may: the names cannot stand in for the attachments, and a row needs a fallback for the empty ones. The query becomes a row per attachment rather than an aggregate, since the names are wanted individually; the three summary fields are folded from those same rows instead of being asked for separately.
Three build fixes merged into the two dependency repositories, and this is
what brings them in.
session-deps (dbda1a9a..30b20092):
- libevent is built --with-pic, so its archive can be linked into a shared
object. Without it an x86-64 link fails with a R_X86_64_PC32 relocation
against event_base_loop.
- CMake-based dependencies are configured with the cross toolchain file
rather than the compiler binary alone. A cross compiler invoked without
its target builds for the host, and the archive links against the wrong
platform's standard library - on Android, libstdc++ symbols that are not
there.
- session_deps_version 1.7 -> 1.8, which is session-deps' own major-version
compatibility marker; the major is unchanged.
session-router (2e0b7dbb..054d3b9f):
- target_architecture() recognises arm64 on macOS instead of calling
FATAL_ERROR on it, so configuring for Apple silicon works.
All three were carried as out-of-tree patches by a consumer until now.
Nothing here changes what libsession-util builds; the .gitmodules URLs are
unchanged and both commits are on their repository's dev branch.
Bump session-deps and session-router for the cross-build fixes
Every client draws a path screen naming the country of each hop, and each of them currently ships and maintains its own geo database to do it. This puts the lookup upstream: session::ip_country::lookup(ipv4) returns an ISO 3166-1 alpha-2 code, with available(), attribution() and database_version() alongside it. It is off by default. With WITH_IP_GEOLOCATION off the compiled-in database is an empty one rather than absent, so every lookup misses and a client needs no #ifdef of its own; available() is a link-time fact, not a macro, so nothing about the option reaches a header. data.cpp and no_data.cpp define the same accessors and the lookup itself is identical either way -- only the table it searches differs. DB-IP Lite is the source, chosen over MaxMind's GeoLite2 on licensing rather than accuracy: CC BY 4.0 permits redistribution and has no clause requiring a copy to stay current, which is what makes a bundled snapshot viable at all. Its IPv4 rows already tile the address space with no gaps and no mergeable neighbours, so a range needs only its first address -- the next range's start ends it. That leaves parallel arrays of 357k ipv4 starts and uint8_t country indices plus a 246-entry code table: 5 bytes a range, 1.79MB, of which a binary search touches only the 1.43MB of starts. The codes are numbered by descending range count. The countries holding the most ranges get the shortest indices, which is worth ~0.4MB of generated source, and the rare countries -- the ones that come and go between releases -- land at the end where nothing follows them to renumber. Numbering them alphabetically instead rewrote 86% of the table across the Aug->Sep refresh, where two mid-alphabet countries disappeared; this ordering rewrote 9%. The generated table is not committed: utils/update-ip-country-db.py downloads a release and generates it, and cmake refuses to configure with the option on until it has been run. Nothing downloads during a build. The tests run in both configurations and check the mechanism -- table invariants, range boundaries, the unknown path -- rather than pinning countries, save for one anchor commented as expected to move when the snapshot does.
POSIX declares `::wait` in <sys/wait.h>, so a translation unit that does
`using namespace session::client;` and pulls in that header -- which
macOS and glibc both make easy -- finds two `wait`s and can use neither:
error: reference to 'wait' is ambiguous
note: candidate found by name lookup is 'wait'
note: candidate found by name lookup is 'session::client::wait'
There is no fix for that at namespace scope. A using-declaration next to
the using-directive is rejected outright ("target of using declaration
conflicts with declaration already in scope"), and a using-directive in an
inner namespace does not help, since it injects the name into the nearest
namespace enclosing both -- the global one, where `::wait` already is.
What is left is a block-scope using-declaration in every function that
wants it, or qualifying every call site, and neither is something to ask
of a client.
So the tag becomes `block_t`/`block`, and its doc block says why, so that
the next reader does not rename it back. The reason it is not a variable
problem in the first place is that `wait` is an object: a function of that
name would merge into an overload set with `::wait(int*)` and resolve by
arity.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename the blocking-call tag from `wait` to `block`
Add an optional IP-to-country lookup behind a cmake option
`client` branched off `pfs` and was meant to sit on top of it, but the two ran side by side for a month: 192 commits here, 38 there. This closes that, and `pfs` is retired -- future work happens on `client`. It also fixes CI. The Pro backend dropped `"version": 0` from the proof response on 2026-08-17 (session-pro-backend 2ce104a), and PR #130 answered it the same day by deleting the field outright -- a proof's format is fixed by the endpoint that issued it and bound into `sig` by the domain prefix, so the only party that has to discover a version is an offline peer, which reads it from the protobuf envelope. That went to `dev` and to `pfs` (PR #131) and never reached `client`, whose [pro_live] job has been failing on `Key 'version' is missing` ever since. Three resolutions needed a decision rather than a pick: - generate_download_url: `client` gave it a `stream_encrypted` parameter and `sr=` fragments; `pfs` gave it the port suffix. Both are wanted, so the signature is client's and the port handling pfs's. The url tests move to the port-bearing expectations, since `example.com:123` on http states a port now. - proto/debug_print.cpp exists only on `client`, so it merged untouched while the schema under it lost `ProProof.version`. Its print block goes; an old proof still carrying field 1 falls to `unknown_fields`, which that function already prints. - "Configs: what a skipped seqno costs afterwards" pinned the spurious seqno increment as observed behaviour, and ba569b2 fixes it -- as the test's own closing note anticipated. Rewritten to assert what now holds: the duplicate is adopted at its own seqno, so the peer's next change lands cleanly and owes no conflict push. testAll: 392 cases pass. [pro_live] against session-pro-backend dev: 3 cases, 100 assertions, all pass.
Forward-port of dev commit fea5acc; line-for-line identical to dev's, as client's pipeline list still had Static iOS in the position dev moved it from.
First dev->client marker merge, replacing the dev->pfs ones now that pfs is retired. 61f9529 brought all of pfs into client, so dev was already recorded merged through cd2aca9 and only #139 remained. Records dev up to 1d565e3 as merged, keeping client's tree unchanged (-s ours): - #139's url fix (24bd95c) forward-ported in 6aa2608. - fea5acc (CI: run static iOS first) forward-ported in a8805d8. 24bd95c's other half bumps 1.9.0 -> 1.9.1 and is deliberately not taken: dev carries the 1.x line, client the in-development 2.x, so dev's version bumps are never applied here.
`block` already means something else in this library: `set_blocked(bool blocked, block_t)` had one word doing two unrelated jobs in a single declaration. `await` is free of both hazards the name has to clear -- it is not a keyword (`co_await` is), and nothing libc or POSIX declares at global scope shadows it, so it survives a `using namespace session::client;` in application code the way `wait` would not.
Rename the blocking-call tag to `await`
add_logger built a sink around the caller's callback and discarded the only reference to it, so the callback could never be retired: clear_loggers drops every logger in the process, including ones this caller does not own. A consumer whose callback captures something shorter-lived than the process is then left with a registered callback pointing at freed memory. That is what session-app hit - a bridge that can be closed and reopened, logging from threads its teardown cannot join - and it had to hold the bridge weakly to work around it. add_logger now returns a handle and remove_logger takes it. The handle is a shared_ptr to a forward-declared formatted_callback_sink: spdlog stays out of the public header, and the handle still names a type, so an unrelated shared_ptr cannot be passed as one. Logging is serialised against removal, so once remove_logger returns the callback is neither running nor reachable. The C API is unchanged; it never exposed removal and adding it needs an opaque handle of its own.
Conversation::messages passes `limit` straight through to SQLite as `LIMIT ?`, where a negative value means no limit at all and zero means the end of the history. So a caller asking for -1 messages gets every message in the conversation loaded into memory, and one asking for 0 gets an empty page that looks like the end of the history rather than an error. The parameter was undocumented, so neither reading was wrong from the caller's side: the header explains `before` and `include_deleted` at length and said nothing about the range of `limit`. _require_page joins the other _require_ guards on the calling thread, where a caller bug surfaces at the call site with a stack still under it rather than as a string in an error leg. The two terminal overloads are the only callers of _messages, so guarding them covers all eight public spellings; the defaulted ones pass 50 and are unaffected.
Refuse a page size that is not a page
Let a logger be taken back off
Without this we could try to use a system-installed 1.2.1 that doesn't have the needed sink removal code.
TESTNET_CONFIG.pubkey_hex held crypto_sign_ed25519_pk_to_curve25519 of the server's Ed25519 key rather than the key itself. to_request() derives the X25519 form from this field, so on testnet the derivation ran against a value that is not a valid Ed25519 point, threw, and failed every file upload before it reached the network. The value was correct when it was written: `pubkey_hex` used to be the X25519 key, and to_request used it directly. The pfs merge (61f9529) changed the field to hold the Ed25519 key and derive X25519 from it at all five call sites, which is what iOS, Android and Desktop all do -- libsession was the odd one out. DEFAULT_CONFIG was updated to match; TESTNET_CONFIG was missed, and so was the comment on the struct member, which still described the old meaning. Both configs now derive pubkey_hex from the QUIC_FS_ED_PUBKEY_* constants they are supposed to match, so the hex string cannot drift from the key again, and the pinned-pubkey test covers testnet alongside mainnet. That test lives in testAll: the only thing previously covering this path was a live test in the one CI stage that runs them, which is why a two-month-old mistake surfaced as every pull request failing at once.
`p=` names the key a request to that file server gets encrypted to, and it arrives from whoever sent the message. It was checked for being 64 hex digits and nothing else, so a value that is not a key got as far as the derivation in `to_request`, where it either threw -- reported as ERROR_UNKNOWN through the routers' catch-alls -- or, for the ~6% of 32-byte values that are points on the main subgroup, produced a well-formed key that simply never decrypts. `ed25519::is_valid_pubkey` wraps crypto_core_ed25519_is_valid_point, which is the question actually being asked: canonical encoding, on the curve, in the prime-order subgroup. `parse_download_url` now rejects the whole url when `p=` fails it, rather than skipping the fragment the way it does an unknown one: a skipped `p=` leaves the url's host in place but falls back to our own file server's key, so the request would go out to a host that cannot read it. `generate_download_url` no longer emits `p=` for an empty pubkey, which named a key the url did not carry and which the parser now rejects. An empty custom pubkey means "use the default", as it already does on iOS. The url-parsing tests used a placeholder pubkey that is not a point on the curve; they now use a real one, since that is what the parser requires.
pubkey_from_srouter_address already rejected the obviously wrong shapes -- no dot, a leading dot, a label outside the base32z alphabet, a label that does not decode to 32 bytes -- but 32 bytes are not a key, and the result goes straight to a file client as the remote's identity. Anything that gets past the length check is now checked for being a point as well. All three callers already treat nullopt as "cannot address this", so this only widens what they reject. ed25519::is_valid_pubkey gets direct coverage, which is where the question of what makes a key usable belongs: real keys, a value off the curve, the identity and other small-order points, and an X25519 pubkey alongside the Ed25519 key it was derived from. The download-url test sheds the cases that duplicated and keeps the ones it owns -- that the check is reached, and the malformed `p=` shapes the url parser turns away before decoding a key at all.
Fix testnet file server pubkey
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This draft PR tracks the full client support being developed in libsession, to serve as the logic core of future versions of Session. This builds on top of PFS+PQ support (PR #103), and adds a ton of new features and capabilities needed to build a full Session client. The most notable starting point here is the
Clientclass which is the entry point for an active programmable Session client.This branch is not intended for review, but rather merely tracks the progress of the ongoing
clientbranch (which will eventually become thedevbranch).