Finalize ADR-028

[robert-zaremba]

Description

closes: #8397

---

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

[ValarDragon]

Can this be more explicit? What tools in particular, is the goal here to just be keeping secp key formats as is?

[robert-zaremba]

Mainly wallets or services which already relay on the current addresse. More details about this is at #8041 I will link it directly in the ADR.

[ValarDragon]

Why not do a personalization string if the underlying hash supports it?

Its also worth noting, that for efficiency, hash(typ) should ideally have length block size OR size(hash(type) + pubkey) < 1 block size

[robert-zaremba]

I'm not sure I follow you:

• What do you mean with _ personalization string_?
• The returned bytes are capped at A_LEN. hash is defined in the section above. I specifically, didn't want to name it here -- in case we will edit it, we only need to change one place.
• With block size - you mane a blockchain block size? That's totally different scale.

[clevinson]

Thanks for the updates @robert-zaremba! Can you make these last few updates and then mark re-request review so we can get this merged in ?

[aaronc]

Please remove this section and consider reverting to the previous multisig format. As we discussed live generalizing multisig keys to composed addresses is a nice idea but doesn't work because there are too many specifics like threshold which isn't here.

[robert-zaremba]

I reverted and merged the Multisig Addresses section here. My motivation here is to show a generic function to compose pubkeys, and use multisig as an example. I think it's better to make this abstraction, rather than re-implement it in other account types.

[aaronc]

Well even so, I don't think this is actually correct, the concatenated addresses each need to be length prefixed because they can be variable length. But otherwise, maybe it works...

[robert-zaremba]

That's why we use .Address() function for all sub addresses - it will have the required logic. Compose is a one way function, there is nothing to decode.

[aaronc]

Maybe my comment wasn't clear but we need to length prefix before concatenation because otherwise the boundary between addresses isn't clear

[robert-zaremba]

Let me give more context. The motivation to have this section is to:

1. Show that we are not in the dead end and we have positive consequences of address composability (as outlined in the consequences section)
2. we have algorithm for that
3. new multisig is just an example, but the main topic is composition.

[aaronc]

I think that's okay, I'm not there's another use case but it looks okay.

This is what I was trying to say: https://github.com/cosmos/cosmos-sdk/pull/8398/files#r566337924

[aaronc]

Suggested change

	return address.Hash(typ, addresses[0] + ... + addresses[n])
	return address.Hash(typ, LengthPrefixAddress(addresses[0]) + ... + LengthPrefixAddress(addresses[n]))

[aaronc]

Without length prefixing each address here you don't know the boundary. The addresses could be 20 or 32 bytes or some other number of users choose. So you can't successfully compose without that. Otherwise, there could be some weird collisions however unlikely.

[robert-zaremba]

Composed is a one-way function - once hashed, we can't go back. We don't need to know boundaries - finding a collision with length prefixing is as hard as finding a collision without them. We can map LengthPrefixAddress to all sub addresses, but I don't see any justification for that. Meaning it obfuscates the code but it doesn't enhance the security.

[robert-zaremba]

Practically speaking, the only way we could get a collision is when we have two sequences of addresses which concatenate to the same string:

as, bs [][]byte <- list of addresses

So, the question is not about the hash collision, but key construction. Specifically: is it harder to find 2 different list of addresses (as != bs) for having a match in A or in B?

A:  as[0]+as[1]+...+as[n] =? bs[0]+bs[1]+...+bs[m]
B:  lenpref(as[0])+lenpref(as[1])+...+lenpref(as[n]) =? lenpref(bs[0])+lenpref(bs[1])+...+lenpref(bs[m])

so, lets think...

[robert-zaremba]

Each problem can be solved by a construction: we map and concatenate. Then for constructing bs we try to slice the concatenate sequence just before some 1 bits.
Specifically, for problem B, here is an example of conflicting addresses in binary form:

as = {0101, 1000, 1}
bs = {00, 101000 11}
map(lenpref, as) = {10 0  100,  10 1000, 11  }
map(lenpref, as) = {10 0, 100   10 1000  11 }

[robert-zaremba]

I was thinking more about it. Since the length prefix is always 1 byte, then we can't construct conflicting address sequences unless the individual addresses are the same - so the lenpref in this case will work.
Other solution I had in mind was to hash each subaddress before concatenation - this way all parts have exactly same length.

[aaronc]

Yeah, the collision risk may be small but length-prefixing is cheap and ensures zero collisions in the pre-image.

[robert-zaremba]

@aaronc , yes, in OP, when you wrote about collision I thought you are writing about hash collision ;) In the (updated) text I use conflict instead of collision.

[alessio]

Nihil obstat from me

[aaronc]

ACK 🎉

Left a bunch of minor suggestions, mostly grammar related

[robert-zaremba]

I'm changing the status back to Proposed
This week we added more considerations for module derived accounts. We left few notes in the ADR to continue discussion about this.

The basic part: address generation of simple accounts (key paris) and modules we consider stable and validated.