Add ETH crypto - #1843
Add ETH crypto#1843brenzi wants to merge 10 commits into
Conversation
secp256k1 (vendored libsecp256k1 v0.8.0, recoverable ECDSA), BIP-39 mnemonics, BIP-32 derivation, Keccak-256, EIP-55 addresses and EIP-712 typed data hashing. Client-side signing only: no RLP, no transaction construction, no chain writes. The resolver path stays read-only. Verified against published vectors — the 24 official BIP-39 English vectors, BIP-32 spec vectors 1 and 2, the EIP-55 spec addresses and the EIP-712 Mail example. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![simplex-chat-agent[bot]](https://avatars.githubusercontent.com/in/3823167?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the whole branch: the secp256k1 FFI bindings, BIP-32/BIP-39, Keccak, EIP-55 addresses, EIP-712 hashing, ERC-5564 stealth addresses, the test suite, and the cabal/submodule wiring.
This is well-structured, and the safety-critical decisions are the right ones and are documented where it matters:
- Trust boundaries are parsed, not validated.
PrivateKey,PublicKey,Address,Mnemonicare all abstract with private constructors reachable only through validating smart constructors, so an invalid key/address is unrepresentable downstream.mkPrivateKeyrejects zero and>= n, which is exactly what makespublicKey/signRecoverabletotal. - Secrets carry redacting
Show(PrivateKey,ExtendedKey,Mnemonic) andPrivateKeyuses constant-timeEq. - Keccak-256 vs SHA3-256 is isolated behind one module with a warning, and the ECDH point is exposed raw specifically because
secp256k1_ecdhwould hash with SHA-256 instead of keccak256 — the ERC-5564 interop trap is avoided deliberately. - Signing is RFC-6979 deterministic and low-s (EIP-2), and
isLowS' half-order constant is correct (n/2 for secp256k1). - The shared context is randomized once at CAF init and only read afterward, so the pure-over-
unsafePerformIOAPI and cross-thread sharing are sound.
I verified the numeric paths by hand against the specs: BIP-39 checksum bit extraction for all five strengths, mnemonicToEntropy bit arithmetic (totalBits*32 div 33), EIP-712 int256/uint256 range checks and two's-complement encoding, EIP-55 nibble/case logic, and the stealth ECDH/tweak algebra (sender P_spend + s_h·G, recipient p_spend + s_h). All consistent. The tests pin published vectors (Trezor BIP-39, BIP-32 vectors 1/2, EIP-55, the EIP-712 Mail example) plus an independent Python cross-check for stealth, rather than the code's own output — the right way to test crypto.
FFI signatures, context flags (CONTEXT_NONE=1, EC_COMPRESSED=258), and the ENABLE_MODULE_RECOVERY build flag all line up with the vendored libsecp256k1.
No blocking issues found. Minor, non-blocking notes:
- Private key material lives in ordinary
ByteStrings that are never zeroized. This matches the rest of the codebase and is hard to change under GHC's GC, so it is a known limitation rather than a regression — worth a note only. - Small duplication (
beToIntegerin bothSecp256k1andBIP39, local hex helpers inAddress) is fine at this size; not worth extracting. secp256k1Ctxcan, under a rare CAF-init race, create two contexts and leak one. Harmless.
Approving.
# Conflicts: # simplexmq.cabal
The BIP-44 path is the same for every address under an account, so the caller that wants one names it, rather than rebuilding the path around the one this hard-coded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rvc3HbiWBTqbAvRT45G5oX
The list was transcoded into Haskell, which can only be audited by reading 2048 words against the source. The upstream english.txt is vendored instead and embedded at compile time, so auditing it is one sha256sum, and a test pins that checksum so the copy cannot drift. file-embed moves into the library, where it was already a dependency of the executables and the test suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rvc3HbiWBTqbAvRT45G5oX
|
|
||
| -- | Parse a path such as @m\/44'\/60'\/0'\/0\/0@. A leading @m@ or @M@ is | ||
| -- optional; both @'@ and @h@ mark a hardened index. | ||
| parsePath :: ByteString -> Either String [Word32] |
There was a problem hiding this comment.
this needs to be written as parser grammar, not as an imperative parser, using Attoparsec
| -- 'Show' is redacting and 'Eq' is constant-time, both deliberately: this key | ||
| -- authorises transfers of assets with monetary value, so it must not reach a | ||
| -- log through a derived 'Show' and must not leak through comparison timing. | ||
| newtype PrivateKey = PrivateKey ByteString |
There was a problem hiding this comment.
this conflicts with existing constructors in Crypto.hs
| newtype PublicKey = PublicKey ByteString | ||
| deriving newtype (Eq) | ||
|
|
||
| instance Show PublicKey where |
There was a problem hiding this comment.
there is no need for custom show instances ever, we don't use them
| -- seed: 'mnemonicPhrase' always rebuilds the canonical lowercase sentence from | ||
| -- the wordlist, and that is what 'mnemonicToSeed' hashes. | ||
| parseMnemonic :: ByteString -> Either String Mnemonic | ||
| parseMnemonic phrase |
| -- EIP-2 compliant; 'isLowS' is provided so callers can assert that rather than | ||
| -- trust it. Note there is deliberately no normalization entry point: we never | ||
| -- accept a foreign signature, we only produce our own. | ||
| module Simplex.Messaging.Crypto.Secp256k1 |
There was a problem hiding this comment.
this seems to re-invent a lot of pre-existing conventions in Crypto.hs.
What could be reasonable is to integrate Secp256k1 as additional algorithm there, or at least it should not use conflicting names
|
|
||
| data RecSigRaw | ||
|
|
||
| foreign import ccall "secp256k1_context_create" |
There was a problem hiding this comment.
so there would be one module doing FFI binding and then interfaces in Crypto.hs would use them
| -- | Parse @0x@-prefixed or bare hex. A mixed-case address is checked against | ||
| -- its EIP-55 checksum; an all-lowercase or all-uppercase one carries no | ||
| -- checksum and is accepted as-is, which is what every Ethereum client does. | ||
| parseAddress :: ByteString -> Either String Address |
There was a problem hiding this comment.
also, why not parsing grammar?
| -- | ||
| -- 'VStruct' takes an already-computed 'hashStruct' result, which is how nested | ||
| -- structs are encoded; 'VArray' hashes the concatenation of its members. | ||
| data Value |
There was a problem hiding this comment.
this is used by Aeson library to mean JSON value.
4 participants
In order to support in-app name purchases, we need ETH crypto primitives available. This PR adds them
Tested against a simple use case in simplex-chat:
https://github.com/simplex-chat/simplex-chat/blob/ab/eth-crypto-test/src/Simplex/Chat/Names/Wallet.hs