PSBT Cosign WIP #2557

limpbrains · 2021-02-01T07:31:22Z

closes #976

psbt.cosign.mp4

If psbt can be fully-signed, UI shows TX hex-encoded and ready to be broadcasted.

If psbt can be party-signed, UI shows PSBT

limpbrains · 2021-02-01T08:01:19Z

@Overtorment I'm thinking of how to test it. Let's create 3 wallets. HDLegacyP2PKHWallet, HDSegwitP2SHWallet and HDSegwitBech32Wallet. And one transaction to use UTXO from all of them, then try to sign this psbt using them one by one. In the result it should be new valid transaction.

ncoelho · 2021-02-01T11:02:08Z

tACK, worked well for me.

Some inconsistencies with the current PSBTs and signing flows:
1- Sing button is located on send > top right options (I would prefer to keep it there for now)
2- On current PSBT signed screen we show the "hex string" and not the QR code. Should we change it to QR as well on the other? The QR code + share button, seems to be the more elegant GUI.

Overtorment · 2021-02-01T16:55:52Z

class/wallets/abstract-hd-electrum-wallet.js

+  calculateHowManySignaturesWeHaveFromPsbt(psbt) {
+    let sigsHave = 0;
+    for (const inp of psbt.data.inputs) {
+      if (inp.finalScriptSig || inp.finalScriptWitness || inp.partialSig) sigsHave++;


hmm, this summarizes signatures for all inputs.
theoretically, there could be a tx with 8 inputs, but only 7 are signed. this makes no sense, no one would produce such tx and hope to do smth useful with it, but still.
anyway, for single sig wallets we only care if that method returns something greater than zero or not
(similar to multisig where we care if this method returns something greater than M or not).

I think of it this way: you can have 3 offline wallets and want to create one tx to spend all the funds from them. In this case somewhere in the middle of signing process you will have a PBST with 3 inputs and only 2 of them will be signed

how is transactino crafted, adding inputs one by one? its kind of custom transaction. i dont know if theres a software that allows users to craft it.
but could be useful for payjoins and mixes tho

class/wallets/abstract-hd-electrum-wallet.js

screen/send/ScanQRCode.js

screen/send/psbtSign.js

Overtorment · 2021-02-01T17:18:01Z

screen/send/psbtSign.js

+    Linking.openURL('https://coinb.in/?verify=' + txHex);
+  };
+
+  const exportPSBT = async () => {


exportPSBT we could move to this screen:

so if this screen received not only txhex but also psbt - provide option to export it

not sure about this. Looks like I can break something

screen/wallets/details.js

Overtorment · 2021-02-01T17:41:22Z

also, needs unit test (at the very least, we can use seed for zpub6rDWXE4wbwefeCrHWehXJheXnti5F9PbpamDUeB5eFbqaY89x3jq86JADBuXpnJnSvRVwqkaTnyMaZERUg4BpxD9V4tSZfKeYh1ozPdL1xK ), and probably e2e

limpbrains · 2021-02-06T17:09:45Z

2- On current PSBT signed screen we show the "hex string" and not the QR code. Should we change it to QR as well on the other? The QR code + share button, seems to be the more elegant GUI.

Agree, will do it in another PR

limpbrains · 2021-02-06T17:11:20Z

cosign2.mov

What do you guys think of this one? If everything is ok, I will start working on e2e test

ncoelho · 2021-02-06T17:14:37Z

Is that a send to many (batch) example?

limpbrains · 2021-02-06T17:17:18Z

@ncoelho no

limpbrains · 2021-02-06T17:30:26Z

Example has 2 outputs. One of them is change. But on details page both are shown. I think this is correct behavior.

Imagine having 2 wallets and creating one tx using UTXOs from both of them.
Now you need to sign this tx. If I try to guess change output and hide it from Details screen, then it will depend on signing order. And Details screen will be different in each case.

limpbrains · 2021-02-06T17:32:25Z

Do you think I should move "Coin control" and "Sign a transaction" to the top of actions list?
"Add Recipient" and "Remove Recipient" are the most used options here

ncoelho · 2021-02-06T17:44:00Z

I understand that. But the job of a wallet is to transform outputs in a user/human understandable GUI.

I expect to see the balance I'm sending plus the fee, independently of the number of outputs the transaction is generating. And we can't assume the user knows what change is (that's a big assumption). Change should just be/stay in my wallet in a transparent way.

This is the logic that happens in all transactions flow. We can improve the verify screen with all information, but the confirmation screen should not show outputs.

Balance + address + fee on the confirm screen. User then can verify or send.

ncoelho · 2021-02-06T17:58:02Z

Do you think I should move "Coin control" and "Sign a transaction" to the top of actions list?
"Add Recipient" and "Remove Recipient" are the most used options here

For now I will just try to make as similar as the multisig setup as possible. We do need to "redesign" these screens for consistency later, but let's make them work first. In multisig you have:
1- use full balance
2- import transaction
3- sign transaction

Here you are adding the sign transaction with both importing and scanning, which should be the correct behaviour. So as 2nd option is fine. Leave coin control in the end for now.

limpbrains · 2021-02-06T20:58:19Z

I understand that. But the job of a wallet is to transform outputs in a user/human understandable GUI.

I expect to see the balance I'm sending plus the fee, independently of the number of outputs the transaction is generating. And we can't assume the user knows what change is (that's a big assumption). Change should just be/stay in my wallet in a transparent way.

This is the logic that happens in all transactions flow. We can improve the verify screen with all information, but the confirmation screen should not show outputs.

Balance + address + fee on the confirm screen. User then can verify or send.

Can you please elaborate on what exactly you want me to do. I see 2 options:

try to find change output and show correct info on Confirm. Will work most of the time.
hide Confirm, because we can't show it correctly, user should rely on tools where PSBT was created. It could be BW on another device or HW

ncoelho · 2021-02-08T11:45:38Z

I create a PSBT, imported it to the wallet with the seed. And land on a screen to sign the transaction. Am I doing something wrong?

I would expect to land on the screen with the transaction signed, ready to copy or broadcast.

Screen.Recording.2021-02-08.at.12.40.45.mov

limpbrains · 2021-02-08T12:04:56Z

@ncoelho It can't sign any inputs. Strange. Can't reproduce this. Are you sure psbt was created using zpub from the wallet you are trying to sign tx?

ncoelho · 2021-02-08T13:05:16Z

@ncoelho It can't sign any inputs. Strange. Can't reproduce this. Are you sure psbt was created using zpub from the wallet you are trying to sign tx?

Sorry my mistake. Imported the wrong psbt file on that video. Worked for me now.

ncoelho · 2021-02-08T14:29:07Z

Transaction signed with the seed, gives QR with HEX. QR is imported to online watch-only to broadcast, doesn't recognize HEX. Something is wrong on the flow right? We are giving HEX and watch-only is expecting PSBT?

Screen.Recording.2021-02-08.at.15.25.57.mov

limpbrains · 2021-02-08T15:19:40Z

@ncoelho
I'm not familiar with this flow.

BlueWallet/screen/send/details.js

Line 897 in 52f1bdf

// this looks like NOT base64, so maybe its transaction's hex

It says
this looks like NOT base64, so maybe its transaction's hex
we dont support it in this flow

May be we should? cc @Overtorment

When you generate PSBT there is a "Scan Signed Transaction" button. It works fine.

limpbrains · 2021-02-08T15:21:25Z

2 questions

probably we should change text "This wallet is not being used in conjunction with a hardware wallet. Would you like to enable hardware wallet use?" to something like "This wallet is not being used in conjunction with an offline signing. Would you like to enable it now?
do you think we should move this feature under Anvanced? In order not to overload the interface.

ncoelho · 2021-02-08T15:33:33Z

2 questions

probably we should change text "This wallet is not being used in conjunction with a hardware wallet. Would you like to enable hardware wallet use?" to something like "This wallet is not being used in conjunction with an offline signing. Would you like to enable it now?

do you think we should move this feature under Anvanced? In order not to overload the interface.

Yes, the text should be changed.
We can keep it for now

limpbrains · 2021-02-09T09:11:51Z

class/wallets/abstract-hd-electrum-wallet.js

+      // TODO make this work
+      // try {
+      //   psbt.signInputHD(cc, hdRoot);
+      // } catch (e) {
+      //   // protects agains duplicate cosignings
+      //   if (e.message !== 'Can not add duplicate data to array') throw e;
+      // }


@Overtorment I copy-pasted this from multisig cosign function. This never worked for me in tests. Always throws Need one bip32Derivation masterFingerprint to match the HDSigner fingerprint error.
Can you please take a look? Maybe I should remove this ?

thats because you used fp 00000000 in unit test
signInputHD tries to match path & fp, and it has proper fp from the seed.
PS. this reminds me. when we have option to display xpub for BIP84/49/44 whatever, we should encode fp & path in json inside qr, not just xpub string. i think we had a ticket for this somewhere...

yeah, would be nice to make signInputHD work, and keep the fallback as well (pls add comments)

heres how psbt's input data looks like when BW crafts it (having only Zpub):

Overtorment · 2021-02-16T16:22:19Z

It says
this looks like NOT base64, so maybe its transaction's hex
we dont support it in this flow
May be we should? cc @Overtorment

no, PSBT was specifically invented so developers would not have to pass hex around.
txhex doesn't have necessary metadata so parsing it to sign it would be a guesswork/bruteforce prone to errors

Overtorment

i like how it works in general, tested it on my devices.
but requires some refactoring.
ill probably work on this branch myself.

screen/wallets/details.js

tests/e2e/bluewallet.spec.js

Overtorment · 2021-02-16T17:25:46Z

class/wallets/abstract-hd-electrum-wallet.js

+      // TODO make this work
+      // try {
+      //   psbt.signInputHD(cc, hdRoot);
+      // } catch (e) {
+      //   // protects agains duplicate cosignings
+      //   if (e.message !== 'Can not add duplicate data to array') throw e;
+      // }


thats because you used fp 00000000 in unit test
signInputHD tries to match path & fp, and it has proper fp from the seed.
PS. this reminds me. when we have option to display xpub for BIP84/49/44 whatever, we should encode fp & path in json inside qr, not just xpub string. i think we had a ticket for this somewhere...

Overtorment · 2021-02-16T17:28:47Z

tests/e2e/bluewallet.spec.js

@@ -831,6 +831,61 @@ describe('BlueWallet UI Tests', () => {

    process.env.TRAVIS && require('fs').writeFileSync(lockFile, '1');
  });
+
+  it('can co-sign psbt with HD wallets', async () => {


time comes when we will have to split e2e specs on several scenarios.
one spec should work with the app where HD_MNEMONIC_BIP84 is already imported. and NOT restart the app (currently app wipes and restarts for each test case).
that should save us lots of time on CI

Overtorment · 2021-02-16T17:29:51Z

tests/unit/cosign.test.js

+    // but for now, we will construct psbt by hand
+
+    const sequence = HDSegwitBech32Wallet.defaultRBFSequence;
+    const masterFingerprintBuffer = Buffer.from([0x00, 0x00, 0x00, 0x00]);


incorrect fp

Overtorment · 2021-02-16T22:13:11Z

class/wallets/abstract-hd-electrum-wallet.js

+      // TODO make this work
+      // try {
+      //   psbt.signInputHD(cc, hdRoot);
+      // } catch (e) {
+      //   // protects agains duplicate cosignings
+      //   if (e.message !== 'Can not add duplicate data to array') throw e;
+      // }


yeah, would be nice to make signInputHD work, and keep the fallback as well (pls add comments)

Overtorment · 2021-02-16T22:13:22Z

class/wallets/abstract-wallet.js

@@ -119,6 +119,10 @@ export class AbstractWallet {
    return false;
  }

+  allowCosignPsbt() {


Overtorment · 2021-02-16T22:24:04Z

tests/unit/cosign.test.js

+      '020000000001035fbc74110c2d6fcf4d1161a59913fbcd2b6ab3c5a9eb4d0dc0859515cbc8654f000000006a473044022041df555e5f6a3769fafdbe23bfe29de84a1341b8fd85ffd279e238309c5df07702207cf1628b35ccacdb7d34e20fd46a3bc8adc0b1bd3b63249a3a4442b5a993d73501210316e84a2556f30a199541633f5dda6787710ccab26771b7084f4c9e1104f47667000000806d93fb792280e26304b370abebd237430ceddb5670877f1d2a98d80924a900ad01000000000000008087c9acd9d5714845343b18abaa26cb83299be2487c22da9c0e270f241b4d9cfe0000000017160014a239b6a0cbc7aadc2e77643de36306a6167fad15000000800110270000000000001976a9144dc6cbf64df9ab106cee812c7501960b93e9217788ac0002483045022100efe66403aba1441041dfdeff1f24b5e89ab5728ae7ceb9edb264eee004d5883c02207bf03cb611c9322086ac75fa97c374e9540c911359ede4f62de3c94c429ea2320121027aaff1bd274812d012464be25dc06587287a4b578678e58c949a133b9fb93c7f0247304402207a99c115f0b372d151caf991bb5af9f880e7d87625eeb4233fefa671489ed8e702200e5675b92e4e22b2fe37f563b2a0e75fb81def5a6efb431c7ca3b654ef63fe5801210202ac3bd159e54dc31e65842ad5f9a10b4eb024e83864a319b27de65ee08b2a3900000000',
+    );
+  });
+});


i like the approach. solid test

Overtorment · 2021-02-16T22:35:24Z

class/wallets/abstract-hd-electrum-wallet.js

+      // TODO make this work
+      // try {
+      //   psbt.signInputHD(cc, hdRoot);
+      // } catch (e) {
+      //   // protects agains duplicate cosignings
+      //   if (e.message !== 'Can not add duplicate data to array') throw e;
+      // }


heres how psbt's input data looks like when BW crafts it (having only Zpub):

Overtorment · 2021-02-16T22:44:29Z

screen/send/details.js

+      },
+      () => {
+        const { fromWallet } = this.state;
+        this.props.navigation.navigate('PsbtSign', {


screen PsbtSign is not necessary.
first, you could cosign transaction right on that screen (dont forget to set isLoading) - similar to how its done in importTransactionMultisigScanQr on the same screen.
and then, if you have a txhex - navigate to send/create and pass an option that along with multiline input with txhes should also be rendered animated qr (dynamicqrcode, bc-ur encoded). boom! code reuse. flow unification

Screen unification is a good thing. I just thoungt send/details is already overloaded

limpbrains · 2021-02-17T10:21:26Z

i like how it works in general, tested it on my devices.
but requires some refactoring.
ill probably work on this branch myself.

Sure, you are welcome!

I'm trying to generate a correct fingerprint using:

  const seed = bip39.mnemonicToSeed(process.env.HD_MNEMONIC);
  const root = HDNode.fromSeed(seed);
  const child = root.derivePath(path);
  const fp = child.fingerprint

and use this fingerprint in psbt.addInput

but psbt.signInputHD still doesn't work for me with the same error

Overtorment · 2021-02-17T18:45:21Z

@limpbrains i simplified it. removed unnecessary screen. drawback is that I had to remove the flow with some inputs being unsigned (class still supports it, so we can bring it back in UI anytime - we just need a use case for it).
I reused send/Confirm screen, but screwed up with react markup so visually its a bit broken.
I also uncommented signInputHD and added a testcase that tests it.

This reverts commit 3bc91f4.

GladosBlueWallet · 2021-02-18T12:03:00Z

♫ This was a triumph. I'm making a note here: HUGE SUCCESS ♫

[android in browser] https://appetize.io/app/32vde681zvm7yfk2u8hy735xvw

download apk

limpbrains added the WIP label Feb 1, 2021

Overtorment requested changes Feb 1, 2021

View reviewed changes

limpbrains force-pushed the limpbrains-psbt-sign branch from 55d8e5f to d461305 Compare February 8, 2021 10:47

ADD: psbt cosign

9a51cb8

limpbrains force-pushed the limpbrains-psbt-sign branch from 63b200c to 9a51cb8 Compare February 8, 2021 18:33

limpbrains commented Feb 9, 2021

View reviewed changes

Overtorment requested changes Feb 16, 2021

View reviewed changes

limpbrains added 3 commits February 17, 2021 11:14

FIX: revert skip test

dd3f05e

TST: check wallet.calculateHowManySignaturesWeHaveFromPsbt()

d8cae4a

FIX: revert import sorting in wallets/details

8956472

WIP: ref

fa5175a

Overtorment and others added 11 commits February 17, 2021 19:21

WIP: ref

21c285b

WIP: ref

28e9c24

Merge branch 'master' into limpbrains-psbt-sign

5c5895b

Update confirm.js

3bc91f4

Revert "Update confirm.js"

a42e2ab

This reverts commit 3bc91f4.

REF: Move QRCode on TX Details screen

65306a2

ADD: resolve helpers/scan-qr to null if qr code scanner was closed

bb53d3f

FIX: change loc key for enabling offline signing

800671b

Merge remote-tracking branch 'origin/master' into limpbrains-psbt-sign

f312bba

WIP: fix

c3a36cf

WIP: fix

59d1f89

Overtorment merged commit 5322edc into master Feb 18, 2021

Overtorment deleted the limpbrains-psbt-sign branch February 18, 2021 13:37

limpbrains mentioned this pull request Mar 9, 2021

assume utxo / provide utxo #1019

Closed

PSBT Cosign WIP #2557

PSBT Cosign WIP #2557

Conversation

limpbrains commented Feb 1, 2021 • edited

limpbrains commented Feb 1, 2021

ncoelho commented Feb 1, 2021

Choose a reason for hiding this comment

limpbrains Feb 6, 2021 • edited

Choose a reason for hiding this comment

Overtorment Feb 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Overtorment commented Feb 1, 2021

limpbrains commented Feb 6, 2021 • edited

limpbrains commented Feb 6, 2021

ncoelho commented Feb 6, 2021

limpbrains commented Feb 6, 2021

limpbrains commented Feb 6, 2021

limpbrains commented Feb 6, 2021

ncoelho commented Feb 6, 2021

ncoelho commented Feb 6, 2021 • edited

limpbrains commented Feb 6, 2021

ncoelho commented Feb 8, 2021

limpbrains commented Feb 8, 2021

ncoelho commented Feb 8, 2021

ncoelho commented Feb 8, 2021

limpbrains commented Feb 8, 2021

limpbrains commented Feb 8, 2021

ncoelho commented Feb 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Overtorment commented Feb 16, 2021 • edited

Overtorment left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

limpbrains commented Feb 17, 2021

Overtorment commented Feb 17, 2021

GladosBlueWallet commented Feb 18, 2021

limpbrains commented Feb 1, 2021 •

edited

limpbrains Feb 6, 2021 •

edited

Overtorment Feb 16, 2021 •

edited

limpbrains commented Feb 6, 2021 •

edited

ncoelho commented Feb 6, 2021 •

edited

ncoelho commented Feb 8, 2021 •

edited

Overtorment commented Feb 16, 2021 •

edited