Skip to content
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

Armored Slates #53

Closed
wants to merge 2 commits into from
Closed

Armored Slates #53

wants to merge 2 commits into from

Conversation

j01tz
Copy link
Member

@j01tz j01tz commented Apr 26, 2020

@yeastplume
Copy link
Member

Looks good! Couple of questions

  • There was some discussion earlier about having a 'return address' or 'method' encoded at some point at a higher level than the slate itself (I think the slatepack idea included this?). Is this still envisaged here or somewhere else?
  • I think the concept of 'encrypting with the wallet's ed25519 public key (aka tor address as mentioned here WIP: e2e encrypted slates over http(s) #50) is something we'd all like to support, however this would obviously relate to the payload rather than the armoring. Perhaps this concept should be merged into the slate serialization PR?

Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @j01tz ! Very readable and detailed RFC.
I did a quick typo check. Other than that I have few questions (maybe already answered on Keybase during the previous discussions).

  • Any reason why we want to break the slate pack into chunk? I don't see anyone checking manually the characters for a typo.
  • Why the need for the dot notation (or at least the third dot)?
  • Finally, and that's just an idea, why not use bech32 encoding for the slate? We would have the error detection properties? The example slate would be:
grin1qqzqqqmc6zlet7v4fuug399m4tfhwflhqyrqqqqqqp8mzqzqqqqqqqqqdt8uqqgqqdxp6tf8j72q5xvckmadcxkk5k4yak6rug5qathu5wvw8ejxjznhvqmldvulrxdcachv95q3yfguffx9y3nlqfwwah8h9mr7d3tqkktn0qqq74c4hu

which is a bit larger than the example (183 vs 152 characters).

text/0000-armored-slates.md Outdated Show resolved Hide resolved
text/0000-armored-slates.md Outdated Show resolved Hide resolved
text/0000-armored-slates.md Outdated Show resolved Hide resolved
text/0000-armored-slates.md Outdated Show resolved Hide resolved
text/0000-armored-slates.md Outdated Show resolved Hide resolved
@j01tz
Copy link
Member Author

j01tz commented Apr 27, 2020

Thanks @yeastplume

There was some discussion earlier about having a 'return address' or 'method' encoded at some point at a higher level than the slate itself (I think the slatepack idea included this?). Is this still envisaged here or somewhere else?

This was discussed to an extent to be included at the armor layer j01tz/slatepack#2 but after feedback seems to be a better fit for the slate object or the serialization layer. It wasn't planned for inclusion as I was trying to keep things as simple as possible and add features only as necessary. The serialization layer does seem to be the most natural fit (though adds some complexity at serialization that could otherwise be fairly simple).

To accomplish this at serialization layer we may need something like a Payload and Header packet, similar to what was proposed in the original SlatePack idea. Does this seem worth pursuing if the cost is added complexity for serialization?

I think the concept of 'encrypting with the wallet's ed25519 public key (aka tor address as mentioned here #50) is something we'd all like to support, however this would obviously relate to the payload rather than the armoring. Perhaps this concept should be merged into the slate serialization PR?

Similar to above, if we complicate serialization a bit by breaking the data up into multiple packets (Payload and Header) we could include information in the Header so that wallets can be prepared to handle this feature right away.

If the above features make sense to add, we may need to add a fair amount of complexity to serialization (which I'm not entirely opposed to- it would essentially be the original SlatePack proposal using our custom binary serialization instead of MessagePack serialization).

Thanks @quentinlesceller

Any reason why we want to break the slate pack into chunk? I don't see anyone checking manually the characters for a typo.

This is done for human aesthetics when viewing on screens. It (subjectively) looks better when using big blocks of text across large and small screen sizes. If slates are compact enough though the word spacing might not be as helpful here. Is your intuition that armored slates might look and handle better without spacing?

Why the need for the dot notation (or at least the third dot)?

This is mostly for convenience to accurately capture the framing and payload for sanitizing. The third dot helps with trailing characters that sometimes attach when copying long strings in various applications that could be confused as part of the footer. They are not strictly necessary though, just a convenience. Do you think the format would be cleaner and as easy to handle without the dots?

Finally, and that's just an idea, why not use bech32 encoding for the slate? We would have the error detection properties?

One of the reasons SimpleBase58Check was chosen is because it is already known and simple to implement in most languages (though the bech32 implementation seems light enough). Bech32 could be an attractive option if it "feels" lighter when representing slate data (even though more characters, with reduced framing, no whitespaces, and being more familiar for some users it might "feel" better).

Bech32 would also provide much better error checking (base58check doesn't actually have any error detection guarantees, I should probably clarify this in the RFC). Unfortunately we don't gain the same QR efficiency benefits because QR slates would be encoded directly from the serialized slate and not the armored slate (as currently envisioned). However this means that the efficiency losses from simplebase58check to bech32 would be less significant.

I want to spend some time thinking about bech32, it seems to have potential here and after looking at the rust implementation is less complex than I remember.

My initial thought is that the added complexity and length do not give us the same benefits as bitcoin receives by adopting this, but I really like the possibility of condensing slates into "bech32 strings" instead of the clunkier feeling "ascii armor strings". A concern is that the "framing" in this case is not robust enough to handle multi part messages (though we may ultimately not want to support multi part slate messages at all).

@j01tz
Copy link
Member Author

j01tz commented Apr 27, 2020

Comparison between encoding the same slate with bech32:

grin1qqzqqq6ta2gmsxdcfuqgr7f9kmkas9fhqgqqyqqzy4qce5terzrse3a7td3fe4l9xxywpr0mdvzreugjzjh3uaajaees8ydhjfe0efe76dq7pspnjdtendzf85e8xpj2fskkl2wpcszsyuwvqyp3tqtan7smq6ufc8kfsapgn5eedtyl54hmc0y3ds3twmlejawtkhgzcr49czv7fp08dvu6gya6tsfdn6ldeu7es4kz9d6slghfcz8c4zmvp6jupx0yshnkkwdyzwa9cykea0ku70vc2mpzkag05t5upru23dj4zhnuem7maz99d4u8af3cvsg9swufkgrsetan9kwdcx9vlt972vqsqqgpqqynua6zew0hgr4u74gcltm8dknc9x2xcav6evz46dfysxkfppdj7nqqqqqqqqqqq23mtuuq37vd6nf6h6snwwjvtaqvk63jctlgnufdqrf0mz4phde9hmlf5jpqvcttyfh7f03hdp0v9avghwscp4zc9la26y3ft32v6jdyxyyuzjmtevq7esdcupt4lq9kwl4qy2yczgwdyl0dys0zny7jsa8fnyl4xfsa4nlxg5yjtu8v98tt05ak0zmj7yr0p4na4l899mwtygvhrh6zw4e8qwg20fyf827v5a3gyyt2rqx9ftfwr226r9r494s7w87rr723r7n7gehc53cgpg9uwhzeds99fgj7llupqtvqfuse6zcyv397jg3wxjkgjxtqy7t9cl44jpj5e5pn3e8hfrssah8mgw83q3xdq42zt2eq6d0kj5hwn3486wjquwjrk5fe9fhpup6a4up4utw7cksrm9yl89nhlr72pn4dmax7qqwq85xnyxjtj5s3d6w0kv0p903uwljjlwr5v6n05t3esrr87w7znw308rhy7gpfsqshtq8etvr36r48z3k5dt02gzccpj8u3aremfmvdjff5g4pylkcgwrafg00ycj2g5tnmygjruq20yq7sfd7qnl8k72hjvfq82ygjelrwhccfnlp530ed84kndne0w7jtl2celfweeyt0wfl5elwtcvxd8e6hrmxvwhx7s6x6v2w3d7sx9c7e43zgqv85cvwgylwnpk62lu8ga3vrs8lrssqlxjx9wcwux60yewyx7frvngh62nvu0m57nkplynjr2qwyutpjx3r33r8p7k903gapdxwqyy6ee3l45v8c7hs5xdkfrzvk3fjyk9g9sf74lz02qddpqqyfdrzgq8dxsgw9jpz3apfne6jejwunz68m89t05f8hn588t6v5w0ppaer8u83hxre0gweevrtwaxuy4x66dhtm5e5h86vt76eprxp6azvl4pegkcs8ca5xmy8503xuxu9yemg6lqx5gj4m2pf8gs5max8yvmgme9trknd4e9lenlzfs6zrv3tfrzxptqgvr2fl0fartnw7qgxcj7pqzq5v508lwyn5vrxhaydmde2a3gn09h88yuet0782ntxpmjsufuw26

or simplebase58check + framing:

BEGINSLATEPACK. gFiKaEQYKRJu2C1 tqoYgqt6NfMLieE wXWSe186Mi7AUQ6 KvMWUk7wDBDRqZU T3n7eKcHzj7W1CD 4B6zbwZoynadgwH tM6fGJQtFu9yKS3 8EPSHZj2nrSJqQz UTNaPC2NGwWb5is xpbK4odKMLngVJm QNc6m1SM7KvCNRK XkBRFhLPZTEx2t2 2WLjqNsZFvNeQLe jofEkxm3K24csz4 W2uuNA6RSPineET cah4cUWLVqUqUeA 2Qi9Z15o1bbQ9EL 8JMr4nEhav2kPcX 8MH9h1GASX74xNv SPGCVAYrGt1oRHm q84FdJiSAd6PB3x phE65C79oirW93q fqi44FXbwnZzCxJ the7U4mzQNjzNxx ehXy4BVjgPzMyNf h5rA2yWmrHtzqax JW2pjU1T2AF6FLR MmdETC2mYGUvFLz jt3WM8p1X95wv8Z 9MFtgKbujvBV9Nt n8n2W6EAv7misuS uUyJHCaeyaQGtZX wUaqP1thmc9atbQ 6wKRApAErVERvm1 7r2aipvQcvRipFY 4egsqwe516SnNV7 46t2ohaWq3XWEaq 2esvZCx1LKU8eUW NG4EU1WjEXpUfBU MwhhcNpfLbYqLN3 L7pn58eXv5GzhUE mm6B6ui5j14ciMr bhmUXHubhosL3vV BeTJHUi5p9ieE3t wvtu5gzVgXLWmc6 j5DKdWFxMUTLWkt 8ZJQ9wd4sSAUYZa cjnfSoNGhma7SKb JqKv56sCQPZMQiR Z58gF5fF8zNJ8i9 pXB5UPWYEvnEw3Y Fe3WC8esaVwgfYj qqSDgT5MA1ECJdw pSqj4yapJ4YHqrP zY384kBLypdnk8A FTkorpqycqSK3hT GQ6VfHpD7Dxwj9b 4VGU9rrz8TTBppC UzDF5pdW5G2nMZn CTy246iW1bebWMU 31XHL3egWWi7YEj hqsdbet3dtK4qDj bebLHjJBPosQmXq iyWcRSUaWsaDgi8 iQduC8SippvYkcE YudbxJn7zk6ZSYF krxKdi2tSZwq5Xc PuLSmZ6rL2n9vUn 5gt1yCEnJaXCsH5 LGXcAzD2WSsQHbU Rhaj8urGM2FCXWd qGwB8pu9yEhmgND uV9Kt3VgCrGfaAr Rk2BaSd1nbi3gzE rwB2nV2cazbaC5x 3gAtKY4sswA1WsR Z3p52XbK8WjPyxb Ym68EtwQkjAZMm4 bDdB4erjF3XwFn1 T5hgLaFoFymXhbz zejjAdHrRemeHph SW9y4JrZhmsr3fV cUiH32krUVNCqN3 gThkBZNcwqKHmdS tTXBmMi3UzGFHxp 4jM41cVme8R2cU1 iL. ENDSLATEPACK.

Note that formally bech32 is limited to 90 characters to retain its error check properties but could be increased by increasing size of checksum.

We should probably do some more formal reasoning here about error correction anyway because in simplebase58check as specified there is no size limit and the error checking properties weaken as the payload increases. Fortunately for Grin the stakes are lower because the wallet must further sanitize and validate the data before a valid spend can occur.

@lehnberg lehnberg added the wallet dev Related to wallet dev team label Apr 27, 2020
@yeastplume
Copy link
Member

@j01tz Thanks, apologies that I've been so deep into the slate compacting that I might have missed a few conversations, but since that's hopefully coming to a close I hope to spend a bit more time focusing on this.

After some thought, I agree that armoring should concentrate on armoring and nothing more. I also think the slate object should remain focused on the details needed at each stage of a transaction, and we should resist bloating there. So I think that leaves us wanting an optional layer between the slate and the armoring.

At the moment, I'm thinking about a slatepack like the following, similar to the original serialized slates RFC. Like slates, the JSON is still the first-order version, but serialized to binary for armoring/QR very similarly to how the slate itself is serialized.

{                                
  "slatepack": 1,                                
  "return" : "http://listening_grin_wallet_url|tor|relay"                                
  "pubkey": "3a425bd5da5f0f78593251ede7fad0ecf7a95679d84b2cb405255d97ce068234",                                
  "mode": 0|1|2 etc                                
  "payload": {                                
      <either base64 encoded/encrypted slate or the slate itself depending on mode>                                      
  }                                
}                                

slatepack - file identifier + slatepack version
ret_addr - return method + address

  • (Optional) Return method + address to send response to.
    ret_pubkey·

  • Empty (not specified) means response should not be encrypted

  • If specified and ret_addr is tor, use as tor address to send response

  • If specified and ret_addr is not tor, recipient should encrypt response with this key using 'mode'
    mode·

  • 0 - No encryption, assumed default if mode missing

  • 1 - Encrypted ed25519 etc with recipient's wallet address

  • 2 - More encryption methods defined over time if needed

  • return, and pubkey can potentially be stripped during S2/I2 depending on response type, or slate can just be sent back directly to specified address or relay?

  • note that wallet addresses are along a derivation path, so it should be possible to generate a one-time pubkey address. Should this include an (encrypted) derivation path?

  • Wallet foreign listener can add a separate function to receive packed/encrypted slates

  • Perhaps the armoring then gets a flag that specifies whether the payload is a slate or slatepacked?

Haven't put hours of thought into this just yet or what the exact fields should be, but do you think this kind of approach makes sense?

@j01tz
Copy link
Member Author

j01tz commented Apr 28, 2020

do you think this kind of approach makes sense?

The overall approach of avoiding complexity in the slate object and armor by building this from a new first-order JSON seems to align with the general direction of the previous work. In that lens it feels better than building custom binary packets as in the original slate-serialization RFC draft 👍

So I think that leaves us wanting an optional layer between the slate and the armoring.

Perhaps the armoring then gets a flag that specifies whether the payload is a slate or slatepacked?

Why not require slatepacking as you described for all offline/async methods? It may reduce overall complexity (is this is slate or a slatepack?) and wouldn't add much size if most of the fields are optional (maybe a word or two for armor?).

Some thoughts on overall scope: if we are adding a new layer here between the slate objects and the armoring, it sounds like it will probably need its own RFC? As I'm looking at what we have and what we are trying to cover, I'm wondering between slate-serialization, armored-slates, compact-slates(v4) and a potentially new slatepacking RFC, if it is possible or even sensible to condense things.

A bit of a reach, but just thinking out loud: if we go the route of requiring that all offline/async slates that leave the wallet in any form (string/QR/file) to be first serialized as binary, slatepacked and armored (or at least serialized as binary and slatepacked), we could end up with a simpler overall standard for offline/async slate handling.

These "raw slatepack/armored slatepack" slates could then be exchanged as a string, a file or encoded directly to QR: all possible methods of representing offline/async slate data would decode back into the same format: binary serialized, slatepacked and potentially ascii armored.

Though it might just be simpler to add a --slatepack option to get the extra data we might need batched up in the format you described before transport and call it a day.

@j01tz
Copy link
Member Author

j01tz commented Apr 30, 2020

Just wanted to share this thought as an extension to my comment above:

Instead of expecting wallets and services to support many possible transaction types:

  • Tor
  • Armored JSON serialized slate string
  • Armored binary serialized slate string
  • Armored slatepacked JSON serialized slate string
  • Armored slatepacked binary serialized slate string
  • Armored JSON serialized slate file
  • Armored binary serialized slate file
  • Armored slatepacked JSON serialized slate file
  • Armored slatepacked binary serialized slate file
  • JSON serialized slate file
  • Binary serialized slate file
  • Slatepacked JSON serialized slate file
  • Slatepacked Binary serialized slate file

To improve the way users and developers can think about transactions we might be able to simplify transaction options into two basic options:

  • Tor
  • Slatepack (string or file)

Choice of armor, serialization, metadata inclusion could all be reduced to a single default slatepack format that can be stored as a string or file.

A slatepack would be:

  1. serialize slate payload to binary
  2. build "slatepack metadata" JSON object containing payload from step one and optional metadata for version return pubkey etc.
  3. serialize output from step two to binary
  4. armor the output from step three to build a complete "slatepack"
  5. The slatepack can be exchanged directly as a string or written to a .slatepack file

The main differences to how things are currently done:

  • when building slates for use in async tx methods a binary serialization would always be used
  • binary serialized slate data would be included as a payload in a json object containing optional metadata which is serialized to binary and used instead of a slate directly serialized to binary
  • all slate data used in async transactions would be armored before leaving the wallet
  • data written in files would be a slatepack instead of the raw slate binary or JSON serialization
  • async transactions would all use grin-wallet send -m slatepack which would accept optional -d if user wants the slatepack written to a file instead of printing the string, along with other optional flags to include metadata that are yet to be specified

The main negative I can see is the extra data required to build the slatepack transaction (metadata placeholders in the slatepack that may otherwise not be desired for a given tx). By eyeballing it seems like this would only result in an extra word or two in an armored slate.

Note that these thoughts are mostly from a UX/external DX lens, I'm not sure if this would actually make the codebase as written today any simpler or not. Curious for any thoughts from those that have spent more time in this area, if this would make things easier for users and wallet developers to grok and adopt.

@j01tz
Copy link
Member Author

j01tz commented Apr 30, 2020

Another point to mention: if QR codes are expected to be serialized from and deserialzied to a slatepack as described above, some efficiency will be lost on encoding because instead of binary serialized slate + metadata -> QR the encoding process would be binary serialized slate + metadata -> ascii armor -> QR.

Both encodings to QR would be done with the QR byte encoding mode and more bytes would be required to represent the ascii armored data bytes than the raw pre-armor bytes. As a result we may lose ability to encode small slates to static QR codes.

A possible solution is to use a WeakBech32 encoding (the weak part just allows more than 90 characters) instead of SimpleBase58Check encoding as @quentinlesceller pointed out above.

This would allow the ascii armored string representing the slatepack to be encoded more efficiently as a QR at the cost of losing efficiency as a string. The result is a ~>30% efficiency gain for QR encoding at the cost of a ~>10% efficiency loss for string encoding for an overall ~20% more data able to fit in a given QR (based on output from example above).

This is because WeakBech32 encoded slatepacks could use the QR alphanumeric encoding mode which has a max characters a 40-L code can contain of 4296 as opposed the QR byte encoding mode which has a max characters per line of 2953.

If the above "default" slatepack model is considered for adoption and QRs are seen as an important async transfer channel it might be worthwhile to consider the WeakBech32 encoding option before finalizing this RFC.

@yeastplume
Copy link
Member

yeastplume commented May 1, 2020

Just wanted to share this thought as an extension to my comment above:

Instead of expecting wallets and services to support many possible transaction types:

  • Tor
  • Armored JSON serialized slate string
  • Armored binary serialized slate string
  • Armored slatepacked JSON serialized slate string
  • Armored slatepacked binary serialized slate string
  • Armored JSON serialized slate file
  • Armored binary serialized slate file
  • Armored slatepacked JSON serialized slate file
  • Armored slatepacked binary serialized slate file
  • JSON serialized slate file
  • Binary serialized slate file
  • Slatepacked JSON serialized slate file
  • Slatepacked Binary serialized slate file

To improve the way users and developers can think about transactions we might be able to simplify transaction options into two basic options:

  • Tor
  • Slatepack (string or file)

Choice of armor, serialization, metadata inclusion could all be reduced to a single default slatepack format that can be stored as a string or file.

A slatepack would be:

  1. serialize slate payload to binary
  2. build "slatepack metadata" JSON object containing payload from step one and optional metadata for version return pubkey etc.
  3. serialize output from step two to binary
  4. armor the output from step three to build a complete "slatepack"
  5. The slatepack can be exchanged directly as a string or written to a .slatepack file

The main differences to how things are currently done:

  • when building slates for use in async tx methods a binary serialization would always be used
  • binary serialized slate data would be included as a payload in a json object containing optional metadata which is serialized to binary and used instead of a slate directly serialized to binary
  • all slate data used in async transactions would be armored before leaving the wallet
  • data written in files would be a slatepack instead of the raw slate binary or JSON serialization
  • async transactions would all use grin-wallet send -m slatepack which would accept optional -d if user wants the slatepack written to a file instead of printing the string, along with other optional flags to include metadata that are yet to be specified

Yes, I think I can get behind all of this.

The main negative I can see is the extra data required to build the slatepack transaction (metadata placeholders in the slatepack that may otherwise not be desired for a given tx). By eyeballing it seems like this would only result in an extra word or two in an armored slate.

Worth experimenting with the Weakbech32 encoding mentioned in the other comment, but I don't think this would add much to the binary slate in the minimal case. Similar to how the slate size can get larger if you want a payment proof, more outputs etc, the metadata object would grow if you wanted to include a return address, encrypt the payload, etc, but for rounds 1 and 2 in most cases we should be well within QR and tolerable cut/paste behaviour.

Note that these thoughts are mostly from a UX/external DX lens, I'm not sure if this would actually make the codebase as written today any simpler or not. Curious for any thoughts from those that have spent more time in this area, if this would make things easier for users and wallet developers to grok and adopt.

Implementing this shouldn't affect any current send/recieve functionality, since the slatepacking would be handled at the extremities. The core libs would continue to work exclusively on VersionedSlates as they currently do. We'd likely provide some OwnerAPI functions to create and read slatepack and any encryption, then just call them at the points where we're currently reading/writing slate files (actually all nicely factored into a single location in the code at the moment).

@j01tz j01tz mentioned this pull request May 8, 2020
@j01tz
Copy link
Member Author

j01tz commented May 12, 2020

Closing in favor of #55

@j01tz j01tz closed this May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wallet dev Related to wallet dev team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants