Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ADR 028: Public Key Addresses #7086
ADR 028: Public Key Addresses #7086
Changes from 3 commits
66f6583
9621bb7
a758d4d
4ef7702
e5227b3
d575b5f
bbdcadd
4cabd3b
5026296
79bff6a
5e5979e
91e36a2
b7d0133
e7ea7f8
049a550
49f8444
4cf17f4
a0b2a41
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I don't think the prefix will solve the security problem here. I was reading #3685 and I'm not sure if the solution solves the security problem. Here is my reasoning (maybe it's wrong):
Let's say we have to PK algorithms:
A
andB
. For a user with(pk1, sk1)
key pair, we have two possible attacks:A
became vulnerable. Attacker is able to create a valid signature without knowingsk1
. In this case we don't solve anything with this proposal.(pk2, sk2)
, possible belonging to a different PK scheme, such thataddress(typ(pk2), pk2) == address(typ(pk1), pk1)
. Then he basically broke the cryptographic hash function. The key type (and the prefix) is not important here, because the attacker has an algorithm how to find an pre-image.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.
To be clear - I'm not saying that adding prefix is bad. I'm not sure it solves anything. @ethanfrey ?
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.
There is one more attack, which in fact is important here, and this update address it:
(1.)
:A
became vulnerable. Attacker is able to forge a signature forpk \in A
. In all places where we don't store any relationship between addresses and(PK, PK scheme)
pair, the attacker will be able to spend address assets.This proposal (including
scheme
url / name in the address algorithm) protects against the attack described #3685 (isolating address spaces to protect against attacks when one of the PK scheme is broken).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.
Current secp256k1 uses ripemd160 instead of [:20]. The latter is faster to compute, but are there other trade-offs (in particular, security-wise)?
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.
It is needed for bitcoin compatibility. There were other designs, but the 2017 fundraiser addresses were generated with a bitcoin secp256k1 address derivation and the go code had to be compatible.
But I cannot tell you if 20 bytes prefix is just as safe
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.
ripemd160 is an odd and basically pointless cryptographic choice and there is no reason to retain it.
It's a slow hash function and doesn't improve security in a meaningful way.
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.
If we are changing this, how about using
blake2b
? Which is faster, and many believe that it's more secure.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.
Why not blake3 @robert-zaremba ?
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.
Not worse, and it's more random.
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.
It uses a different mechanism, so I can't say if the reduction of number of rounds impacts the security vs Blake2 or not. Probably asking one of the authors (Zooko?) would be the best approach.
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.
This seams to be the balanced solution. Although it would be still good to check out with zcash about their view. I will try to reach out to some of them.
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 new draft uses
blake2b
. Both because it has better library support and has been around longer and vetted more thoroughly (as I understand it).I'm assuming
blake2b
is preferable toblake2s
for our use case?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.
BLAKE2b is designed and optimized for 64-bit CPUs. BLAKE2s is designed and optimized for 8, 16, 32-bit CPUs.
BLAKE2 has indeed excellent Golang and Rust libraries support. Today there are very few BLAKE3 supported implementations. The speed boost observed with BLAKE3 is not really a selling point regarding ADR 028.
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.
Hex encoding the pubkey bytes in the preimage seems a bit strange actually. Usually we're hashing raw pubkey bytes, so hex encoding will double the size of the pubkey being hashed. Not that I'd really be worried about the performance, more so the weirdness of hex-encoding bytes before they're hashed. Even if we're prefixing with a string, I would think
Sha256(fmt.Sprintf("%s/%s, proto.MessageName(key), key.Bytes())[:20]
is fine?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.
Actually, why bother with the
/
either? In #5694 we talked aboutSha256(KeyTypePrefix || Keybytes)[:20]
which is just pure concatenation. I guess because the prefix is a string not just a byte, so we need a domain separator? It seems like maybe we're optimizing for human readability of the pre-image; is that important for something?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.
PubKeys are not meant to be human readable. They are long. That's why we are creating addresses.
So we don't even need to convert to string. We can concatenate bytes, without conversion to string.
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.
I've updated this ADR to define an
AddressHash
function which takes a stringprefix
and binarycontents
and separates them with a null-terminator0
byte without hex-encoding the binary. Can you take a look again @ebuchman ?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.
Same thoughts as above re hex (and maybe the "/"s).
Why not proto-encode the above PubKey structure here and otherwise use the same address format as the non-multisig, where the pubkey bytes are just the proto encoding of the Proto multisig PubKey structure?
Each nested PubKey would have to be typed too, but that seems fine? Then no need to convert nested keys to addresses or have more ad hoc encoding.
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.
Because we have to deal with non-deterministic encoding of protobuf (i.e. ADR 027). This avoids that. Can you see if what I have their now which does sorting is okay?
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.
Would it be helpful to try and put this info in bech32 encodings? Is access to this info really that important?
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.
I don't know. You're saying that maybe we can have the bech32 have an extra prefix that isn't present in the actual address? Would this be something like
cosmossecp256k1sdgh3sghlsdsdg
. Or would the prefix get added before bech32 encoding?