MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.86k stars 844 forks source link

Change `GetKey()` to boolean with no exceptions thrown #1078

Closed ghost closed 2 years ago

ghost commented 2 years ago

What?

https://github.com/MetacoSA/NBitcoin/blob/a9b8e592fa7df0e97bdffe7de94ed58a87d92144/NBitcoin/BIP38/BitcoinEncryptedSecret.cs#L211. That NBitcoin's GetKey method is designed in a way that SecurityException is thrown when the provided password is incorrect. For WW use case, it would be better to have bool GetKey(string password, out Key? key) method (not implemented at the moment) and avoid throwing exceptions as that would be slightly faster for us and it would convey the meaning better.

SecurityException is may be a confusing name, I guess.

https://github.com/zkSNACKs/WalletWasabi/pull/6791#issuecomment-989172772

Why?

Password Finder is an option which is used once in a while when user cannot remember the password for a wallet. There is one empty catch block used in the Wasabi code which is not a best practice and can lead to unexpected behavior or even vulnerabilities in future.

https://github.com/zkSNACKs/WalletWasabi/pull/6791 was an attempt to add one line for adding exception to logs in the catch block, however it was closed as reviewers did not agree with the changes in pull request. There was one suggestion to make changes in NBitcoin to resolve this issue. I do not find changing API as the best approach in this context however after lot of discussions, one more issue created to add a comment in empty catch block: https://github.com/zkSNACKs/WalletWasabi/issues/6954, finally there is some consensus on making changes in NBitcoin to resolve this issue.

How?

Let me know if this is the right thing to do or there are better solutions to fix this issue. I looked at the code and think it won't be 1 line change and will involve lot of things being changed. Hoping this won't break anything.

I will have to figure out if there are no NACKs to make this change in NBitcoin.


I had shared few links in related PR, issue earlier and its a basic thing IMO, however if Microsoft links are preferred here's one: https://docs.microsoft.com/en-us/powershell/utility-modules/psscriptanalyzer/rules/avoidusingemptycatchblock