I’ll split out the interface changes and PR them to bitcoin/bitcoin later.
Sjors force-pushed
on Jun 18, 2020
Sjors force-pushed
on Jun 19, 2020
Sjors force-pushed
on Jun 19, 2020
Sjors force-pushed
on Jun 25, 2020
Sjors force-pushed
on Jul 2, 2020
Sjors force-pushed
on Jul 6, 2020
Sjors force-pushed
on Jul 14, 2020
laanwj referenced this in commit
31bdd86631
on Jul 15, 2020
Sjors force-pushed
on Jul 18, 2020
luke-jr
commented at 7:32 pm on July 31, 2020:
member
Shouldn’t the external signer program be per-wallet?
Sjors force-pushed
on Aug 7, 2020
Sjors
commented at 11:33 am on August 7, 2020:
member
@luke-jr in practice there’s only 1 program out there: HWI. It already knows what to do for different wallets based on their fingerprint. So, unless more such tools show up, I prefer to wait until we can store arbitrary settings in the wallet: https://github.com/bitcoin/bitcoin/issues/13044 and then keep -signer as the default.
Sjors force-pushed
on Aug 28, 2020
Sjors force-pushed
on Sep 15, 2020
Bosch-0
commented at 4:20 am on September 16, 2020:
none
Hey Sjors,
I could not see the signer script UI in the wallets settings when I compiled.
Sjors force-pushed
on Sep 16, 2020
Sjors
commented at 10:29 am on September 16, 2020:
member
@Bosch-0 oops, I accidentally lost a commit in my rebase fury. Try again.
Bosch-0
commented at 9:28 am on September 23, 2020:
none
Awesome, that worked! Having some issues with HWI though. After installing and setting the path to the hwi.py file I keep getting an exception error when creating a wallet.
How would we overcome having to install HWI separately? Would it have to be something like shipping HWI alongside the GUI. I’m a not too familiar with this process but having the path set by default behind the scenes would be ideal from a UX perspective.
Sjors
commented at 12:42 pm on October 1, 2020:
member
What’s the error you’re seeing?
Also, can you try from the command-line: hwi.py enumerate to see if HWI works standalone? This PR will certainly need followups to make it more user friendly.
Sjors force-pushed
on Oct 1, 2020
Bosch-0
commented at 7:32 am on October 6, 2020:
none
Here is the error, though I think HWI is not installed properly.
Testing is being done on windows 10.
Installation appears to be successful when running pip3 install .
When running ./hwi.py enumerate I get the below error. Tried the other dependency installation methods as well as installing libusb-1.0.dll and including it in HWI directory but no luck yet.
Sjors
commented at 9:44 am on October 8, 2020:
member
Bitcoin QT shouldn’t crash like this, so that’s a bug in this PR. But for the problem with HWI itself, I suggest opening an issue in the HWI repository. We don’t do a lot of Windows testing, so that’s useful (cc @achow101).
Sjors force-pushed
on Oct 8, 2020
Sjors force-pushed
on Oct 15, 2020
laanwj referenced this in commit
924a4ff7eb
on Oct 29, 2020
Sjors force-pushed
on Nov 17, 2020
promag
commented at 11:02 pm on November 22, 2020:
contributor
Concept ACK.
@Sjors you mention blocking calls, are you going to fix that here?
Bitcoin QT shouldn’t crash like this
Agree, I’m still on the base branch though.
Sjors
commented at 8:55 am on November 23, 2020:
member
I’m not very good at multi-threading stuff, so I might leave that problem to others, either via followup PR or cherry-picking a solution.
ryanofsky
commented at 4:09 pm on November 23, 2020:
contributor
I’m not very good at multi-threading stuff, so I might leave that problem to others, either via followup PR or cherry-picking a solution.
A lot of current GUI multithreading code is stupidly complicated. I wrote a utility function AsyncUpdate(async_fn, update_fn) to run slow blocking code and then update the GUI in https://github.com/bitcoin/bitcoin/commit/39c5492fabab52a888e8bc5303877ff6e4cce6f8 (example usage https://github.com/bitcoin/bitcoin/commit/eacd34144c1ee989c9ab43eb439fecca76addc76 and description https://github.com/bitcoin/bitcoin/issues/16874#issuecomment-632424392). AsyncUpdate takes two lambdas. The first lambda runs asynchronously in a thread, and when it finishes, its return value is passed as an argument to the second lambda which runs synchronously. The first lambda can run slow and blocking code and take long as it wants without freezing the GUI, while the second lambda can take the result and update the GUI. Restrictions are straightforward: The first lambda can’t access widgets directly (it can only read initial values, return a final result, and send signals if it wants to show progress messages or update a progress bar). The second lambda needs to be fast and not block, but it can directly update and access Qt widgets with the final result.
promag
commented at 4:58 pm on November 23, 2020:
contributor
@ryanofsky your AsyncUpdate is fine when async function doesn’t require user input, doesn’t give feedback like progress, or both, possibly multiple times. Otherwise, in these cases it would have to call multiple AsyncUpdate and then it starts to be a mess chaining those.
AsyncUpdate is also fine when there’s just one outcome, otherwise the async function needs to return a variant with each possible outcome and then the update function would have to check the effective result.
There’s also the case where the user wants to interrupt the async function, like quit, and in this case it wouldn’t work.
Another interesting problem with asynchronous updates are bursts, and usually most async calls should be avoid/debounced. I think we have a couple of these cases.
This is not to say AsyncUpdate isn’t useful, but to say that things get stupid very easily.
Lastly, I’d love to improve those stupidly complicated cases so we can use a consistent approach everywhere, and therefore anyone could feel confident and unafraid of asynchronous tasks.
ryanofsky
commented at 5:48 pm on November 23, 2020:
contributor
re: #4 (comment)@promag a lot of that comment isn’t true, and none of it is a reason not to write blocking code instead of calling AsyncUpdate.
Writing an AsyncUpdate call is about as easy as writing blocking code, and it prevents the GUI from freezing while an operation is running. The only major thing it doesn’t support currently is cancel notification (but it could take an optional cancel_fn argument in the future). So if the parent widget is deleted right now, GUI freeze will happen at the point of deletion. This is still better than freezing during the entire operation.
Almost everything in #4 (comment) is misleading. I would not suggest chaining AsyncUpdate calls: the only type of usage that makes sense is to having one AsyncUpdate call per long running operation. If there is a sequence of serial operations, they should all be in the same lambda. Supporting progress messages and progress updates is supported and trivial with signals. Input is possible with atomics, message queues, signals, or any other way you could think of passing data to an asynchronous thread. I don’t know what you are suggesting regarding multiple outcomes. Any outcome that can be represented as an object can be handled in the async_fn/update_fn paradigm, and if you are lazy and don’t want to specify an outcome type you can even have the async_fn return a std::function capturing arbitrary state and have the update_fn call that.
There are cases where you might prefer a more complicated approach over calling AsyncUpdate, like if you want a thread pool. But there should be not many cases where freezing the UI thread is preferable to calling AsyncUpdate. And there is existing GUI code that could be simplified and made more responsive with AsyncUpdate.
Sjors force-pushed
on Nov 24, 2020
jonasschnelli referenced this in commit
c33662a0ea
on Dec 2, 2020
Sjors force-pushed
on Dec 4, 2020
RandyMcMillan
commented at 4:27 pm on December 4, 2020:
contributor
checking whether to build with external signer support… configure: error: “External signer support requested but requires Boost.Pocess”
Boost.Process?
Sjors
commented at 5:01 pm on December 4, 2020:
member
@RandyMcMillan what operating system are you on? On macOS and Linux Boost Process comes with Boost, but on Windows it’s a separate package.
Sjors force-pushed
on Dec 10, 2020
Sjors force-pushed
on Dec 17, 2020
laanwj referenced this in commit
e520e091db
on Jan 6, 2021
Sjors force-pushed
on Jan 22, 2021
Sjors
commented at 7:47 pm on January 22, 2021:
member
I’m surprised the master branch passes given this error:
@MarcoFalke does bitcoinbuilds.org not test a build with --with-boost-process? (Travis did when I added RunCommandParseJSON)
Sjors force-pushed
on Jan 29, 2021
Sjors
commented at 3:05 pm on January 29, 2021:
member
@jonasschnelli I believe the bitcoinbuilds.org Win64 machine needs to add --without-boost-process --disable-external-signer to its BITCOIN_CONFIG. I don’t know if that causes problems when building stuff before this PR. Alternatively it should not ignore ci/test/00_setup_env_win64.sh.
https://bitcoinbuilds.org/?build=7162
Sjors force-pushed
on Jan 29, 2021
Sjors force-pushed
on Feb 13, 2021
Sjors force-pushed
on Feb 21, 2021
laanwj referenced this in commit
a9335e4f12
on Feb 23, 2021
Sjors force-pushed
on Feb 23, 2021
luke-jr
commented at 6:15 pm on February 28, 2021:
member
rpc_help test needs updating
Sjors force-pushed
on Mar 3, 2021
Sjors
commented at 12:49 pm on March 28, 2021:
member
Note to other reviewers. The QLineEdit::textChanged signal has the QString parameter that actually is a new text. Using a lambda instead of a slot makes discarding of this parameter explicit. I think this is a good practice.
There is #295 that tries to unify new setting names. If you are agree with suggested convention, maybe rename strExternalSignerPath? If not, mind leaving a comment in #295?
Do you mind making a favor to translators by adding a translator comment, smth like this:
0 //: "External signing" means using devices such as hardware wallets.
1 ui->external_signer_checkbox->setToolTip(tr("Compiled without external signing support (required for external signing)"));
Sjors
commented at 11:42 am on May 10, 2021:
member
Odd, maybe a rebase issue; it should have checked the “disable private keys” box.
Update: I was able to reproduce. It was because calling setChecked triggers the corresponding connect().
in
src/qt/receiverequestdialog.h:32
in
8fcf25301coutdated
I think we should try to have it in v22.0. Maybe add more banners that this feature is experimental?
hebasto added this to the milestone 22.0
on May 10, 2021
hebasto
commented at 12:16 pm on May 10, 2021:
member
~Having compile errors:~
EDIT: Sorry for noise, forgot make clean
Sjors force-pushed
on May 10, 2021
Sjors
commented at 1:49 pm on May 10, 2021:
member
Rebased and addressed comments.
Sjors force-pushed
on May 10, 2021
Sjors
commented at 3:48 pm on May 10, 2021:
member
I modified 214346f6a1d021a0c986512f17b419ae26bb1af4 to use WalletModelTransaction.sendCoins rather than directly broadcasting it. This takes care of adding labels. In order to do this, I had to add setWtx(const CTransactionRef&); to WalletModelTransaction which is a bit ugly. I believe @achow101 is working on a PR to use PSBT internally everywhere, which would probably be a good place to clean this up.
Sjors
commented at 6:24 pm on May 10, 2021:
member
When installing with make deploy I get an error message using hwi.py from the cloned repo. However when I use the binary HWI release it works fine. My guess this is a problem with my local Python environmental disaster.
DrahtBot added the label
Needs rebase
on May 10, 2021
hebasto approved
hebasto
commented at 8:34 am on May 11, 2021:
member
The 214346f6a1d021a0c986512f17b419ae26bb1af4 commit breaks a bit changes from #220, but we could leave string optimizations and translation improvements for follow ups.
Sjors force-pushed
on May 11, 2021
Sjors
commented at 8:50 am on May 11, 2021:
member
Rebased and add QLatin1String(" (*.psbt)").
DrahtBot removed the label
Needs rebase
on May 11, 2021
hebasto approved
hebasto
commented at 9:39 am on May 11, 2021:
member
re-ACK8e6b5468323e0c0cddda028c3dc2b0eef883cf4a, only rebased and comment addressed since my previous review.
hebasto requested review from achow101
on May 11, 2021
hebasto requested review from promag
on May 11, 2021
hebasto requested review from meshcollider
on May 11, 2021
in
configure.ac:336
in
8e6b546832outdated
332@@ -333,9 +333,9 @@ AC_ARG_ENABLE([werror],
333 [enable_werror=no])
334335 AC_ARG_ENABLE([external-signer],
336- [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is no, requires Boost::Process)])],
337+ [AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is yes, requires Boost::Process)])],
Would be nice to drop #ifdef ENABLE_EXTERNAL_SIGNER in the interface here and just return an empty vector in the implementation or throw a not-implemented exception. Cross-process interfaces are trying not to be conditional to avoid build complexity and fragile incompatibilities between builds.
Also maybe in general in this PR, preprocessor is a little more widely used that it needs to be. Preprocessor is great when you need to write platform specific fragments or cut out a significant external or internal dependency like boost or the wallet. But if you just want to act conditionally, normal code is a good way to go. I’d try to avoid conditional compilation except for the parts of the code directly depending on boost.
Sjors
commented at 11:40 am on May 13, 2021:
member
meshcollider
commented at 2:50 am on May 15, 2021:
By the below comment The order matters, because connect() is called when toggling a checkbox, it would seem that the checkboxes should be disabled first before toggling?
Related: I notice currently that enabling external signer actually leaves blank_wallet_checkbox ticked. I think that’s because in setting disable_privkeys_checkbox as checked, it re-checks blank automatically. (on Mac)
I’m not sure if programatically toggling a disabled checkbox prevents it from calling toggled, but I’ve moved the disabling code up.
I also changed the behavior a bit: it now restores all other options to their default when unselecting external signer. I also documented that behavior.
in
src/qt/sendcoinsdialog.cpp:325
in
6080b0dcceoutdated
noPrivateKeys() is a big ambiguous. It would have to be something like noPrivateKeysAndNoExternalSigner() which is pretty ugly…
meshcollider
commented at 1:51 am on June 9, 2021:
What about noPrivateKeyAccess() or something - the condition is that there are keys either in the wallet itself or that the wallet can ‘use’ via the external signer. Not a big deal though.
meshcollider
commented at 3:06 am on May 15, 2021:
contributor
Code review and light manual testing ACK6080b0dccea11f21f54a073c97c9a45d19b401b5 modulo comments below
MarcoFalke referenced this in commit
c857148636
on May 15, 2021
hebasto added the label
Waiting for author
on May 20, 2021
jonatack
commented at 7:10 pm on May 20, 2021:
contributor
Big concept ACK.
Sjors
commented at 8:01 pm on May 20, 2021:
member
(I’m going to accumulate a few more reviews before updating)
hebasto removed the label
Waiting for author
on May 20, 2021
Sjors force-pushed
on May 25, 2021
Sjors
commented at 4:44 pm on May 25, 2021:
member
Rebased an addressed (most of) @meshcollider’s comments.
Sjors force-pushed
on May 26, 2021
DrahtBot added the label
Needs rebase
on May 27, 2021
gui: add external signer path to options dialog6cdbc83e93
Sjors
commented at 12:43 pm on May 27, 2021:
member
Rebased and slightly clarified the last commit.
DrahtBot removed the label
Needs rebase
on May 27, 2021
hebasto
commented at 4:30 pm on June 1, 2021:
member
Tested 1c4b456e1a0ccf0397d652f8c18201c3224c5c21 with HWW 2.0.2-rc.1 on Linux Mint 20.1 (Qt 5.12.8).
UPDATE: it was my fault – hww connection issues. Ignore this.
~The “External signer” checkbox is disabled when trying Menu -> File -> Create Wallet…~
~It could be enabled by checking the “Encrypt Wallet” checkbox, then unchecking it.~
hebasto approved
hebasto
commented at 6:05 pm on June 1, 2021:
member
ACK1c4b456e1a0ccf0397d652f8c18201c3224c5c21, tested on Linux Mint 20.1 (Qt 5.12.8) with HWW 2.0.2-rc.1.
Tested:
build with --without-bdb configure option
build with --without-sqlite configure option
creating a wallet
verifying an address on the hww screen
signing a tx
The following comments could be addressed in follow ups.
If no external signer is connected, it is possible to activate the “External signer” checkbox by checking the “Encrypt Wallet” checkbox, then unchecking it. Of course, the further attempt to create a wallet with an external signer will ends with the crash. This illness pattern works even if the client is compiled without SQLite support:
The “Confirm send coins” could still be opened while verifying transaction details on a signing device.
All new translatable strings could be annotated with translator comments.
in
src/qt/createwalletdialog.cpp:33
in
eef8d64529outdated
26@@ -27,14 +27,39 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
27 });
2829 connect(ui->encrypt_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) {
30- // Disable the disable_privkeys_checkbox when isEncryptWalletChecked is
31+ // Disable the disable_privkeys_checkbox and external_signer_checkbox when isEncryptWalletChecked is
32 // set to true, enable it when isEncryptWalletChecked is false.
33 ui->disable_privkeys_checkbox->setEnabled(!checked);
34+ ui->external_signer_checkbox->setEnabled(!checked);
in
src/qt/createwalletdialog.cpp:128
in
24815c6309outdated
123+ // The order matters, because connect() is called when toggling a checkbox:
124+ ui->blank_wallet_checkbox->setEnabled(false);
125+ ui->blank_wallet_checkbox->setChecked(false);
126+ ui->disable_privkeys_checkbox->setEnabled(false);
127+ ui->disable_privkeys_checkbox->setChecked(true);
128+ const std::string label = signers[0].m_name;
Could add comment “TODO show available signers in combobox, for now use 1st signer name”?
meshcollider
commented at 1:48 am on June 9, 2021:
@promag it is just the name of the wallet so I don’t think it would be worth adding a combobox tbh. That might be confusing because it wouldn’t actually change between signers, it would just change the name.
in
src/qt/createwalletdialog.h:31
in
24815c6309outdated
Mostly reviewed the code, did a little bit of testing. I noticed that when HWI is doing something, there is no indication to the user that it is. It would be nice if there was some dialog box that said something was happening, otherwise there can be a minute or two where Core doesn’t indicate anything is happening, and nothing appears on the device.
in
src/qt/createwalletdialog.cpp:125
in
24815c6309outdated
120+ ui->external_signer_checkbox->setChecked(true);
121+ ui->encrypt_wallet_checkbox->setEnabled(false);
122+ ui->encrypt_wallet_checkbox->setChecked(false);
123+ // The order matters, because connect() is called when toggling a checkbox:
124+ ui->blank_wallet_checkbox->setEnabled(false);
125+ ui->blank_wallet_checkbox->setChecked(false);
meshcollider
commented at 1:46 am on June 9, 2021:
nit: to be consistent with gui: create wallet with external signer, this should be true (modulo comment about it being ambiguous anyway).
meshcollider
commented at 1:54 am on June 9, 2021:
contributor
Agree with achow101’s comment about a ‘working’ message.
meshcollider merged this
on Jun 9, 2021
meshcollider closed this
on Jun 9, 2021
Sjors deleted the branch
on Jun 9, 2021
sidhujag referenced this in commit
fb0bf712b4
on Jun 9, 2021
fanquake referenced this in commit
7c561bea52
on Jun 17, 2021
jarolrod
commented at 4:34 am on June 18, 2021:
member
post-merge ACK, wanted to document that I tested this out with a coldcard and the process was smooth.
jb55
commented at 6:54 pm on June 23, 2021:
contributor
@Sjors I tried this with my trezor1, I unlocked it on the command line on a wallet with a bip39 password, this is what pops up when I click create wallet:
Sjors
commented at 5:50 pm on June 24, 2021:
member
Mmm, I’m not sure how the passphrase flow works. @achow101?
I might be that we need to add support for that, e.g. if needs_pin_sent or needs_passphrase_sent returns true, we prompt for that.
However in your case those are false. Or did you actually enter a password in the command line example? (given --stdinpass)
achow101
commented at 5:53 pm on June 24, 2021:
member
The passphrase should be cached by the device, so if you try again after running HWI manually, it should work. But we do need to implement a passphrase workflow for needs_passphrase_sent: true.
jb55
commented at 6:38 pm on June 24, 2021:
contributor
@achow101 I’ve never been able to get the trezor_1 passphrase cached with hwi, I’ve always had to do --stdinpass on every command. hmm maybe I need to update firmware or something…
achow101
commented at 6:53 pm on June 24, 2021:
member
Oh right, Trezor 1 doesn’t have that, only Trezor T.
jb55
commented at 7:23 pm on June 24, 2021:
contributor
@achow101 fwiw it caches fine when using trezorctl, dunno what they are doing.
hebasto added the label
Needs release notes
on Jun 28, 2021
hebasto
commented at 4:30 am on June 28, 2021:
member
This is a metadata mirror of the GitHub repository
bitcoin-core/gui.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-10-23 00:20 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
