Update contracts to current staging + fix #729 - #754
Conversation
* Add prototype contract implementation Lacks: - Payment proofs (add early payment proofs) - Tests - Better structure * Separate contract utilities * Simplify the main setup/sign code flow * Remove commented initial_sec_key assignment * Simplify sign::compute function * Add logic for "removal" of secret keys from the context * Further simplify a bit sign and setup * No need for mutable context when adding outputs * Refactor the commented code (lol) * Refactor a bit * tmp --------- Co-authored-by: oryhp <gtrphyro@gmail.com>
* beginning to add and modify slate version 5 * completion of conversions from V5 to V4 Slate * timestamp and memo fields * upgrade/downgrade serialization of v5 slates * add v5 binary slate versions + start of tests * add bin slate ser/deser to tests * ensure serialization of timestamp always excludes milliseconds * start to update v5 tests, update v5 documentation * add fn to generate populated internal slate for conversion testing * add basic tests to convert all slate versions
* add types and beginnings of signature utils * add proof serialization * serialisation of proof data + signature operation * add serialization type for invoice proof + separate bin wrapper version * add witness data + serializion to invoice payment proof, insert verfication functions in place in order to begin verification testing * tests and infrastructure in place for validation * verification of promise sig * added verification of promise signature, infrastructure up to the point where a signature must be subtracted * attempting to figure out differences between recipient nonce that's getting stored and calculated recipient nonce * implementation of witness verification function, retrieve relevant values and re-validate derived recipient partial signature * move stored portion of invoice proof into core types for storage, need to rename invoice proof * define/refine the stored portion of payment proofs type 2? * Folding all proof data into tx log entry storage * back to importing master * remove cargo files from diffs * remove a lot of extra debug output * return proof witness as part of proof retrieval, define json serialization of invoice proof + witness fields * finish adding verification steps to foreign API * remove redundant promise sig field * move lcation of sign/verify calls * Replace Azure Pipelines with Github Actions (mimblewimble#688) * Update CI Badge on README.MD (mimblewimble#690) * Trigger CI on push and pull request (mimblewimble#693) * Update versioning to 5.2.0-beta.1 against grin 5.2.0-beta.3 (mimblewimble#691) * update versioning to 5.2.0-beta.1 against grin 5.2.0-beta.3 * tweak for CI trigger --------- Co-authored-by: Quentin Le Sceller <q.lesceller@gmail.com> --------- Co-authored-by: Quentin Le Sceller <q.lesceller@gmail.com>
* clean up warnings in libwallet crate * clean up warnings in controller crate * update all contract tests with awareness of new proof structure
* integrating onion library * updates and changes to support newly included mwmixnet types * add (incorrect) owner api function * turn off test for now * switch working grin branch to master * fix doctests for build * update cargo lock in attempt to fix croaring build on CI server * update cargo lock with upstream thiserror crate * update test dependency for croaring
* Add self spend transaction state * subtle errors with output states and tx lookups - fixes
* add V5 deserialization test + fixes * clarify comment * upwrap fix during v4 deserialization * further unwrap removal
* add tests + legacy self send cancel * add missing file
* update/rename mwixnet onion classes * fix serialize trait errors
…imble#720) * update srs test * updating comsig creation, attempt to get working * implementation of comsig given an output commit * lock output on mwixnet creation, add further testing
Bring the contracts feature branch up to current master, which has since landed rust 1.80 support (mimblewimble#722), the canonical mwixnet onion code (mimblewimble#726), and node-client proxy support (mimblewimble#738). Conflict resolution: - libwallet/src/mwixnet/*: take master's (mimblewimble#726), dropping the contracts branch's older divergent copy. owner.rs::create_mwixnet_req updated to master's API (use_test_rng) and master's flat re-export imports. - Cargo manifests/lock/.cargo: master's deps & version; re-add serde_with for Slate v5; keep contracts' .cargo/config.toml (Windows static-CRT). - lib.rs: keep serde_derive AND serde_with macro_use; single mwixnet decl. - error.rs: union of master + contracts variants; flat mwixnet re-exports. - api/libwallet owner.rs: master's mimblewimble#738 + create_mwixnet_req plus the contract_* entry points; drop duplicate older create_mwixnet_req method. - slatepack/armor.rs, node_clients/resp_types.rs: take master's. Workspace builds; libwallet lib tests pass (34/34). The Slate version() bug, issue mimblewimble#729 fix, and de-panicking follow in subsequent commits.
OutputSelectionArgs.make_outputs was a comma-separated grin string parsed inside libwallet via amount_from_hr_string. Move that parsing to the CLI boundary and carry explicit nanogrin u64 amounts through the API and Context instead: - OutputSelectionArgs.make_outputs: Option<String> -> Option<Vec<u64>>; output_amounts() becomes a plain accessor, num_custom_outputs() uses len(). - The CLI (wallet_args) parses grin -> nanogrin per amount; ContractNewArgs and ContractSetupArgs carry Vec<u64> and copy it through. - Update the two tests that supplied make_outputs as a string. Addresses PR mimblewimble#754 review (types.rs, wallet_args.rs).
De-panic the contract paths flagged in review plus several found alongside them: - new: use unsigned_abs (abs() panics on i64::MIN). - proofs: surface InvoiceProof serialization errors instead of expect(); bounds- check the participant index before indexing participant_data. - utils/slate: propagate the error when a context input is missing from the db instead of unwrapping. - revoke: return an error when a locked input has no cached commitment. - selection: checked i64/u64 conversions so large amounts can't wrap into a wrong (possibly negative) change. - types: checked sum of output amounts. Addresses PR mimblewimble#754 review (new.rs, proofs.rs, utils.rs, selection.rs).
The serde deserialize helpers copied fixed-length byte arrays (b.copy_from_slice(&bytes[0..N])) without checking the decoded length, so a slatepack with a short pubkey/signature/uuid field panicked the wallet during deserialization. The binary path was hardened in 25dab9f; do the same for the JSON modules: reject any field whose decoded length is not exactly N. Adds a regression test that exercised the panic.
save_step wrote the tx file (store_tx) before the atomic batch commit, so a DB failure could leave the stored tx out of sync with wallet state. Move store_tx to after batch.commit() (and after the post-commit signing-context check), matching the core convention in internal::selection. mimblewimble#755 keeps stored txs as files for all flows, so this reorder closes the desync window without diverging from core. Addresses PR mimblewimble#754 review (utils.rs).
revoke cancels the original tx (unlocking its inputs) before the replacement self-spend is built, and the steps commit separately, so a crash in between left the inputs Unspent with no self-spend while a retry silently did nothing. - Find the contributed inputs by tx id whether they are still Locked (original tx active) or already Unspent (a prior revoke cancelled them but the self-spend did not finish), instead of Locked-only. - Cancel only when the tx is still in a cancellable state, so a resumed or repeated revoke skips the cancel and re-spends the Unspent inputs. - Once the self-spend completes the inputs reference its tx id, so a later revoke is a no-op. Adds a regression test that interrupts a revoke after the cancel and confirms the next revoke resumes and produces the self-spend. Addresses PR mimblewimble#754 review (revoke.rs).
revoke builds the self-spend via new(), which used a random slate id, so a crash between new() and sign() left an orphaned self-spend and a retry created another one. Derive the self-spend slate id deterministically from the revoked slate (blake2b, already a dependency) and let contract::new take an optional slate id; a retry then reuses the in-progress context via get_or_create instead of orphaning a fresh self-spend. The other new() caller passes None (random id). Adds a unit test that the derived id is deterministic and distinct.
contract view derived its result without checking the slate. Reject an Unknown slate state and an out-of-range num_participants (mirroring context::create), so a malformed or tampered slate can't feed a bogus view to a caller/UI. Also fix num_sigs, which counted participants still missing a signature rather than the signatures present. Addresses PR mimblewimble#754 review (view.rs).
When downgrading a payment proof with no sender address (current -> V5/V4) we emit an all-zero placeholder key. Replace the speculative TODOs with the invariant: it can never be accepted as a real sender because verification binds the sender address into the signed message (contract::proofs::verify_promise_signature and the legacy verify_payment_proof via payment_proof_message), so a proof only validates against the address the verifier supplies. Addresses PR mimblewimble#754 review (slate.rs).
The separate contract_setup API was dropped in favor of contract_new/contract_sign. Remove the two commented-out contract_setup stubs left in the owner RPC trait and impl, and drop the README action-list items they (and earlier fixes) resolved: the 'remove setup API/CLI' item, the make_outputs-nanogrins item (now done), and the move-test-utilities item (the per-scenario contract_*.rs tests already cover it). Addresses PR mimblewimble#754 review (owner_rpc.rs, README.md).
|
Round 2: merged the latest staging (including #755's multi-LMDB backend) and addressed the review.
One proactive find worth flagging: the JSON serde path in Open by design: >2-party support, the early-payment-proofs RFC, and the payjoin input-selection refinement. Replied inline on each open thread. |
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
-
cargo fmt --all -- --check currently fails in several new contract files. The build also reports new unused imports and variables, together with deprecated chrono calls. Please format the complete diff and clean all warnings introduced by this PR before merge.
-
The PR cannot currently be merged into the latest staging. Please rebase the PR.
| // Emit the lowest version that can represent this slate, for maximum | ||
| // interoperability. Only the (opt-in) payment proof needs V5 fields | ||
| // (timestamp/memo) that V4 cannot carry; everything else stays V4. | ||
| let (version, version_num) = if slate.payment_proof.is_some() { |
There was a problem hiding this comment.
The new version selection fixes the contract-proof case, but it also emits V5 for classic send payment proofs. These legacy proofs do not use timestamp or memo and are fully representable as V4. This unnecessarily breaks manual slatepack exchange with V4-only wallets. Please select V5 only when the proof really needs the new V5 fields, and add a V4 interoperability test for the legacy send flow.
| let already_ours = slate | ||
| .participant_data | ||
| .iter() | ||
| .any(|p| p.public_blind_excess == our_pub_key); |
There was a problem hiding this comment.
The new guard still misses one case. A full slate containing our public excess with a different nonce is already considered ours. add_participant_info then appends another participant, and the signing path can return successfully without adding our signature. Please match the exact excess/nonce pair and reject every inconsistent participant list.
| where | ||
| K: Keychain, | ||
| { | ||
| for i in 0..self.num_participants() as usize { |
There was a problem hiding this comment.
This loops up to num_participants but indexes participant_data directly. A counterparty can provide fewer entries and trigger a panic during contract signing, including the later sender_index ^ 1 lookup in update_tx_log_entry. The || also accepts a match on only the excess or only the nonce. Please validate that the participant count is exact before signing, iterate safely over the actual entries, and require both keys to match.
| SlateState::Invoice2 => SlateState::Invoice3, | ||
| SlateState::Standard1 => SlateState::Standard2, | ||
| SlateState::Standard2 => SlateState::Standard3, | ||
| _ => { |
There was a problem hiding this comment.
This fallback also turns Unknown or an already completed slate into a successful Standard3 transition. The slate has already been signed at this point and the wallet state is persisted afterwards. Please accept only the explicit valid transitions and return an error for every other state.
| Some(&parent_key_id), | ||
| )?; | ||
| let outputs = res.iter().map(|m| m.output.clone()).collect(); | ||
| updater::cancel_tx_and_outputs(wallet, keychain_mask, tx, outputs, parent_key_id)?; |
There was a problem hiding this comment.
We can now cancel several tx log entries for one slate, but every entry is committed separately. A failure between two commits leaves a partially cancelled state, and a retry can stop on the entry that is already *Cancelled. Please cancel all related entries in one batch, or make this path fully idempotent.
| { | ||
| // For now, we don't compact slates with sl.compact(). We first make them work without compaction. | ||
| let slate_out = | ||
| prepare_slatepack(api, keychain_mask, &slate, &counterparty_addr, out_file).unwrap(); |
There was a problem hiding this comment.
prepare_slatepack can fail for normal file, serialization or encryption errors, but the result is unwrapped here. Reading the slatepack from stdin has the same problem with expect. Please make print_slatepack return Result and propagate both errors through the normal CLI error path.
| &setup_args, | ||
| context.fee, | ||
| )?; | ||
| assert_eq!(my_fee.fee(), context.fee.unwrap().fee(), "my_fee!=ctx.fee"); |
There was a problem hiding this comment.
A mismatch between the stored and recalculated fee terminates the wallet process here. Even if this is an expected invariant, old or damaged context state and future fee changes can reach this path. Please return a normal error containing both fee values instead.
| let static_secp = static_secp_instance(); | ||
| let static_secp = static_secp.lock(); | ||
| receiver_public_nonce = | ||
| PublicKey::from_slice(&static_secp, &reader.read_fixed_bytes(33)?).unwrap(); |
There was a problem hiding this comment.
This reader is currently used mainly by tests, but it is compiled as a normal Readable path and unwraps several externally shaped binary values. Please map invalid secp and Ed25519 keys to grin_ser::Error so the reader remains fully fallible.
| //! Test contract utils | ||
| #[macro_use] | ||
| extern crate log; | ||
| extern crate grin_wallet_controller as wallet; |
There was a problem hiding this comment.
This file is not included as a test module and mostly contains commented-out experimental code. Please remove it, or move only the helpers that are really used into the existing shared test module.
| Additionally, we could fetch the existing Context before the call to avoid doing db fetch. | ||
| Separating side effects until the 'save_step' part would make these functions much easier to test. | ||
|
|
||
| #### TODOs |
There was a problem hiding this comment.
This list still contains open correctness, security and test requirements, while other entries are already implemented or obsolete. For a complete implementation of the contract flows exposed by this PR, every relevant item should be resolved now. Implement missing behaviour and tests, document intentional limitations clearly, and remove completed entries. The important remaining areas include tx/tx-log persistence, locking and key races, foreign or inconsistent slates, coinbase and account flows, repeated signing, --no-payjoin, zero-value outputs and negative tests. The introduction should also describe the current lifecycle without the removed /setup endpoint and reflect the real status of view and revoke.
Brings the branch current with staging for review. - ed25519-dalek 1.0.0-pre.4 to 2: PublicKey is now VerifyingKey, Keypair is replaced by SigningKey, and from_bytes takes a fixed-size array. Ported the contract proof code and the V5 slate serializers, following the pattern staging already uses in v4_bin. - owner_single_use and foreign_single_use now take the wallet instance and a config path rather than an optional API handle. Updated the contract call sites in command.rs and the contract tests. - update_tx_slate_state moved to api_impl::types; dropped the local copy. - Took staging's fixed-size-array form of the slate signature guards; it is equivalent to 25a5434 and keeps the file in sync with upstream. - The grin node is now a submodule, so a build needs git submodule update --init.
cargo fmt --all -- --check is now clean across the workspace.
- Remove unused imports in api_impl::owner, backend, contract::utils, v5_bin and the contract tests. - Remove unused test bindings, and document the shared slate test fixtures module. - Drop the commented-out assertion block in the mwixnet contract test, along with the height bookkeeping and import that only it used.
NaiveDateTime::from_timestamp_opt and DateTime::from_utc are deprecated in favour of DateTime::from_timestamp. Two call sites in contract::proofs converted a timestamp to a NaiveDateTime and immediately back to the same i64; those now use the value directly, which also removes an unwrap.
VersionedSlate and VersionedBinSlate are untagged and try V5 first, so without a version check a V4 slate parses as V5. Only visible without a payment proof; with one the V4 and V5 proof shapes differ enough for serde to pick the right variant by chance. Reject a mismatched version in the V4 and V5 deserializers, for both the JSON and binary encodings, and set the declared version from the conversion so it always matches the structure emitted. Add V4/V5 round trip tests over both encodings, with and without a proof. The V3 API examples showed V4 slates declaring version 5; they now declare 4.
A legacy send proof uses no V5 field: V4 carries the sender and receiver addresses and the signature, and only the timestamp and memo need V5. Sending those as V5 stopped V4-only wallets from reading the slate. Add SlateVersion::lowest_for and use it from create_slatepack and from the contract owner API methods, which returned V4 unconditionally and so dropped the timestamp and memo that an early proof's promise signature binds. contract_sign keeps the version the counterparty sent when the slate does not need more.
find_index_matching_context accepted an entry matching either our excess or our nonce, while fill_round_2 requires both, so the two could disagree on which entry is ours and signing could return success without adding a signature. Both also indexed participant_data by position up to num_participants, which a counterparty can send short. Match on both keys and iterate the entries the slate carries. Require the participant count to be exact before signing a contract, and check for two participants in the contract tx log before pairing them with 'xor 1'.
add_payment_proof read the clock again after generate_invoice_signature had already signed over its own reading. The promise signature binds the timestamp, so a tick between the two reads produced a proof that failed verification with 'Invalid recipient signature'. Carry over the timestamp that was signed.
- Context::get_net_change unwrapped setup_args, which is None for the standard flows that share Context, so a non-contract context reaching a contract path terminated the wallet. It returns a Result now. - A recomputed fee that no longer matches the stored one was an assert_eq, which aborts; report both values instead. - Selection used abs(), which panics on i64::MIN, and unchecked addition for the amount equation. - build_output_amount_list subtracted the custom output sum from the input sum as u64, before the checked conversion that was meant to guard it. A receiver without a payjoin contributes no inputs while --make-outputs is still allowed, so that underflows. The balance is evaluated in checked i64 now, with a test for the no-payjoin case.
add_keys matched a participant on the public excess alone, so an entry carrying our excess with a nonce that is not ours counted as ours and the full-slate guard was skipped. add_participant_info only replaces an entry when both keys match, so that entry was kept and ours appended, leaving a two party slate with three participants. pub_nonce_sum and pub_blind_sum fold in every entry, so the partial signature is then made over an aggregate that includes the spurious one. Match on the excess and nonce pair, the same test add_participant_info and fill_round_2 already apply, and reject a list carrying our excess with a foreign nonce rather than treating that entry as ours. transition_state turned every other state, including Unknown and an already completed slate, into a successful Standard3. That happens after signing and before the wallet state is persisted, so accept only the four transitions the contract flows use and report anything else.
cancel_tx committed each entry separately, so a failure part way through left some entries cancelled and the rest untouched, and a retry then stopped on the entry that was already cancelled. Collect the entries and their outputs first and cancel them in a single commit, so the partial state cannot arise. Cancelling an already cancelled transaction still reports an error, as db_wallet_tx_rollback expects.
print_slatepack unwrapped prepare_slatepack, which fails on ordinary file, serialization and encryption errors, so those reached the user as a panic. Return a Result and propagate it from the contract commands. Reading the slatepack from stdin used expect for the same reason.
contract view was in the CLI help but the command returned Not implemented, and there was no owner API entry point for the existing libwallet implementation. Add owner::contract_view through to the CLI and the owner RPC, with a display for the result. The slate is taken from --input or prompted for, as receive does. is_executed was hardcoded to false. Derive it from our own tx log entry for the slate, which is confirmed once the contract has executed.
controller/tests/contract/mod.rs was never declared as a test module, so none of it was compiled, and most of it was commented out. Its create_wallets is an older copy of the one in the shared test module, which every contract test already uses.
InvoiceProofBin is a normal Readable, so a malformed encoding must come back as a serialization error. The two secp keys, the ed25519 sender address and the memo were unwrapped or copied without a length check. Map them all to grin_ser errors, and take the proof type with a cast that cannot lose data rather than a fallible conversion.
save_step writes the signed transaction after the wallet state batch has committed, so the write can fail with the tx log entry and the input locks already persisted. store_tx writes a file outside LMDB, as it does for every transaction in the wallet, so the two cannot share a commit without moving stored transactions into the database, which backend.rs already carries a TODO for. Cancelling is the way out: it releases the inputs without reading the stored tx, and nothing has been broadcast because the error returns before the caller posts. Tested by removing the file and cancelling. Also set stored_tx on the entry, as internal::selection does for a standard send. It was left unset, so txs reported no transaction data for every contract even though the transaction was stored.
The introduction still described a /setup endpoint that no longer exists and called view and revoke future work. Describe the lifecycle as it is: setup is a step inside new and sign, not a call, and all four actions have owner API methods and commands. Prune the TODO and test lists to what is still open, and record what the contract flows do not support.
Catches up with mimblewimble#773, mimblewimble#779 and mimblewimble#781. Only the import blocks in command.rs and api/foreign.rs conflicted.
|
Thanks @wiesche89. Both issues resolved: Rebase: merging staging in needed ports for ed25519-dalek 1.0.0-pre.4 to 2, the Slate versioning: the untagged enum affected the binary path as well as JSON, and only Participant matching: Cancel: all of a slate's tx log entries are cancelled in one commit rather than making the Panics: contract view: wired through the owner API, RPC and CLI with an end to end CLI test. store_tx: it writes a file outside LMDB, so it cannot share the commit without moving Two other items: Two open items:
Note: README rewritten: no |
2 participants
Brings the contracts branch current with
stagingand fixes the contract-transaction failure in #729, plus a hardening pass. Original phyro/Yeastplume history preserved;cargo test --workspacegreen (52 binaries / 185 tests).Fixes #729 (
46999ce), payment proofs made opt-in. The receiver'scontract signunconditionally built an invoice promise with no sender address available, failing withNoSenderAddressProvided. Verified end-to-end with a real mainnet contract round-trip (kernel08274ee4463a100334c2c4094cb1d2c429769dafb4f26c7fd615e5bab26ffcb5ff).Also in this PR:
stagingin (grin 5.4.0 dep bump; onlyCargo.lockconflicted).VersionedSlate::version()(was reporting V5 slates as V4).contractCLI args now return errors instead of crashing the wallet.SEC_KEY_FAKEscrubbing. Nonce reuse is already prevented by deleting the signing context after signing, now verifies the delete happened.Slate version default.
create_slatepackcurrently emits v5 for every slate (the contracts branch changed it from v4). v5 is required for contract proofs' memo/timestamp, but defaulting all sends to v5 means v4-only wallets can't read them. Right fix is "emit the lowest sufficient version" (v4 unless the slate needs v5 fields)?