gui: save and load PSBT #17509

pull Sjors wants to merge 6 commits into bitcoin:master from Sjors:2019/11/gui-psbt-save changing 13 files +221 −52
  1. Sjors commented at 5:32 pm on November 18, 2019: member

    This adds:

    • a dialog after Create Unsigned, which lets you save a PSBT file in binary format, e.g. to an SD card
    • a “Load PSBT” menu entry lets you pick a PSBT file. We broadcast the transaction if complete

    Save flow

    By default the file name contains the destination address(es) and amount(s).

    We only use the binary format for files, in order to avoid compatibility hell. If we do want to add base64 file format support, we should use a different extension for that (.psbt64?).

    Load flow

    Select a file:

    Offer to send if complete:

    Tell user if signatures are missing, offer to copy to clipboard:

    Incomplete for another reason:

  2. fanquake added the label GUI on Nov 18, 2019
  3. DrahtBot commented at 9:48 pm on November 18, 2019: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18656 (gui: Add a Make unsigned button next to Send by achow101)
    • #18246 (wip: gui: Drop connectSlotsByName usage by promag)
    • #18027 (“PSBT Operations” dialog by gwillen)
    • #17463 (Bugfix: GUI: Restore SendConfirmationDialog button default to “Yes” by luke-jr)
    • #17457 (gui: Fix manual coin control with multiple wallets loaded by promag)
    • #17034 (Bip174 extensions by achow101)
    • #16463 ([BIP 174] Implement serialization support for GLOBAL_XPUB field. by achow101)
    • #14485 (Try to use posix_fadvise with CBufferedFile by luke-jr)

    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.

  4. practicalswift commented at 10:12 am on November 19, 2019: contributor

    I haven’t investigated thoroughly yet but it seems like at least a subset of the issues regarding handling of malformed PSBT inputs reported in #17149 (“Potential PSBT issues found by the PSBT fuzzer”) are reachable from this code?

    That is not a problem once the #17149 issues have been fixed by making the PSBT code more robust against malformed PSBT input, but I wanted to mention the dependency here so that the PSBT issues get fixed before the merge of this PR :)

  5. in src/qt/walletview.cpp:246 in a48b7dec21 outdated 
    241+        Q_EMIT message(tr("Error"), tr("Unable to decode PSTB file") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR);
242+        return;
243+    }
244+
245+    // PSBTAnalysis analysis = AnalyzePSBT(psbtx);
246+    // bool have_all_sigs = (analysis.next == PSBTRole::FINALIZER) || (analysis.next == PSBTRole::EXTRACTOR);
    Sjors commented at 10:13 am on November 20, 2019:

    @achow101 I uncommented this because the next role was UPDATER, which seems similar to what @justinmoon found: https://bitcoin.stackexchange.com/questions/89203/analyzepsbt-says-next-role-is-updater-but-nothing-is-missing

    Will investigate more thoroughly when I work on this PR again, if I still see it.

  6. Sjors force-pushed on Nov 23, 2019
  7. Sjors force-pushed on Nov 23, 2019
  8. Sjors force-pushed on Nov 26, 2019
  9. gwillen commented at 11:26 pm on December 9, 2019: contributor
    UI comment: I think “create unsigned” / “save” is a weird juxtaposition. Perhaps “Copy PSBT” / “Save PSBT”? Or perhaps “Create Unsigned” with further options in a dialog?
  10. Sjors force-pushed on Dec 10, 2019
  11. in src/qt/walletview.cpp:241 in f7b26ef6f0 outdated 
    236+    std::string data(std::istreambuf_iterator<char>{in}, {});
237+
238+    std::string error;
239+    PartiallySignedTransaction psbtx;
240+    if (!DecodeRawPSBT(psbtx, data, error)) {
241+        Q_EMIT message(tr("Error"), tr("Unable to decode PSTB file") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR);
    gwillen commented at 2:26 am on December 14, 2019:
    Typo (PSBT -> PSTB)
  12. promag commented at 1:25 am on December 23, 2019: member
    Concept ACK.
  13. Sjors commented at 10:14 am on January 2, 2020: member

    Rebased. I can address feedback for #17463 here if needed.

    Or perhaps “Create Unsigned” with further options in a dialog?

    Yes, this seems like a better idea. It’s especially useful to have some flexibility after loading a PSBT.

  14. Sjors force-pushed on Jan 2, 2020
  15. Sjors force-pushed on Jan 4, 2020
  16. Sjors force-pushed on Jan 4, 2020
  17. Sjors marked this as ready for review on Jan 4, 2020
  18. Sjors commented at 1:18 pm on January 4, 2020: member
    I added a dialog for Create Unsigned that appears after the usual transaction approval. The PSBT is automatically copied to clipboard as before, but the dialog offers to also save it. When loading a PSBT, the dialog asks if the users wants to broadcast. When there’s a problem, e.g. missing signatures, it offers to copy it (so you can e.g. inspect it with RPC).
  19. Sjors force-pushed on Jan 6, 2020
  20. Sjors commented at 4:36 am on January 6, 2020: member
    I dropped the dependency on #17463; should be easy to rebase. This is now ready for review.
  21. gwillen commented at 1:57 am on January 28, 2020: contributor

    Tested ACK e6ea593. Thoughts:

    • Obviously I’m not exactly an unbiased reviewer since some of this is based on some of my work, but I think my ACK probably still counts. :-)
    • I don’t like the verbosity of the save filenames, but that’s so easy to change later, and so subject to bikeshedding, that I think it should be left alone for this PR.
  22. luke-jr commented at 3:56 am on January 29, 2020: member

    No way to actually sign an incomplete one?

    Maybe it should load raw signed transactions too…

  23. Sjors commented at 8:04 am on January 29, 2020: member

    @luke-jr both should be possible, but I prefer to keep this PR simple. @practicalswift wrote:

    I haven’t investigated thoroughly yet but it seems like at least a subset of the issues regarding handling of malformed PSBT inputs reported in #17149 (“Potential PSBT issues found by the PSBT fuzzer”) are reachable from this code?

    That is not a problem once the #17149 issues have been fixed by making the PSBT code more robust against malformed PSBT input, but I wanted to mention the dependency here so that the PSBT issues get fixed before the merge of this PR :)

    These are merged now in #17156, though I haven’t rebased to preserve @gwillen’s ACK.

  24. gwillen commented at 8:14 pm on January 29, 2020: contributor

    @luke-jr FWIW, I already have a change prepared on top of this one which introduces more functionality when loading, including the ability to sign. I have been kind of dithering over details but it’s basically ready to become a PR. I will probably PR it at least as a WIP as soon as this one’s in (maybe I should just do that now.)

    (It doesn’t load raw signed transactions, but that would seem pretty easy to add, in fact. Maybe not a bad idea.)

  25. practicalswift commented at 9:20 pm on January 29, 2020: contributor

    @Sjors Nice to see #17156 merged - that makes the handling of malformed PSBT much more robust. Please note that we still have the misaligned pointer use in ProduceSignature(…) (via prevector) to resolve – see #17156 (comment) for details and #17708 for fix.

    I would suggest waiting with merging this PR until #17708 is merged.

    After that the handling of malformed PSBT input should be free from known robustness issues AFAIK :)

  26. Sjors commented at 9:58 am on January 30, 2020: member
    @practicalswift this PR doesn’t sign PSBT’s, so it seems that particular code path is avoided?
  27. practicalswift commented at 11:27 am on January 30, 2020: contributor
    @Sjors Oh, of course - you’re right. It is a PSBT issue, but not an issue for this PR :)
  28. in src/qt/sendcoinsdialog.h:63 in e6ea593f51 outdated 
    59@@ -60,6 +60,7 @@ public Q_SLOTS:
60     Ui::SendCoinsDialog *ui;
61     ClientModel *clientModel;
62     WalletModel *model;
63+    std::unique_ptr<WalletModelTransaction> m_current_transaction;
    luke-jr commented at 6:04 am on February 6, 2020:
    This looks like an ugly hack? Why is it not just passed as a reference to PrepareSendText?
    Sjors commented at 1:56 pm on February 10, 2020:
    on_sendButton_clicked() also needs access to m_current_transaction
    promag commented at 2:05 pm on March 15, 2020:
    This is never reset, maybe clear it after send?
    promag commented at 10:52 pm on April 16, 2020:
    Currently m_current_transaction is only used in send, in https://github.com/promag/bitcoin/commit/241d17111e5c21b9d0bd564310212bef07cc5712 I’ve dropped it and changed PrepareSendText to return the transaction (or null). Feel free to ignore, I’m happy to submit as follow up.
  29. luke-jr referenced this in commit f09f664c57 on Feb 9, 2020
  30. luke-jr referenced this in commit 7e6b20ffa0 on Feb 9, 2020
  31. luke-jr referenced this in commit 7aca36601d on Feb 9, 2020
  32. laanwj added this to the "Blockers" column in a project
  33. instagibbs commented at 7:15 pm on February 20, 2020: member
    concept ACK, will review and test soon
  34. in src/qt/walletview.cpp:252 in e6ea593f51 outdated 
    247+
248+    CMutableTransaction mtx;
249+    bool complete = FinalizeAndExtractPSBT(psbtx, mtx);
250+
251+    QMessageBox msgBox;
252+    msgBox.setText("Partially Signed Bitcoin Transaction");
    instagibbs commented at 7:16 pm on February 20, 2020:
    “Completed”? Might want to be very obvious to user that there’s nothing else to be done.
    gwillen commented at 7:18 pm on February 20, 2020:
    That’s a good point re: wording, but I would suggest not bikeshedding on it because it’s going to get changed again in my #18027 .
    Sjors commented at 8:51 am on February 28, 2020:
    It’s not necessarily complete. The setInformativeText below covers the various options. But I changed it to “PSBT” to hopefully reduce confusion.
  35. in src/qt/walletview.cpp:270 in e6ea593f51 outdated 
    265+            std::string err_string;
