feat(key-wallet): explicit UTXO override for asset-lock funding#850
Closed
lklimek wants to merge 2 commits into
Closed
feat(key-wallet): explicit UTXO override for asset-lock funding#850lklimek wants to merge 2 commits into
lklimek wants to merge 2 commits into
Conversation
Give callers a way to fund an asset lock from exactly one pre-verified
UTXO, bypassing the automatic coin selector — without disturbing the
default path any existing caller exercises. The root-cause index defect
(out-of-order-rescan retaining spent coins as confirmed) is worked
around here, not fixed.
- `TransactionBuilder::set_funding_with_inputs`: sibling of `set_funding`
that sources candidates from the caller instead of `funds_acc.utxos`,
keeping change-address derivation and the reservation capture/skip
guard verbatim. The candidate pool is isolated structurally — a
poisoned or absent index entry can neither leak in nor gate selection.
- `build_asset_lock_with_signer` gains `override_utxo: Option<Utxo>`.
`None` reproduces today's behavior byte-for-byte. `Some` makes the
override the sole candidate; a non-final override is rejected eagerly
with the new typed `AssetLockError::OverrideInputNotFinal` rather than
silently filtered into a generic insufficient-funds error.
- `impl std::error::Error for AssetLockError` with `source()` delegating
to the wrapped `BuilderError`, so downstream `#[from]` chains reach the
structured `InsufficientFunds { available, required }` fields.
Tests: 10 override cases plus a builder-level pool-isolation unit test.
The soft `build_asset_lock` path is deliberately untouched (no consumer).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #850 +/- ##
==========================================
+ Coverage 73.35% 73.48% +0.13%
==========================================
Files 324 324
Lines 72933 73351 +418
==========================================
+ Hits 53502 53905 +403
- Misses 19431 19446 +15
|
…d + tests Address QA LOW-severity polish on the explicit-UTXO-override path: - SEC-001: reject an override whose `address` and `txout.script_pubkey` disagree with the new typed `AssetLockError::OverrideInputScriptMismatch`, eagerly in `build_asset_lock_with_signer` before any signing. Signing derives the key from `address` but signs over `script_pubkey`; the guard stops an account-derivable address from ever producing a signature over an unrelated script — defense-in-depth against a signing oracle if this path is ever exposed beyond the trusted native-Rust caller. - QA-001: add a committed regression test that a foreign-account override (address not derivable by the funding account) fails closed at signing with `BuilderError::SigningFailed` and strands no reservation — locks in the previously-incidental safety property. - RUST-003: trim `set_funding_with_inputs`'s rustdoc to the relaxed public-API length cap without losing the isolation or locking contract. Deferred by design (not addressed): SEC-003 (distinct error for locked/immature-coinbase override — already fails closed) and RUST-001 (share the reservation/change tail between `set_funding` variants). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
Why this PR exists
ManagedWalletInfo::build_asset_lock_with_signerhardcodesTransactionBuilder::set_funding(...)— automatic coin selection over all of an account's locally-indexed UTXOs. That local index can retain entries flaggedis_confirmed = truethat are, in reality, already spent on-chain. This is an out-of-order rescan/event-processing defect (block-processing order isn't guaranteed height-order within a batch — see#649), independent of the separately-fixed#836(unconfirmed-funds guard, which does not cover this case).key-walletselects a UTXO that Insight confirms was spent ~54 days / ~31,000 confirmations earlier. The resulting asset-lock transaction is a double-spend from the network's perspective — no honest peer relays it, so it never gets an IS-lock or ChainLock, and the caller'swait_for_proofburns its full timeout waiting on a proof that can never arrive. Reproduced against real chain state (not a test fixture artifact) three times, with three different stale UTXOs, including after a from-genesis rescan and with a fresh real deposit in the wallet.managed_core_funds_account.rs::update_utxos. This PR does not attempt that — it gives callers who have independently verified a UTXO against real chain state (e.g. via their own oracle) a way to force the builder to use it, bypassing the (sometimes-wrong) local auto-selector for asset-lock funding specifically. A companiondashpay/platformPR consumes this once merged.What was done
build_asset_lock_with_signergains anoverride_utxo: Option<Utxo>parameter.Nonereproduces today's behavior byte-for-byte (fully backward compatible).TransactionBuilder::set_funding_with_inputs(funds_acc, acc, inputs)— a sibling ofset_fundingthat preserves its two side effects (change-address derivation, reservation-set population/double-spend guard) but sources candidate inputs from the caller instead offunds_acc.utxos. A rawadd_inputs()swap was considered and rejected: it does neither of those, which would silently produce no change output and no local double-spend guard on the override input.override_utxoisSomebut notis_confirmed || is_instantlocked, the builder eagerly returns a newAssetLockError::OverrideInputNotFinalinstead of silently filtering it out downstream — the caller gets an explicit signal, not a mysteriously-ignored input.impl std::error::Error for AssetLockErroradded (was missing), enabling the companion platform-side PR to wrap it via#[from]instead of erasing it to a string.build_asset_lockpath is untouched — it has no override consumer.Testing
cargo test -p key-wallet: 546 passed, 0 failed (10 new override cases + 1 builder pool-isolation unit test; TDD — red demonstrated before green).cargo clippy -p key-wallet --all-features --all-targets -- -D warnings: clean.cargo fmt: clean.Breaking changes
build_asset_lock_with_signer's signature gains a parameter. Its only known consumer (dashpay/platform'srs-platform-wallet) is updated in the companion PR to passNoneeverywhere except the new override path.Checklist
cargo clippy --all-features --all-targets -- -D warningscleancargo fmtcleandashpay/platformPR (pin bump) — opens once this is reviewedAttribution
🤖 Co-authored by Claudius the Magnificent AI Agent