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

[Prototype/Do-not-Merge] Add NonWitness UTXO in PSBT #3793

Closed

Conversation

jmacato
Copy link
Contributor

@jmacato jmacato commented Jun 19, 2020

DO NOT MERGE since this is only to verify if the solution works

This is a prototype for resolving the Trezor hardware wallet issue.

Depends on MetacoSA/NBitcoin#873
Depends on bitcoin-core/HWI#340
Contributes to #3734

@yahiheb
Copy link
Collaborator

yahiheb commented Jun 19, 2020

This is the psbt when building a transaction

{
  "fee": "0.00000110 BTC",
  "feeRate": "1 Sat/B",
  "tx": {
    "hash": "a4d55d78b55376636a59bfa07a9978070c9f071a09445567990f459363fdeb5c",
    "ver": 1,
    "vin_sz": 1,
    "vout_sz": 1,
    "lock_time": 0,
    "size": 82,
    "in": [
      {
        "prev_out": {
          "hash": "89600973f98c7fba0443ab785cbde2ba94b0b90ce0d8039b8c58d8cd9e7d221a",
          "n": 0
        },
        "scriptSig": ""
      }
    ],
    "out": [
      {
        "value": "0.00099890",
        "scriptPubKey": "0 5353cb751a8616e6f7a9b584f63b612d2dc023af"
      }
    ]
  },
  "inputs": [
    {
      "index": 0,
      "partial_signatures": {},
      "non_witness_utxo": {
        "hash": "89600973f98c7fba0443ab785cbde2ba94b0b90ce0d8039b8c58d8cd9e7d221a",
        "ver": 2,
        "vin_sz": 1,
        "vout_sz": 2,
        "lock_time": 1772273,
        "size": 136,
        "in": [
          {
            "prev_out": {
              "hash": "cdc92401e1b23bd72d412a0dc26f1b13d3f977369a36610312e03c4562959c7e",
              "n": 1
            },
            "scriptSig": "0014148cdae5ffcf48bcef27d7f6222f8af2a976b573",
            "sequence": 4294967294
          }
        ],
        "out": [
          {
            "value": "0.00100000",
            "scriptPubKey": "0 51a1379ddea95aa5a77e00ecc12fe64ec0aa6ab0"
          },
          {
            "value": "0.01851315",
            "scriptPubKey": "0 b0f031e5cddf1befd1428bc305af6c7c2840a357"
          }
        ]
      },
      "witness_utxo": {
        "value": "0.00100000",
        "scriptPubKey": "0 51a1379ddea95aa5a77e00ecc12fe64ec0aa6ab0"
      },
      "bip32_derivs": [
        {
          "pubkey": "0325ccb1f60a3d0640cbc3bfa1cefc34512d50c32d0e7c102b62e18f23ab69fbc5",
          "master_fingerprint": "e5dbc9cb",
          "path": "84'/0'/0'/0/0"
        }
      ]
    }
  ],
  "outputs": [
    {
      "bip32_derivs": [
        {
          "pubkey": "033e76da92a86af2436ad8c77222df038097189d64de79a64d329577b6c8305279",
          "master_fingerprint": "e5dbc9cb",
          "path": "84'/0'/0'/0/2"
        }
      ]
    }
  ]
}

@yahiheb
Copy link
Collaborator

yahiheb commented Jun 19, 2020

there is this error when signing:

2020-06-19 05:24:39 ERROR       SendControlViewModel (361)      WalletWasabi.Hwi.Exceptions.HwiException: PSBT is not sane
   at WalletWasabi.Hwi.HwiClient.ThrowIfError(String responseString, IEnumerable`1 options) in C:\Users\HOME\Desktop\WalletWasabi - Copie\WalletWasabi\Hwi\HwiClient.cs:line 271
   at WalletWasabi.Hwi.HwiClient.SendCommandAsync(IEnumerable`1 options, Nullable`1 command, String commandArguments, Boolean openConsole, CancellationToken cancel, Boolean isRecursion) in C:\Users\HOME\Desktop\WalletWasabi - Copie\WalletWasabi\Hwi\HwiClient.cs:line 57
   at WalletWasabi.Hwi.HwiClient.SignTxImplAsync(Nullable`1 deviceType, String devicePath, Nullable`1 fingerprint, PSBT psbt, CancellationToken cancel) in C:\Users\HOME\Desktop\WalletWasabi - Copie\WalletWasabi\Hwi\HwiClient.cs:line 175
   at WalletWasabi.Hwi.HwiClient.SignTxAsync(HDFingerprint fingerprint, PSBT psbt, CancellationToken cancel) in C:\Users\HOME\Desktop\WalletWasabi - Copie\WalletWasabi\Hwi\HwiClient.cs:line 169
   at WalletWasabi.Gui.Controls.WalletExplorer.SendTabViewModel.BuildTransaction(String password, PaymentIntent payments, FeeStrategy feeStrategy, Boolean allowUnconfirmed, IEnumerable`1 allowedInputs) in C:\Users\HOME\Desktop\WalletWasabi - Copie\WalletWasabi.Gui\Controls\WalletExplorer\SendTabViewModel.cs:line 60
   at WalletWasabi.Gui.Controls.WalletExplorer.SendControlViewModel.<.ctor>b__28_29() in C:\Users\HOME\Desktop\WalletWasabi - Copie\WalletWasabi.Gui\Controls\WalletExplorer\SendControlViewModel.cs:line 346

@yahiheb
Copy link
Collaborator

yahiheb commented Jun 19, 2020

The test was done using the HWI that comes within Wasabi

Copy link
Contributor

@molnard molnard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK. What BitcoinStore do if the tx is not there?

@jmacato
Copy link
Contributor Author

jmacato commented Jun 19, 2020

@molnard it returns false, which skips the input's non segwit field. We can do a warning of some kind if it does that

@lontivero
Copy link
Collaborator

If the transaction is not in the TransactionStore then we are trying to spend a coin that is not ours. That can happen only in coinjoin transactions (wasabi coinjoin and payjoin) in nonde of those cases Trezor (or any other HW at the moment) can be used.

@jmacato jmacato force-pushed the trezor-nonwitnessutxo-fix-prototype branch from b264751 to b44cb53 Compare June 22, 2020 10:34
@molnard
Copy link
Contributor

molnard commented Jun 22, 2020

So we should not support PayJoin with Hardware wallets?

@lontivero
Copy link
Collaborator

@jmacato please make this PR reviewable.

  • NBitcoin 5.0.43 was already released with your chnages. Update it here on in other PR and remove the submodule.
  • Update the transactions in the UpdatePSBTInfo method. That is the method where all the additional info is added.
  • I see HWI binaries for Linux and Windows only. Where is the one for macos?

IMO all these should be done in separate PRs because we need them independently of this specific PR.

@jmacato
Copy link
Contributor Author

jmacato commented Jun 23, 2020

@lontivero yes but to reiterate again: this PR is only for prototyping and i don't recommend reviewing this. I'll be posting series of PR's for this issue tomorrow.

@lontivero
Copy link
Collaborator

So we should not support PayJoin with Hardware wallets?

Trezor requires the inclusion of the transactions that create each coin that is being spent and in payjoin we don't have the transaction for the coin added by the receiver so, given we don't have the required info Trezor will refuse to sign that payjoin transaction.

Note: the receiver could include that transaction in the psbt that he responds with but that requires a change in the bip78 (that bip is being discussed right now so, we could suggest this)

@jmacato
Copy link
Contributor Author

jmacato commented Jun 23, 2020

also i didnt made any macOS build of HWI since docker doesnt support them (and i dont have mac too...) we need to have a new release from HWI

@lontivero
Copy link
Collaborator

I see. What are we going to do with these binaries? I think we have to wait for the official release. @achow101, what is the plan? are you going to release a new version of HWI containing the change for Trezor only or that is just one of many other changes that will be included in a much bigger release version?

@yahiheb
Copy link
Collaborator

yahiheb commented Jun 23, 2020

There is a release candidate bitcoin-core/HWI#340 (comment)

@yahiheb
Copy link
Collaborator

yahiheb commented Jul 1, 2020

#3822 is merged so closing this.

@yahiheb yahiheb closed this Jul 1, 2020
@jmacato jmacato deleted the trezor-nonwitnessutxo-fix-prototype branch October 16, 2020 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants