TST: add unit tests for rust silent-payment scanning - #133
TST: add unit tests for rust silent-payment scanning#133NAME-ASHWANIYADAV wants to merge 2 commits into
Conversation
|
@chaitika @theanmolsharma @notTanveer PTAL !! |
|
i actually finished this on my local branch about a month ago, but i've been waiting on the indexer-side deployment while working on scan speed improvements. as part of that work, i completely removed json from the scan pipeline, which makes some of the tests here no longer relevant. before opening a pr, it would probably help if you discussed the approach with the team first, either in a github issue or on discord. that way we're all aligned and can avoid duplicated work (saves all of us time and effort). but yeah, keep up the great work!! |
|
@notTanveer! I wasn't aware this was already being worked on locally since there wasn't an open issue tracking it. While the JSON pipeline changes might invalidate some tests, the core crypto math for BIP-352 (ecdh_shared_secret, derive_expected_pubkey, etc.) should still be identical. Do you think we can salvage these core unit tests and integrate them into your new branch? |
Closes #132
What
Adds 52 unit tests for the BIP-352 scanner in
rust_jsi_bridge/src/lib.rs, plus a CI workflow that runs them on every PR.Two commits:
TST: add unit tests for rust silent-payment scanningrust_jsi_bridge/src/lib.rsOPS: run rust unit tests on pull requests.github/workflows/rust-test.ymlCoverage:
compute_shared_secret_hash,ecdh_shared_secret,derive_expected_pubkey,parse_scan_tweak,build_output_map,scan_outputs/scan_transaction,process_transactions_parallel, the serde wire contract withmodules/RustJsiBridge.ts, and the FFI boundary.Why
This is the crypto core of the wallet and had no coverage. A silent regression here means missed payments or an unspendable UTXO.
How - the tests are deliberately not circular
Building a fixture with
derive_expected_pubkeyand then assertingscan_transactionfinds it would pass even if the crypto were entirely wrong. Instead every expected value is computed from the sender's side of BIP-352, via different secp256k1 APIs than the scanner uses:B_scan * a(mul_tweak) against the scanner'sA * b_scan(shared_secret_point)P_k = B_spend + t_k*Gis rebuilt withfrom_secret_key+combineagainstthe scanner's
add_exp_tweaksha256, bypassing thesha256t_hash_newtype!macro - this pins theBIP0352/SharedSecrettagstring rather than restating it
The headline test takes the
tweakHexa match returns, derivesd = b_spend + t_k, and assertsP(d)equals the matched output's key - i.e. it proves the scanner's output actually lets the wallet spend the coin, which is the real contract with the TypeScript side.Testing
I also verified the tests can fail, by mutating the production crypto and confirming the suite goes red:
to_be_bytes()→to_le_bytes()on the output indexBIP0352/SharedSecret→BIP0352/SharedSecretX[1..33]→[0..32]All mutations were reverted; production lines 1–341 are byte-identical to
master, and the diff contains zero deletions.Cargo.tomlandCargo.lockare untouched#[cfg(test)] mod tests, since nearly every function under test is privatenpm run rust:build) is unaffectedA note on the CI workflow
The workflow runs
cargo testonly. I left outcargo fmt --checkandcargo clippybecause both currently fail on existingmaster:cargo fmt --checkreports diffs in several production linescargo clippyerrors onclippy::not_unsafe_ptr_arg_deref(deny-by-default)for the
extern "C"exportsAllowing just that one lint, clippy is clean - so the new tests introduce no warnings. Reformatting and the clippy fix felt like a separate
REF:commit rather than something to bundle into a tests PR, but happy to add either here if you'd prefer.Follow-ups (characterized, not fixed)
Three existing behaviours are pinned with
// KNOWN LIMITATION:comments so the tests document reality. None are fixed here - each is production code and would belong in its own PR:isSpentfails the entire batch.helpers/silent-payments/types.ts:8declaresisSpent: boolean | number("0 = false, 1 = true") and
modules/RustJsiBridge.tspropagates that union, but Rust deserializes into a strictbool. Sinceparse_transactions_from_ffidecodes the whole array at once, a single numericisSpentfrom the indexer fails every transaction in the batch withInvalid JSON: invalid type: integer '0', expected a boolean. The fix is a bool-or-int serde shim. This is the one worth looking at.transactionIdis required but never read -IndexerOutputhas no#[serde(default)]on it, yetcreate_matched_utxoalways usestx.id.pubKeys collapse inbuild_output_map(last write wins), so an earlier UTXO would be unrecoverable. Legal in Bitcoin, though real silent payments assign each output a distinctk, so impact is minimal.