ui: make send a wizard #16966

Implements #16954 for the current functionality + #16944 (gui: create PSBT with watch-only wallet). This PR splits the send screen into three tabs, like a wizard.

This frees up UI real estate where we can add support for PSBT, hardware wallets and education.

I renamed SendCoinsDialog to SendCompose and SendConfirmationDialog to SendSignfor clarity, which ended up (trivially) touching lots of src/qt/locale/bitcoin_##.ts files. This is contained in two move-only commits.

Tab 1: Draft

<img width="926" alt="Schermafbeelding 2019-09-26 om 15 59 16" src="https://user-images.githubusercontent.com/10217/65695736-606d8b80-e078-11e9-84d9-fe2d37f932e8.png">

Same as the current send screen: enter destination, do coin selection, set fee etc. This can be split further in future PRs for a less cluttered experience, e.g. one tab for coin selection (if enabled), one for destination(s) and one for fees. Having a separate tab for fees also provides an entry point for RBF, which currently doesn't let the user pick an amount.

Tab 2: Sign

This asks to unlock the wallet if needed.

<img width="931" alt="Schermafbeelding 2019-09-26 om 16 14 42" src="https://user-images.githubusercontent.com/10217/65695992-c9ed9a00-e078-11e9-9ee7-4d60d0499049.png">

Display transaction details like the current popup does.

<img width="933" alt="Schermafbeelding 2019-09-26 om 15 59 35" src="https://user-images.githubusercontent.com/10217/65695773-6fecd480-e078-11e9-95fa-498f844c5eb1.png">

Edit jumps back to Draft. Send jumps to Finish, unless something goes wrong.

Bump fee jumps straight to this tab: <img width="855" alt="sign" src="https://user-images.githubusercontent.com/10217/65713022-89057d80-e098-11e9-80d5-0a2abdde0871.png">

For watch-only wallets it displays the same text as #16944.

Tab 3: Finish

<img width="932" alt="Schermafbeelding 2019-09-26 om 16 08 21" src="https://user-images.githubusercontent.com/10217/65695799-7b400000-e078-11e9-967a-7581f15ecdd5.png">

This is where the actual broadcast takes place, or where the PSBT is copied to the clipboard. In a followup we can add support for saving the PSBT to disk, or for copying a signed transaction hex to clipboard if the user wants to broadcast that elsewhere.

Bump fee shows both the previous and new transaction index: <img width="847" alt="sent" src="https://user-images.githubusercontent.com/10217/65713060-9c184d80-e098-11e9-879a-f10822d4ad2b.png">

The manual "Show" button has the nice side-effect of fixing #16875 / #16876 in two out of three places.

Todo:

• clean up WalletModelTransaction and CoinControl object passing mess
• restore test

Followups:

