Add electrum_libsparkmobile

[levoncrypto]

No description provided.

[codeant-ai]

🤖 CodeAnt AI — Review Status

Status	Commit	Started (UTC)	Finished (UTC)
✅ Reviewed your PR	d70d381	Sep 02, 2026 · 12:22	12:26

[codeant-ai]

---

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

[codeant-ai]

CodeAnt-AI Description

Add a native Spark wallet library for Electrum-Firo

What Changed

• Adds a shared native library that lets Electrum-Firo create and validate Spark addresses, recover coins, build mint and spend transactions, estimate fees, and create Spark Name scripts.
• Provides a C-compatible interface for loading the library from Python, including full-view-key serialization and controlled memory release.
• Rejects malformed inputs, invalid network addresses, unusable group elements, oversized lists, and invalid fixed-size data with safe error results instead of allowing failures to escape.
• Builds reproducibly on Linux, macOS, and Windows using pinned Spark Mobile, OpenSSL, and Boost dependencies with verified downloads and dependency checkouts.

Impact

✅ Spark wallet operations available to Electrum-Firo
✅ Clearer errors for invalid transaction and address data
✅ Reproducible native builds across desktop platforms

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

[codeant-ai]

CodeAnt Nitpicks

2 code suggestions

1. These variable names do not match the consumer, which checks GMP_INCLUDE_DIR and GMP_LIBRARIES; GMP is therefore silently disabled even when installed.

Api mismatch · src/cmake/FindGMP.cmake:1-2

---

2. CTxOut constructors leave nRounds uninitialized, so newly created outputs contain indeterminate round data.

Logic error · src/transaction.h:142

[reubenyap]

Requesting changes: this PR is not ready for a post-H2, V2-only Electrum release.

Release blockers

1. [P0] The ABI creates only legacy V1 spends. The current wrapper exposes neither V2 nor the extension commitment and calls the pre-V2 builder. At H2 (height 1,371,000), V1 remains single-input; multi-input is enabled only by the separate V2 format. For this wallet's post-fork contract, expose no version selector: pin sparkmobile a9d078955312b3467f5bb14cbd64ab2ed8852256, hardcode SpendTransactionVersion::V2 in spend, fee, and Spark Name paths, and require exactly 32 commitment bytes. Plain spends use zero; Spark Name uses its computed commitment and appends the matching extension. The Electrum caller must emit and parse nVersion=3, nType=11, never silently fall back to type 9.

2. [P1] The pinned Spark revision contains reproduced memory-safety defects. ASan confirms:

   • every spend-key derivation writes 32 bytes through a vector after its size is cleared;
   • an authenticated one-byte memo claiming length 31 causes a 31-byte stack overread;
   • authenticated recipient data can declare and allocate a 32 MiB memo before EOF rejection.
     The same pin also suppresses coin-identification failure and returns partially uninitialized metadata. Updating to a9d0789 fixes these root causes, its first-use parameter race, and noncanonical address decoding.

3. [P1/P2] Outer serialized-coin parsing is still allocation-amplifiable, including on a9d0789. AEADEncryptedData generic-deserializes attacker-declared vector sizes before checking fixed sizes. A direct test allocated exactly 5,000,000 bytes before reporting EOF. The wrapper also accepts trailing bytes: a valid 244-byte coin with 1 MiB appended was accepted. Use fixed-size field parsing, a tight total-size bound, and require stream exhaustion in the shared decoder.

4. [P2 privacy] The server controls anonymity-set age. Core accepts a canonical historical reference within a group. Without an independent latest-mature-state check, an Electrum server can supply an older valid prefix containing the selected coin and reduce anonymity without invalidating the spend. Require the latest mature independently verified group reference and a minimum/expected set size.

Other confirmed findings

• Negative amounts/counts return successful fee estimates; UINT64_MAX mint values and negative vout wrapping are accepted. Core limits this to invalid-transaction/DoS behavior, not inflation.
• Returned serializedCoinContext is always empty. This breaks bookkeeping, although it does not invalidate the proof.
• The dylib exports 4,689 globals: 17 intended C functions and 4,672 dependency/C++ symbols. Hardening flags cover only the wrapper files.
• There are no registered tests, and the documented public Electrum binding/type-11 integration is absent.
• OpenSSL 1.1.1w is EOL, and referenced LGPL/Boost license texts are missing.

Minimum path to merge

• Bump the Spark pin and patch the remaining fixed-size AEAD parser.
• Make the ABI unconditionally V2 and update Electrum atomically for type 11 and extension commitments.
• Centralize strict coin/numeric/aggregate validation.
• Validate cover-set freshness.
• Add one post-H2 end-to-end check covering single- and multi-input V2, local V1 rejection, malformed coins, and stale cover sets.
• Restrict exports and resolve OpenSSL/license packaging.

I built the exact head twice, built it again with ASan/UBSan, and independently reproduced the high-severity paths. I did not find a reachable value-inflation or key-extraction flaw in a9d0789's V2 proof construction.

[reubenyap]

[P2 privacy] The server supplies the cover set and historical block reference. A canonical but stale prefix can still yield a valid spend with reduced anonymity. The integration must require the latest mature independently verified group reference and a minimum/expected set size.

[reubenyap]

Second-pass review of c5cc9bb: the important fixes are real. Spend construction and fee estimation are now V2-only, the Spark pin contains the prior memory-safety fixes, a clean macOS build and ASan/UBSan build pass, GMP is gone, and the dylib exports only the intended 16 C symbols.

I am still requesting changes for the dependency-verification bypass, malformed-input allocation amplification, late V2 resource checks, incomplete redistribution notices, and the missing executable caller/CI gate.

Release requirements outside this repository also remain: the companion Electrum change must use nVersion=3/nType=11; testnet and devnet must disable Spark sends because Core keeps V2 activation at INT_MAX; and Electrum must independently require the latest mature cover-set reference because consensus accepts valid historical prefixes.

Correction to my earlier parser comment: do not require blanket stream exhaustion. A valid first-mint payload is a 244-byte coin followed by a 66-byte Schnorr proof. The remaining parser defect is nested length allocation before fixed-size validation, described inline below.

[reubenyap]

[P2] The 64 KiB outer cap does not stop nested CompactSize allocation amplification. A reproduced 108-byte ABI input declared a 5,000,000-byte AEAD ciphertext and directly caused a 5,000,000-byte allocation before rejection. Validate the fixed ciphertext/tag/commitment sizes before resizing in sparkmobile, add the malformed-coin regression, and repin. Do not require stream.empty() here: the first mint output legitimately carries a trailing 66-byte proof.

[reubenyap]

[P1] This referenced binding/test file is absent from public Electrum-Firo, this repository's ctest reports No tests were found, and the PR head has no GitHub status checks. Link the companion type-11 caller PR and make its V2/ABI regression test a required CI gate before merging this native library.

[reubenyap]

Update after reviewing companion firoorg/electrum-firo#88: the earlier statement that the binding and tests are absent is now stale. PR #88 does contain the exact-head ctypes binding and test_libsparkmobile.py, and its manual release workflow builds packages on macOS, Linux, Win32, and Win64. The remaining blocking point is test enforcement: this repository's ctest still has no tests, the Python tests skip when the native library is unavailable, and no required PR status runs them against the built library. Please keep this thread open until the ABI/V2 suite is a required gate.

[reubenyap]

Current-head triage for 03cae944.

Address in this library PR

• P1: dependency pin verification must include Git-ignored files; an ignored OpenSSL header was compiled into the supposedly pinned archive.
• P2: fix the reproduced nested CompactSize allocation amplification in sparkmobile, add the malformed-input regression, and repin. The existing line-64 thread is the canonical report.
• P2: raise the declared CMake minimum to match the commands actually used.
• P3: correct the bundled sparkmobile copyright notice.
• P3: reject negative key indexes and diversifiers. The existing negative-diversifier thread is valid but low severity, not “Major.”

Required in the companion Electrum integration before release

• Add type-11 parsing/construction, the ctypes caller, and required end-to-end/ABI CI.
• Enforce independently verified latest-mature cover-set references and a minimum anonymity set.
• Disable Spark sends on testnet/devnet; do not add a V1 fallback.
• Ensure the packaging script selects a valid target platform; an unset platform currently produces a dylib with an extra @rpath/libsparkmobile.dylib dependency.

Not actionable for this PR

The unresolved Boost-wrapper comments are either disproved by explicit exact-head builds or concern unused MPI, Locale, and iOS infrastructure. The shipped target links only Boost.Thread and Boost.Chrono. The UsedCoin and null-plus-zero comments are also false positives. V2 selection, the zero plain-spend commitment, V2 limits, amount validation, and the broad license-text issue are resolved.

[reubenyap]

Final review triage (03cae944)

The unresolved threads now represent only work that should be acted on:

Fix in this library PR

1. Include Git-ignored files in dependency pin verification.
2. Fix the bounded nested CompactSize allocation amplification upstream, add the regression, and repin.
3. Raise the CMake minimum to 3.13.
4. Correct the pinned sparkmobile copyright to Copyright (c) 2022 Firo.

Complete in the companion Electrum work before release

1. Add type-11 parsing/construction, the ctypes binding, and executable ABI/end-to-end CI.
2. Enforce latest-mature cover-set references and a minimum anonymity set in the wallet layer.
3. Disable Spark sends on testnet/devnet without adding V1 fallback.
4. Ensure packaging always selects a supported target platform.

Resolved as false positive, not applicable, or non-blocking

• The Boost test/serialization/system/wave/fiber/coroutine/filesystem reports are false positives for this build.
• MPI, Locale, and the iOS toolchain are unused by this desktop target.
• UsedCoin is already validly declared.
• Null-plus-zero pointer arithmetic is defined in C++17.
• Negative index/diversifier behavior is optional caller-contract hardening, not a demonstrated defect.
• The older parser thread was superseded by the precise line-64 allocation report.
• V2-only selection, zero plain-spend commitment, V2 limits, amount checks, and bundled license texts are resolved.

The 5 MB parser case is allocation amplification, not memory corruption. Zero in-repo ctest tests is not independently a defect; the release issue is that the claimed caller-side tests and CI are not publicly present.

[reubenyap]

Reviewed exact head 40db977ea76c9c43f33d78a314097eb8e870a6fb against base 53f496cc420fb948680c8605ae2329b08f00602e, together with the intended Electrum consumer in firoorg/electrum-firo#88 at 45bf26cf6ea8e1721bf6c3edad32a55295a1d51f.

This library is needed in principle. sparkmobile exposes C++ objects and STL types that Python cannot safely consume with ctypes directly. The C ABI, hardcoded V2 behavior, type-11 caller contract, length guards, exception boundaries, ownership functions, and exact dependency pin are justified. I found no key-extraction, inflation, proof-construction, cleanup, or current network-activation defect. The previous recommendation to disable testnet/devnet is stale: current Firo Core activates H2/V2 on all configured networks.

I am still requesting changes for these release blockers:

• [P1] Candidate coins are capped at the protocol's selected-input limit. This blocks every estimate and spend once the wallet has more than 100 spendable Spark notes, even when selection would use one note. Details are inline.
• [P1] The produced Windows packages cannot load the native library on a clean machine. The exact Win32/Win64 artifact omits required MinGW runtime DLLs. Details are inline.
• [P2] Nested CompactSize allocation amplification remains. The existing src/utils.cpp:64 thread is still current. I reproduced a 5,000,000-byte allocation from a 108-byte malformed coin against this exact head.
• [P2 privacy] Cover-set freshness is not independently established. The existing src/electrum_libsparkmobile.cpp:607 thread remains current. This primarily needs trusted chain-state enforcement in the caller because the native ABI receives no trusted tip or expected latest-mature reference.
• Companion integration blocker: PR #88's incremental anonymity-set fetch asks for only size - prevSize entries but starts fetching at sector zero, so it duplicates the cached prefix and omits the new suffix. See spark_interface.py lines 637-645. This is outside this repository, but the paired release is not correct until it is fixed.

Validation performed:

• Built this exact head with 64-bit MinGW, all 728 build steps completed, and confirmed the DLL exposes exactly the 16 intended C symbols.
• Ran a basic allocation/free and null-input smoke check successfully.
• ctest --test-dir build --output-on-failure reports No tests were found!!!.
• Verified the exact-head Electrum manual release build completed for macOS, Linux, Win32, and Win64. It builds packages but does not execute the ABI test suite, and this PR has no required status checks.
• Downloaded the workflow's windows-bin artifact, verified its SHA-256 against GitHub, inspected both architectures, and reproduced the Win64 DLL load failure under a clean Windows PATH.
• git diff --check reports only imported/vendor whitespace and missing-final-newline issues.

Lower-priority pre-release work remains: add an ABI version handshake for stale-library detection, replace lengthless C strings before freezing the interface, make the estimator reject the same 15-output boundary as the builder, and remove the apparently unused compiled Boost dependency noted inline.

Verdict: the component is needed and its current direction is mostly sound, but this head is not merge-ready or release-ready.

[reubenyap]

[P1] Do not apply the selected-input limit to the candidate wallet. This rejects 101 or more spendable coins before SelectSparkCoins runs, and both the spend and fee paths call it. The companion Electrum code passes the full unspent set, so a wallet with 101 notes cannot estimate or construct even a payment that would select one note. Upstream already enforces MAX_CHAUM_V2_INPUTS after selection. Remove this cap from candidate coins and candidate-group metadata, retain aggregate resource bounds, and add a regression with 101 candidates where one is selected.

[reubenyap]

[P1] Make the Windows DLL self-contained, or package every required runtime. In the exact PR #88 artifact, Win64 imports libgcc_s_seh-1.dll, libssp-0.dll, and libstdc++-6.dll, while its installer and portable package include none. Win32 includes only libgcc_s_sjlj-1.dll and still omits libssp-0.dll and libstdc++-6.dll. I verified the artifact digest and reproduced NativeLibrary.Load failing under a clean Windows PATH. Either statically link the MinGW GCC, C++, and SSP runtimes here or bundle the correct architecture-specific DLLs in Electrum, then add a clean-environment load smoke test.

[reubenyap]

[P3] The compiled Boost dependency appears unnecessary. In the pinned sparkmobile production sources, the active use is header-only boost::optional; the Boost.Thread header in bitcoin/sync.h has no active include. This PR nevertheless vendors 46 Boost CMake files and links Boost::thread, accounting for roughly 74% of its added lines. Prefer replacing the optional with std::optional upstream and dropping this wrapper, or at least use Boost headers without building Thread. This is a scope and supply-chain recommendation, not a release blocker.