-
Notifications
You must be signed in to change notification settings - Fork 65
Keyset ID V2 #182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Keyset ID V2 #182
Conversation
|
Thank you! Please:
|
|
Quite honest, I am not sure such small change justifies creating a new keyset version and the headache attached to it. Maybe we should keep this improvement in mind and apply it together with some future proposal that changes something significant? |
|
@prusnak this was suggested to enable wallets to store proofs without referencing mint URLs. @thesimplekid @ok300 do you think this is still necessary? |
The longer keyset ids to reduce the chance of a collision and the keyset expiry are things we've talked about awhile and I would like to see, I think it makes sense to move forward with this. I cant think of anything else we wanted to include that we should hold this for, but if there is I am open to it. |
I agree. This is exactly the kind of change I was mentioning when I said to "justify" creating a new keyset version. |
|
A few NITs, otherwise it's an ACK from me: The specified behavior is a bit ambiguous in 2 places.
This doesn't say if it's the long or short ID. It should probably say "short ID" or rename the variable to
This makes it sound preferable, but optional to use the shorter |
Why? |
It is non-standard and does not bring any single benefit imo. Only headache. |
|
IMO there is an ambiguity in the new spec:
This implies the Is there any reason where the long ID makes sense? If not, I'd suggest to change that The only reason I can think of is to avoid short ID collisions, but that's already covered by another section:
|
Co-authored-by: ok300 <106775972+ok300@users.noreply.github.com>
Fair point - was just that adding I've raised a seperate issue for consideration: #289 |
agreed
having two different separators is ugly and we're only going to hash the result anyway. I'd also vote for 1 in favor of dropping all separators ( |
13.md
Outdated
| // Derive using BIP32 paths: | ||
| // secret_path = `m/129372'/0'/{keysetIdInt}'/{counterK}'/0` | ||
| // r_path = `m/129372'/0'/{keysetIdInt}'/{counterK}'/1` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant comments?
|
ACK 1efb5a4 |
|
Update: this comment is now OUTDATED. See @callebtc below
|
Update: SIG_ALL does not contain the keyset ID anymore, I think we can mark this comment as outdated. |
|
ACK 1efb5a4 |
|
ACK 9f5a747 |
13.md
Outdated
| - Set `counter = 0` | ||
| - Restore `Proofs` in batches of 100 using the legacy derivation, and increment `counter` | ||
| - Repeat until three consecutive batches are returned empty | ||
| - Reset `counter` to the value at the last successful restore + 1 | ||
| - Restore `Proofs` in batches of 100 using the HMAC-SHA256 derivation, and increment `counter` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unnecessary to try both methods, assuming that we transition to the new KDF only for V2 keysets
thesimplekid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 97b9573
robwoodgate
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just flagging one area of concern.
| 1 - sort public keys by their amount in ascending order | ||
| 2 - concatenate all public keys to a single byte array | ||
| 3 - add the lowercase UTF8-encoded unit string to the byte array (e.g. "unit:sat") | ||
| 4 - If a final expiration is specified, add the UTF8-encoded string (e.g "final_expiry:1896187313") | ||
| 4 - HASH_SHA256 the concatenated byte array | ||
| 5 - prefix it with a keyset ID version byte "01" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The keyset ID derivation doesn't protect/guarantee the amounts have not been tampered with, only that the amounts are still ordered the same.
What stops a mint from changing the amounts bound to each key to devalue and "inflate" away their liabilities?
eg: 1, 2, 4, 8, 16, 32, 64
could become: 1, 2, 3, 4, 8, 16, 32
or maybe even: 1, 1, 1, 1, 1, 1, 1
Perhaps we should concat all the amounts in order too... or maybe just adding the sum of key amounts would be enough to reduce the risk of serious damage?
Summary
This PR introduces keyset ID version 2 and updates the key derivation function to improve security and eliminate mint ambiguity.
Changes
Keyset ID v2 (NUT-02)
01+ 32-byte SHA256 hash)final_expiryfield for keyset expirationVersioned Secret Derivation (NUT-13)
00uses BIP32 derivation (backward compatible)01uses HMAC-SHA512 derivationHMAC_SHA512(seed, "Cashu_KDF_HMAC_SHA512" || keyset_id_bytes || counter_bytes)Implementation Status