Walletwasabi: Move Schnorr blind signature from NBitcoin to Wasabi

[lontivero]

The Schnorr blinding signature's algorithms in NBitcoin depend on BouncyCastle classes that are not public. These classes are needed for EC computations. We have three options:

• Let that in NBitcoin (@NicolasDorier ?)
• Reference BouncyCastle package (or copy a bunch of classes from it)
• Write the EC algorithms in Wasabi (I have a some experience with this)

Any suggestion?

[nopara73]

@NicolasDorier says on Slack that there may be no need for this.

[lontivero]

I got it.

Btw and just for the record, I reviewed the NBitcoin PR and the only endianess switch related to Schnorr blinding signatures that I found is in the message to be signed. This means that it should not break the Wasabi API.

[NicolasDorier]

I documented the breaking change on my PR, this is related to endianness of uint256 messages.

[nopara73]

@lontivero This issue isn't resolved. There's a breaking change. https://github.com/MetacoSA/NBitcoin/pull/821

[NicolasDorier]

@lontivero this does not break if both the client and server update at the same time.
If only the server update and not the client, you will have problems.

Way to fix are:

Trying both endianness, if one fail, flip the bytes.

EDIT: Can't work, old client can't flip bytes.

• Adding version information to calls from clients so the server have a clue about which endianness to use.
• Adding a new uint256 field for each already existing field that new client will use. Keep the old one as they are.
• Wrapping all the call to NBitcoin's schnorr stuff with your own wrapper which flip bytes before passing stuff to NBitcoin. (easiest, but I think it make the API weird, but that's not a big deal)

~Clean cut hardfork style: Ask everybody to update their Wasabi wallet.~

EDIT: This would kill liquidity for a while just for a simple change

[lontivero]

I don't understand why you both say this is a breaking change when it is just a change in the endianess of the message. Currently the client blinds the message "Hello world" and sends the blinded message to the server. The server signs the blinded message and sends it back to the client which unblinds it and verifies that "Hello world" was signed.

This process has to work exactly in the same way if the client sends "dlrow olleH" instead of "Hello world" because changing the endianess of the message is the same that signing a different message. It should not break.

[nopara73]

I don't understand why you both say this is a breaking change when it is just a change in the endianess of the message.

To be honest I'm just parroting @NicolasDorier. Getting surprised here would be the end of the world. Can you test your theory and keep this issue open until it's proven to be safe?

[NicolasDorier]

@lontivero that's difficult to for me to explain, I guess you can see it buy just updating the coordinator to use the new version of NBitcoin while wasabi client is using the old version of NBitcoin. It will clearly be appearant.

[lontivero]

This repo proves the there is no breaking change: https://github.com/lontivero/test-nbitcoin-compatibility/tree/master

[NicolasDorier]

you did not tried the blinded signatures

[lontivero]

What do you mean? The test goes thorough all the steps blinds, signs, unblinds and verify the signature. Basically it has two parts, one for the client (message blinding/unblinding and signature verification) and the other part for the server (blinded message signature):

+------------+          +------------+
| Client     |          | Server     |
+-----+------+          +------------+
      |                      |
      +---------------+      |
      | bm=Blind(msg) |      |
      | <-------------+      |
      |   bs=Sign(bm)        |
      +--------------------->+
      |                      |
      +----------------+     |
      | sig=Unblind(bs)|     |
      | <--------------+     |
      |                      |
      +----------------+     |
      | Verify(msg,sig)|     |
      | <--------------+     |

I will make a new test using the released nuget packages instead of the repository branches just in case I made any mistake anyway.

[lontivero]

Okay, I have tested it using nuget packages instead of referencing NBitcoin branches as git submodules. The client part uses a previous nbitcoin version to blind, unblind and verify the signature while the server part uses the latest nbitcoin version to sign the blinded message.

[nopara73]

@lontivero I get https://github.com/zkSNACKs/WalletWasabi/pull/3211 merged so you can make a cleaner PR for this.

[nopara73]

@lontivero done. You can now create a PR with updating NBitcoin.

[lontivero]

A PR for what exactly?

[nopara73]

For updating NBitcoin.

[NicolasDorier]

I plan to make a public library of my port of secp256k1 so you can implement those things on your side later. I still keep it in NBitcoin for a while until I made it public and then when you have time to take take your code.

[NicolasDorier]

@lontivero

I released Secp256k1 in a public library that you can reference in WasabiWallet.
https://github.com/MetacoSA/NBitcoin/tree/master/NBitcoin.Secp256k1

Note that NBitcoin statically built it inside so NBitcoin does not have dependency on it, this will prevent any versioning issue between NBitcoin and Secp256k1.

Can you take out all the code from NBitcoin for blinding?
I already ported the blinding code to use the library, so this should not be a big job.

Bonus point: You can fix the endianness if it is a problem for you.

[nopara73]

I can confirm, it's a breaking change: https://github.com/zkSNACKs/WalletWasabi/pull/3234#issuecomment-596979342

[dangershony]

I plan to make a public library of my port of secp256k1

@NicolasDorier why keep it under NBitcoin why not a separate repo? that is more close to the original approach.

[nopara73]

KISS

[NicolasDorier]

@NicolasDorier why keep it under NBitcoin why not a separate repo? that is more close to the original approach.

As @nopara73 said, KISS. This library is very young and always moving, putting it in separate repo means I need to often release new package to update NBitcoin instead of just changing the files there.

Also I want NBitcoin to have no dependency to remove versioning issues that often happen when libraries change a lot their interface, so I need to include the files in the NBitcoin project directly instead of referencing Secp256k1 as a package..

I can confirm, it's a breaking change: #3234 (comment)

You can probably easily fix the break now. Just reference NBitcoin.Secp256k1, copy and paste my blinding signature stuff to your repo. Then everywhere I was using a uint256 inside a hash, you need to Reverse the bytes first.

[lontivero]

Just reference NBitcoin.Secp256k1, copy and paste my blinding signature stuff to your repo.

I don't think this is something easy to do because that code uses a lot of things that are not available outside of NBitcoin, for example the ECKey, NBitcoinContext, Scalar.ctor(Span<byte>, overflow), etc.

[NicolasDorier]

@lontivero the NBitcoinContext you can replicate in Wasabi, this is easy code, everything should be public in the lib. Alternatively, just use Context.Instance.

ECKey is refering to a class inside secp256k1, so should be ok. Scalar ctor should be public as well.

[NicolasDorier]

I am opening all the ctors. If I forgot some let me know.

[NicolasDorier]

@lontivero I opened up the ctor in Scalar in 1.0.1

var r = R._ECKey.sec;

You can replace by Context.Instance.TryCreatePrivKey(R.ToBytes(), out var r); (or the wasabi context if you blind it)

[lontivero]

Thank you very much @NicolasDorier I've just finished moving the code to Wasabi and changed the hashes endianess to make it work as before. As soon as I finished the work on Wasabi I will remove the schnorr blinding stuff from NBitcoin.

[molnard]

@lontivero is this issue still valid?

[lontivero]

It was done months ago.

[yahiheb]

As soon as I finished the work on Wasabi I will remove the schnorr blinding stuff from NBitcoin.

@lontivero Should SchnorrSignature.cs and SchnorrSignerTests.cs be removed from NBitcoin?

[NicolasDorier]

@yahiheb no, SchnorrSignature, if I did not removed it, is the one needed for taproot.

[yahiheb]

@yahiheb no, SchnorrSignature, if I did not removed it, is the one needed for taproot.

@NicolasDorier Yes it is was not removed https://github.com/MetacoSA/NBitcoin/blob/master/NBitcoin/Crypto/SchnorrSignature.cs

Thanks for the clarification.