Repository navigation
Upgrade Cashu to CDK 0.18.2 with wallet migration and recovery - #114
Conversation
Upgrade the Cashu dependencies from CDK 0.16 to 0.18.2 and adapt the wallet, FFI, mint test harness, and storage implementation to the new APIs. Persist operation state and migrate legacy wallet records so existing wallets can reopen with the new adapter. Recover incomplete operations before exposing the wallet, map transaction states into payment status, and select the relevant outgoing attempt for each quote. Keep adapter unit tests alongside the implementation; cross-version fixtures and live upgrade coverage follow in a separate commit.
Exercise the upgraded adapter with persisted data produced by CDK 0.16, including a fixture generator built against the old adapter. Add wallet-store contract checks and live funded-wallet upgrade coverage to catch compatibility regressions beyond fresh-wallet tests. Run the funded upgrade check in CI and document the fixture provenance, upgrade procedure, and rollback limitations.
thesimplekid
changed the title
benthecarman
left a comment
benthecarman
left a comment
There was a problem hiding this comment.
Thanks, the upgrade tests are thorough. I checked the store against cdk-sql-common 0.18.2: the version checks on quotes and sagas, all-or-nothing reservation, release only touching RESERVED/PENDING, and the rekey all match upstream. The main concern is the startup recovery failure below.
Nits:
- Why was
update_proofs_cache_deduplicates_added_proofs_by_yremoved? The dedup logic is still there and the test looks like it would still pass. - The generator builds a second dependency tree (cdk 0.16 plus an ldk-node git rev), which will add noticeable time to the Cashu CI job.
|
|
||
| // Recover before exposing the wallet or starting tasks that create operations. | ||
| // This must run on every open, independently of the seed-restore flag. | ||
| let recovery = cashu_wallet.recover_incomplete_sagas().await.map_err(|e| { |
There was a problem hiding this comment.
A failed recovery makes Cashu::init fail, which makes Wallet::new fail. Recovery can fail while the mint is unreachable. If a mint request hits a timeout or 5xx, CDK leaves the issue saga in MintRequested (issue/saga/mod.rs, the non-definitive branch runs no compensation); killing the app mid-receive does the same. On the next start, try_replay_mint falls back to /restore on a network error, and recover_outputs_from_blinded_messages returns Err for a non-definitive HTTP error, so the saga counts as failed. The wallet then won't open until the mint is back, which also blocks access to LDK funds.
The existing unavailability test only covers a melt saga, where the quote-state check skips first. Could failed recoveries be logged with startup continuing, since the proofs stay reserved? Could we also add an issue-saga-offline test?
There was a problem hiding this comment.
Addressed in b1ca7a16. Individual recovery failures now log a warning and allow startup to continue, retaining reservations for retry on the next open. Database initialization errors and an unreadable recovery queue still fail startup.
Added cdk016_funded_wallet_opens_with_issue_saga_offline_then_recovers: it kills a funded 0.16 wallet after the mint processes issuance but before the response arrives, verifies startup succeeds while the mint returns HTTP 503 and recovery reports a failed issue saga, then reopens online and checks that all 64 sats and exactly one completed incoming transaction are recovered across repeated opens. This test and the five existing funded upgrade tests pass locally.
| outgoing_quotes.insert(quote_id.clone(), selected.len()); | ||
| } | ||
| } | ||
| // Reusable incoming quotes can represent multiple actual receipts. |
There was a problem hiding this comment.
Minor: CDK 0.18 now writes a Pending incoming transaction before the mint request and marks it Failed on a definitive error, so non-completed incoming payments now reach list_payments. needs_trusted_rebalance in rebalancer.rs doesn't check payment.status, so it will record TxMetadata for them and can pick one as the rebalance trigger. Today Orange doesn't retry a quote after a failed mint, so I don't think a failed plus completed pair with the same payment ID can happen yet, but it would if quotes ever get retried. Worth having the rebalancer skip non-Completed payments?
There was a problem hiding this comment.
Addressed in b1ca7a16. needs_trusted_rebalance now skips payments whose status is not TxStatus::Completed before reading or writing their metadata or considering them as a trigger. Outbound payments remain excluded.
| &self, mint_url: Option<MintUrl>, unit: Option<CurrencyUnit>, state: Option<Vec<State>>, | ||
| spending_conditions: Option<Vec<SpendingConditions>>, | ||
| ) -> Result<Vec<ProofInfo>, cdk::cdk_database::Error> { | ||
| let _lock = self.proofs_mutation_lock.lock().await; |
There was a problem hiding this comment.
Optional: reads now take proofs_mutation_lock (here and in get_proofs_by_ys), so get_balance waits behind a full proof-set write. With VSS that's a remote round trip. Could reads take the lock only when proofs_cache_needs_reload is set?
There was a problem hiding this comment.
Updated in b1ca7a16. Both read paths now use a shared helper that reads the cache directly when proofs_cache_needs_reload is clear and acquires the mutation mutex only when a reload may be needed. The regression test checks that proof lookups and balance reads complete while the mutation mutex is held and the cache is clean.
The flag remains set during persistence, so reads still wait for an in-flight remote write or an uncertain write to be resolved. This removes unconditional locking without changing that consistency guarantee.
| else | ||
| source_path=orange-sdk/src/dyn_store.rs | ||
| fi | ||
| if [[ -d .jj ]]; then |
There was a problem hiding this comment.
Nit: the jj branch is local tooling and shouldn't be in a CI script.
There was a problem hiding this comment.
Removed in b1ca7a16. The CI script now always uses git show to extract the pinned old adapter. The funded upgrade harness passes with that path.
Log individual saga recovery failures and continue startup while retaining reservations for the next recovery attempt. Keep unreadable recovery queues fatal, and verify offline issue recovery with a funded CDK 0.16 wallet. Exclude unfinished receipts from rebalance selection. Let clean proof-cache reads bypass the mutation mutex and restore the deduplication regression test. Use Git to extract the old adapter in CI and cache the standalone fixture workspace dependencies. Validation: 95 library tests and 6 funded upgrade tests passed; formatting and shell syntax checks passed.
|
For the two nits in the review summary, b1ca7a16 restores The Cashu CI cache now includes the standalone fixture generator workspace so its dependencies can be reused between runs. A cold build still compiles the second dependency tree; this preserves the tests against the actual 0.16 adapter. I have not measured the CI time savings. The dependency bump is separate in 2fda04f8, targeting CDK 0.18.3. Local validation of the pushed tree passed: 95 library tests, all 6 funded upgrade tests, formatting, and shell syntax checks. |
Upgrade the Cashu backend and test mint dependencies from CDK 0.16 to 0.18.2, including migration of existing Orange wallet stores and recovery of interrupted payments.
Adds migration and storage-failure regression tests using records produced by the original CDK 0.16 adapter. Funded upgrade tests exercise prepared, paid, failed, and pending melts, lost responses, mint unavailability, and repeated startup against a local mint with a fake Lightning backend. The Cashu CI job now runs these tests through
bash ci/test-cashu-upgrade.sh; they remain ignored in ordinary test runs because they require the separately built old-version generator.Documents the upgrade procedure and limitations: back up the complete store, use one active wallet instance per store, and restore the pre-upgrade backup if rollback is needed. Downgrading an upgraded store is unsupported.