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

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

Open
SomberNight opened this issue Aug 8, 2022 · 13 comments

Comments

@SomberNight
Copy link
Contributor

Unfortunately Google made breaking changes in protobuf (see e.g. here), and hence the current _pb2.py generated files in latest release/master of keepkeylib cannot be parsed with new versions of protobuf (4.x).

AFAICT protoc >=3.19 generates _pb2 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 can only parse the new format
user@user-VirtualBox:~/wspace/tmp$ git clone https://github.com/keepkey/python-keepkey
Cloning into 'python-keepkey'...
remote: Enumerating objects: 8669, done.
remote: Counting objects: 100% (54/54), done.
remote: Compressing objects: 100% (37/37), done.
remote: Total 8669 (delta 31), reused 33 (delta 17), pack-reused 8615
Receiving objects: 100% (8669/8669), 56.08 MiB | 606.00 KiB/s, done.
Resolving deltas: 100% (3780/3780), done.


user@user-VirtualBox:~/wspace/tmp$ python3 -m venv env
user@user-VirtualBox:~/wspace/tmp$ source env/bin/activate

(env) user@user-VirtualBox:~/wspace/tmp$ cd python-keepkey/
(env) user@user-VirtualBox:~/wspace/tmp/python-keepkey$ git log -n1
commit 7bddc11d7ee8208161d8b428ad5b6760dd450a98 (HEAD -> master, origin/master, origin/HEAD)
Merge: 4054710 866fc63
Author: markrypt0 <[email protected]>
Date:   Sun Aug 7 07:34:19 2022 -0600

    Merge pull request #145 from keepkey/xpub-support
    
    Xpub support

(env) user@user-VirtualBox:~/wspace/tmp/python-keepkey$ pip install -e .
Obtaining file:///home/user/wspace/tmp/python-keepkey
  Preparing metadata (setup.py) ... done
Collecting ecdsa>=0.9
  Using cached ecdsa-0.18.0-py2.py3-none-any.whl (142 kB)
Collecting hidapi>=0.7.99.post15
  Using cached hidapi-0.12.0.post2-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (892 kB)
Collecting libusb1>=1.6
  Using cached libusb1-3.0.0-py3-none-any.whl (62 kB)
Collecting mnemonic>=0.8
  Using cached mnemonic-0.20-py3-none-any.whl (62 kB)
Collecting protobuf>=3.0.0
  Using cached protobuf-4.21.4-cp37-abi3-manylinux2014_x86_64.whl (408 kB)
Collecting six>=1.9.0
  Using cached six-1.16.0-py2.py3-none-any.whl (11 kB)
Requirement already satisfied: setuptools>=19.0 in /home/user/wspace/tmp/env/lib/python3.10/site-packages (from hidapi>=0.7.99.post15->keepkey==7.0.3) (59.6.0)
Installing collected packages: libusb1, six, protobuf, mnemonic, hidapi, ecdsa, keepkey
  Running setup.py develop for keepkey
Successfully installed ecdsa-0.18.0 hidapi-0.12.0.post2 keepkey-7.0.3 libusb1-3.0.0 mnemonic-0.20 protobuf-4.21.4 six-1.16.0

(env) user@user-VirtualBox:~/wspace/tmp/python-keepkey$ python ./helloworld.py 
Traceback (most recent call last):
  File "/home/user/wspace/tmp/python-keepkey/./helloworld.py", line 4, in <module>
    from keepkeylib.client import KeepKeyClient
  File "/home/user/wspace/tmp/python-keepkey/keepkeylib/client.py", line 37, in <module>
    from . import mapping
  File "/home/user/wspace/tmp/python-keepkey/keepkeylib/mapping.py", line 1, in <module>
    from . import messages_pb2 as proto
  File "/home/user/wspace/tmp/python-keepkey/keepkeylib/messages_pb2.py", line 17, in <module>
    from . import types_pb2 as types__pb2
  File "/home/user/wspace/tmp/python-keepkey/keepkeylib/types_pb2.py", line 34, in <module>
    _descriptor.EnumValueDescriptor(
  File "/home/user/wspace/tmp/env/lib/python3.10/site-packages/google/protobuf/descriptor.py", line 755, in __new__
    _message.Message._CheckCalledFromGeneratedFile()
TypeError: Descriptors cannot not be created directly.
If 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.
 2. Set PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python (but this will use pure-Python parsing and will be much slower).

More information: https://developers.google.com/protocol-buffers/docs/news/2022-05-06#python-updates
(env) user@user-VirtualBox:~/wspace/tmp/python-keepkey$ 

related: BitBoxSwiss/bitbox02-firmware#951


I would suggest

  1. using latest protoc to re-generate the _pb2.py files
    $ mkdir -p /opt/protoc && \
      curl -L0 https://github.com/protocolbuffers/protobuf/releases/download/v21.2/protoc-21.2-linux-x86_64.zip -o /tmp/protoc-21.2-linux-x86_64.zip && \
      unzip /tmp/protoc-21.2-linux-x86_64.zip -d /opt/protoc
    $ PATH="/opt/protoc/bin:$PATH"
    $ ./build_pb.sh
    
  2. bumping required protobuf in setup.py to "protobuf>=3.20"
    'protobuf>=3.0.0',
  3. then releasing a new version of this library on PyPI
    • btw, I see there has been multiple git tags since the last PyPI release (6.3.1 in Dec 2019). Please also release there.
@SomberNight
Copy link
Contributor Author

What is the state of this? A reply would be nice. :)

Note: for continued support of the keepkey plugin in Electrum, we need a new release of the library on PyPI with these changes.
If you've lost the keys to the PyPI package, just release under a new name and state it in the README or similar.

@SomberNight
Copy link
Contributor Author

Even if you don't release a new version on PyPI, could you regenerate the pb2 files in the git repo?
I can open a PR for it if you want, but please say so here then.

Otherwise if this library is unmaintained, again, we might have to remove keepkey support from Electrum.

@MrNaif2018
Copy link

CC @markrypt0 @pastaghost (I see they are not in repository's watchers list, so they might miss the issue accidentally)

@pastaghost
Copy link
Contributor

@SomberNight @MrNaif2018 - thanks for tagging me on this issue; we'll give this some attention right away.

@markrypt0
Copy link
Collaborator

There are several reasons this hasn't been updated. First is that it isn't a seamless upgrade in firmware and because it hasn't been a critical issue, nothing has been changed. Second, the current keepkey pypl account credentials are not available so this is a problem that needs to be fixed.

@MrNaif2018
Copy link

@markrypt0 As for the latter: I think it's not much of a problem, another pypi package can be created, or even it's possible to install from github
I think it shouldn't be a breaking change anyway but an improvement in speed

@cculianu
Copy link

cculianu commented Aug 23, 2024

BUMP. Can you please just create a new pypi package with latest and with recompiled _pb2.py files? Electron Cash for BCH also would like this else we may drop keepkey support as well.

EDIT: And like @SomberNight said, you can create a new package under a different username on pypi. It's fine.

@MrNaif2018
Copy link

Technically worst case it is even possible to load from a github tag specified in requirements.txt, but indeed nothing stops from just publishing a package under different name

@zquestz
Copy link

zquestz commented Aug 24, 2024

I have a fork that has implemented these changes. Would love some extra eyes and testers.

https://github.com/zquestz/python-keepkey

@PiRK
Copy link

PiRK commented Sep 2, 2024

I have a fork that has implemented these changes. Would love some extra eyes and testers.

https://github.com/zquestz/python-keepkey

I don't have a keepkey device for testing this properly with Electrum ABC or Electron Cash, but I'm getting an error with a basic import

$ pip install https://github.com/zquestz/python-keepkey/archive/9e4af3c0e349eaaa395091b35a7202066fc820af.tar.gz
$ python
Python 3.10.12 (main, Jul 29 2024, 16:56:48) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import keepkeylib
>>> from keepkeylib import client
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/client.py", line 37, in <module>
    from . import mapping
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/mapping.py", line 1, in <module>
    from . import messages_pb2 as proto
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/messages_pb2.py", line 25, in <module>
    import types_pb2 as types__pb2
ModuleNotFoundError: No module named 'types_pb2'

Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Sep 2, 2024
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
@zquestz
Copy link

zquestz commented Sep 2, 2024

I have a fork that has implemented these changes. Would love some extra eyes and testers.
https://github.com/zquestz/python-keepkey

I don't have a keepkey device for testing this properly with Electrum ABC or Electron Cash, but I'm getting an error with a basic import

$ pip install https://github.com/zquestz/python-keepkey/archive/9e4af3c0e349eaaa395091b35a7202066fc820af.tar.gz
$ python
Python 3.10.12 (main, Jul 29 2024, 16:56:48) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import keepkeylib
>>> from keepkeylib import client
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/client.py", line 37, in <module>
    from . import mapping
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/mapping.py", line 1, in <module>
    from . import messages_pb2 as proto
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/messages_pb2.py", line 25, in <module>
    import types_pb2 as types__pb2
ModuleNotFoundError: No module named 'types_pb2'

Will take a look at this shortly. =)

@zquestz
Copy link

zquestz commented Sep 2, 2024

I have a fork that has implemented these changes. Would love some extra eyes and testers.
https://github.com/zquestz/python-keepkey

I don't have a keepkey device for testing this properly with Electrum ABC or Electron Cash, but I'm getting an error with a basic import

$ pip install https://github.com/zquestz/python-keepkey/archive/9e4af3c0e349eaaa395091b35a7202066fc820af.tar.gz
$ python
Python 3.10.12 (main, Jul 29 2024, 16:56:48) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import keepkeylib
>>> from keepkeylib import client
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/client.py", line 37, in <module>
    from . import mapping
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/mapping.py", line 1, in <module>
    from . import messages_pb2 as proto
  File "/home/pierre/.local/lib/python3.10/site-packages/keepkeylib/messages_pb2.py", line 25, in <module>
    import types_pb2 as types__pb2
ModuleNotFoundError: No module named 'types_pb2'

Pushed a fix. Client now imports correctly, had some issues with Mayachain code and the way the imports were generated.

Python 3.12.5 (main, Aug  9 2024, 08:20:41) [GCC 14.2.1 20240805] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import keepkeylib
>>> from keepkeylib import client
>>> client
<module 'keepkeylib.client' from '/home/quest/src/python-keepkey/keepkeylib/client.py'>

@PiRK
Copy link

PiRK commented Sep 3, 2024

Pushed a fix. Client now imports correctly, had some issues with Mayachain code and the way the imports were generated.

OK, my very basic unit test is passing now. Good enough for me, until we find an actual tester with access to a device. Thanks @zquestz!

Fabcien pushed a commit to Bitcoin-ABC/ElectrumABC that referenced this issue Sep 3, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants