search

namecoin / electrum-nmc

Namecoin port of Electrum Bitcoin client.
https://www.namecoin.org/
MIT License
29 stars 24 forks source link

Deterministic salts #247

Closed JeremyRand closed 4 years ago

JeremyRand commented 4 years ago

Refs https://github.com/namecoin/meta/issues/58

Refs https://github.com/namecoin/electrum-nmc/issues/237

JeremyRand commented 4 years ago

Okay, I think this is working. Salt is equal regardless of Cryptodome vs Cryptography; it's constant for a given name+address pair; it changes when I change the name; it changes when I change the address; it's equal for name_new and name_firstupdate. For now, there's no GUI for creating a name_firstupdate, but it's easy to do from the console.

@domob1812 Would you like to confirm whether the salts generated by this PR match your implementation in Namecoin Core? I've used a 32-byte HKDF with SHA256, truncated to 20 bytes, so that it should be easy to implement equivalent logic in Namecoin Core.

domob1812 commented 4 years ago

Code looks good to me, although I think you might want to add unit tests as well as perhaps clean up the commit history a bit.

Does that work for multisig wallets also?

As I do not yet have an implementation of this in Namecoin Core (and don't know when I will have time to create one), I can't really verify if it produces the same values.

domob1812 commented 4 years ago

To add to my previous comment: I think it would be great to add explicit unit tests for what the expected salt is given a name and address (to which the private key is hardcoded as well in the test). Then we could add the exact same test to Namecoin Core as well to ensure compatibility when I'm doing that.

JeremyRand commented 4 years ago

Code looks good to me, although I think you might want to add unit tests

@domob1812 Done.

as well as perhaps clean up the commit history a bit.

Good point, I'll do that shortly.

Does that work for multisig wallets also?

I didn't test it manually, but the expected behavior is that it will work fine for multisig wallets; the salt generated will depend on which cosigner added the name_new output to the transaction. I did add unit tests for 2-of-3 multisig wallets (both P2SH and P2WSH) and they appear to work fine.

As I do not yet have an implementation of this in Namecoin Core (and don't know when I will have time to create one), I can't really verify if it produces the same values.

Okay. Should we merge this PR without waiting for Namecoin Core to implement it, or should we wait? (AFAICT it's still useful even if it's specific to Electrum-NMC, so I'd lean toward the former.)

To add to my previous comment: I think it would be great to add explicit unit tests for what the expected salt is given a name and address (to which the private key is hardcoded as well in the test). Then we could add the exact same test to Namecoin Core as well to ensure compatibility when I'm doing that.

Yes, seems reasonable. I think my unit tests meet these criteria, feel free to check if they're suitable for comparing to Namecoin Core.

domob1812 commented 4 years ago

Thanks, the tests look indeed exactly like what I had in mind. I think it is fine to merge this. As you say, this is even useful if specific to Electrum-NMC. And I also don't see any specific issue in getting the same results with a future implementation in Namecoin Core.

JeremyRand commented 4 years ago

@domob1812 Do we still need to clean up the Git history in this PR, or is it okay to merge as-is?

@gits7r My loose understanding is that the crypto code in master is very different from 3.3.10, and as a result I suspect this would be very messy to backport. Do we need to backport this one or should we save it for the 4.0.0 release?

domob1812 commented 4 years ago

@JeremyRand: I think it is fine. Perhaps I would personally have merged the very first two commits about HKDF together, but that's up to you.

gits7r commented 4 years ago

This is a major feature and quite useful, so if the backport is messy I think it's ok to keep it for 4.0.0 release. Also, we should document it at protocol level so that any different implementation will follow the deterministic salts -- they are very useful and can prevent a lot of accidents and misuse / misconfigurations.

I think we can leave it with 4.0.0 where we will also keep an eye on our auxpow changes.