SeedSigner / seedsigner

Use an air-gapped Raspberry Pi Zero to sign for Bitcoin transactions! (and do other cool stuff)
MIT License
699 stars 161 forks source link

Changed error flow when wrong seed for PSBT #468

Closed jdlcdl closed 1 year ago

jdlcdl commented 1 year ago

fixes #467

Instead of redirecting to NotYetImplementedView, redirects to PSBTSigningErrorView where user will at least realize what went wrong and can choose another seed.

Flow tests are limited, and I was bumping my head thru most of it. Flow test is not so complete that it finishes with a happy ending, Into the Error View and terminates where another seed can be chosen.

jdlcdl commented 1 year ago

This pr is far from ideal.

The wrong seed is loaded first, then a PSBT that does not contain the fingerprint for this seed is loaded. User is walked thru all the way to change verification before being redirected to being informed "Not able to add a valid signature". More ideal would be for the PSBTOverviewView to fail immediately into allowing the user to select another seed. But at this late stage just before release, I was aiming for a safe one-liner that would be "useful" and "better" instead of "ideal" which might break in unexpected places w/ little time to do comprehensive testing.

I feel this is probably a rare user-error use-case and that most users will quickly realize something is wrong, trying again with more care, and may not even complain. I wonder if Tim was just sharing an error while testing, because he knew exactly what the problem was.

kdmukai commented 1 year ago

I think this is enough of an edge case that we don't need to rush it into v0.7.0.

We already check if we think each onboard seed can sign a given psbt (e.g. the "?" appended if we can't match the fingerprint in any of the inputs' bip32 derivations). If you saw the "?" and selected that seed anyway, such is life (for now).

However, if a VALID seed is selected and a psbt output identifies itself as one of your receive/change addrs AND YET it doesn't match the seed... then we still have an unresolved situation. I'm not sure this would ever actually happen, though perhaps some PayJoin coordinator might potentially offer the same psbt to each signer (your seed obv won't match the fingerprint for my output and vice versa).

I think we'd have to learn more about this edge case; it might not even be a real-world possibility.

newtonick commented 1 year ago

I think this is enough of an edge case that we don't need to rush it into v0.7.0.

I agree

In practice I believe this error is most commonly encountered when doing testing with fake funds (testnet/regtest). Most commonly happens in my experience when you're not being careful and signing a PSBT willy nilly with any seed.

I believe there is opportunity to refine the seed selection experience and provide more appropriate notification screens when signing a PSBT.