py/bitbox02/communication: update protobuf generated files by Beerosagos · Pull Request #951 · BitBoxSwiss/bitbox02-firmware

Beerosagos · 2022-07-13T07:45:11Z

Recent HWI update requires protobuf files generated with protoc =>
3.19.0. This update regenerates the files with protoc 3.21.1

EDIT: as @benma pointed out, the issue is not actually related to HWI, which currently requires protobuf 3.19.1.
The compatibility issue is between protobuf >= 3.21 and current protobuf files.

To solve it protobuf files has been regenerated with protoc >= 3.19 and protobuf dependency of bitbox02 py lib set to >= 3.21

benma · 2022-07-13T07:47:15Z

The protobuf files are generated by executing cd py && make (inside Docker), and this should continue working so it's easy to generate the files when changing the protobuf messages.

Please update the Dockerfile accordingly :)

Beerosagos · 2022-07-13T09:48:46Z

The protobuf files are generated by executing cd py && make (inside Docker), and this should continue working so it's easy to generate the files when changing the protobuf messages.

Please update the Dockerfile accordingly :)

Didn't think about that! :| Now should be ok, thanks.

benma · 2022-07-13T19:17:14Z

Recent HWI update requires protobuf files generated with protoc

Are you sure it's HWI? The issue originally was this:

f this call came from a _pb2.py file, your generated code is out of date and must be regenerated with protoc >= 3.19.0.
If you cannot immediately regenerate your protos, some other possible workarounds are:
 1. Downgrade the protobuf package to 3.20.x or lower.

So it was about the protobuf package being newer than 3.20. HWI's dep list uses 3.19.1. So maybe it was something else that upgraded your protobuf package 🤔

Anyway, would be good to include the above in the commit msg :)

benma · 2022-07-13T19:36:16Z

We have a problem maybe. When I now run send_message.py, I get:

from google.protobuf.internal import builder as _builder

My local protobuf installation is at 3.19.4:

Upgrading it fixes it, but that still means it's broken for protobuf <3.20.

At the very least we need to then bump https://github.com/digitalbitbox/bitbox02-firmware/blob/57d62a98cf52fd4d700551083e925019e5eb16f3/py/bitbox02/setup.py#L78. Maybe that's sufficient 🤷

benma · 2022-07-14T08:00:15Z

Please also update py/bitbox02/CHANGELOG.md under 6.1.0 (to be released).

Beerosagos · 2022-07-14T08:23:47Z

We have a problem maybe. When I now run send_message.py, I get:
from google.protobuf.internal import builder as _builder
My local protobuf installation is at 3.19.4:

Upgrading it fixes it, but that still means it's broken for protobuf <3.20.

At the very least we need to then bump

https://github.com/digitalbitbox/bitbox02-firmware/blob/57d62a98cf52fd4d700551083e925019e5eb16f3/py/bitbox02/setup.py#L78
. Maybe that's sufficient shrug

Thanks for pointing this out, didn't notice it! Yes, updating setup.py seems to be sufficient, and a good way to avoid compatibility issues.

Beerosagos · 2022-07-14T08:27:10Z

Recent HWI update requires protobuf files generated with protoc

Are you sure it's HWI? The issue originally was this:
f this call came from a _pb2.py file, your generated code is out of date and must be regenerated with protoc >= 3.19.0.
If you cannot immediately regenerate your protos, some other possible workarounds are:
 1. Downgrade the protobuf package to 3.20.x or lower.
So it was about the protobuf package being newer than 3.20. HWI's dep list uses 3.19.1. So maybe it was something else that upgraded your protobuf package thinking

Anyway, would be good to include the above in the commit msg :)

You are right, I was misled by the fact that both I and the user in #949 encountered the error while working on HWI.

Beerosagos · 2022-07-14T12:05:57Z

I am not really understanding why the CI failed. Made some tests on my local, but can't reproduce the error.

benma · 2022-07-14T12:13:14Z

I am not really understanding why the CI failed. Made some tests on my local, but can't reproduce the error.

I think pylint is just very bad with the member checks with protobuf, they are probably false positives. That's why you see so many # pylint: disable=no-member scattered :( I don't know why more of them are triggered now... Maybe there is a way to disable that warning altogether, otherwise try putting # pylint: disable=no-member and seeing if that helps.

You can lint it locally with ./scripts/docker_exec.sh ./scripts/lint-python .

benma · 2022-07-14T12:30:21Z

Fyi the member check by pylint is not so important because we also have types everywhere, and mypy does a full type check on the package, so mypy would catch any such member mistakes.

Beerosagos · 2022-07-18T09:31:52Z

I am not really understanding why the CI failed. Made some tests on my local, but can't reproduce the error.

I think pylint is just very bad with the member checks with protobuf, they are probably false positives. That's why you see so many # pylint: disable=no-member scattered :( I don't know why more of them are triggered now... Maybe there is a way to disable that warning altogether, otherwise try putting # pylint: disable=no-member and seeing if that helps.

You can lint it locally with ./scripts/docker_exec.sh ./scripts/lint-python .

Thanks for the explanation. I disabled no-member checks for protobuf generated modules.

benma

Could you update https://github.com/digitalbitbox/bitbox02-firmware/blob//.ci/run-container-ci#L28 to version 26? I just pushed the updated Docker image with the new protoc there.

I also accidentally overwrote the 25 tag on Docker hub - will re-build and re-push that one 🙈

benma · 2022-07-19T07:20:47Z

py/bitbox02/CHANGELOG.md


 ## 6.1.0
 - Add `eth_sign_typed_msg()`
+- Update brotobuf dependency to >= 3.21


brotobuf? 😂

benma · 2022-07-19T09:20:02Z

.pylintrc

+
+[TYPECHECK]
+# Ignore protobuf generated modules for no-member checks
+ignored-modules=bitbox02.communication.generated.btc_pb2,bitbox02.communication.generated.cardano_pb2,bitbox02.communication.generated.common_pb2,bitbox02.communication.generated.eth_pb2,bitbox02.communication.generated.hww_pb2


I couldn't figure out how to do this - thanks!!

Please update the commit msg to explain this and that mypy does the member checks instead.

Recent protobuf versions (>=3.21) have compatibility issues with files generated with protoc < 3.19.0. This commit updates Dockerfile to install a recent version of protoc (3.21.2), updates generated protobuf files and increase Docker image version in `run-container-ci`. Since there is also a compatibility issue between protobuf files generated with protoc >= 3.19 and protobuf < 3.21, protobuf dependency of bitbox02 python library has been updated to >= 3.21. Protobuf generated modules have been excluded from pylint no-member checks, because of a large number of false-positive errors. A full type check is done on the package by mypy, anyway. It should be enough.

SomberNight · 2022-08-08T17:05:07Z

py/bitbox02/setup.py

        "hidapi>=0.7.99.post21",
        "noiseprotocol>=0.3",
-        "protobuf>=3.7",
+        "protobuf>=3.21",


AFAICT protobuf 3.20.x is able to parse both the old and the new format
(while <3.20 can only parse the old format, and >=3.21 can only parse the new format)

Hence, would it not be better to set this to "protobuf>=3.20" ?

If you also find this to be correct, please relax the requirement.
See spesmilo/electrum#7922

Interesting, how did you figure this out? The release notes don't exactly give it away.

Anyway, we're happy to make a patch release that relaxes the constraint.

Fyi, protobuf>=3.20 requires Python 3.7, see also: bitcoin-core/HWI#626

Interesting, how did you figure this out? The release notes don't exactly give it away.

When you have new protobuf and old generated files, the runtime error says
1. Downgrade the protobuf package to 3.20.x or lower..
I manually tested a bunch of protobuf versions from PyPI around that boundary, hoping some can parse both and to learn the behaviour.
I had tried reading the release notes first but could not make sense of them either :)

Fyi, protobuf>=3.20 requires Python 3.7, see also: bitcoin-core/HWI#626

That's fine. (in Electrum, we already bumped the minimum to Python 3.8)

EDIT:

Anyway, we're happy to make a patch release that relaxes the constraint.

Thanks, that would be great!

6.1.1 is out, please give it a try.

Beerosagos force-pushed the protobuf-update branch from e1d1f7a to af10ea0 Compare July 13, 2022 09:21

Beerosagos force-pushed the protobuf-update branch from af10ea0 to 1fe857d Compare July 14, 2022 09:07

Beerosagos force-pushed the protobuf-update branch from 1fe857d to 88a5b7e Compare July 18, 2022 09:09

benma approved these changes Jul 19, 2022

View reviewed changes

benma reviewed Jul 19, 2022

View reviewed changes

Beerosagos force-pushed the protobuf-update branch from 88a5b7e to 00d9788 Compare July 19, 2022 13:38

Beerosagos merged commit d4fc023 into BitBoxSwiss:master Jul 19, 2022

benma mentioned this pull request Aug 2, 2022

protobuf and Python 3.6 compatibility problems - drop support for Python 3.6, support 3.7? bitcoin-core/HWI#626

Closed

SomberNight reviewed Aug 8, 2022

View reviewed changes

This was referenced Aug 8, 2022

please regenerate protobuf _pb2.py files using recent protoc, and publish new release on PyPI keepkey/python-keepkey#146

Open

protobuf breaking changes between 3.20.x and 4.21 (4.x) spesmilo/electrum#7922

Closed

Conversation

Beerosagos commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benma commented Jul 13, 2022

Uh oh!

Beerosagos commented Jul 13, 2022

Uh oh!

benma commented Jul 13, 2022

Uh oh!

benma commented Jul 13, 2022

Uh oh!

benma commented Jul 14, 2022

Uh oh!

Beerosagos commented Jul 14, 2022

Uh oh!

Beerosagos commented Jul 14, 2022

Uh oh!

Beerosagos commented Jul 14, 2022

Uh oh!

benma commented Jul 14, 2022

Uh oh!

benma commented Jul 14, 2022

Uh oh!

Beerosagos commented Jul 18, 2022

Uh oh!

benma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomberNight Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Beerosagos commented Jul 13, 2022 •

edited

Loading

SomberNight Aug 9, 2022 •

edited

Loading