nanocurrency / protocol

Implementation independent content related to the Nano protocol
BSD 2-Clause "Simplified" License
36 stars 10 forks source link

Various fixes #24

Closed zhyatt closed 2 years ago

zhyatt commented 2 years ago

NOTE: If any changes need to be made the nano.png file will need to be regenerated and committed

cryptocode commented 2 years ago

According to the node source code, the node accepts telemetry_ack's that are larger than the current one, probably to be lenient during protocol upgrades.

This means that the protocol spec must ignore trailing data, just like the node code, otherwise generated parsers will get out of whack. This can be done by comparing current position to the header's telemetry_size.

Adding the following at the very end of msg_telemetry_ack: should do the trick:

      - id: unknown_data
        type: u8
        repeat: until
        repeat-until: _io.pos == _root.header.telemetry_size
        if: _io.pos < _root.header.telemetry_size
dsiganos commented 2 years ago

@cryptocode You are correct that a telemetry_ack can contain some data at the end of it and that we need something extra to handle that case but I do not know if your proposed solution would work. I do not understand kaitai sufficiently well to say authoritative if your solution would work or not.

I would expect the solution to be using the telemetry_size field. https://github.com/nanocurrency/protocol/blob/b94bb467bf5264603b8a1870524791279988783e/reference/protocol.ksy#L133

The size of any telemetry_ack is determined by the telemetry_size field, which is set in the extensions bits in the header.

cryptocode commented 2 years ago

@dsiganos The proposed solution does use the header field you are referring to. The generated parser seems to be correct.

dsiganos commented 2 years ago

@cryptocode Ah yes, you are right, I misread it. Do you need to apply the mask 0x3ff or is that done automatically by kaitai?

cryptocode commented 2 years ago

@cryptocode Ah yes, you are right, I misread it. Do you need to apply the mask 0x3ff or is that done automatically by kaitai?

Yep, Kaitai's generated parsers does the masking 💪

zhyatt commented 2 years ago

@dsiganos @cryptocode This fell off my radar but I just updated per https://github.com/nanocurrency/protocol/pull/24#issuecomment-936109704

dsiganos commented 2 years ago

This looks good but we also need to document that the 4 LSB bits of the timestamp are now a duration field.

zhyatt commented 2 years ago

This looks good but we also need to document that the 4 LSB bits of the timestamp are now a duration field.

I added a ticket for this here: https://github.com/nanocurrency/protocol/issues/26