ADR 028: Public Key Addresses #7086

aaronc · 2020-08-18T15:01:18Z

This ADR proposes a canonical extensible address format for new public key types, and addresses the multisig pubkey address issue.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-08-18T15:08:35Z

Codecov Report

Merging #7086 into master will increase coverage by 0.39%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7086      +/-   ##
==========================================
+ Coverage   54.21%   54.60%   +0.39%     
==========================================
  Files         611      547      -64     
  Lines       38464    37121    -1343     
==========================================
- Hits        20852    20270     -582     
+ Misses      15503    15192     -311     
+ Partials     2109     1659     -450

docs/architecture/adr-028-public-key-addresses.md

ethanfrey

Sounds good

docs/architecture/adr-028-public-key-addresses.md

clevinson · 2020-08-25T20:31:05Z

The proposal here looks good to me, in terms of the specification for new addresses. However, I'm a bit confused how this actually relates to existing address formats, and I think the wording around that could be clarified.

If i understand correctly, this means that legacy account addresses would be unchanged and remain in their bech32 prefixed form, but accounts generated with non secp256k1 pubkeys would use the new format (no prefix, just a hash as described above)?

How does one distinguish between a "legacy" or "new" multisig? Is this about addresses that exist onchain on a current zone, vs ones that do not? Or about what pubkey type the multisig address is comprised of (where secp256k1 is deemed "legacy"). How does that then relate to multisig with keys of multiple types?

amaury1093

From the comments in this ADR and in the linked issues, the feeling I get is that everyone here is aligned to say "it should be okay" (incl. me).

Would it make sense to run this by a cryptographer, to get validation "yes, this is okay". I'd trust the word of a cryptographer over my gut feeling any day of the week.

docs/architecture/adr-028-public-key-addresses.md

amaury1093 · 2020-08-28T11:32:30Z

docs/architecture/adr-028-public-key-addresses.md

+key type prefix when creating addresses.
+
+We define the canonical address format for new (non-legacy) public keys as
+`Sha256(fmt.Sprintf("%s/%x, proto.MessageName(key), key.Bytes())[:20]`. This takes


Current secp256k1 uses ripemd160 instead of [:20]. The latter is faster to compute, but are there other trade-offs (in particular, security-wise)?

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

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.

If we are changing this, how about using blake2b? Which is faster, and many believe that it's more secure.

Why not blake3 @robert-zaremba ?

So hypothetically it would be easier to find a pre-image for a SHA256 address than a Blake2 address?

Not worse, and it's more random.

Blake3 reduced the number of rounds from Blake2 to get a speed up

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.

maybe we err on the side of caution and go with Blake2

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.

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 to blake2s for our use case?

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.

amaury1093 · 2020-08-28T11:48:21Z

@clevinson I can try to answer your questions, given my understanding of this ADR:

If i understand correctly, this means that legacy account addresses would be unchanged and remain in their bech32 prefixed form, but accounts generated with non secp256k1 pubkeys would use the new format (no prefix, just a hash as described above)?

There's public key ([]byte), address (also []byte, derived from pubkey & keytype, what we're discussing here) and bech32-encoding of an address (e.g. a cosm-prefixed string). Whatever address format is chosen here, we can still bech32-encode it.

How does one distinguish between a "legacy" or "new" multisig? Is this about addresses that exist onchain on a current zone, vs ones that do not? Or about what pubkey type the multisig address is comprised of (where secp256k1 is deemed "legacy"). How does that then relate to multisig with keys of multiple types?

Given a random 20-byte address, I believe there's no way to distinguish if it's a "legacy" or "new" multisig, or any other single public key type. This ADR describes the other direction, given 1/ threshold and 2/ an array of public keys (each being potentially of a different key type), how to construct "legacy" and "new" multisig addresses.

Also, I don't think secp256k1 is deemed "legacy"; it just has a different address generation algorithm.

robert-zaremba · 2020-08-28T12:08:54Z

For new account address format, how about using the same algorithm as Algorand is using.

They have one of the best cryptographers form the world so they know best what / how to use.

Additional advantage is that it has a checksum, which protects against a wrong copy paste!

alessio · 2020-08-28T12:49:34Z

The proposal looks good, though I'm interested in understanding better what are the consequences for multisig keys (see Cory's questioin: How does one distinguish between a "legacy" or "new" multisig? )

robert-zaremba · 2020-10-08T18:09:45Z

docs/architecture/adr-028-public-key-addresses.md

+	if len(contents) != 0 {
+		preImage = append(preImage, 0)
+		preImage = append(preImage, contents...)
+	}


When implementing this we should use use make([]byte, correctSize) and copy they content using for or copy (into slice starting from index 2).

robert-zaremba · 2020-10-08T18:25:46Z

I've sorted the addresses in the current multisig format. Does that address your concerns?

Yes, but we will still need to rethink how to handle dynamic accounts (more about it below).

What about multisig accounts with dynamic addresses? I think the solution will be to use a native multisig address if such one is provided (I proposed it in one comment).

I'm not sure what you mean by this? Would group accounts (https://github.com/regen-network/cosmos-modules/tree/master/incubator/group) satisfy would you mean by dynamic multisig @robert-zaremba ?

Maybe. This will go into the category of a complex / multisig account with a native address. I think we will need to go back to the ADR28 once we will put all accounts together and before we will implement it. So for the moment I would like to mark it only.

robert-zaremba · 2020-10-08T18:29:37Z

In general I would use a pseudo code for explaining the algorithm. The reason is that we should discuss implementation details separately, which will involve things like integration and optimization, and language specific things.

zmanian · 2020-10-08T21:00:54Z

docs/architecture/adr-028-public-key-addresses.md

+We use a 256-bit `blake2b` hash instead of `sha256` because it is generally considered more secure. Blake hashes
+are considered "random oracle indifferentiable", a stronger property which `sha256` does not have.


Random oracle indifferentiability is cryptographically irrelevant for constructing addresses.

https://blog.cryptographyengineering.com/2012/07/17/indifferentiability/

Cause see the prefix of an address always specifies the length of suffix. Ie. sr25519 key is always 32bytes. In the context of this protocol, a MD hash like sha256 is "random oracle indeffentiable"

I'm just saying this because I'm always going to strongly advocate for a really high bar for adding a new hash function to the Cosmos trusted computing base.

@zmanian I agree with your general approach. But we don't propose a any exotic algorithm. Blake2 is a part of at least Go and Python3 stdlib and it's an improvement. Some cryptographers claim that sha256 is easier to break than blake2 because we already know how to break other hash functions from the MD family.

Some cryptographers claim that sha256 is easier to break than blake2.

Some references would be appreciated 👍

I'm just saying this because I'm always going to strongly advocate for a really high bar for adding a new hash function to the Cosmos trusted computing base.

So I want to summarize what I heard from @zmanian on our last call about this. Basically:

there is no consensus on the replacement for SHA256 yet in the crypto community and there's a widespread sense that it won't be broken anytime soon

we want to minimize what is needed for someone to implement "cosmos" on a broad array of devices including hardware wallets and embedded devices

Thus for now it's probably preferable to stick with SHA256. Is that accurate @zmanian ?

@alessio any progress on getting the cryptographer at AiB to take a look at this?

W3F Research / Polkadot is using blake2b and avoiding sha2.

…ronc/adr-028-pub-key-addresses

aaronc · 2020-10-21T13:54:35Z

I have reverted to change to blake2b and this ADR now uses sha256 again. I would reconsider an alternate hash if a good argument is made, but it's contentious for now and I want to at least be able to move forward with the non-contentious parts.

In the interest of progress and the points raised in #7498 can we merge this ADR with status proposed and address remaining issues as follow-up issues and or PRs?

I still would like to get a cryptographer's opinion on the hashing if we can, but I think I have addressed what I can for now.

So basically I would like to get people's opinion as to whether this is a good enough starting point that we can move forward with it at least preliminarily. We need something to work off of within the SDK. Our plan is to include this in the v0.41 release so we can open a few follow-up issues tagged as v0.41 and make sure we address them before release.

robert-zaremba

👍

robert-zaremba

Let's add notes around the parts which:

Require opinion of qualified person
Are still under consideration

And then merge

aaronc · 2020-10-21T16:00:44Z

Let's add notes around the parts which:

Require opinion of qualified person

Are still under consideration

And then merge

@robert-zaremba could you open an issue documenting those things?

aaronc added 2 commits August 18, 2020 10:59

ADR 028: Public Key Addresses

66f6583

Cleanup

9621bb7

aaronc marked this pull request as ready for review August 18, 2020 15:04

aaronc requested review from alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners August 18, 2020 15:04

alexanderbez reviewed Aug 18, 2020

View reviewed changes

docs/architecture/adr-028-public-key-addresses.md Outdated Show resolved Hide resolved

docs/architecture/adr-028-public-key-addresses.md Show resolved Hide resolved

aaronc added the T: ADR An issue or PR relating to an architectural decision record label Aug 18, 2020

Address review comments

a758d4d

aaronc requested review from atheeshp, amaury1093, anilcse, blushi, ebuchman, ethanfrey, sahith-narahari and zmanian August 18, 2020 16:27

ethanfrey reviewed Aug 18, 2020

View reviewed changes

docs/architecture/adr-028-public-key-addresses.md Show resolved Hide resolved

docs/architecture/adr-028-public-key-addresses.md Show resolved Hide resolved

clevinson mentioned this pull request Aug 24, 2020

Tracking Issue: Stargate Release Candidate #7152

Closed

25 tasks

amaury1093 reviewed Aug 28, 2020

View reviewed changes

amaury1093 requested a review from robert-zaremba August 28, 2020 11:50

aaronc requested a review from robert-zaremba October 8, 2020 17:48

robert-zaremba reviewed Oct 8, 2020

View reviewed changes

jgimeno requested a review from robert-zaremba October 9, 2020 10:38

Merge branch 'master' into aaronc/adr-028-pub-key-addresses

049a550

zmanian reviewed Oct 9, 2020

View reviewed changes

robert-zaremba mentioned this pull request Oct 9, 2020

ADR034: Account Rekeying #7491

Merged

9 tasks

aaronc added 2 commits October 21, 2020 09:44

Revert to sha256

49f8444

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into aa…

4cf17f4

…ronc/adr-028-pub-key-addresses

aaronc requested review from zmanian, ebuchman, alexanderbez and amaury1093 October 21, 2020 13:54

robert-zaremba requested changes Oct 21, 2020

View reviewed changes

robert-zaremba approved these changes Oct 21, 2020

View reviewed changes

alexanderbez approved these changes Oct 21, 2020

View reviewed changes

zmanian approved these changes Oct 21, 2020

View reviewed changes

Merge branch 'master' into aaronc/adr-028-pub-key-addresses

a0b2a41

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 21, 2020

mergify bot merged commit f24ad5d into master Oct 21, 2020

mergify bot deleted the aaronc/adr-028-pub-key-addresses branch October 21, 2020 16:03

aaronc mentioned this pull request Oct 21, 2020

Implement ADR 028 and Address Follow-ups #7618

Closed

8 tasks

robert-zaremba mentioned this pull request Oct 22, 2020

Snapshot sync: use hasher for chunk hashes (#7215) #7259

Merged

10 tasks

jackzampolin mentioned this pull request Oct 23, 2020

GetSignBytes broken for IBC messages #7638

Closed

robert-zaremba mentioned this pull request Nov 27, 2020

Extensible address format #5694

Closed

robert-zaremba mentioned this pull request Jan 20, 2021

Finalize ADR-28 #8397

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR 028: Public Key Addresses #7086

ADR 028: Public Key Addresses #7086

aaronc commented Aug 18, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

ethanfrey left a comment

clevinson commented Aug 25, 2020 •

edited

Loading

amaury1093 left a comment

amaury1093 Aug 28, 2020

ethanfrey Aug 28, 2020

zmanian Sep 16, 2020

robert-zaremba Oct 2, 2020

aaronc Oct 6, 2020

robert-zaremba Oct 7, 2020

robert-zaremba Oct 7, 2020

robert-zaremba Oct 7, 2020

aaronc Oct 8, 2020

alain2sf Nov 25, 2020

amaury1093 commented Aug 28, 2020 •

edited

Loading

robert-zaremba commented Aug 28, 2020 •

edited

Loading

alessio commented Aug 28, 2020

robert-zaremba Oct 8, 2020

robert-zaremba commented Oct 8, 2020

robert-zaremba commented Oct 8, 2020

zmanian Oct 8, 2020

robert-zaremba Oct 9, 2020 •

edited

Loading

alessio Oct 9, 2020

aaronc Oct 20, 2020

robert-zaremba Oct 21, 2020

aaronc commented Oct 21, 2020

robert-zaremba left a comment •

edited

Loading

robert-zaremba left a comment •

edited

Loading

aaronc commented Oct 21, 2020

		We use a 256-bit `blake2b` hash instead of `sha256` because it is generally considered more secure. Blake hashes
		are considered "random oracle indifferentiable", a stronger property which `sha256` does not have.

ADR 028: Public Key Addresses #7086

ADR 028: Public Key Addresses #7086

Conversation

aaronc commented Aug 18, 2020

codecov bot commented Aug 18, 2020 • edited Loading

Codecov Report

ethanfrey left a comment

Choose a reason for hiding this comment

clevinson commented Aug 25, 2020 • edited Loading

amaury1093 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaury1093 commented Aug 28, 2020 • edited Loading

robert-zaremba commented Aug 28, 2020 • edited Loading

alessio commented Aug 28, 2020

Choose a reason for hiding this comment

robert-zaremba commented Oct 8, 2020

robert-zaremba commented Oct 8, 2020

Choose a reason for hiding this comment

robert-zaremba Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Oct 21, 2020

robert-zaremba left a comment • edited Loading

Choose a reason for hiding this comment

robert-zaremba left a comment • edited Loading

Choose a reason for hiding this comment

aaronc commented Oct 21, 2020

codecov bot commented Aug 18, 2020 •

edited

Loading

clevinson commented Aug 25, 2020 •

edited

Loading

amaury1093 commented Aug 28, 2020 •

edited

Loading

robert-zaremba commented Aug 28, 2020 •

edited

Loading

robert-zaremba Oct 9, 2020 •

edited

Loading

robert-zaremba left a comment •

edited

Loading

robert-zaremba left a comment •

edited

Loading