bring signatory up to date with the remote signer spec #1257
Conversation
lescuer97
changed the title
lescuer97
force-pushed
the
signatory_up_to_spec
branch
from
e44ec92 to
4e26d87
Compare
| /// Big endian encoded integer of the first 4 bytes of the sha256 hash of the unit string. | ||
| pub fn derivation_index(&self) -> u32 { | ||
| use bitcoin::hashes::sha256; | ||
|
|
||
| let unit_str = self.to_string().to_lowercase(); | ||
| let bytes = <sha256::Hash as BitcoinHash>::hash(unit_str.as_bytes()); | ||
| // Take the first 4 bytes and convert to u32 (big endian) make sure the integer | ||
| u32::from_be_bytes([bytes[0], bytes[1], bytes[2], bytes[3]]) & !(1 << 31) |
There was a problem hiding this comment.
yeah, it has always been part of the remote signer spec: https://github.com/cashubtc/nuts/pull/250/files.
The only new addition is the modulo on the sha256sum. @lollerfirst had a look at that and gave me some thumbs up on it
There was a problem hiding this comment.
@lescuer97 This will be a breaking change for the known types, how are planning to address that?
There was a problem hiding this comment.
yes I have open PRs to cdk and the NUTs repo to implement deterministic derivation paths
cdk PR #1181
NUTs PR cashubtc/nuts#292
There was a problem hiding this comment.
yes I have open PRs to cdk and the NUTs repo to implement deterministic derivation paths cdk PR #1181 NUTs PR cashubtc/nuts#292
I was not aware of this PRs. @vnprc would you like to use mine? they seems to be equally as effective.
There was a problem hiding this comment.
@lescuer97 This will be a breaking change for the known types, how are planning to address that?
you are right, it would break it for library users. I could make a new version and make a deprecation warning for the older function. What do you think?
There was a problem hiding this comment.
@crodas I added back and depracted the original index function.
| #[deprecated( | ||
| since = "0.14.0", | ||
| note = "This function is outdated; use `hashed_derivation_index` instead." | ||
| )] |
There was a problem hiding this comment.
I don't think we should deprecate this. Instead we keep the hard coded for the ones we already have and then hash anything else.
There was a problem hiding this comment.
I don't think that would be a good idea. It would not make cdk spec compliant.
I don't think breaking types here is a big deal thought.
As an alternative, I can also just do a simple hack and return Some(u32). even thought there is no option. This would mantain type consistent
|
Are there still open questions on this should we bump it to 0.15? |
| /// Big endian encoded integer of the first 4 bytes of the sha256 hash of the unit string. | ||
| pub fn hashed_derivation_index(&self) -> u32 { | ||
| use bitcoin::hashes::sha256; | ||
|
|
||
| let unit_str = self.to_string().to_lowercase(); | ||
| let bytes = <sha256::Hash as BitcoinHash>::hash(unit_str.as_bytes()); | ||
| // Take the first 4 bytes and convert to u32 (big endian) make sure the integer | ||
| u32::from_be_bytes([bytes[0], bytes[1], bytes[2], bytes[3]]) & !(1 << 31) | ||
| } | ||
| } |
There was a problem hiding this comment.
I am concerned that this is not a hardened solution. It would derive different derivation indexes for currency unit strings that are almost but not quite the same:
- leading and trailing whitespace
- capitalization differences
- functionally equivalent unicode character combinations
The version in this PR also doesn't include a reserved range for the existing hard coded currency units. Over a long enough timeframe this will introduce derivation path conflicts with sats, millisats, USD, and EUR.
As written in this PR, the hashed_derivation_index function would open the door to bugs and exploits. Resolving those issues in a backward compatible way would be more complex and difficult than just addressing the issues now before any hashed currency unit derivation paths are introduced in the wild.
I just removed the draft setting on #1181. I will rebase it to the latest commit on main. I think we should get 1181 merged and rebase this commit on top of it. Y'all should also review the NUT spec PR: cashubtc/nuts#292. Once I get some feedback on it I would be happy to open a PR to nutshell to get the spec finalized.
lescuer97
force-pushed
the
signatory_up_to_spec
branch
from
891ac38 to
473d6c1
Compare
|
@crodas could you re run the test? I think there was an error |
Its not an error. Cargo.lock file needs to be updated since you added a dep. |
4 participants
Description
Changes the signatory keyset generation and updates the proto spec to follow this pr
1- Generates new keysets with the correct derivations paths.
2- returns the derivation_index as version
3- add test vectors from remote signer
Notes to the reviewers
This should not affect any already created keyset.
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just final-checkbefore committing