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

Inconsistent revision of FW between TT and T1 #1620

Closed
tomasklim opened this issue May 19, 2021 · 4 comments · Fixed by #1624
Closed

Inconsistent revision of FW between TT and T1 #1620

tomasklim opened this issue May 19, 2021 · 4 comments · Fixed by #1624
Labels
bug Something isn't working as expected
Milestone

Comments

@tomasklim
Copy link
Member

Describe the bug
On T1 the revision of firmware is in UTF-8/ASCII.
However, on TT the revision of firmware consists from raw bytes and after hex encoding to UTF-8 I get the format I have on T1 from beginning.

Firmware version and revision
T1: 1.10.0 & 4424ece1ccb7fc0d6cad00ff840fac287a34f07
TT: 2.3.6 & 623139636266363763

Expected behavior
I would expect to have these revisions united not only by encoding but also by its length.

@tomasklim tomasklim added the bug Something isn't working as expected label May 19, 2021
@prusnak
Copy link
Member

prusnak commented May 19, 2021

TT: 2.3.6 & 623139636266363763

This is correct. Trezor returns the revision in bytes field, which got encoded in javascript to hex string.

623139636266363763 => b"b19cbf67c" => valid git revision for firmare 2.3.6 = b19cbf6

T1: 1.10.0 & 4424ece1ccb7fc0d6cad00ff840fac287a34f07

This, however, seems broken.

@matejcik
Copy link
Contributor

this is probably a copy-paste error, the right revision is f4424ece1ccb7fc0d6cad00ff840fac287a34f07 -> f4424ec

Anyway this is a mess in any case. The revision field is bytes. T1 sends the full hash of the commit, while TT sends string output of git describe.

We should:

  • either flip the field to string, but that seems like a backwards compatibility can of worms,
  • or send bytes from core as well, even if it's just a byte encoding of the short version

@prusnak
Copy link
Member

prusnak commented May 20, 2021

I am all for consistency here.

Using string has the advantage of being able to express stuff such as b19cbf67c-dirty, but I am not sure we are really in desperate need of this.

So maybe sending the revision as binary bytes makes sense the most and we can decide whether we want to send the whole thing (20 bytes) or just a prefix to save some space.

@tsusanka tsusanka added this to the 21.06 milestone May 20, 2021
@tsusanka
Copy link
Contributor

As discussed we want to change TT behavior to T1 behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants