Lily-Technologies / lily-wallet

The best way to secure your bitcoin.
https://lily-wallet.com
Other
94 stars 15 forks source link

Account and change addresses are not recognized #36

Open bjarnemagnussen opened 4 years ago

bjarnemagnussen commented 4 years ago

Currently when sending from a vault the wallet will add the change output, however without adding meta information to the PSBT that would allow hardware wallets to understand it is a change output.

To allow hardware devices to understand it is a change output the following "per-output" types for the change output should be set as defined by BIP-174: Lily Wallet should set the key PSBT_OUT_WITNESS_SCRIPT with value corresponding to the witness script of the change output, and set keys for PSBT_OUT_BIP32_DERIVATION with fingerprints and derivation paths for the public keys involved in the change output.

Likewise to be done for account addresses if they should also be recognized by the devices.

KayBeSee commented 4 years ago

Is this supported in bitcoinjs-lib?

bjarnemagnussen commented 4 years ago

bitcoinjs-lib does support it.

In addition to adding the values to the output data as explained above, it would also be a good idea to add the extended pubkey globally, which would allow Trezor devices to verify that the change outputs are for the exact same cosigners as for the input(s).

I don't know too much JavaScript, but looking into the bitcoinjs-lib I think I found which values must be set.

On the output data: The values have to be added to the psbt.addOutput method here https://github.com/KayBeSee/lily-wallet/blob/634e8d21ea1203c11e57b43ad50ac00f7ccee764/src/pages/Send/utils.js#L185-L190

The fields to add are witnessScript and bip32Derivation, similar to how they are done for psbt.addInput.

I tried to see if the values can be found on the unusedChangeAddress[0] object, but I don't know enough JavaScript and couldn't figure it out.

But as an example it should look like:

psbt.addOutput({
      script: address.toOutputScript(unusedChangeAddresses[0].address, currentBitcoinNetwork),
      value: spendingUtxosTotal.minus(outputTotal).toNumber(),
      witnessScript: Bytes.from('00', 'hex'),
      bip32Derivation: [{
         masterFingerprint: Buffer.from('abcabc', 'hex'),
         pubkey: Buffer.from('000000', 'hex'),
         path: "m/48'/0'/0'/2'/1/0"
       },{
         masterFingerprint: Buffer.from('defdef', 'hex'),
         pubkey: Buffer.from('111111', 'hex'),
         path: "m/48'/0'/0'/2'/1/0"
       }, {...}]
})

Additionally, to add the extended pubkey globally, you can use psbt.updateGlobal like this:

psbt.updateGlobal({
    globalXpub: [{
      extendedPubkey: Buffer.from('02aa7ed3XPUB1', 'hex'),
      masterFingerprint: Buffer.from('abcabc', 'hex'),
      path: "m/48'/0'/0'/2'"
    }, {
      extendedPubkey: Buffer.from('02aa7ed3XPUB2', 'hex'),
      masterFingerprint: Buffer.from('defdef', 'hex'),
      path: "m/48'/0'/0'/2'"
    }, {...}]
})
bjarnemagnussen commented 4 years ago

I was trying to hard-code values and see if my Trezor would recognize the change output, which never was the case. For Electrum it is not a problem but when I used a PSBT Electrum made and sent it to the Trezor with the HWI that Lily Wallet uses the Trezor did again not recognize the change output.

It seems that setting extended public keys in the global is not supported by the bitcoin-core HWI does yet, see bitcoin-core/HWI#355. Hopefully this will change soon.