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

Slatepack armor formatting #420

Merged
merged 1 commit into from
May 27, 2020
Merged

Slatepack armor formatting #420

merged 1 commit into from
May 27, 2020

Conversation

j01tz
Copy link
Member

@j01tz j01tz commented May 27, 2020

This PR improves support for line formatting for an armored SlatepackMessage as specified in the RFC.

Note that the HEADER is a 'word' for the purposes of formatting (and was specifically chosen to be the same length as the default WORD_LENGTH). Otherwise the first line in the output is off by one word and doesn't look nice (this could be approached by making format_slatepack() smarter instead but this seemed easier given our existing choices and needs).

After thinking through 'multipart' messages more I'd like to propose some armor changes for handling edge cases of large txs (which shouldn't require changes to existing slatepack impl work beyond this PR):

  • Remove support for 'multipart' SlatepackMessage
  • Add default 1MB size limit to SlatepackMessage (enforced during validation of incoming/outgoing SlatepackMessage in workflow functions)
  • Update SlatepackWorkflow to output a txUUID-stage.slatepack file if the SlatepackMessage string is > 1MB

My reasoning for removing support for multi-part messages and limiting messages to 1MB, with a file as the final fallback to cover these edge cases: the UX/DX for receiving multiple string messages is probably just as much trouble if not more than supporting files (neither of which will likely be bothered with by the majority of service/exchange UXs anyway unless we provide the tooling libs). It should be rare to exceed 1MB with compact slates but we will want to keep an eye on this as we test as it may need to be adjusted.

This size limit should just be an adjustable parameter with no hard technical limitation outside of the specified limits in the SlatepackWorkflow (and available memory/restrictions in various apps/clipboards). This should also make implementation easier as it shouldn't require changes to existing code (except some safety/sanity checks- I expect not much would change for services/exchanges as they would not likely support multipart messages nor file fallbacks).

A concern about the above change to the SlatepackWorkflow is that it could potentially further complicate things from a conceptual perspective (armor string output is only the final step in cases where the string would be <= 1MB, otherwise final step is .slatepack armored file output instead). This shouldn't be a problem for 99.9% of txs (source TBD) but slatepack does need to make sure that whatever the final fallback method is can support the largest possible slate data required for a spend in cases where Tor is blocked at the packet level.

Hopefully this would simplify things from a practical/implementation perspective even if it seems to add another step to the conceptual workflow- anyone have other thoughts/feedback there? Is there a good argument for why supporting multipart messages is a better UX/DX than supporting files for edge cases?

If the above makes sense I'll update RFC to reflect these changes- otherwise I'll take a closer to look at adding something like MAX_MSG_SIZE and MAX_MSG_PARTS and avoid trying to pass the complexity of multipart messages to a final file step to handle edge cases.

@yeastplume
Copy link
Member

Okay all good. I had the words per line output set to match what I was doing in my display terminal, but I can see it's derived from saltpack so thanks for making it align with the spec.

Definitely happy to remove the complexity (developmental and UX) of multipart messages in favour of a size limit / outputting to file if that limit is exceeded.

@yeastplume yeastplume merged commit 788d050 into mimblewimble:master May 27, 2020
@lehnberg
Copy link
Collaborator

Definitely happy to remove the complexity (developmental and UX) of multipart messages in favour of a size limit / outputting to file if that limit is exceeded.

Agreed. 👍 Guess we'll have to see how this plays out in the wild, quite likely we'll have to do further tweaks as we learn more from usage. But there's nothing to suggest that multi-part slatepack messages would be less complex to handle for these (hopefully rare) use cases.

antiochp pushed a commit to antiochp/grin-wallet that referenced this pull request Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants