feat: adopt nim-ffi 0.2 CBOR ABI (v0.2.0-rc.1)#83
Draft
Ivansete-status wants to merge 13 commits into
Draft
Conversation
nim-ffi 0.1.5 reworked the context pool to recycle contexts (reuse the worker thread + its fds) instead of tearing them down per cycle, which is what fixes the per-create/destroy fd leak. Two consumer-side changes are required for libsds to work with it: - SdsCreateRmReq: nim-ffi no longer points `myLib` at a thread-stack var; it owns it as a createShared'd object and frees it on recycle. So the handler must (re)allocate `ctx.myLib` before assigning, otherwise the first create dereferences a nil `myLib` and segfaults. - SdsCleanupReliabilityManager: use `releaseFFIContext` (recycle) instead of `destroyFFIContext` (full teardown). Recycle keeps the worker and its fds alive for the next manager; destroy would reintroduce the leak. It is non-blocking and fires `callback` from the FFI thread once drained, so the synchronous RET_OK fire is dropped. Lock nim-ffi to 0.1.5 (f6a3a33). Verified: libsds builds (--mm:refc) and the status-go reliability suite (incl. the multi-manager fd test) is green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Regenerating the lock with newer nimble (0.22.3) rewrote entries to the "#<rev>" special-version form, added a `nim` compiler entry, and listed `nim` as a dependency everywhere. The nimble bundled with the nim 2.2.4 toolchain (used by status-go CI) can't consume that: it fails on the nim checksum, then "key not found: nim", then `git init` on a "#"-versioned bearssl dir. Take the pre-existing lock format (proper tagged versions, no `nim` entry, bearssl_pkey_decoder pinned as 0.1.0) and only bump the `ffi` entry to 0.1.5. Verified building libsds in a fresh clean-room under both nimble 0.18.2 and 0.22.3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
libsds is loaded into the Go-based status-go process, where the Go runtime must own SIGSEGV (it relies on it for nil-deref -> sigpanic recovery, stack growth and goroutine preemption). The Makefile->nimble build refactor dropped the -d:noSignalHandler flag that the previous `make ... NIMFLAGS=-d:noSignalHandler` build passed, so the embedded Nim runtime began installing its own signal handlers and hijacking the signals Go needs. This crashed status-go functional tests (e.g. test_offline_node_backfills_history_on_login) on login. Add the flag to buildLibrary (covers all desktop dynamic/static tasks) and to the iOS and Android builds so every Go-linked libsds keeps Go's signal handling intact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nim-ffi 0.1.6 fails the build if -d:noSignalHandler is missing, so the libsds build can no longer silently drop the flag that lets the Go host keep ownership of OS signal handlers (the regression that crashed status-go functional tests). sds.nimble already passes the flag; this wires the compile-time guard in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Esteban C Borsani <ecastroborsani@gmail.com>
Rebuild the C wrapper on nim-ffi 0.2's high-level macros (declareLibrary +
{.ffiCtor.}/{.ffi.}/{.ffiEvent.}), which marshal parameters and results as
CBOR and expose snake_case sds_* entry points. Replaces the previous
hand-written positional/JSON ABI.
- request/response objects are {.ffi.} types (CBOR); the unwrap response is
a proper nested object (message, channelId, missingDeps) instead of
hand-built JSON, and retrievalHint travels as raw bytes (no base64).
- events become {.ffiEvent.} procs (message_ready, message_sent,
missing_dependencies, periodic_sync, repair_ready), delivered to the host
via sds_add_event_listener.
- the retrieval-hint provider stays hand-written (a C function pointer has
no CBOR form); its pointers travel as uint64 through the request channel,
and the provided buffer is freed with libc free to match the host's
malloc (Go's C.CBytes).
- pin nim-ffi at the fix/foreign-host-concurrency-v0.2 branch (recycle pool
+ foreign-thread GC fixes) and add the cbor_serialization dependency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Nix build pre-populates deps into pkgs2 and can't clone in the sandbox. nim-ffi isn't in the nimble registry, so requiring it by URL made nimble re-clone it during resolution (offline -> fail). Resolve ffi by name from the installed pkgs2 in the Nix build (gated by SDS_NIX_DEPS), keeping the URL fetch for plain `nimble` builds; the URL now points at the published v0.2.0-rc.1 tag. Also drop the `nim` entry from the lock (the sandbox can't fetch Nim by SHA), record numeric lock versions for ffi (0.2.0) and cbor_serialization (0.3.0) so they resolve cleanly, and regenerate nix/deps.nix to include cbor_serialization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`nimble setup -l` (used by status-go's from-source builds: the functional-test Docker image with nim 2.2.4 and the Windows job) walked each locked package's `dependencies` array, looked every name up in the lock's package table, and crashed with "key not found: nim" because the `nim` package entry was removed to keep the offline Nix build working. Strip the implicit `nim` compiler from the dependency arrays instead of re-adding it as a package: re-adding it makes `nimble setup -l` clone and rebuild the Nim compiler from source (slow, pointless — the toolchain is already installed) and its checksum is nimble-version-specific. With no `nim` references, `setup -l` resolves cleanly against the system compiler, and the Nix build (which never choked on the missing key) is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Brings the nim-ffi 0.2 CBOR ABI branch up to date with master so it can be PR'd cleanly: - Takes #79 (move API module into srcDir so `import sds` resolves for nimble consumers) — the only non-conflicting incoming change. - Resolves the sole conflict in sds.nimble in favour of this branch: keep the `nim-ffi#v0.2.0-rc.1` pin / `ffi >= 0.2.0` floor and the cbor-serialization requirement. Master's #80 relaxes the floor to `>= 0.1.3` for core-only consumers, which is intentionally overridden here — adopting the 0.2 CBOR ABI is the whole point of this branch. Validated: libsds builds clean and `nimble test` is green (bloom, reliability, persistence, snapshot_codec) on nim 2.2.4. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
|
@Ivansete-status why is this needed now? 🤔 |
The v0.2.0-rc.1 tag was re-pointed from c3d135f to 7362bfd (adds the internal cwire codec + ABI-format annotations; +1.4k lines). The generated libsds CBOR `sds_*` ABI is unchanged, so this is a drop-in nim-ffi bump — no nim-sds/library or sds-go-bindings change needed. - nimble.lock: ffi vcsRevision -> 7362bfd, checksum -> 544b7be2 (the from-source `setup -l` path). - nix/deps.nix: ffi rev -> 7362bfd, sha256 -> 0lwyzqcg… (the offline Nix build path). Validated: `nimble test` green (106 OK / 0 failed) and `nix build .#libsds` SuccessX, both against nim-ffi 7362bfd. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The cwire-only nim-ffi (7362bfd, the re-pointed v0.2.0-rc.1 tag) crashed with a foreign-thread GC null-avltree fault inside sds_unwrap_received_message: that line branched off before the foreign-host-concurrency fix and was missing the cycle-collector rooting (GC_ref(myLib[])), so the SDS manager ref could be collected mid-use and fault on a later alloc. a66c53a is nim-ffi release/v0.2 = cwire + the full fix cherry-picked (a6b22fc event-listener GC setup + c3d135f context-recycling / GC_ref rooting). Pinned by immutable SHA (lock + deps.nix + sds.nimble) to avoid the mutable-tag drift that caused this. Validated: libsds builds, nimble test green (106/0), and the previously crashing status-go TestAcceptCRRemoveAndRepeat passes 5/5. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Collaborator
Author
|
@igor-sirotin - I use that to validate the upcoming nim-ffi release v0.2.0. |
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
2 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.
Adopts the nim-ffi 0.2 CBOR
sds_*ABI, pinningnim-ffi#v0.2.0-rc.1(7362bfd(the tag was re-pointed fromc3d135f; cwire codec, ABI unchanged)) plusnim-cbor-serialization#v0.3.0. This is the branch status-go PR status-im/status-go#7533 pins (f5d7b28, still an ancestor here) and which its CI builds green (Nixpackage-nix+ from-source Docker, Linux/macOS/Windows).What's here
feat: adapt libsds to nim-ffi 0.2 CBOR ABI— thesds_*snake_case + CBOR ABI.SDS_NIX_DEPSresolvesffi/cbor by name from pre-populatedpkgs2).fix(nimble): drop nim from lock dependency arrays for setup -l— lets the from-source path (nimble setup -l, nimble 0.18.2) resolve without crashing on a missingnimkey or rebuilding the Nim compiler.master(fix: move API module into srcDir so 'import sds' resolves for nimble … #79 srcDir module move). Thesds.nimbleconflict was resolved in favour of thev0.2.0-rc.1pin, intentionally overriding fix: relax nim-ffi floor to >= 0.1.3 (only library uses it) #80's relaxed>= 0.1.3floor.Validation
libsds builds clean and
nimble testis green (bloom, reliability, persistence, snapshot_codec) on nim 2.2.4; full stack validated via status-go#7533 CI.Relationship to #75
Supersedes the approach in #75 (
use-latest-nim-ffi), which targeted nim-ffi master via{.ffi.}macros + event registry rather than the taggedv0.2.0-rc.1CBOR ABI. This branch is the one actually consumed downstream. Maintainers to decide which lands; closing #75 in favour of this (or vice-versa) is left to review.Consumed by: status-im/status-go#7533, logos-messaging/sds-go-bindings#13.
Draft pending the status-go consumer landing.