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

Bugfix: Device does not have a matching signing key (Trezor) #1977

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

moneymanolis
Copy link
Collaborator

@moneymanolis moneymanolis commented Nov 17, 2022

Fixes #1946 works with bitcoin-core/HWI#644

Supplying an empty string as passphrase if the Trezor is passphrase enabled does not result in an error anymore.

@netlify
Copy link

netlify bot commented Nov 17, 2022

Deploy Preview for specter-desktop-docs canceled.

Name Link
🔨 Latest commit 55a446d
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/63e6a606c8100d00082b8ccd

@stepansnigirev
Copy link
Collaborator

Did you also check it still works:

  • if the password is disabled in Trezor
  • with other hardware wallets i.e. Ledger

@k9ert
Copy link
Collaborator

k9ert commented Dec 5, 2022

I was not able to verify, that this fixes the bug. So this should work without a new HWI-version, right?
So as a workaround, download the https://trezor.io/trezor-suite and disable passphrase.

@moneymanolis
Copy link
Collaborator Author

moneymanolis commented Dec 5, 2022

I was not able to verify, that this fixes the bug. So this should work without a new HWI-version, right? So as a workaround, download the https://trezor.io/trezor-suite and disable passphrase.

Have you tested it with bitcoin-core/HWI#644?
You can directly disable the passphrase in Specter, too.

@moneymanolis
Copy link
Collaborator Author

moneymanolis commented Dec 16, 2022

I've tested with Model T and Model One, with and without passphrase enabled and with non-empty and empty passphrases.
@k9ert can you try again, the PR mentioned above was merged by now, so you can just check with hwi master.

I double-checked again, the original issue was only with the Model One.

It seems like the changes in hwi would also just work with Specter master but the changes in this PR are still worthwhile with regards to more code clarity.

What is missing here is of course a change of the hwi version, but there has been no release lately. I will add the label waiting for upstream

@k9ert
Copy link
Collaborator

k9ert commented Jan 11, 2023

I think a double-check should not be needed. Next step would be to wait for the release. Hopefully we'll see that at the end of the month as Andrew said on the linked issue:

I plan on making a release at the beginning of the new year.

@k9ert
Copy link
Collaborator

k9ert commented Jan 27, 2023

* Some smaller improvements and fixes

* mitigating 2078

* removed Microsoft link tracking

* wrap up a failed update properly

* better decode and better error_handling
* change passwordaspin auth implementation

* env-var for RASPIBLITZ_SPECTER_RPC_LOGIN_BITCOIN_CONF_LOCATION

* remove default

* fix tests

* fix the tests part2

* fix cypress tests

* docstrings and other default removals

* improve error_handling

* do not raise exception
* added websocket reconnect javascript

* timeout fix

* FIXED server timout issue

* working port config

* added websockets

* fixed cypress test

* graceful exit, but client is slow

* fast shutdown

* Switch to other port, if this one is blocked

* better error handling

* undid the port retrying

* trying CI with different port for websockets

* added getting port from ENV

* test CI with longer wait time for cypress

* reset wait time to 1000

* test fix

* doc

* Bugfix: Flash cannot be called without a context.

If a WebAPI (initiated from a different thread) fails, and flash is acalled as a backup the flash is without context.

Now I simply added a attribute that flash cannot be called from any thread.  It now skips it during the "Trying to rebroadcast"

* restore feature that the WebAPI permission is updated

* js formatting

* doc and reshuffling methods

* localhost fix for cypress

* fixed bug that i introduced recently.

* doc

* refactoring

* refactoring

* fix bug and refactoring

* removed use_reloader=false

* made the CreateNotification more robust

* better log

* bug fix

* pytest fix

* bug fix

* pytest works... but ugly

* do not start the websockets server in the tests

* Introducing SPECTER_WEBSOCKET_ACTIVE

* fix for incomplete SPECTER_WEBSOCKET_ACTIVE

* fix for incomplete SPECTER_WEBSOCKET_ACTIVE

* working websockets server shutdown when main thread dies

* doc

* camelCase  for js

also changed WebAPI --> webapi

* better naming

* ssl working for python client and server, but not javascript

* minimal working example for self-signed ssl

* working minimal echo server in specter

works with self signed cert

* working user recognition

* working websocket server.  The client shows an ssl error

* added ssl_context, but doesn't work yet

* no error... not tested yet

* working

* working, but not in js yet

* almost working!!!

* client is not working

* still cannot get a 1 server to many clients working

* minimal client example

* working minimal exaple to send to all connected clients

* working (at first, but then disconnecting)

* working. but cient needs some simplifications

* working great!!!

* fix closing connection handling

* also for non ssl working

* doc and requirements

* better config

* fix for multiple connections of 1 user

* imports

* doc

* better logging

* added endpoint to test

* cleanup

* added a possibility to quit websocket connections via a special command.

* fix error message

* doc

* refactoring

* remove old code

* undo

* Docs: Add mobile access question to the FAQ (cryptoadvance#1829)

* starting pytest

pytest mocking doesn't work yet

working pytest

* better descriptions

* more notification arguments

* added pytest

* move notifications.js such that it is always available

* fix css issue

* A test-page in the devhelper extension

* defaults that shows a message without changing values

* doc and rename: clarifying point about the flask flash

* improved logging formatting

* js_message_box  reflects noy the notification_type

* doc

addressing cryptoadvance#1766 (comment)

* doc

addressing cryptoadvance#1766 (comment)

* fix pytest

* doc and use get instead of []

* redirect flash call

* feature now complete

* documentation

* rename callback

* Adding add_wallettab

* moving to server_endpoints

* proper removal

* refactor robust_json_dumps import

* forgot one reference of robust_json_dumps

* remove unintended changes

* import fixes and polishing

* docs revised

* working notifications

* working

almost completely moved

* not working yet.... somehow sending to the server doesn't work.

* working

* added TODO

* redirect flash

* fix for flash

* implemented callbacks

Can test with js code
a = await pythonCommand("app.specter.service_manager.execute_ext_callbacks(        'create_and_show_notification', 'title', target_uis='js_message_box', timeout=3000)")

* invisible in sidebar and enabled by default

* added logo

* adding wallet_alias and docs

* pictures for the docs

* added import

* forgotten change

* pytest fix

* forgotten part 2

* add HOST to config and fix pytest

* examples in settings and wallet menu

* app is not required in callback_after_serverpy_init_app

* better error logging

* fix: better error logging

* fix: include

* Removing Werkzeug Request logs for Cypress

* selectWallet twice to fix weird issues

* explicitly returing None if no user found

* better structuring in server function

* reset lots of flash changes

* add timeout for websocket server connections

* reset helpers.py

* added connection_report

- also changed the registering logic slightly,  to now have an easy overview of the open connections
- also added constants INTERNAL_NOTIFICATION_TITLES

* better default services in user.py

* tiny cleanup

* avoid slow_request_detection_stop

* move css

cryptoadvance#1766 (comment)

* typo

cryptoadvance#1766 (comment)

* callback_cleanup_on_exit

cryptoadvance#1766 (comment)

* move also css colors to service

* upgrade simple_websocket got rid of ssl error on close connection

* fix tests

* better default loading of notifications

* fix bug

see cryptoadvance#1766 (comment)

* better callback_cleanup_on_exit

* deleted files

* fix 1 pytest

* removed example code

cryptoadvance#1766 (comment)

* cleaned up js logic for the initial connection

* moved  sendRequest

* doc

* doc

* increase ping/timeout interval, because the websocket connection closure works well now

* increase migration count

* renaming

* changed order of notifications cypress test

* disable the websockets during all pytest

* reenable websockets for cypress

* fix bug

* pytest debugging  ONLY. Have to rollback

* Revert "pytest debugging  ONLY. Have to rollback"

This reverts commit ccee38f.

* info output for pytest debugging

* pytest experiment...  Can be rolled back.

* Revert "info output for pytest debugging"

This reverts commit 07f3aaa.

* fix

see cryptoadvance#1907 (comment)

* added view_functions comment

* trying to fix flaky cpyress

* fix for hardcoded url

* added auto-reconnection on request failiure

* remove user_secret_decrypted_required

* remove user_secret_decrypted_required

* Removed verbose logging unless logging level < 10

* remove verbose_debug from notification.id

* fix

* fix pytest

* fix non-working migration

* set devstatus_alpha and opt-in

* delete cypress test and remove migration from pytest

* len(classlist) seems to differ in CI and locally

* remove useless code

* done cryptoadvance#1766 (comment)

- removed duplicate sendRequest

* moved service_manager_cleanup_on_exit

see cryptoadvance#1766 (comment)

* added comment

see cryptoadvance#1766 (comment)

* attempt to fix cryptoadvance#1766 (comment)

* pytest works. Was a variation in requirements.txt

---------

Co-authored-by: Manolis Mandrapilias <70536101+moneymanolis@users.noreply.github.com>
Co-authored-by: Kim Neunert <k9ert@gmx.de>
Co-authored-by: moneymanolis <moneymanolis@protonmail.com>
…ryptoadvance#2109)

* added support for qr message signing for jade

* remove taproot support for jade

---------

Co-authored-by: Manolis Mandrapilias <70536101+moneymanolis@users.noreply.github.com>
Co-authored-by: moneymanolis <moneymanolis@protonmail.com>
* ping black version

* change to version 22.3.0 as in pre-commit

* proper version

* kick
…nce#2162)

Bumps [http-cache-semantics](https://github.com/kornelski/http-cache-semantics) from 4.1.0 to 4.1.1.
- [Release notes](https://github.com/kornelski/http-cache-semantics/releases)
- [Commits](kornelski/http-cache-semantics@v4.1.0...v4.1.1)

---
updated-dependencies:
- dependency-name: http-cache-semantics
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
k9ert and others added 3 commits February 3, 2023 13:15
) (cryptoadvance#2161)

* fixes psbt key error
* use lock to avoid concurrent access
Bumps [@sideway/formula](https://github.com/sideway/formula) from 3.0.0 to 3.0.1.
- [Release notes](https://github.com/sideway/formula/releases)
- [Commits](hapijs/formula@v3.0.0...v3.0.1)

---
updated-dependencies:
- dependency-name: "@sideway/formula"
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@moneymanolis
Copy link
Collaborator Author

When building a new requirements.txt to include hwi 2.2.0 (from scratch) I get conflicting indirect dependencies:

Could not find a version that matches typing-extensions<4.0,>=3.7,>=3.7.4,>=4.2.0 (from hwi==2.2.0->-r requirements.in (line 11))
Tried: 3.6.2, 3.6.2, 3.6.2.1, 3.6.2.1, 3.6.5, 3.6.5, 3.6.6, 3.6.6, 3.7.2, 3.7.2, 3.7.4, 3.7.4, 3.7.4.1, 3.7.4.1, 3.7.4.2, 3.7.4.2, 3.7.4.3, 3.7.4.3, 3.10.0.0, 3.10.0.0, 3.10.0.1, 3.10.0.1, 3.10.0.2, 3.10.0.2, 4.0.0, 4.0.0, 4.0.1, 4.0.1, 4.1.0, 4.1.0, 4.1.1, 4.1.1, 4.2.0, 4.2.0, 4.3.0, 4.3.0, 4.4.0, 4.4.0
There are incompatible versions in the resolved dependencies:
  typing-extensions>=4.2.0 (from SQLAlchemy==2.0.3->Flask-SQLAlchemy==2.5.1->cryptoadvance.spectrum==0.3.1->-r requirements.in (line 37))
  typing-extensions>=3.7.4 (from bitbox02==6.1.1->hwi==2.2.0->-r requirements.in (line 11))
  typing-extensions<4.0,>=3.7 (from hwi==2.2.0->-r requirements.in (line 11))

Related:
PR in hwi:
bitcoin-core/HWI#662
Issue in hwi:
bitcoin-core/HWI#572

@k9ert
Copy link
Collaborator

k9ert commented Jan 16, 2024

The dependency has been updated in hwi 2.2.1. Removing the waiting_for_upstream label.

@k9ert k9ert added bug Something isn't working and removed waiting_for_upstream bug Something isn't working labels Jan 16, 2024
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

Successfully merging this pull request may close these issues.

Unable to sign with Trezor: device does not have a matching signing key
5 participants