Skip to content

Mark Keysets as Deleted.#797

Open
KvngMikey wants to merge 23 commits into
cashubtc:mainfrom
KvngMikey:wallet-deleted-keysets
Open

Mark Keysets as Deleted.#797
KvngMikey wants to merge 23 commits into
cashubtc:mainfrom
KvngMikey:wallet-deleted-keysets

Conversation

@KvngMikey

@KvngMikey KvngMikey commented Oct 5, 2025

Copy link
Copy Markdown
Collaborator

Fixes #731

Summary

This PR fixes the wallet behavior when a keyset disappears from a mint's /keysets response.

Changes

  • Updated load_mint_keysets to mark missing keysets as deleted.
  • Added update_keyset_active in crud.py to use named SQL parameters for SQLAlchemy async compatibility.
  • Verified with pytest tests/wallet/test_wallet.py -k keyset (all keyset tests now pass).

Comment thread cashu/wallet/wallet.py Outdated
Comment thread cashu/wallet/wallet.py
Comment thread tests/wallet/test_wallet.py Outdated
Comment thread tests/wallet/test_wallet.py Outdated

@callebtc callebtc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but one outstanding question regarding the removed line await self.load_keysets_from_db()

what's the justification for that?

@KvngMikey

KvngMikey commented Oct 8, 2025

Copy link
Copy Markdown
Collaborator Author

LGTM but one outstanding question regarding the removed line await self.load_keysets_from_db()

what's the justification for that?

@callebtc I replaced "await self.load_keysets_from_db()" because we now build self.keysets inline with the already-fetched list (keysets_in_db) filtered by k.unit == self.unit and k.active.
That list is fresh (we re-fetch after marking inactive), so the old helper’s extra query + loop is redundant.
The trace message is still printed two lines below, so no observability is lost.

@KvngMikey KvngMikey requested a review from callebtc October 8, 2025 14:21
@callebtc

Copy link
Copy Markdown
Collaborator

Hi @KvngMikey, sorry for getting back to you late. We have a crud called update_keyset that supports setting the active flag (latest main also can update the fee, worth a merge of main into this branch). Why not use that directly?

@KvngMikey

Copy link
Copy Markdown
Collaborator Author

Hi @KvngMikey, sorry for getting back to you late. We have a crud called update_keyset that supports setting the active flag (latest main also can update the fee, worth a merge of main into this branch). Why not use that directly?

hi @callebtc , thanks for checking my work, I have addressed your request now.

@callebtc

Copy link
Copy Markdown
Collaborator

we talked about this off band: there should probably be a boolean deleted column in the keyset table of the wallet and if we flip it to True, the keyset shouldn't be loaded from the db, when we use load_keysets(). wanna try that?

@KvngMikey

Copy link
Copy Markdown
Collaborator Author

we talked about this off band: there should probably be a boolean deleted column in the keyset table of the wallet and if we flip it to True, the keyset shouldn't be loaded from the db, when we use load_keysets(). wanna try that?

yes, i remember and i'm working on it.

@KvngMikey KvngMikey force-pushed the wallet-deleted-keysets branch from 9a9688c to dfd79ed Compare December 18, 2025 21:28
@KvngMikey

Copy link
Copy Markdown
Collaborator Author

@callebtc PR updated, ready for re-review, thank you.

@KvngMikey

Copy link
Copy Markdown
Collaborator Author

question:
when this PR goes in, would we need to update NUT01 to show that keysets can now be marked as deleted ?

cc @callebtc @a1denvalu3

@codecov

codecov Bot commented Jan 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.57895% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.34%. Comparing base (8d23ea9) to head (84788c1).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
cashu/core/base.py 58.33% 5 Missing ⚠️
cashu/wallet/wallet.py 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #797      +/-   ##
==========================================
+ Coverage   75.27%   75.34%   +0.07%     
==========================================
  Files         111      111              
  Lines       12431    12451      +20     
==========================================
+ Hits         9357     9381      +24     
+ Misses       3074     3070       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@ye0man ye0man added this to nutshell Jan 21, 2026
@github-project-automation github-project-automation Bot moved this to In progress in nutshell Jan 21, 2026
@callebtc

Copy link
Copy Markdown
Collaborator

question: when this PR goes in, would we need to update NUT01 to show that keysets can now be marked as deleted ?

cc @callebtc @a1denvalu3

nope, this is just for the wallet and not related to the API. the mint just deletes the keyset

@KvngMikey

Copy link
Copy Markdown
Collaborator Author

question: when this PR goes in, would we need to update NUT01 to show that keysets can now be marked as deleted ?
cc @callebtc @a1denvalu3

nope, this is just for the wallet and not related to the API. the mint just deletes the keyset

great, thank you !

@KvngMikey KvngMikey force-pushed the wallet-deleted-keysets branch from d1aa49d to 6624071 Compare February 9, 2026 10:05
@KvngMikey

Copy link
Copy Markdown
Collaborator Author

@callebtc - this has also been rebased and is ready.

cc @a1denvalu3

Comment thread cashu/core/base.py Outdated
@ye0man ye0man added this to the 0.20.0 milestone Mar 10, 2026
Copilot AI review requested due to automatic review settings March 27, 2026 10:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Comment thread cashu/wallet/wallet.py Outdated
Comment thread cashu/wallet/wallet.py Outdated
@KvngMikey KvngMikey force-pushed the wallet-deleted-keysets branch from 4b4ccac to 5364262 Compare March 27, 2026 18:45
Comment thread cashu/wallet/wallet.py Outdated
@a1denvalu3

Copy link
Copy Markdown
Collaborator

title: "Wallet permanently loses unspent funds when a mint stops reporting a keyset"
slug: wallet-loses-funds-on-deleted-keyset
date: 2026-04-14
status: confirmed
severity: high
target: nutshell
nuts: [NUT-02]

Summary

The changes introduced in PR #797 mark keysets as deleted (deleted_at = int(time.time())) if they disappear from the mint's /v1/keysets response. However, get_keysets defaults to excluding deleted keysets, which causes load_keysets_from_db() to remove these keysets from self.keysets. Consequently, load_proofs() completely ignores any unspent proofs belonging to deleted keysets. This causes valid, unspent user funds to silently vanish from the wallet's balance and memory, rendering them permanently unspendable.

Root Cause

In cashu/wallet/wallet.py (load_mint_keysets), keysets missing from the mint's response are marked as deleted:

        for key_id in (local_ids - active_ids):
            ks = keysets_in_db_dict[key_id]
            if ks.deleted_at is None:
                ks.deleted_at = int(time.time())
            await update_keyset(keyset=ks, db=self.db)

Subsequently, load_keysets_from_db() filters out these keysets. In cashu/wallet/wallet.py (load_proofs), the wallet only loads proofs if their keyset ID exists in self.keysets:

            else:
                for keyset_id in self.keysets:
                    proofs = await get_proofs(db=self.db, id=keyset_id, conn=conn)
                    self.proofs.extend(proofs)

Because the deleted keysets are no longer in self.keysets, their unspent proofs are skipped entirely.

Attack Steps

  1. A user holds unspent proofs associated with a specific keyset in their wallet.
  2. The mint rotates its keysets and decides to remove the old keyset from its /v1/keysets payload (e.g., to reduce payload size or deprecate the keyset).
  3. The user connects their wallet to the mint, triggering load_mint_keysets().
  4. The wallet marks the old keyset as deleted in its database since it is no longer reported by the mint.
  5. The wallet calls load_proofs(), which fails to load any unspent proofs belonging to the deleted keyset.
  6. The user's balance for those proofs drops to zero, and the funds are permanently lost from the wallet UI and internal memory.

Impact

Any user holding tokens from a keyset that a mint stops returning will permanently lose access to those funds in their wallet. The proofs are silently hidden and cannot be swapped, sent, or recovered without manually editing the SQLite database. This constitutes a high-severity loss of funds/denial of service.

Test Results

A test script confirming this behavior failed initially (showing the balance dropped from 100 to 0). Applying the proposed fix successfully resolved the test.

Balance before mint drop: 100
Balance after mint drop: 0
VULNERABILITY CONFIRMED: Proofs from deleted keyset are no longer loaded into the wallet, causing permanent loss of funds.

Proposed Fix

Before marking a keyset as deleted in load_mint_keysets, check if there are any unspent proofs associated with it. If there are, do not mark it as deleted.

        for key_id in (local_ids - active_ids):
            ks = keysets_in_db_dict[key_id]
            
            # Check if there are any unspent proofs for this keyset
            unspent = await get_proofs(db=self.db, id=key_id)
            if unspent:
                logger.warning(f"Keyset {key_id} disappeared from mint but has unspent proofs. Not deleting.")
                continue

            logger.debug(f"Keyset {key_id} no longer reported by mint, marking as deleted")
            if ks.deleted_at is None:
                ks.deleted_at = int(time.time())
            await update_keyset(keyset=ks, db=self.db)

@a1denvalu3

Copy link
Copy Markdown
Collaborator

title: "Reappearing keysets are never undeleted, causing permanent loss of funds"
slug: permanent-keyset-deletion
date: 2026-04-13
status: confirmed
severity: high
target: nutshell
nuts: [NUT-02]

Summary

The wallet tracks keysets that are reported by the mint and marks them as deleted (deleted_at = int(time.time())) if they disappear from the mint's active keysets list. However, if a keyset reappears (e.g., after a mint configuration fix, network error, or pagination bug), the wallet fails to clear the deleted_at flag. Consequently, the keyset remains permanently excluded from self.keysets, rendering any proofs associated with it permanently unspendable by active_proofs().

Root Cause

In cashu/wallet/wallet.py (inside load_mint_keysets), when updating existing keysets, the deleted_at column is never reset to None if the keyset is found in the current mint's response.

        for mint_keyset in mint_keysets_dict.values():
            if mint_keyset.id in keysets_in_db_dict:
                changed = False
                if (not mint_keyset.active ... ):
                    ...
                if (mint_keyset.input_fee_ppk ... ):
                    ...
                # MISSING: Check and reset deleted_at

Because get_keysets defaults to exclude_deleted=True, load_keysets_from_db() subsequently fails to load this keyset into self.keysets. Thus, active_proofs() will permanently filter out any proofs matching this keyset ID, causing a loss of funds for the user.

Attack Steps

  1. The user's wallet loads a valid keyset and mints proofs against it.
  2. The mint temporarily fails to include the keyset in its /v1/keys response (e.g., due to a temporary misconfiguration, outage, or reverse proxy bug).
  3. The user's wallet calls load_mint_keysets() and permanently marks the keyset with deleted_at = timestamp.
  4. The mint resolves the issue, and the keyset reappears in /v1/keys.
  5. The user's wallet calls load_mint_keysets() again. It detects the keyset in the DB and checks for active or fee changes but fails to reset deleted_at = None.
  6. The keyset remains deleted in the DB. The user cannot spend any proofs from this keyset.

Impact

Users can permanently lose access to their funds if the mint temporarily fails to report a keyset. The wallet will permanently drop the keyset, rendering valid proofs unspendable. This constitutes a high-severity denial of service and potential loss of funds.

Test Results

A test script confirming this behavior failed initially (showing the keyset was permanently excluded from wallet.keysets). Applying the proposed fix successfully resolved the test.

Proposed Fix

In cashu/wallet/wallet.py, load_mint_keysets, reset deleted_at to None when an existing keyset reappears in the mint's keyset list:

                if keysets_in_db_dict[mint_keyset.id].deleted_at is not None:
                    keysets_in_db_dict[mint_keyset.id].deleted_at = None
                    changed = True

@ye0man ye0man modified the milestones: 0.20.0, 0.21.0 May 5, 2026
@ye0man ye0man moved this from In progress to Needs Review in nutshell Jun 5, 2026
@a1denvalu3

Copy link
Copy Markdown
Collaborator

Summary

The pull request introduces a deleted_at column for keysets, which marks them as deleted if the mint no longer advertises them. However, when redeeming tokens (e.g. during a cashu receive), the wallet must verify the DLEQ proofs of incoming tokens using verify_proofs_dleq. This verification strictly requires the incoming proofs' keyset ID to be present in memory (self.keysets). Since the PR modifies load_keysets_from_db to implicitly exclude deleted keysets by default, verify_proofs_dleq will raise an AssertionError for any valid proof belonging to a deleted keyset, preventing users from receiving valid tokens that the mint would otherwise still honor.

Root Cause

In cashu/wallet/wallet.py, load_keysets_from_db populates self.keysets from the database:

    async def load_keysets_from_db(
        self, url: Union[str, None] = "", unit: Union[str, None] = ""
    ):
        ...
        keysets = await get_keysets(mint_url=url, unit=unit, db=self.db)
        self.keysets = {k.id: k for k in keysets}

The get_keysets query defaults to exclude_deleted=True (added in cashu/wallet/crud.py). This strictly omits keysets with deleted_at IS NOT NULL.

During token reception, redeem() calls verify_proofs_dleq() which verifies the DLEQ of incoming proofs. It checks if the keyset is known:

            assert proof.id in self.keysets, (
                f"Keyset {proof.id} not known, can not verify DLEQ."
            )

If a user receives a token belonging to a deleted keyset, verify_proofs_dleq fails because the deleted keyset was omitted from self.keysets, aborting the operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

Bug: Wallet should mark keysets as "deleted" if they disappear from the mint's keysets response

5 participants