266+            CTransactionRef tx = MakeTransactionRef(mtx);
267+
268+            TransactionError result = BroadcastTransaction(*clientModel->node().context(), tx, err_string, COIN / 10, /* relay */ true, /* await_callback */ false);
269+            if (result == TransactionError::OK) {
270+                Q_EMIT message(tr("Success"), tr("Broadcast transaction sucessfully."), CClientUIInterface::MSG_INFORMATION | CClientUIInterface::MODAL);
    instagibbs commented at 9:04 pm on February 20, 2020:
    s/Broadcast/Broadcasted/
  36. in src/qt/walletview.cpp:259 in e6ea593f51 outdated 
    254+    if (complete) {
255+        msgBox.setInformativeText("Would you like to send this transaction?");
256+    } else if (!have_all_sigs) {
257+        msgBox.setInformativeText("Transaction needs more signatures. Copy to clipboard?");
258+    } else {
259+        msgBox.setInformativeText("Transaction is incomplete. Copy to clipboard?");
    instagibbs commented at 9:06 pm on February 20, 2020:
    what situations can this hit?
    gwillen commented at 9:22 pm on February 20, 2020:
    From my own study of this question, I believe the the answer is “no situations, unless there are bugs in AnalyzePSBT”, but I think we’ve run into some bugs in AnalyzePSBT while figuring this out.
    Sjors commented at 9:58 am on February 21, 2020:
    I don’t feel confident enough about AnalyzePSBT (yet) to put an assert here.
    instagibbs commented at 1:57 pm on February 21, 2020:
    Worst thing I’d do is stick a log line in debug or something. Fine without.
    achow101 commented at 8:49 pm on February 27, 2020:

    I believe this can be hit if there are final_scriptSig/final_scriptWitness but with invalid signatures. So have_all_sigs == true because there are sigs there (it does not validate), but complete == false because it does validate and finds the sigs to be invalid.

    Some things that could happen where we might want better error messages:

    • Logically invalid psbt (see the conditions where next is Creator)
    • Missing things, in general needs updating rather than signing.
    achow101 commented at 9:23 pm on February 27, 2020:

    Actually I just tested this case and it seems that because it is already finalized, the final_scriptSig/final_scriptWitness is assumed to be valid so complete == true. So what we actually get is a failed attempt to broadcast. But there’s no error message, just a blank error dialog.

    I think in general for this section, we should do the analysis first and do a switch on the result to give more specific errors and messages about the analysis. Then if finalize/extract is the next step, do that. It may also be prudent to have FinalizeAndExtractPSBT to always verify.

  37. instagibbs commented at 9:20 pm on February 20, 2020: member
    tACK e6ea593f510481ddabc042403a5f6f0200551ffb, Tested using HWI with base64->binary conversion command detailed to save the signed version of the transaction here https://github.com/bitcoin-core/HWI/blob/master/docs/examples.md#binary-format-handling
  38. in src/qt/sendcoinsdialog.cpp:442 in 61454e4270 outdated 
    437+            break;
438+        }
439+        case QMessageBox::Discard:
440+            break;
441+        default:
442+            // should never be reached
    achow101 commented at 8:42 pm on February 27, 2020:
    nit: assert(false)?
  39. in src/qt/walletview.cpp:271 in e6ea593f51 outdated 
    282+    }
283+    case QMessageBox::Cancel:
284+        break;
285+    default:
286+        // should never be reached
287+        break;
    achow101 commented at 8:51 pm on February 27, 2020:
    nit: assert(false)?
  40. achow101 commented at 8:51 pm on February 27, 2020: member
    Looked through the commits, seems good. Will test with some PSBTs.
  41. in src/qt/sendcoinsdialog.cpp:422 in e6ea593f51 outdated 
    418+            for (const SendCoinsRecipient &rcp : m_current_transaction->getRecipients()) {
419+                if (!first) {
420+                    fileNameSuggestion.append("-");
421+                }
422+                QString amount = BitcoinUnits::format(model->getOptionsModel()->getDisplayUnit(), rcp.amount);
423+                fileNameSuggestion.append(rcp.address + "-" + amount);
    achow101 commented at 9:07 pm on February 27, 2020:
    For better UX, we should probably use the recipient label if it is available and truncate the 0’s on the amount.
    Sjors commented at 10:35 am on February 28, 2020:
    Added the recipient name and switched to formatWithUnit. We could add an argument to BitcoinUnits::format to remove trailing zeros … in a later PR.
  42. gwillen commented at 10:08 pm on February 27, 2020: contributor
    @achow101 General note: keep in mind that a lot of the logic here gets moved in #18027, and changed somehwat (and I think my approach to error-handling there is pretty well thought out). So I wouldn’t try to optimize too hard here, assuming you believe that follow-on CL is well-designed and will go in relatively quickly.
  43. achow101 commented at 10:25 pm on February 27, 2020: member

    @gwillen I think all of my comments still stand for #18027. The cases I mentioned aren’t particularly well handled there either.

    Here are 2 psbts that cause some issues:

    070736274ff010071020000000119cec02d4d6b871d80feb6de5ed937342d9336e5526e131381bc4d6a403a2e3f0000000000fdffffff0200e9a43500000000160014c62c8874a843b36bc064428ef26293144dc23e62d0cff5050000000016001419157d423d5ea8a81889053ed1b405502229ae0a7e000000000100740200000001fb152056b6e92c38d7280b0101ccea8c9cd816d25e44e02860bf06974929cab60100000000feffffff0200ca9a3b000000001976a91411ae3a77c1d64e5f25736f0f7503c7ba07426e7588acb872357700000000160014e769d4e5dd4828602c65a6b548eca940680ab39e7d00000001076a47304402207b02cff4f53b89736bdba8cf6c6be232923f7b62cab0dd35c03edf48ba81f64f0220710cb80236e7a33e067f002f4c2d2c78a20c0a4efeebc0789ffe8cc2712a4bc9012102dcd90a9ea5060041fda8b288b9a369698495d4b986ab76fb6de5f494d046ba64002202028f2577c5721c06e1861297c4d99b92d42578efa4f70910de0f2ee97a7f9ae52f18731118202c00008001000080000000800000000001000000002202030a0abdd877fc063f4243b7a1a48d13798cfdb444dd7fb1bac3450eada7aa95a118731118202c0000800100008000000080010000000300000000
170736274ff01009a020000000219cec02d4d6b871d80feb6de5ed937342d9336e5526e131381bc4d6a403a2e3f0000001000fdffffff18e53a3dbfe1b3c855be9fb4cc84454f8697866016e77921e55f292d1e0ece740000000000fdffffff026c94904100000000160014c62c8874a843b36bc064428ef26293144dc23e62fc1bb9290000000016001477c401d765b76f3c43ececca543ddd8f0cb400d27e000000000100740200000001fb152056b6e92c38d7280b0101ccea8c9cd816d25e44e02860bf06974929cab60100000000feffffff0200ca9a3b000000001976a91411ae3a77c1d64e5f25736f0f7503c7ba07426e7588acb872357700000000160014e769d4e5dd4828602c65a6b548eca940680ab39e7d000000220602dcd90a9ea5060041fda8b288b9a369698495d4b986ab76fb6de5f494d046ba6418731118202c000080010000800000008000000000020000000001011ffcfcae2f000000001600141cc76b754beaaa261e3b9cbb6d708f85d9751984220602df34a1b8478dd1448bac50cfab7d6bf3737d7e210779a23b40d4d852f19d54b818731118202c00008001000080000000800100000001000000002202028f2577c5721c06e1861297c4d99b92d42578efa4f70910de0f2ee97a7f9ae52f18731118202c000080010000800000008000000000010000000022020251f1e9da83aebd9fa4e7664e95168eea376925aca28e1395e4f04437505f02e918731118202c0000800100008000000080010000000200000000

    The first one is finalized but has an invalid signature. The second specifies a non-existant prevout (index is too high).

  44. Sjors force-pushed on Feb 28, 2020
  45. Sjors commented at 10:34 am on February 28, 2020: member

    Addressed feedback, while trying to minimise the impact on #18027. I also rebased because #18027 seems to need that.

    I’m now using a switch for the analyzer result. I made a seperate PR #18220 that fixes the analyzer issue I ran to before #17509 (review), and this PR builds on top of that. That seems preferable my earlier UI workaround. @achow101 your first example now fails when you try to broadcast (on testnet), withbad-txns-inputs-missingorspent. Your second example now gets caught early with “PSBT is incomplete”.

    Maybe it makes sense to, in another PR, improve the analyzer by giving it an optional chain or even a mempool argument?

  46. Sjors force-pushed on Feb 28, 2020
  47. Sjors force-pushed on Feb 28, 2020
  48. achow101 commented at 11:15 pm on February 28, 2020: member
    Should probably rebase on top of #18224
  49. achow101 commented at 11:39 pm on February 28, 2020: member
    ACK, modulo rebase
  50. fanquake added the label Waiting for author on Feb 28, 2020
  51. Sjors force-pushed on Feb 29, 2020
  52. Sjors commented at 8:01 am on February 29, 2020: member
    It’s rebased now.
  53. fanquake removed the label Waiting for author on Feb 29, 2020
  54. meshcollider referenced this in commit 1f886243e4 on Mar 2, 2020
  55. meshcollider commented at 9:51 am on March 2, 2020: contributor
    Thanks Sjors, you can rebase on master now #18224 is merged :)
  56. Sjors force-pushed on Mar 2, 2020
  57. Sjors commented at 12:02 pm on March 2, 2020: member
    @meshcollider rebased
  59. jonasschnelli commented at 4:01 pm on March 2, 2020: contributor

    I created a watch only wallet (disabled private keys), imported an address, and tried to create a watch-only-“unsigned” via the GUI. Though it complained about “The amount exceeds your balance” while fundraw with watching worked.

  60. in src/qt/walletview.cpp:215 in f48130c331 outdated 
    206+        tr("Load Transaction Data"), QString(),
207+        tr("Partially Signed Transaction (*.psbt)"), nullptr);
208+    if (filename.isEmpty()) return;
209+    std::ifstream in(filename.toLocal8Bit().data(), std::ios::binary);
210+    // https://stackoverflow.com/questions/116038/what-is-the-best-way-to-read-an-entire-file-into-a-stdstring-in-c
211+    std::string data(std::istreambuf_iterator<char>{in}, {});
    jonasschnelli commented at 4:11 pm on March 2, 2020:
    Would adding a bound check (max filesize) make sense at this point?
    promag commented at 10:37 am on March 15, 2020:
    Agree. Lame example ln -s /dev/zero /tmp/u.psbt and then open /tmp/u.psbt.
    jonatack commented at 4:56 pm on March 15, 2020:
    Agree.
    Sjors commented at 11:01 am on March 20, 2020:
    Added. You’d think check the file size trivial in C++…
  61. in src/qt/walletview.cpp:219 in f48130c331 outdated 
    210+    // https://stackoverflow.com/questions/116038/what-is-the-best-way-to-read-an-entire-file-into-a-stdstring-in-c
211+    std::string data(std::istreambuf_iterator<char>{in}, {});
212+
213+    std::string error;
214+    PartiallySignedTransaction psbtx;
215+    if (!DecodeRawPSBT(psbtx, data, error)) {
    jonasschnelli commented at 4:16 pm on March 2, 2020:
    thought/note-to-myself: with the PR, we expose DecodeRawPSBT to possible broader userbase (not only RPC users but now also GUI users). Possible parsing exploits would then be a bigger risk.
  62. Sjors commented at 5:09 pm on March 2, 2020: member
    @jonasschnelli walletcreatefundedpsbt should behave more similar to the GUI than fundrawtransaction. Does it also work?
  63. sidhujag referenced this in commit 24a43a9ab0 on Mar 2, 2020
  64. luke-jr referenced this in commit 98667e167e on Mar 5, 2020
  65. luke-jr referenced this in commit e312ebb153 on Mar 5, 2020
  66. luke-jr referenced this in commit 4b349f5f0a on Mar 5, 2020
  67. achow101 commented at 6:10 pm on March 10, 2020: member
    ACK f48130c331c65b090d4073784bee8687cf484e78
  68. in src/qt/walletview.cpp:220 in f48130c331 outdated 
    211+    std::string data(std::istreambuf_iterator<char>{in}, {});
212+
213+    std::string error;
214+    PartiallySignedTransaction psbtx;
215+    if (!DecodeRawPSBT(psbtx, data, error)) {
216+        Q_EMIT message(tr("Error"), tr("Unable to decode PSBT file") + "\n" + QString::fromStdString(error), CClientUIInterface::MSG_ERROR);
    promag commented at 10:40 am on March 15, 2020:
    The error message could include the file path. BTW, is the error message relevant?
    jonatack commented at 4:48 pm on March 15, 2020:
    I agree.
    Sjors commented at 11:01 am on March 20, 2020:

    I think the error message is useful for debugging. This particular one shouldn’t happen anymore, thanks to the size check.

    I don’t think adding the file name is useful, since the user selected it themselves.

  69. in src/qt/walletview.cpp:232 in f48130c331 outdated 
    223+    QMessageBox msgBox;
224+    msgBox.setText("PSBT");
225+    switch (analysis.next) {
226+    case PSBTRole::CREATOR:
227+    case PSBTRole::UPDATER:
228+        msgBox.setInformativeText("PSBT is incomplete. Copy to clipboard for manual inspection?");
    promag commented at 10:45 am on March 15, 2020:
    Missing tr() here and below.
    Sjors commented at 11:02 am on March 20, 2020:
    Fixed
  70. in src/qt/walletview.cpp:235 in f48130c331 outdated 
    226+    case PSBTRole::CREATOR:
227+    case PSBTRole::UPDATER:
228+        msgBox.setInformativeText("PSBT is incomplete. Copy to clipboard for manual inspection?");
229+        break;
230+    case PSBTRole::SIGNER:
231+        msgBox.setInformativeText("Transaction needs more signatures. Copy to clipboard?");
    jonatack commented at 4:49 pm on March 15, 2020:
    Missing transation here and L228.
  71. jonatack commented at 5:03 pm on March 15, 2020: member
    ACK f48130c331 modulo existing comments: 2 missing translations, file size bound check, relevant error message that displays file path, m_current_transaction reset.
  72. Sjors force-pushed on Mar 20, 2020
  73. DrahtBot added the label Needs rebase on Mar 23, 2020
  74. [gui] send dialog: split on_sendButton_clicked 
    This commit does not change behavior.
    6ab3aad9a5
  75. [util] GetFileSize 86e22d23bb
  76. Sjors force-pushed on Mar 24, 2020
  77. Sjors commented at 7:33 pm on March 24, 2020: member
    Rebased after #18278
  78. DrahtBot removed the label Needs rebase on Mar 24, 2020
  79. in src/qt/walletview.cpp:209 in 841ba3562b outdated 
    204+{
205+    QString filename = GUIUtil::getOpenFileName(this,
206+        tr("Load Transaction Data"), QString(),
207+        tr("Partially Signed Transaction (*.psbt)"), nullptr);
208+    if (filename.isEmpty()) return;
209+    const std::streamsize max_file_size = 100000000;
    instagibbs commented at 4:06 pm on March 25, 2020:
    nit: I feel like this should be some sort of filesize constant somewhere
    Sjors commented at 10:02 am on March 26, 2020:
    A UTXO snapshot can be multiple gigabytes, but a wallet or PSBT is expected to be much smaller. So I don’t think there’s a generic value. PSBT also doesn’t specify a maximum size, so PSBT_MAX_FILE_SIZE could give the wrong impression (nothing a comment can’t solve).
  80. in src/qt/walletview.cpp:256 in 841ba3562b outdated 
    251+    case QMessageBox::Yes: {
252+        if (complete) {
253+            std::string err_string;
254+            CTransactionRef tx = MakeTransactionRef(mtx);
255+
256+            TransactionError result = BroadcastTransaction(*clientModel->node().context(), tx, err_string, COIN / 10, /* relay */ true, /* await_callback */ false);
    instagibbs commented at 4:11 pm on March 25, 2020:
    nit: COIN / 10 hmmmmmmmmmmmmmmm let’s re-use DEFAULT_MAX_RAW_TX_FEE_RATE please
    instagibbs commented at 4:14 pm on March 25, 2020:
    s/await_callback/wait_callback/
  81. instagibbs approved
  82. instagibbs commented at 4:15 pm on March 25, 2020: member

    reACK https://github.com/bitcoin/bitcoin/pull/17509/commits/841ba3562bb027c32d4ff4963acb8ca39ec8e945

    did not manually test changes

  83. Sjors force-pushed on Mar 26, 2020
  84. Sjors commented at 10:26 am on March 26, 2020: member

    I moved DEFAULT_MAX_RAW_TX_FEE_RATE to policy.h so the GUI can use it as well. This maximum fee stuff always has difficulty finding a home for itself, @jnewbery @amitiuttarwar thoughts?

    I also introduced MAX_FILE_SIZE_PSBT with a comment that it’s not required by BIP 174, but just a precaution to prevent OOM.

  85. instagibbs commented at 1:52 pm on March 26, 2020: member

    re-ACK https://github.com/bitcoin/bitcoin/pull/17509/commits/e406414d906814de351127f08fc0d06e927b4b81

    mac run having an unrelated sads

  86. Sjors commented at 1:59 pm on March 26, 2020: member
    I lost my authoritah to restart jobs… @MarcoFalke?
  87. instagibbs commented at 2:00 pm on March 26, 2020: member
    hmm me too
  88. MarcoFalke closed this on Mar 26, 2020
  89. MarcoFalke reopened this on Mar 26, 2020
  90. MarcoFalke commented at 2:01 pm on March 26, 2020: member
    Open-Close to re-run ci. See #15847 (comment)
  91. Sjors commented at 2:04 pm on March 26, 2020: member
    I used to be able to just restart individual jobs from within Travis.
  92. MarcoFalke commented at 2:07 pm on March 26, 2020: member
    Travis is broken on all ends. You have to re-login every 24 hours to get a new token to retain the permission to restart builds.
  93. jonasschnelli commented at 2:11 pm on March 26, 2020: contributor
    utACK e406414d906814de351127f08fc0d06e927b4b81 This seems ready for merge. Without further objections, this will get merged after the 0.20 split-off (early April).
  94. jnewbery commented at 3:16 pm on March 26, 2020: member

    I moved DEFAULT_MAX_RAW_TX_FEE_RATE to policy.h so the GUI can use it as well. This maximum fee stuff always has difficulty finding a home for itself, @jnewbery @amitiuttarwar thoughts?

    policy.h really isn’t the right place for this, since it’s nothing to do with mempool acceptance policy. I think you should be using the wallet’s maxtxfee, not an RPC default constant. You can fetch the wallet’s m_default_max_tx_fee by using the getDefaultMaxTxFee() interface method.

  95. Sjors commented at 4:32 pm on March 26, 2020: member
    @jnewbery this isn’t a wallet feature though: you can load and broadcast a PSBT without a wallet (not sure if I implemented it that way, but in principle it should work when compiled without a wallet).
  96. jnewbery commented at 5:00 pm on March 26, 2020: member

    @jnewbery this isn’t a wallet feature though: you can load and broadcast a PSBT without a wallet (not sure if I implemented it that way, but in principle it should work when compiled without a wallet).

    I haven’t reviewed this PR. I just saw that the DEFAULT_MAX_RAW_TX_FEE_RATE was being accessed by a function in walletview.cpp. Is that expected to be available when the wallet isn’t loaded?

  97. Sjors commented at 10:46 am on March 27, 2020: member

    That same constant is also used by sendrawtransaction. It was initially only available to RPC code, so I had to move it somewhere else. I agree policy.h is not a great place for it either, but afaik we don’t have a misc_constants.h :-) My initial thinking was to put it near mempool settings.

    I think right now loading a PSBT uses the current wallet context, and indeed it’s in walletview.cpp. I don’t want to change this PR too much (leaving that to #18027), but it stands to reason some followup will make it work without wallet context.

  98. jnewbery commented at 1:49 pm on March 27, 2020: member

    My initial thinking was to put it near mempool settings.

    I think src/node/transaction.h is probably the best place for it now. Please don’t put non-policy constants in policy.h!

  99. Move DEFAULT_MAX_RAW_TX_FEE_RATE to node/transaction.h 
    So it can be used in the GUI.
    1d05a9d80b
  100. [gui] save PSBT to file 
    co-authored-by: Glenn Willen <gwillen@nerdnet.org>
    f6895301f7
  101. [gui] load PSBT 
    co-authored-by: Glenn Willen <gwillen@nerdnet.org>
    1cd8dc2556
  102. [psbt] add file size limit 764bfe4cba
  103. Sjors force-pushed on Mar 27, 2020
  104. Sjors commented at 1:59 pm on March 27, 2020: member
    Done!
  105. instagibbs commented at 2:04 pm on March 27, 2020: member

    re-ACK https://github.com/bitcoin/bitcoin/pull/17509/commits/764bfe4cba35c24f7627cc425d9e7eba56e98964

    moving constant to src/node/transaction.h only change

  106. in src/qt/sendcoinsdialog.cpp:319 in 764bfe4cba 
    320     if (model->wallet().privateKeysDisabled()) {
321-        questionString.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME));
322+        question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can save or copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME));
323     } else {
324-        questionString.append(tr("Please, review your transaction."));
325+        question_string.append(tr("Please, review your transaction."));
    jonatack commented at 2:48 pm on March 27, 2020:

    nit: s/Please,/Please/

    (not introduced by this PR, can ignore)

  107. in src/qt/sendcoinsdialog.cpp:317 in 764bfe4cba 
    317 
318-    questionString.append("<br /><span style='font-size:10pt;'>");
319+    question_string.append("<br /><span style='font-size:10pt;'>");
320     if (model->wallet().privateKeysDisabled()) {
321-        questionString.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME));
322+        question_string.append(tr("Please, review your transaction proposal. This will produce a Partially Signed Bitcoin Transaction (PSBT) which you can save or copy and then sign with e.g. an offline %1 wallet, or a PSBT-compatible hardware wallet.").arg(PACKAGE_NAME));
    jonatack commented at 2:50 pm on March 27, 2020:
    • s/Please,/Please/
    • s/wallet,/wallet/
    • could omit “e.g.”

    (these were not introduced by this PR, can ignore)

  108. in src/psbt.h:44 in 764bfe4cba 
    39@@ -40,6 +40,10 @@ static constexpr uint8_t PSBT_OUT_BIP32_DERIVATION = 0x02;
40 // as a 0 length key which indicates that this is the separator. The separator has no value.
41 static constexpr uint8_t PSBT_SEPARATOR = 0x00;
42 
43+// BIP 174 does not specify a maximum file size, but we set a limit anyway
44+// to prevent reading a stream indefinately and running out of memory.
    jonatack commented at 2:53 pm on March 27, 2020:
    s/indefinately/indefinitely/ (not worth fixing unless need to retouch)
  109. jonatack commented at 2:58 pm on March 27, 2020: member
    ACK 764bfe4c
  110. achow101 commented at 4:40 pm on March 27, 2020: member
    ACK 764bfe4cba35c24f7627cc425d9e7eba56e98964
  111. kallewoof commented at 6:40 am on April 9, 2020: member
    Concept and code review ACK, but… I can’t figure out how to actually test this. Do I need to start with specific settings?
  112. instagibbs commented at 12:58 pm on April 9, 2020: member
    @kallewoof definitely a power-user setting, but the way to use this is start/use a blank wallet with no privkeys, aka watch-only wallet
  113. Sjors commented at 11:26 am on April 10, 2020: member
    @kallewoof you can try it with a ColdCard (simulator) on testnet. It has a menu option to export wallet keys to Bitcoin Core (added by yours truly: https://github.com/Coldcard/firmware/pull/32), which you can import into a watch-only wallet.
  114. kallewoof commented at 1:54 am on April 11, 2020: member
    Nice. This is apparently merge-ready so not really necessary, but as a pure reviewer (i.e. not involved in the HWW stuff atm), I find it very hard to test this. (I managed to get a watchonly wallet in bitcoin core, but then what? No keys.. I’m sure there’s a way!)
  115. instagibbs commented at 6:46 pm on April 11, 2020: member
    @kallewoof https://github.com/bitcoin-core/HWI/blob/master/docs/bitcoin-core-usage.md this should get you to the point where this PR becomes apparent to use.
  116. jb55 commented at 9:42 pm on April 16, 2020: member

    Tested ACK 764bfe4cba35c24f7627cc425d9e7eba56e98964

    It looks like #18027, #18656 + more? depend on this so it would be awesome to get this in (after 0.20 of course)

  117. in src/qt/sendcoinsdialog.cpp:411 in f6895301f7 outdated 
    407+        QMessageBox msgBox;
408+        msgBox.setText("Unsigned Transaction");
409+        msgBox.setInformativeText("The PSBT has been copied to the clipboard. You can also save it.");
410+        msgBox.setStandardButtons(QMessageBox::Save | QMessageBox::Discard);
411+        msgBox.setDefaultButton(QMessageBox::Discard);
412+        switch (msgBox.exec()) {
    promag commented at 11:06 pm on April 16, 2020:

    f6895301f768220f3ea70231d5cc5b45ecbf4488

    nit, slight preference for

    0const auto button = msgBox.exec();
1if (button == QMessageBox::Save) {
2    ...
3} else {
4    assert(button == QMessageBox::Discard);
5}
  118. promag commented at 11:24 pm on April 16, 2020: member
    Code review ACK 764bfe4cba35c24f7627cc425d9e7eba56e98964.
  119. Sjors commented at 6:29 pm on April 22, 2020: member
    @promag thanks, I’ll look into those suggestions if there’s a rebase need, but prefer to get it merged as-is.
  120. meshcollider merged this on Apr 23, 2020
  121. meshcollider closed this on Apr 23, 2020
  122. Sjors deleted the branch on Apr 23, 2020
  123. sidhujag referenced this in commit 67b508b313 on Apr 23, 2020
  124. laanwj removed this from the "Blockers" column in a project
  125. luke-jr referenced this in commit 372435dbe0 on Jun 9, 2020
  126. luke-jr referenced this in commit 2e7c1145fa on Jun 9, 2020
  127. luke-jr referenced this in commit 330cec9b4f on Jun 9, 2020
  128. luke-jr referenced this in commit f63666968e on Jun 9, 2020
  129. luke-jr referenced this in commit 9bca6ee10a on Jun 9, 2020
  130. luke-jr referenced this in commit 2bb1294cb5 on Jun 9, 2020
  131. luke-jr referenced this in commit cda877b452 on Jun 9, 2020
  132. in src/qt/walletview.cpp:258 in 764bfe4cba 
    253+            std::string err_string;
254+            CTransactionRef tx = MakeTransactionRef(mtx);
255+
256+            TransactionError result = BroadcastTransaction(*clientModel->node().context(), tx, err_string, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), /* relay */ true, /* wait_callback */ false);
257+            if (result == TransactionError::OK) {
258+                Q_EMIT message(tr("Success"), tr("Broadcasted transaction sucessfully."), CClientUIInterface::MSG_INFORMATION | CClientUIInterface::MODAL);
    luke-jr commented at 0:14 am on June 10, 2020:
    Typo
    Sjors commented at 12:53 pm on June 10, 2020:
    @luke-jr that’s already fixed in master
  133. luke-jr referenced this in commit 1daa234315 on Jun 10, 2020
  134. luke-jr referenced this in commit ab2bcb63b6 on Jun 10, 2020
  135. luke-jr referenced this in commit 6d8f0e2f7d on Jun 12, 2020
  136. luke-jr referenced this in commit 77b1643d38 on Jun 12, 2020
  137. luke-jr referenced this in commit ae8963a567 on Jun 12, 2020
  138. luke-jr referenced this in commit 06c6c40c42 on Jun 12, 2020
  139. luke-jr referenced this in commit 836064a133 on Jun 12, 2020
  140. luke-jr referenced this in commit 89c15a25de on Jun 12, 2020
  141. luke-jr referenced this in commit 0366d06908 on Jun 12, 2020
  142. meshcollider referenced this in commit c27330897d on Jun 21, 2020
  143. deadalnix referenced this in commit faa53213a7 on Oct 23, 2020
  144. jasonbcox referenced this in commit 0bef88c34e on Oct 23, 2020
  145. jasonbcox referenced this in commit 2b61196f19 on Oct 23, 2020
  146. jasonbcox referenced this in commit ec039f6600 on Oct 23, 2020
  147. jasonbcox referenced this in commit 9d528e20ef on Oct 23, 2020
  148. jasonbcox referenced this in commit bcfcc43e59 on Oct 23, 2020
  149. sidhujag referenced this in commit eaf7646859 on Nov 10, 2020
  150. DrahtBot locked this on Feb 15, 2022


Sjors DrahtBot practicalswift gwillen promag luke-jr instagibbs achow101 meshcollider jonasschnelli jonatack MarcoFalke jnewbery kallewoof jb55

Labels
GUI

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me