• add PSBT export to Sign tab (extract from gwillen's branch https://github.com/gwillen/bitcoin/tree/feature-offline-v2, i.e. redo #16944)
• add Load PSBT menu option, jump to Broadcast tab if complete, otherwise to Sign tab
• list connected hardware wallets in Sign tab (redo #16549
• split fee selection into its own tab
• allow custom RBF amounts: bump fee should jump to fee selection tab

Concept ACK. Not sure if the tab view is the best container for the send wizard. Have you considered back/next buttons?

I went back and for myself on the view style and ultimately did settle on a tab view. Ultimately there's never actually a need to press the Back and Next buttons unless you're doing something wrong (or a trivial demo) -- each step of the process will happen on a different machine.

I like the idea of a "load PSBT" menu item taking you to the right tab. In my implementation, there is an "offline" menu from which you have to select "sign" or "broadcast" to open the dialog to the right tab; then you load from there.

There's a lot of stuff that came up in the process of implementing this which I don't know if you've run into yet. For example, the signing wallet may not be able to put up the usual form of the confirmation display, as shown here, unless it has some trustworthy way to know which output is change, which it doesn't except maybe if you're using descriptors to tell it so.

Jumping back to Draft, much like having back/next buttons, is probably mostly not going to get used in real life, where the signing is happening on a different machine from the drafting. Honestly I think the main reason to have the steps of this flow be one dialog are to help the user have a sense of their place in the process, even though I expect probably they will be mostly used separately and on separate machines.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #17219 (wallet: allow transaction without change if keypool is empty by Sjors)
• #17165 (Remove BIP70 support by fanquake)
• #17154 (wallet: Remove return value from CommitTransaction by jnewbery)
• #16944 (gui: create PSBT with watch-only wallet by Sjors)
• #16876 (gui: Drop calls to QCoreApplication::processEvents by promag)
• #16710 (build: Enable -Wsuggest-override if available by hebasto)
• #16341 (Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

the signing wallet may not be able to put up the usual form of the confirmation display, as shown here, unless it has some trustworthy way to know which output is change, which it doesn't except maybe if you're using descriptors to tell it so.

I have a hardware wallet GUI PR #16549 which is built on top of native descriptor wallets #16528.

I'm also thinking of replacing WalletModelTransction, which is a QT-only thing, with PSBT. But perhaps that's better for a followup.

Re tab view: I briefly looked at QWizard but that doesn't give the user an overview of where they are in the flow. The good news is that QTabWidget can easily be swapped with QStackedWidget which can be completely customized, e.g. into something like this:

<img width="247" alt="Schermafbeelding 2019-09-26 om 10 37 33" src="https://user-images.githubusercontent.com/10217/65672650-ab23df00-e049-11e9-9c31-beabfacedfa1.png">

I'll probably disable the other tabs, so that all "navigation" happens through the buttons at the bottom of the screen.

I expect probably they will be mostly used separately and on separate machines

I'm thinking more of the hardware wallet use case. In the simplest configuration of 1 hardware wallet that's physically connected, the Sign tab is a nice place to show it. In case of something like ColdCard or the new BitBox this is where you would save the unsigned PSBT to SD drive.

Added RBF. If a followup PR splits fee selection out of Tab 1 then bump fee could also support a custom amount, rather the current take it or leave it fee.

I'm considering squashing this into a single commit; current commit history is probably more confusing than useful.

Concept ACK

qt/wallettests.cpp breaks because the transactionTableModel doesn't update after the transactions are created. @promag could this be related to #16876 (comment)?

Before this change, a healthy test log contains an updateWallet entry which is missing in the failing test:

PASS   : WalletTests::initTestCase()
QDEBUG : WalletTests::walletTests() TransactionTablePriv::refreshWallet
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: df30faf7e56ab6b7221ca3464427b4a18bf55cb8dd1ea0f2a7f05e29db5fa3fb status= 0"
QDEBUG : WalletTests::walletTests() "NotifyTransactionChanged: ff71c01681d9c02363746313b6e58cd174e0afde19a40e0597e4240a84209668 status= 1"
QDEBUG : WalletTests::walletTests() "NotifyAddressBookChanged: mfWxJ45yp2SFn7UciZyNpvDKrzbhyfKrY8  isMine=0 purpose=send status=0"
QDEBUG : WalletTests::walletTests() "TransactionTablePriv::updateWallet: df30faf7e56ab6b7221ca3464427b4a18bf55cb8dd1ea0f2a7f05e29db5fa3fb 0"
QDEBUG : WalletTests::walletTests() "    inModel=0 Index=103-103 showTransaction=1 derivedStatus=0" 

NotifyTransactionChanged in WalletModel calls updateTransaction using a Qt::QueuedConnection. That caused the test to fail.

@Sjors try like this https://github.com/bitcoin/bitcoin/pull/16876/files#diff-6fcb1829ce66ad65046d6896dbcc910bR73

@promag added.

For some reason the Travis nowallet build is failing, but I can't reproduce:

D__STDC_FORMAT_MACROS  -fstack-reuse=none -Wstack-protector -fstack-protector-all      -fPIE -pipe -O2  -fvisibility=hidden -c -o qt/qt_libbitcoinqt_a-moc_sendcompose.o `test -f 'qt/moc_sendcompose.cpp' || echo './'`qt/moc_sendcompose.cpp
2790In file included from ./wallet/walletdb.h:12:0,
2791                 from ./wallet/wallet.h:23,
2792                 from ./wallet/coincontrol.h:11,
2793                 from ./qt/sendcompose.h:14,
2794                 from qt/moc_sendcompose.cpp:8:
2795./wallet/db.h:24:10: fatal error: db_cxx.h: No such file or directory
2796 #include <db_cxx.h>
2797          ^~~~~~~~~~
2798compilation terminated.
2799make[2]: *** [qt/qt_libbitcoinqt_a-moc_sendcompose.o] Error 1

Update: figured out on IRC, problem is in using CCoinControl in a Q_SIGNAL. Looking at some different approaches to passing the transaction and coin control around.

Travis is happy. TIL that build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj isn't auto generated, so I had to update that as well. Now AppVeyor is happy too.

I'm probably going to add a SendWidget class to share common code between the tabs. I might also replace unique_ptr with shared_ptr as the current passing the baton approach seems a bit brittle.

<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase

Concept NACK. Sorry, this looks ugly, much worse than the current flow.

(Also, normal users probably shouldn't see txids...)

That's subjective. I think it looks better, because it's less cluttered and gets rid of a popup. Happy to hide the transaction id.

The current sign screen (which is a popup) doesn't have space to list hardware wallets. There's also not much room for PSBT creation and/or signing, other than just magically putting it on a clipboard (see #16944). If you have a better design I'd love to see it.

If you have a better design I'd love to see it.

To be clear, I think the current design with popup is better...

This frees up UI real estate where we can add support for PSBT, hardware wallets and education.

Concept ACK on motivation for this. I don't have a view on its ugliness. For these UI redesigns it seems totally dependent on the regular users of the GUI. Personally I support the goal of the GUI being an educational stepping stone to using the CLI. There will be always be better GUIs for the non technically curious which aren't subject to the constraints of Core.

My view is almost entirely the opposite: The JSON-RPC server isn't meant for end users, but for software developers to interface with. The GUI is the primary method for end users to use Bitcoin Core, not some mere "stepping stone" to a half-baked JSON-RPC client...

P.S. In addition to being a developer, I am a regular user of the GUI.

I think @michaelfolkson was referring to having independently developed wallet applications that use the RPC (somewhat limited atm) or IPC (#10102). I use the GUI as well.

I'm not entirely sure who this GUI is being designed for. Long time contributors like Sjors, Luke? Complete Bitcoin beginners? Someone like me who is technically curious and would ideally want to transition from any reliance on the GUI? All of us? (fiendishly difficult as you are making design compromises all over the place).

If it is complete Bitcoin beginners then none of us are the target audience. But this is a meta point and this discussion has probably been rehashed multiple times over the years so I won't clog up this PR with it.

I've opened an issue #17395. I don't want to derail review of this particular PR.

Concept NACK. Sorry, this looks ugly, much worse than the current flow To be clear, I think the current design with popup is better...

What current design allows you to generate PSBT's from the GUI of offline versions of Bitcoin Core for secure multisig? AFAIK, #16944, #16954, and this PR are the only discussions/PRs around this (https://github.com/bitcoin/bitcoin/pull/16546, #16549, and #16895 for HWWs).

The UI isn't beautiful (neither is the rest of Core), but it's not ugly. And I don't believe that's the point, especially from reading #17395, that seems to be the consensus. The point is to be secure, power-user friendly, and mistake-minimizing.

And I don't believe that's the point, especially from reading #17395, that seems to be the consensus. The point is to be secure, power-user friendly, and mistake-minimizing.

Do you agree with that @luke-jr? Feel free to comment on #17395 if you do/don't.

Closing this in favor of more incremental changes, e.g. #17509.