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.

Sign up for GitHub

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

Jump to bottom

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

Open
SomberNight opened this issue Aug 8, 2022 · 4 comments · May be fixed by #9581
Open

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

SomberNight opened this issue Aug 8, 2022 · 4 comments · May be fixed by #9581
Labels
dependencies 🧩
Milestone
backlog

Comments

@SomberNight
Copy link
Member

SomberNight commented Aug 8, 2022 •
edited
Loading

Google made breaking changes in protobuf (see e.g. here), in the generation and parsing of _pb2.py generated files.

Re the python protobuf package, the 3.20.1 release is followed by the 4.21.1 release.

AFAICT protoc >=3.19 generates _pb2.py files in the new format, while older protoc generates the old format.
AFAICT

  • protobuf 3.20.x is able to parse both the old and the new format,
  • protobuf<3.20 can only parse the old format,
  • protobuf>=3.21 (i.e. protobuf>=4) can only parse the new format

We rely on protobuf to parse _pb2.py files at runtime:

  • directly for paymentrequest_pb2.py
  • indirectly via bitbox02 (hww plugin)
  • indirectly via keepkey (hww plugin)
  • Currently, our _pb2.py file(s) is in the old format, and we require protobuf>=3.12,<4.
  • bitbox02 has _pb2.py file(s)
    • for bitbox02<6.1.0, in the old format
    • for bitbox02==6.1.0, in the new format
      • and it requires "protobuf>=3.21" (i.e. "protobuf>=4")
        • related PR
        • would be nice if they relaxed this to accept 3.20.x (done in PR, thanks!)
    • for bitbox02>=6.1.1, in the new format
      • and it requires "protobuf>=3.20"
  • keepkey has _pb2.py file(s) in the old format
    • I've asked them to regenerate in new format and publish a new release
@SomberNight SomberNight added this to the backlog milestone Aug 8, 2022
@SomberNight SomberNight mentioned this issue Aug 8, 2022
Merged
@rt121212121
Copy link

I emailed KeepKey a year or so ago and asked if they could update the PyPI package. Their reply was that they had nothing to do with the Python package and to use Github 🤷‍♂️

@SomberNight
Copy link
Member Author

If they don't have the keys to the PyPI package due to changing maintainers or some other reason, they should release a new package under a different name.
Alternatively we might accept a PR that makes python-keepkey a git submodule in this repo.
Otherwise I am considering removing the plugin...

SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 5, 2022
@SomberNight
requirements: pin protobuf "<4" also in "req..-hw.txt"
449d609 
related:
spesmilo@52b7388
spesmilo#7922
@oxalica oxalica mentioned this issue Oct 7, 2022
Merged
13 tasks
SomberNight added a commit to SomberNight/electrum that referenced this issue Oct 14, 2022
@SomberNight
requirements: pin protobuf "<4" also in "req..-hw.txt"
5f83538 
related:
spesmilo@52b7388
spesmilo#7922
SomberNight added a commit that referenced this issue Jan 28, 2023
@SomberNight
re-generate protobuf _pb2.py files and bump min required protobuf
4f9469b 
upper bound "<4" still needed due to keepkey...

related #7922
@SomberNight
Copy link
Member Author

Switched to new format for our own paymentrequest_pb2.py in 4f9469b.

Something needs to be done with keepkey so that we can drop the protobuf<4 upper bound pin.

PiRK added a commit to PiRK/ElectrumABC that referenced this issue May 11, 2023
@PiRK
bump protobuf and regenerate pb2.py files
0af3bbf 
See spesmilo#7922 for a good description of the problem and the rational for restricting to this range of `protobuf` versions.
tldr; keepkey support restricts us to `protobuf<4` because we need to support both the new and the old format.

This fixes  Bitcoin-ABC#214

This removes a lot of DeprecationWarning messages due to outdated pb2 files.
PiRK added a commit to Bitcoin-ABC/ElectrumABC that referenced this issue May 11, 2023
@PiRK
bump protobuf and regenerate pb2.py files
db2a324 
See spesmilo#7922 for a good description of the problem and the rational for restricting to this range of `protobuf` versions.
tldr; keepkey support restricts us to `protobuf<4` because we need to support both the new and the old format.

This fixes  #214

This removes a lot of DeprecationWarning messages due to outdated pb2 files.
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Sep 2, 2024
@PiRK
[electrum] regenerate protobuf files
28620bd 
Summary:
Update the protobuf files in the newer format supported by protobuf versions > 4. This makes it possible for users with a newer version of protobuf installed to use Electrum ABC.

For now we keep protobuf 3.20 as the official requirement, because this is the only version supporting both the old and new protobuf format. Our dependency to `keepkey` makes it hard to drop support for the old format (see description of the issue here: spesmilo/electrum#7922)

There is ongoing work for trying to make keepkey work with the new protobuf format, but for now I have not been able to use it succesfully. See keepkey/python-keepkey#146

Note that I generated the files with protobuf 5.28 and then manually removed the runtime version check which is not supported by protobuf 3.20
```
diff --git a/electrum/electrumabc_plugins/fusion/fusion_pb2.py b/electrum/electrumabc_plugins/fusion/fusion_pb2.py
index 317bb6e3d5..bc58f947fd 100644
--- a/electrum/electrumabc_plugins/fusion/fusion_pb2.py
+++ b/electrum/electrumabc_plugins/fusion/fusion_pb2.py
@@ -1,11 +1,22 @@
 # -*- coding: utf-8 -*-
 # Generated by the protocol buffer compiler.  DO NOT EDIT!
-# NO CHECKED-IN PROTOBUF GENCODE
 # source: fusion.proto
-# Protobuf Python Version: 5.28.0
 """Generated protocol buffer code."""
 from google.protobuf import descriptor as _descriptor
 from google.protobuf import descriptor_pool as _descriptor_pool
-from google.protobuf import runtime_version as _runtime_version
 from google.protobuf import symbol_database as _symbol_database
 from google.protobuf.internal import builder as _builder
-_runtime_version.ValidateProtobufRuntimeVersion(
-    _runtime_version.Domain.PUBLIC,
-    5,
-    28,
-    0,
-    '',
-    'fusion.proto'
-)
 # @@protoc_insertion_point(imports)

 _sym_db = _symbol_database.Default()
```
I compared the generated paymentrequest file with the one electrum uses and did not find any other discrepancy that could cause an issue with protobuf 3.20

Test Plan:
`python test_runner.py`

Wait for  a successfull fusion.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D16702
Fabcien pushed a commit to Bitcoin-ABC/ElectrumABC that referenced this issue Sep 3, 2024
@PiRK
[electrum] regenerate protobuf files
f3864d2 
Summary:
Update the protobuf files in the newer format supported by protobuf versions > 4. This makes it possible for users with a newer version of protobuf installed to use Electrum ABC.

For now we keep protobuf 3.20 as the official requirement, because this is the only version supporting both the old and new protobuf format. Our dependency to `keepkey` makes it hard to drop support for the old format (see description of the issue here: spesmilo#7922)

There is ongoing work for trying to make keepkey work with the new protobuf format, but for now I have not been able to use it succesfully. See keepkey/python-keepkey#146

Note that I generated the files with protobuf 5.28 and then manually removed the runtime version check which is not supported by protobuf 3.20
```
diff --git a/electrum/electrumabc_plugins/fusion/fusion_pb2.py b/electrum/electrumabc_plugins/fusion/fusion_pb2.py
index 317bb6e3d5..bc58f947fd 100644
--- a/electrum/electrumabc_plugins/fusion/fusion_pb2.py
+++ b/electrum/electrumabc_plugins/fusion/fusion_pb2.py
@@ -1,11 +1,22 @@
 # -*- coding: utf-8 -*-
 # Generated by the protocol buffer compiler.  DO NOT EDIT!
-# NO CHECKED-IN PROTOBUF GENCODE
 # source: fusion.proto
-# Protobuf Python Version: 5.28.0
 """Generated protocol buffer code."""
 from google.protobuf import descriptor as _descriptor
 from google.protobuf import descriptor_pool as _descriptor_pool
-from google.protobuf import runtime_version as _runtime_version
 from google.protobuf import symbol_database as _symbol_database
 from google.protobuf.internal import builder as _builder
-_runtime_version.ValidateProtobufRuntimeVersion(
-    _runtime_version.Domain.PUBLIC,
-    5,
-    28,
-    0,
-    '',
-    'fusion.proto'
-)
 # @@protoc_insertion_point(imports)

 _sym_db = _symbol_database.Default()
```
I compared the generated paymentrequest file with the one electrum uses and did not find any other discrepancy that could cause an issue with protobuf 3.20

Test Plan:
`python test_runner.py`

Wait for  a successfull fusion.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D16702
roqqit pushed a commit to doged-io/doged that referenced this issue Nov 20, 2024
@PiRK @roqqit
[electrum] regenerate protobuf files
4187ca7 
Summary:
Update the protobuf files in the newer format supported by protobuf versions > 4. This makes it possible for users with a newer version of protobuf installed to use Electrum ABC.

For now we keep protobuf 3.20 as the official requirement, because this is the only version supporting both the old and new protobuf format. Our dependency to `keepkey` makes it hard to drop support for the old format (see description of the issue here: spesmilo/electrum#7922)

There is ongoing work for trying to make keepkey work with the new protobuf format, but for now I have not been able to use it succesfully. See keepkey/python-keepkey#146

Note that I generated the files with protobuf 5.28 and then manually removed the runtime version check which is not supported by protobuf 3.20
```
diff --git a/electrum/electrumabc_plugins/fusion/fusion_pb2.py b/electrum/electrumabc_plugins/fusion/fusion_pb2.py
index 317bb6e3d5..bc58f947fd 100644
--- a/electrum/electrumabc_plugins/fusion/fusion_pb2.py
+++ b/electrum/electrumabc_plugins/fusion/fusion_pb2.py
@@ -1,11 +1,22 @@
 # -*- coding: utf-8 -*-
 # Generated by the protocol buffer compiler.  DO NOT EDIT!
-# NO CHECKED-IN PROTOBUF GENCODE
 # source: fusion.proto
-# Protobuf Python Version: 5.28.0
 """Generated protocol buffer code."""
 from google.protobuf import descriptor as _descriptor
 from google.protobuf import descriptor_pool as _descriptor_pool
-from google.protobuf import runtime_version as _runtime_version
 from google.protobuf import symbol_database as _symbol_database
 from google.protobuf.internal import builder as _builder
-_runtime_version.ValidateProtobufRuntimeVersion(
-    _runtime_version.Domain.PUBLIC,
-    5,
-    28,
-    0,
-    '',
-    'fusion.proto'
-)
 # @@protoc_insertion_point(imports)

 _sym_db = _symbol_database.Default()
```
I compared the generated paymentrequest file with the one electrum uses and did not find any other discrepancy that could cause an issue with protobuf 3.20

Test Plan:
`python test_runner.py`

Wait for  a successfull fusion.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D16702
@EchterAgo
Copy link
Contributor

@SomberNight see Electron-Cash#3029 I tested it with my keepkey hardware

SomberNight added a commit to SomberNight/electrum that referenced this issue Feb 24, 2025
@SomberNight
plugins: keepkey: vendor keepkeylib
533ed6c 
This is from PyPI, keepkey==6.3.1 [0] with sha256 hash `cef1e862e195ece3e42640a0f57d15a63086fd1dedc8b5ddfcbc9c2657f0bb1e`,
which is the hash we've had pinned in contrib/deterministic-build/requirements-hw.txt for 5 years.

[0]: https://files.pythonhosted.org/packages/30/38/558d9a2dd1fd74f50ff4587b4054496ffb69e21ab1138eb448f3e8e2f4a7/keepkey-6.3.1.tar.gz

related spesmilo#7922
and keepkey/python-keepkey#146
(i.e. upstream keepkeylib is unmaintained)
SomberNight added a commit to SomberNight/electrum that referenced this issue Feb 24, 2025
@SomberNight
requirements: follow-up prev: rm protobuf upper bound
4032d51 
now that the keepkey pb2's are regenerated using the "new" format,
we don't need an old python3-protobuf to parse them

ref spesmilo#7922
SomberNight added a commit to SomberNight/electrum that referenced this issue Feb 24, 2025
@SomberNight
requirements: follow-up prev: rm protobuf upper bound
321df54 
now that the keepkey pb2's are regenerated using the "new" format,
we don't need an old python3-protobuf to parse them

ref spesmilo#7922
@SomberNight SomberNight linked a pull request Feb 24, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
dependencies 🧩
Projects
None yet
Milestone
backlog
Development

Successfully merging a pull request may close this issue.

plugins: keepkey: vendor keepkeylib SomberNight/electrum
3 participants
@EchterAgo @SomberNight @rt121212121