-
Notifications
You must be signed in to change notification settings - Fork 222
[FEATURE] Seed XOR functionality in Seeds Menu #738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
I would prefer this be a part of the seed loading process, though that doesn't necessarily mean that this standalone process isn't useful (but I'm less sure of that). Think of solving this more from a FLOW-based philosophy that can guide the user right from the beginning (i.e. the difference between: "Load all Something like:
And then the end result would JUST be the final SeedXORed key held in memory. The individual SeedXOR component keys would be discarded. The standalone process UX you've implemented is kind of a "reverse polish notation" approach (load first, then apply the operator). It's not really a big deal for a 2-component key SeedXOR. But if someone has 4 component keys...? And is there any value in having those component keys stay loaded in memory? Just gets too be too much fingerprint management to keep track of which key is the final result vs which are component keys. |
I will make a Flow Diagram for that. Edit : Is this a correct implementation? (LINK) |
I may be misreading the intention, but I think the "More Keys?" decision point can be removed altogether. "Choose Action" already has that internal looping flow covered. |
You're correct...the "More Keys?" decision point is redundant since the "Choose Action" step already includes the option to loop back to loading another key..
|
Yeah, that matches how I was imagining the flow now. You'll need some new |
I will provide an update on progress and briefly touch on the advancements I've made. |
- View for loading a component key in the SeedXOR rebuild flow. - Current combined fingerprint of the SeedXOR components - View for selecting an existing seed to use as a SeedXOR component. - View for finalizing the SeedXOR seed.
…signer into seed-xor-flow
@jdlcdl could you review the latested changes, as you are building this in past I start by loading a seed and choosing the rebuild SeedXOR option. Then I load each component of the seed....either by scanning a SeedQR, typing the mnemonic, or using a seed already in-memory. After all parts are loaded, it shows me a fingerprint to confirm (at least two components are needed to finalize).
Then I choose what to do next, either add a passphrase or just finish it. Once I’m done, the full seed is in memory, and the first component I loaded is gone. Question : |
updated !! |
Just compiled and tested on a real hardware pi0. Here's a few of my notes and thoughts:
I'm sorry for a way too long comment maybe but I'm hoping it might be somehow useful. :) |
@t0m7 Btw its |
tom7, your argument in 7 above is solid. dipunm had convinced me that loading all seeds in a loop before finalizing it would be best, but I'm such a fan of SeedQR that in my case I'd simply be using xor so that I don't have a single qr in one place that is the full secret... and could move short distances with booted signer to retrieve hidden SeedQR components. Being able to load one at a time into memory allows for mistakes to be made in the event that one of the seeds is loaded the hard way. I'm not fully understanding the use case of 8 above, to remove a component seed from the collection. I wonder, if there was a list of the naked fingerprints in the collection (in the order they were added or sorted so they always look the same), then being able to load in any order, or to remove a seed might be more clear to the user.... assuming they could see a list of all components currently added on one screen. so users might record fingerprints like (a=01234567, b=89abcdef, c=deadbeef) and their records might say "My wallet is xor(a,b,c)" but Seedsigner would simply say their xor collection has fingerprints (01234567, 89abcdef, and deadbeef)... if they add deadbeef again, then it would disappear from the list and this would be clear. It could also help to enforce getting familiar with fingerprints, and would make dealing with selecting loaded seeds easy. |
I disagree. The steps:
ALL require the SAME amount of effort no matter what the flow is (enter each seed individually and then activate a SeedXOR mode vs one unified SeedXOR flow). IF a mnemonic is invalid, the user would simply stay in the sub-flow to either correct the mnemonic or discard it and enter a fresh one. But in the meantime, the meta-flow (SeedXOR) would be retained in memory -- along with all the previously entered SeedXOR sub-seeds/shards/whatever the terminology is. This is how all of our sub-flows work (or at least should work; I'm not sure what happens if the PSBT flow goes through Load A Seed and there are loading errors. Do we correctly resume the PSBT flow and re-prompt to load a seed?). However, IF we load each SeedXOR component seed separately, then we have a messier situation where we now have 3+1, 4+1, or 6+1 seeds all in memory (expanding on @t0m7's example). In the most likely use case, would the user do any further operations on those first 3, 4, or 6 seeds? If not, why keep them around? And won't having all those seeds onboard potentially create user confusion? Yes, the user should focus on the fingerprint of the final SeedXOR target result, but why clutter the UX with those sub-seeds that are now irrelevant after the SeedXOR has been re-assembled? |
Note that I have not reviewed the code changes nor have I used this PR hands-on yet, but most of the discussion seems to be at a more abstract "what's the best approach" level vs code nitty gritty details. And in general, just to protect my own time, I'm more inclined to wait until the approach has been more vetted / refined before I invest time to do code review and extensive hands-on testing. But other quick thoughts on @t0m7's thorough feedback (using his number references):
IF we have the flow where arbitrary in-memory seeds can after the fact be SeedXOR'ed together, then we have a muddier UX where, yes, probably makes sense to have multiple REASSEMBLE entry points (SeedOptions, Tools, Load A Seed). Another reason why I dislike this flow approach.
I don't understand this point. I view the verb selection as being the same as you would use for Shamir. You have a bunch of shards that are useless on their own. You want to reassemble (or "rebuild") them into their original form. Building off my reply to 1.) above, I would want the verb to make it clear which operation is intended by the menu option: CREATE the SeedXOR shards or REASSEMBLE/REBUILD the shards into the original seed. Even a momentary "Wait, is this 'SeedXOR' button going to break apart or reassemble?" doubt would be unnecessary friction in our UX. I almost always forget to say it, but thank you for the time and thought you invested in your review, @t0m7. |
Description
This PR adds a new feature that allows users to create a new seed by XORing two existing seeds. This is an advanced feature for users who want to combine seeds for additional security or recovery options.
Characteristics
NOTE : Currently a new setting SETTING__SEED_XOR is enabled for the time being.
This is tested on Emulator, with 2 seeds with BIP39 Mnemonic
Then we need to select 2 seeds for Xoring
which lead to final seed aftering XORing
and BIP39 Mnemonic for this seed are
For testing, I'll use this script and credit goes to 91DarioDev
This pull request is categorized as a:
Checklist
pytest
and made sure all unit tests pass before sumbitting the PRIf you modified or added functionality/workflow, did you add new unit tests?
I have tested this PR on the following platforms/os: