Address binary serialisation #12

NicolasDP · 2019-01-17T16:59:24Z

resolves #2

Propose short address binary serialisation as well as a new human readable encoding for it using bech32

rendered markdown

@NicolasDP

submitted by @NicolasDP @vincenthz

text/0000-address.md

…byte

commandodev

Just a couple of grammar tweaks

text/0000-address.md

Co-Authored-By: NicolasDP <nicolas@primetype.co.uk>

kantp · 2019-01-18T10:56:11Z

text/0000-address.md

+
+* `PublicKey`: this will be necessary to be able to utilise the token;
+* `Discriminant`: this will prevent misuse of addresses between testing
+  environment and production environment;


Should we add the address kind here as well?

kantp · 2019-01-18T11:06:09Z

text/0000-address.md

+It is possible to create a **Grouped Address**, it groups addresses to a
+staking key. To create a **Grouped Address** you need the `PublicKey`,
+the `Discriminant` (just like for a **Single Address**) and another
+`PublicKey` that is associating this address to another hierarchy.


I think we should expand on this, to get the connection to what's in the design document. There, we have four new types of addresses (base, pointer, enterprise, and reward account). This RFC introduces two new kinds of addresses, which can be used to represent three of the addresses in the design document:

base addresses can be implemented in terms of grouped addresses

pointer addresses are not covered here

enterprise addresses can be implemented in terms of single addresses

reward account addresses can be implemented in terms of grouped addresses

I suggest using the address kind to distinguish the two kinds of grouped addresses (base and reward account), and adding pointer addresses.

we're not going to support pointer address (with this rfc). they are not useful for MVP, can be bolted afterwards without a problem from the format PoV, and are raising important security questions.

So are you saying that with this proposed address format, where the additional staking key hash takes only 32 bytes, the savings of space by using pointer addresses is not enough to warrant the additional complexity?

@kantp can you share a document where types of addresses are defined - as I am not aware of the recent development

@akegalj They are defined in the design document for delegation.

text/0000-address.md

nc6

Made a bunch of grammar/consistency suggestions. I also agree that we should have some mention of pointer addresses in here. Otherwise this LGTM

ETA: just seen the note about pointer addresses.

text/0000-address.md

commandodev · 2019-01-21T07:26:42Z

I think we've given this enough time and eyeballs now. @NicolasDP if you could adress @nc6's review I'll go ahead and merge.

Co-Authored-By: NicolasDP <nicolas@primetype.co.uk>

NicolasDP · 2019-01-21T08:09:23Z

Thanks for the feedback and fixes.

NicolasDP · 2019-01-21T08:10:23Z

@Boothead , before merging we need to agree on the CIP number: 0001 seems the obvious choice.

commandodev · 2019-01-21T08:16:07Z

@Boothead , before merging we need to agree on the CIP number: 0001 seems the obvious choice.

Good point! Yes, let's go for 0001!

kantp

Technically, this looks good, but I'd really like to see a connection be made to the address types that we have for Shelley (base, enterprise, and reward account addresses). Ideally, I think we should avoid introducing new terminology when we already have some.

@vincenthz you said that you deliberately did not include pointer addresses, because of security concerns. Could you elaborate on that? If there are security issues with pointer addresses, we should address them in the design.

I think these have been addressed

NicolasDP · 2019-01-21T17:18:36Z

@kantp thanks for your feedback on this. Just like you said in your previous comment : we can implement the spec's addresses with the address types we currently have. The pointer address is not necessary now. Right now we need to focus on delivering the necessary resources before Q2. To do so we need to make choices and axes the non necessary elements. As you notice, there are room for new types (up to 121 new address types) so we can always extend this CIP with another one (and I think the pointer address will need a proper CIP since there are lot of questions that will arise from this).

Addressed by Nicolas

commandodev · 2019-01-21T18:54:33Z

I'm going to go ahead and merge this. Thanks everyone for their input. If we've missed anything, please feel free to put comments here and we can look at re-opening if necessary or superseding with another RFC.

SebastienGllmt · 2019-08-06T19:46:53Z

Two points that I thought might be worth bringing up:

In the original BECH32 spec, there was discussion about whether or not the checksum should come at the start of the data section or at the end. The rationale for putting it at the front is because most users only check that the first few characters in an address match (especially in the hardware wallet case where scrolling an address to the end on a tiny screen is tedious and slow). They ended up having to put it at the end for some compatibility reason but we're not bound to this design decision
BitcoinCash uses : as the separator instead of 1. The rationale is that this makes the addresses not only easier to read, it also makes all your addresses automatically into URI Schemes (ex: bitcoincash:q9adhakpwzztepkpwp5z0dq62m6u5v5xtyj7j3h2ws4mr9g01). The special character ':' is still part of the QR code alphanumeric mode so you still get equivalent compression. The main downside to this is that right now websites are only allowed to register URI Schemes that start with web+ (only desktop applications can omit the web+ prefix). This is why the Cardano URI Scheme spec we wrote for Yoroi uses web+. That being said, it may be worth making this change to cardano: for our addresses also (reference: https://www.bitcoincash.org/spec/cashaddr.html )

SebastienGllmt · 2019-08-08T06:22:05Z

I also noticed the Rust codebase uses many different HRPs for bech32 serialization of internal data structures. Maybe these should be defined in some registry? (ex: https://github.com/input-output-hk/chain-libs/blob/d50858350f37e864aeda6b7ce573f773a2dbecd2/chain-crypto/src/algorithms/ed25519_derive.rs#L45)

ilap · 2019-10-12T04:20:44Z

I think, they should consider ':' separator for addresses, but for the keys we should stick with the 1, which will give some advantages e.g. easily distinguish between keys and addresses. But, IMO, the Bech32 should be standardised first (versioned either) for general use as chain-libs uses some other variant of the original, due to the fact that the original spec has some limitations, the 90 max data size as an example.

ilap · 2019-10-12T09:40:30Z

However, the checksum is not really relevant as it was designed for length 71 and is effective till 89. That's why there is a size restriction (90 i.e. 450 bits data which is 56 length octets) in the original spec.

SebastienGllmt · 2019-10-15T03:03:50Z

I agree. The fact that the Rust codebase uses bech32 for literally everything (not just addresses) is strange because the checksum wasn't designed to be used for arbitrary lengths. As long as we don't care about the checksum it's not an issue though

NicolasDP added 3 commits January 17, 2019 16:54

Address serialisation format

4e01f80

submitted by @NicolasDP @vincenthz

display the key in hexadecimal

427dcb2

fix some markdown rendering

1dc6183

edolstra reviewed Jan 18, 2019

View reviewed changes

text/0000-address.md Outdated Show resolved Hide resolved

edolstra reviewed Jan 18, 2019

View reviewed changes

text/0000-address.md Outdated Show resolved Hide resolved

edolstra reviewed Jan 18, 2019

View reviewed changes

text/0000-address.md Outdated Show resolved Hide resolved

NicolasDP added 2 commits January 18, 2019 09:41

Rename Bod to Bob

9ab4106

specify explicitly the position of the discriminant bit in the first …

afcb31c

…byte

commandodev suggested changes Jan 18, 2019

View reviewed changes

text/0000-address.md Outdated Show resolved Hide resolved

text/0000-address.md Outdated Show resolved Hide resolved

text/0000-address.md Outdated Show resolved Hide resolved

Ben Ford and others added 3 commits January 18, 2019 09:59

fix the motivation introduction line regarding why CBOR was a bad choice

431f25c

Co-Authored-By: NicolasDP <nicolas@primetype.co.uk>

spelling

1edefda

Co-Authored-By: NicolasDP <nicolas@primetype.co.uk>

spelling

e814e59

Co-Authored-By: NicolasDP <nicolas@primetype.co.uk>

commandodev approved these changes Jan 18, 2019

View reviewed changes

kantp suggested changes Jan 18, 2019

View reviewed changes

kantp requested review from dcoutts and nc6 January 18, 2019 11:09

nc6 previously requested changes Jan 18, 2019

View reviewed changes

kantp approved these changes Jan 18, 2019

View reviewed changes

Apply suggestions from code review

fea35ed

Co-Authored-By: NicolasDP <nicolas@primetype.co.uk>

Finalize the CIP number and set the PR number

197e1d0

kantp previously requested changes Jan 21, 2019

View reviewed changes

commandodev merged commit 53b079b into input-output-hk:master Jan 21, 2019

NicolasDP deleted the address branch May 2, 2019 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address binary serialisation #12

Address binary serialisation #12

NicolasDP commented Jan 17, 2019 •

edited

Loading

commandodev left a comment

kantp Jan 18, 2019

kantp Jan 18, 2019

vincenthz Jan 18, 2019 •

edited

Loading

kantp Jan 18, 2019

akegalj Jan 18, 2019

kantp Jan 21, 2019

nc6 left a comment •

edited

Loading

commandodev commented Jan 21, 2019

NicolasDP commented Jan 21, 2019

NicolasDP commented Jan 21, 2019

commandodev commented Jan 21, 2019

kantp left a comment

NicolasDP commented Jan 21, 2019

commandodev commented Jan 21, 2019

SebastienGllmt commented Aug 6, 2019 •

edited

Loading

SebastienGllmt commented Aug 8, 2019 •

edited

Loading

ilap commented Oct 12, 2019 •

edited

Loading

ilap commented Oct 12, 2019 •

edited

Loading

SebastienGllmt commented Oct 15, 2019

Address binary serialisation #12

Address binary serialisation #12

Conversation

NicolasDP commented Jan 17, 2019 • edited Loading

commandodev left a comment

Choose a reason for hiding this comment

kantp Jan 18, 2019

Choose a reason for hiding this comment

kantp Jan 18, 2019

Choose a reason for hiding this comment

vincenthz Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

kantp Jan 18, 2019

Choose a reason for hiding this comment

akegalj Jan 18, 2019

Choose a reason for hiding this comment

kantp Jan 21, 2019

Choose a reason for hiding this comment

nc6 left a comment • edited Loading

Choose a reason for hiding this comment

commandodev commented Jan 21, 2019

NicolasDP commented Jan 21, 2019

NicolasDP commented Jan 21, 2019

commandodev commented Jan 21, 2019

kantp left a comment

Choose a reason for hiding this comment

NicolasDP commented Jan 21, 2019

commandodev commented Jan 21, 2019

SebastienGllmt commented Aug 6, 2019 • edited Loading

SebastienGllmt commented Aug 8, 2019 • edited Loading

ilap commented Oct 12, 2019 • edited Loading

ilap commented Oct 12, 2019 • edited Loading

SebastienGllmt commented Oct 15, 2019

NicolasDP commented Jan 17, 2019 •

edited

Loading

vincenthz Jan 18, 2019 •

edited

Loading

nc6 left a comment •

edited

Loading

SebastienGllmt commented Aug 6, 2019 •

edited

Loading

SebastienGllmt commented Aug 8, 2019 •

edited

Loading

ilap commented Oct 12, 2019 •

edited

Loading

ilap commented Oct 12, 2019 •

edited

Loading