fix restore_promises derivation-path mismatch on subset restores#901
Open
b-l-u-e wants to merge 3 commits intocashubtc:mainfrom
Open
fix restore_promises derivation-path mismatch on subset restores#901b-l-u-e wants to merge 3 commits intocashubtc:mainfrom
b-l-u-e wants to merge 3 commits intocashubtc:mainfrom
Conversation
Signed-off-by: b-l-u-e <winnie.gitau282@gmail.com>
Signed-off-by: b-l-u-e <winnie.gitau282@gmail.com>
Signed-off-by: b-l-u-e <winnie.gitau282@gmail.com>
e1802af to
b70bbbe
Compare
|
Ran this locally and verified that the regression test fails without filtering derivation_paths and passes with the fix. The alignment fix looks correct. |
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
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.
This PR fixes a wallet restore issue where restored proofs could receive the wrong
derivation_pathwhen the mint returns only a subset of requested outputs (or returns them in a non-zero index range/order).It also adds a regression test to ensure this mismatch cannot reoccur.
so here in
restore_promises:secretsandrswere filtered usingmatching_indicesderivation_pathswas not filtered before calling_construct_proofs(...)1) Wallet fix
In
cashu/wallet/wallet.py:derivation_pathsusing the samematching_indicesused forsecrets/rsbefore_construct_proofs(...).secrets = [secrets[i] for i in matching_indices] rs = [rs[i] for i in matching_indices] + derivation_paths = [derivation_paths[i] for i in matching_indices] logger.debug( f"Restored {len(restored_promises)} promises. Constructing proofs." )This restores 1:1 alignment among:
restored_promisessecretsrsderivation_paths2) Regression test
In
tests/wallet/test_wallet_restore.py:test_restore_promises_derivation_paths_subsetderivation_pathmatches the path expected for that proof’ssecretTest Plan
poetry run pytest tests/wallet/test_wallet_restore.py::test_restore_promises_derivation_paths_subset -vNote: While preparing this PR I hit a local pre-commit mypy error:
tests/conftest.py:119: Argument "seed" to "Ledger" has incompatible type "str | None"; expected "str"not sure of the cause but maybe comes from here or rather maybe i am missing somethin???
so i had to add an explicit guard in tests/conftest.py before constructing Ledger:
also i struggled with ruff-format for the cashu/wallet/wallet.py and ended up making changes in other part of the code file which is unrelated to the issue solved