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

gui: save and load PSBT #17509

Open
wants to merge 6 commits into
base: master
from
Open

gui: save and load PSBT #17509

wants to merge 6 commits into from

Conversation

@Sjors
Copy link
Member

Sjors commented Nov 18, 2019

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

Schermafbeelding 2020-01-04 om 20 39 34

Schermafbeelding 2020-01-04 om 20 40 35

Schermafbeelding 2020-01-04 om 20 41 12

Schermafbeelding 2020-01-04 om 20 41 28

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:
Schermafbeelding 2020-01-04 om 21 08 57

Offer to send if complete:

Schermafbeelding 2020-01-04 om 21 09 06

Tell user if signatures are missing, offer to copy to clipboard:
Schermafbeelding 2020-01-04 om 21 15 57

Incomplete for another reason:

Schermafbeelding 2020-01-04 om 21 07 51

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 18, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #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.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 19, 2019

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 :)

@Sjors Sjors mentioned this pull request Nov 19, 2019
1 of 2 tasks complete
}

// PSBTAnalysis analysis = AnalyzePSBT(psbtx);
// bool have_all_sigs = (analysis.next == PSBTRole::FINALIZER) || (analysis.next == PSBTRole::EXTRACTOR);

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 20, 2019

Author Member

@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.

@Sjors Sjors force-pushed the Sjors:2019/11/gui-psbt-save branch 2 times, most recently from 208b9f0 to a2654c0 Nov 23, 2019
@Sjors Sjors force-pushed the Sjors:2019/11/gui-psbt-save branch from a2654c0 to 3eebd15 Nov 26, 2019
@gwillen

This comment has been minimized.

Copy link
Contributor

gwillen commented Dec 9, 2019

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?

src/qt/walletview.cpp Outdated Show resolved Hide resolved
@promag

This comment has been minimized.

Copy link
Member

promag commented Dec 23, 2019

Concept ACK.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Jan 2, 2020

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.

@Sjors Sjors force-pushed the Sjors:2019/11/gui-psbt-save branch 3 times, most recently from 1fc4e70 to 1f57fbe Jan 2, 2020
@Sjors Sjors marked this pull request as ready for review Jan 4, 2020
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Jan 4, 2020

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).

@Sjors Sjors mentioned this pull request Jan 4, 2020
@Sjors Sjors force-pushed the Sjors:2019/11/gui-psbt-save branch from 1f57fbe to e6ea593 Jan 6, 2020
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Jan 6, 2020

I dropped the dependency on #17463; should be easy to rebase. This is now ready for review.

@gwillen

This comment has been minimized.

Copy link
Contributor

gwillen commented Jan 28, 2020

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.
@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Jan 29, 2020

No way to actually sign an incomplete one?

Maybe it should load raw signed transactions too...

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Jan 29, 2020

@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.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Mar 24, 2020

Rebased after #18278

@DrahtBot DrahtBot removed the Needs rebase label Mar 24, 2020
Copy link
Member

instagibbs left a comment

reACK 841ba35

did not manually test changes

src/qt/walletview.cpp Outdated Show resolved Hide resolved
src/qt/walletview.cpp Outdated Show resolved Hide resolved
@Sjors Sjors force-pushed the Sjors:2019/11/gui-psbt-save branch from 841ba35 to e406414 Mar 26, 2020
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Mar 26, 2020

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.

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 26, 2020

re-ACK e406414

mac run having an unrelated sads

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Mar 26, 2020

I lost my authoritah to restart jobs... @MarcoFalke?

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 26, 2020

hmm me too

@MarcoFalke MarcoFalke closed this Mar 26, 2020
@MarcoFalke MarcoFalke reopened this Mar 26, 2020
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 26, 2020

Open-Close to re-run ci. See #15847 (comment)

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Mar 26, 2020

I used to be able to just restart individual jobs from within Travis.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 26, 2020

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 26, 2020

utACK e406414
This seems ready for merge. Without further objections, this will get merged after the 0.20 split-off (early April).

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 26, 2020

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.

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Mar 26, 2020

@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).

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 26, 2020

@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?

@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Mar 27, 2020

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.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 27, 2020

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!

Sjors and others added 4 commits Mar 26, 2020
So it can be used in the GUI.
co-authored-by: Glenn Willen <gwillen@nerdnet.org>
co-authored-by: Glenn Willen <gwillen@nerdnet.org>
@Sjors Sjors force-pushed the Sjors:2019/11/gui-psbt-save branch from e406414 to 764bfe4 Mar 27, 2020
@Sjors

This comment has been minimized.

Copy link
Member Author

Sjors commented Mar 27, 2020

Done!

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Mar 27, 2020

re-ACK 764bfe4

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

Copy link
Member

jonatack left a comment

ACK 764bfe4

} else {
questionString.append(tr("Please, review your transaction."));
question_string.append(tr("Please, review your transaction."));

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2020

Member

nit: s/Please,/Please/

(not introduced by this PR, can ignore)

if (model->wallet().privateKeysDisabled()) {
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));
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));

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2020

Member
  • s/Please,/Please/
  • s/wallet,/wallet/
  • could omit "e.g."

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

@@ -40,6 +40,10 @@ static constexpr uint8_t PSBT_OUT_BIP32_DERIVATION = 0x02;
// as a 0 length key which indicates that this is the separator. The separator has no value.
static constexpr uint8_t PSBT_SEPARATOR = 0x00;

// BIP 174 does not specify a maximum file size, but we set a limit anyway
// to prevent reading a stream indefinately and running out of memory.

This comment has been minimized.

Copy link
@jonatack

jonatack Mar 27, 2020

Member

s/indefinately/indefinitely/ (not worth fixing unless need to retouch)

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Mar 27, 2020

ACK 764bfe4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.