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

Phrasing about extensions in RFC 3 can be misleading #352

Closed
sharafian opened this issue Dec 12, 2017 · 8 comments
Closed

Phrasing about extensions in RFC 3 can be misleading #352

sharafian opened this issue Dec 12, 2017 · 8 comments
Assignees
Labels
help wanted ilp Related to the core protocol specs spec debt wontfix

Comments

@sharafian
Copy link

At several points in RFC 3, the spec describes the extension byte at the end of the packet as being "always 0." This could be very misleading to implementors, who might write a parser that expects the extensions byte to be zero. If someone wrote a parser in that way, it would be unable to parse packets where the extensions are actually used. We should instead say that the packet should always be written as 0 for now, but should be entirely ignored while parsing.

@michielbdejong
Copy link
Contributor

@emschwartz
Copy link
Member

It doesn't have the field but I think @justmoon said that was because he realized you didn't need the extra byte there in order to stick extensions on the end later (if I'm right about that, we need to document exactly what the expectations are around passing on extensions)

@adrianhopebailie
Copy link
Collaborator

you didn't need the extra byte there in order to stick extensions on the end later

I don't think that is true. If you are parsing a sequence then there is no leading length indicator to tell you if the sequence is longer than the parts you expect.

If we change the format though we can just introduce new packet type identifiers. The disadvantage being that anyone that encounters them and doesn't support them fails vs extensions which can be parsed but ignored

@sharafian
Copy link
Author

If you are parsing a sequence then there is no leading length indicator to tell you if the sequence is longer than the parts you expect.

That's why we have the envelope, though. You may read the whole sequence inside of the envelope, and then have more bytes before the end of the envelope, which you should discard if you don't understand.

@adrianhopebailie
Copy link
Collaborator

That's why we have the envelope, though.

There is no envelope around that sequence. The only way to know if the packet is bigger is to depend on the transport protocol you are using to envelope the packets.

@sharafian
Copy link
Author

They're all wrapped in https://github.com/interledger/rfcs/blob/master/asn1/InterledgerPacket.asn#L54 , which includes the length prefix and the type ID

@adrianhopebailie
Copy link
Collaborator

Based on the discussion around how to encode an Open Type in OER some of this discussion is incorrect.

There IS a length indicator for the packet data after the packet type code. However there is not an ASN.1 extensions indicator in the spec so decoders should NOT expect to find an extension byte.

If a decoder encounters a packet with new fields added then it should ignore those fields and simply pass the remaining bytes in the packet data on to the next node.

@sharafian 's point is still valid that this needs to be documented somewhere.

@adrianhopebailie adrianhopebailie added the ilp Related to the core protocol specs label Mar 16, 2018
@stale
Copy link

stale bot commented May 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. \n\n If this issue is important, please feel free to bring it up on the next Interledger Community Group Call or in the Gitter chat.

@stale stale bot added the wontfix label May 10, 2018
@stale stale bot closed this as completed May 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted ilp Related to the core protocol specs spec debt wontfix
Projects
None yet
Development

No branches or pull requests

5 participants