LedgerHQ / app-bitcoin-new

Modern Bitcoin Application based on PSBT and Descriptors
Apache License 2.0
93 stars 69 forks source link

Using sha256 returns an error (0x6a80) #257

Closed AndreasGassmann closed 3 months ago

AndreasGassmann commented 4 months ago

I have been experimenting with miniscript and ledger, and I ran into numerous issues. If I register a simple policy like wsh(sortedmulti(2,@0/**,@1/**)), everything seems to work. So I assume my project is set up correctly.

For our use-case, we would need a single public key in the script, but it seems, according to https://github.com/LedgerHQ/app-bitcoin-new/issues/210 and other issues I found, that this is not yet possible.

I saw that according to the source code, the sha256 instruction should be supported: https://github.com/LedgerHQ/app-bitcoin-new/blob/master/src/common/wallet.h#L139. However, I was not able to get this to work. This is the code I tried:

const multisigPolicy = new WalletPolicy(
  "Test",
  'wsh(sha256(9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08))', // Hash of "test"
  []
)
const [policyId, policyHmac] = await app.registerWallet(multisigPolicy)

I receive the following error:

TransportStatusError: Ledger device: Invalid data received (0x6a80)

I have tried many different ways, eg. using a @0 placeholder and passing the hash in the keys array, I also tried other instructions like ripemd, but no luck.

Am I doing something wrong here, or is this feature not supported? According to https://github.com/LedgerHQ/app-bitcoin-new/blob/develop/doc/wallet.md it should work because the possible values are any valid [miniscript](https://bitcoin.sipa.be/miniscript) template (only inside top-level wsh, or in TREE).. (NOTE: I'm not sure if my miniscript is valid, it cannot be analyzed by https://bitcoin.sipa.be/miniscript. However, I tried other examples with the sha256 hash and they also didn't work, the example I used here is the simplest form to eliminate any other errors.)

bigspider commented 4 months ago

The app doesn't just check the validity of the miniscript, but also whether it is sane, that is, it satisfies a bunch of stricter security properties. Read for example here.

A sha256() fragment is not sane because satisfying it doesn't require a signature. This is generally unsafe because it makes the transaction malleable: someone could see your transaction in the mempool, and make a different valid transaction that sends somewhere else the money of an input protected with such a script (that is: they can steal the money).

A wallet policy like and_v(v:pk(@0),sha256(9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08)) (compiled from and(pk(A),sha256(9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08)) on https://bitcoin.sipa.be/miniscript) should work correctly, as the presence of a signature removed the malleability concerns. In general, you always want at least a signature as part of the spending policy.

AndreasGassmann commented 4 months ago

Thanks so much for your response. The explanation, and example, make a lot of sense! We didn't know that additional checking is done, so we were trying to test with the most simple example. It would have been great if it would have not returned a generic "unknown error" or "invalid data", but rather something more specific.

We were able to get a bit further. But maybe let me back up a bit and explain what we try to do (probably also relevant to https://github.com/LedgerHQ/app-bitcoin-new/issues/210).

We are building an app where we generate user specific "deposit" addresses that are controlled by a multisig. The multisig itself would simply be multisorted(1,pk1,pk2,pk3), so a 1 out of 3 multisig. All these keys are owned by our system and are there for redundancy and legal purposes. Having this multisig with the same keys for everyone would obviously generate the same address for every user. That's why we want to add data that is linked to the user so both the user and us can verify that the deposit address is linked to them.

const script = `
wsh(
  or_d(
    multi(1,@0/<0;1>/*,@1/<0;1>/*,@2/<0;1>/*),
    and_v(
      v:0, // False, this should be unspendable
      sha256(9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08) // Hash contains the user data, so everyone who has access to the input can verify the miniscript / redeemScript / deposit address.
    )
  )
)`

We have originally simply used an OP_DROP instruction for the additional data, but it seems that Ledger cannot sign that and most BTC wallets cannot understand the descriptor. That's why we're trying to go with miniscript approach. This was how we constructed the redeemScript before: const redeemScriptHex = `${targetRecipientPackageHash}${OP_DROP}${OP_1}${sortedPubKeys[0]}${sortedPubKeys[1]}${sortedPubKeys[2]}${OP_3}${OP_CHECKMULTISIG}`;

In any case, with your help, we were able to create the valid miniscript above and can register it on the ledger.

Then we came across the next problem. How to create a PSBT so we can sign it with our newly generated WalletPolicy.

I will post the relevant parts of the script here:

// Get the master key fingerprint
const fpr = await app.getMasterFingerprint()
console.log('Master key fingerprint:', fpr.toString('hex'))

// Get App and Version
const version = await app.getAppAndVersion()
console.log('App and version 1:', version)

// Define the miniscript
const script = `
wsh(
  or_d(
    multi(1,@0/<0;1>/*,@1/<0;1>/*,@2/<0;1>/*),
    and_v(
      v:0,
      sha256(9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08)
    )
  )
)`

// Remove all whitespaces from the script
const scriptWithoutWhitespaces = script.replace(/\s+/g, '');

// Our key info
const ourKeyInfo = `[${fpr}/48'/1'/0'/2']${ourPubkey}`;

// Create the multisig policy
const multisigPolicy = new WalletPolicy(
    "Test",
    scriptWithoutWhitespaces,
    [
        ourKeyInfo,
        "tpubDCBWBScQPGv4Xk3JSbhw6wYYpayMjb2eAYyArpbSqQTbLDpphHGAetB6VQgVeftLML8vDSUEWcC2xDi3qJJ3YCDChJDvqVzpgoYSuT52MhJ",
        "tpubDCUQwB7GDsQKGfGk1CpCxzkWwWQodwKRttFB55vhCbMu8RGdQZ1k2ayVXmdJrER313963TTB4dRdx12JLjjBNpcs3v6shG93ci6A2XiGuJN",
    ]
)

// Register the wallet
const [policyId, policyHmac] = await app.registerWallet(multisigPolicy)

// Get the wallet address
const bitcoinAddress = await app.getWalletAddress(multisigPolicy, policyHmac, 0, 1)

// Construct the PSBT
const psbt = new PsbtV2()
psbt.setGlobalPsbtVersion(2);
psbt.setGlobalInputCount(1)
psbt.setGlobalOutputCount(1)

// Set the input
psbt.setInputPreviousTxId(0, Buffer.from("1b642d5ad08c84b561128953b235f8cb474ba825a77e32e1805f902dc40a9d4c", "hex"))
psbt.setInputOutputIndex(0, 5)
psbt.setInputWitnessUtxo(0, 1000, Buffer.from("002068aa57a9ac1b988447a169af51f486e0615958124db3b1279a10d4bcb1b76a62", "hex"))
psbt.setInputRedeemScript(0, Buffer.from("2102f48be6a1d632d18acbdf75d506ea220aa307f63c828ddb0caaf40bd2384473c3ac", "hex"))
psbt.setInputBip32Derivation(0, Buffer.from("02f48be6a1d632d18acbdf75d506ea220aa307f63c828ddb0caaf40bd2384473c3", "hex"), Buffer.from("558f6baa", "hex"), [0, 1])

// Set the output
psbt.setOutputAmount(0, 700)
psbt.setOutputScript(0, Buffer.from("a914630dcbfa906fe91d7dfd9dd608c9f582b244f2b287", 'hex'))

// Signing the PSBT
// Here we get the error: "TransportStatusError: Ledger device: Invalid data received (0x6a80)"
const psbtResult = await app.signPsbt(psbt, multisigPolicy, policyHmac, () => {
    // This is never logged
    console.log('PROGRESS!')
})

// We never get here
console.log('PSBT result:', psbtResult)

NOTE: We were testing around a lot, some values in the PSBT may be inconsistent or wrong. But I doubt that the ledger transport/app checks the chain, so signing should still be possible, even if the tx is then not valid on the network.

I was not able to find an example of how to construct / validate / sign a PSBT constructed this way. I'm unsure what data has to be provided, eg. it seems that we don't have access to the redeemScript that gets generated inside the walletPolicy? And do we have to provide the Bip32Derivation?

Thanks again for your help and I hope you can shed some light on the issue we are having now as well.

bigspider commented 4 months ago

If the PSBT is wrong, there is a very high chance that the app would throw an error, as there are many checks that depend con the content of the PSBT.


I don't think that's a good use case for using hashes in miniscript. Two immediate problems:

I think you should rather look for solutions that generate unique pubkeys for each user based on the user_hash. Assuming that the user_hash is 32 bytes, a solution might be to generate "synthetic xpubs" for each user as follows:

Repeating this process for each of the three keys, you can then use a simply policy like wsh(multi(1,@0/<0;1>/*,@1/<0;1>/*,@2/<0;1>/*)) (or the same with sortedmulti). This avoid wasting space, and makes it impossible for an external observer to distinguish these UTXOs from any other 1-of-3 multisig (assuming that the user_hash of each user is kept private).

(Note that the loss of the user_hash would lead to funds being unaccessible; but that's generally true for the loss of the complete descriptor)

There might of course be other approaches.

dcale commented 4 months ago

If the PSBT is wrong, there is a very high chance that the app would throw an error, as there are many checks that depend con the content of the PSBT.

ok, but do you have an example of a PSBT that is "right"? Like which fields are required, etc?

Your proposed solution is also interesting but would not solve the issue, because even for a normal multisig (or even a 'wsh(pk(XXX))') we cannot create a valid PSBT ledger would sign.

bigspider commented 3 months ago

If the PSBT is wrong, there is a very high chance that the app would throw an error, as there are many checks that depend con the content of the PSBT.

ok, but do you have an example of a PSBT that is "right"? Like which fields are required, etc?

Your proposed solution is also interesting but would not solve the issue, because even for a normal multisig (or even a 'wsh(pk(XXX))') we cannot create a valid PSBT ledger would sign.

Sorry, I missed this message.

BIP-0174 details the expected fields. Ledger signs transactions that follow the model described in BIP-388, which adds some specific restrictions on the kind of descriptors that can be used.