paulmillr / scure-btc-signer

Audited & minimal library for creating, signing & decoding Bitcoin transactions.
https://paulmillr.com/noble/#scure
MIT License
134 stars 33 forks source link

Taproot script path spend doesn't work as expected. #51

Closed finian closed 11 months ago

finian commented 1 year ago

I'm writing a simple demo showing the usage of "Taproot script path spend". (like this article A Guide to creating TapRoot Scripts with bitcoinjs-lib but using scure-btc-signer)

const internalPrivateKey = secp256k1.utils.randomPrivateKey()
const internalPublicKey = schnorr.getPublicKey(internalPrivateKey)

// pay-to-pubkey spend path
const script = btc.Script.encode([ internalPublicKey, 'CHECKSIG' ])
const scriptP2tr = btc.p2tr(internalPublicKey, { script }, btc.TEST_NETWORK, true)

// deposit some btc to scriptP2tr.address
// ...

// spend it
const tx = new btc.Transaction({ allowUnknowInput: true })
tx.addInput({
  txid: 'xxx',
  index: 0,
  witnessUtxo: {
    amount: 10000n,
    script: scriptP2tr.script,
  },
  tapLeafScript: scriptP2tr.tapLeafScript,
  sequence: 0xfffffffd,
})
tx.addOutputAddress(
  'tb1pxxx',
  9000n,
  btc.TEST_NETWORK,
)
tx.signIdx(internalPrivateKey, 0)
tx.finalize()
console.log('tx:', tx.id, tx.hex)

It throws with the error: "No taproot scripts signed".

The main issue is here: https://github.com/paulmillr/scure-btc-signer/blob/e9fdfbc1fe8ce43efa384e1f433f72c21ce25562/index.ts#L2301-L2309

getTaprootKeys function changes the privateKey and publicKey (tweaked with the empty merkleRoot because the schnorrPub and cb.internalKey are the same: internalPublicKey here). So the new tweaked publicKey doesn't match the one in the script (const script = btc.Script.encode([ internalPublicKey, 'CHECKSIG' ])) and it skips thinking there is no public key in tapLeafScript.

If I change the codes eliminating the tweak (getTaprootKeys function does here), keeping the privateKey and publicKey the internal one (internalPrivateKey and internalPublicKey), the demo codes work as expected.

My question is that why tweak the keys here if schnorrPub and cb.internalKey are the same? In my demo, they are the same, being internalPublicKey.

paulmillr commented 1 year ago

@finian

const scriptP2tr = btc.p2tr(internalPublicKey, { script }, btc.TEST_NETWORK, true)

The code looks very weird, since it will be 'A or A' (same key) multisig, which works as Key-spend and Script-spend at the same time.

Please use library API, instead of creating the script manually:

const script = btc.Script.encode([ internalPublicKey, 'CHECKSIG' ])

can be written as:

btc.p2tr_pk(internalPublicKey)

Also, you are missing other fields from scriptP2tr.

My question is that why tweak the keys here if schnorrPub and cb.internalKey are the same? In my demo, they are the same, being internalPublicKey.

Because 'internalKey' is key before tweak and you need to tweak it to get real key.

Here examples how you can do Key-spend / Script-spend / (Script+Key)-spend using library API:


const regtest = { bech32: 'bcrt', pubKeyHash: 0x6f, scriptHash: 0xc4 };
const internalPrivateKey = secp256k1.utils.randomPrivateKey();
const internalPublicKey = schnorr.getPublicKey(internalPrivateKey);

// Key-spend
{
  const scriptP2tr = btc.p2tr(internalPublicKey, undefined, regtest, true);
  const tx = new btc.Transaction({ allowUnknowInput: true });
  tx.addInput({
    txid: 'c061c23190ed3370ad5206769651eaf6fac6d87d85b5db34e30a74e0c4a6da3e',
    index: 0,
    witnessUtxo: {
      amount: 10000n,
      script: scriptP2tr.script,
    },
    ...scriptP2tr,
    sequence: 0xfffffffd,
  });
  tx.addOutputAddress(
    'bcrt1pea3850rzre54e53eh7suwmrwc66un6nmu9npd7eqrhd6g4lh8uqsxcxln8',
    9000n,
    regtest
  );
  tx.signIdx(internalPrivateKey, 0);
  tx.finalize();
  console.log('tx1:', tx.id, tx.hex);
}
// Script-spend
{
  const scriptP2tr = btc.p2tr(undefined, btc.p2tr_pk(internalPublicKey), regtest, true);
  const tx = new btc.Transaction({ allowUnknowInput: true });
  tx.addInput({
    txid: 'c061c23190ed3370ad5206769651eaf6fac6d87d85b5db34e30a74e0c4a6da3e',
    index: 0,
    witnessUtxo: {
      amount: 10000n,
      script: scriptP2tr.script,
    },
    ...scriptP2tr,
    sequence: 0xfffffffd,
  });
  tx.addOutputAddress(
    'bcrt1pea3850rzre54e53eh7suwmrwc66un6nmu9npd7eqrhd6g4lh8uqsxcxln8',
    9000n,
    regtest
  );
  tx.signIdx(internalPrivateKey, 0);
  tx.finalize();
  console.log('tx2:', tx.id, tx.hex);
}
// Or even both (but kinda pointless, since same key): can be spent by using key or script
{
  const scriptP2tr = btc.p2tr(internalPublicKey, btc.p2tr_pk(internalPublicKey), regtest, true);
  const tx = new btc.Transaction({ allowUnknowInput: true });
  tx.addInput({
    txid: 'c061c23190ed3370ad5206769651eaf6fac6d87d85b5db34e30a74e0c4a6da3e',
    index: 0,
    witnessUtxo: {
      amount: 10000n,
      script: scriptP2tr.script,
    },
    ...scriptP2tr,
    sequence: 0xfffffffd,
  });
  tx.addOutputAddress(
    'bcrt1pea3850rzre54e53eh7suwmrwc66un6nmu9npd7eqrhd6g4lh8uqsxcxln8',
    9000n,
    regtest
  );
  tx.signIdx(internalPrivateKey, 0);
  tx.finalize();
  console.log('tx3:', tx.id, tx.hex);
}
/*
tx1: 62c4ee8db462610f38f51ef6ca445ad43b45ae70ac6c5b7701027f9eca626e64 020000000001013edaa6c4e0740ae334dbb5857dd8c6faf6ea5196760652ad7033ed9031c261c00000000000fdffffff012823000000000000225120cf627a3c621e695cd239bfa1c76c6ec6b5c9ea7be16616fb201ddba457f73f010140efc6ba21cf1d84cc4f5206b1c6a30048760c6cdf31582dc806d4497ef46cd462e75b1c49facf481c23fa4a32f28cfd1052be3f3073029aa7c2dad17aeffc8a8700000000
tx2: 62c4ee8db462610f38f51ef6ca445ad43b45ae70ac6c5b7701027f9eca626e64 020000000001013edaa6c4e0740ae334dbb5857dd8c6faf6ea5196760652ad7033ed9031c261c00000000000fdffffff012823000000000000225120cf627a3c621e695cd239bfa1c76c6ec6b5c9ea7be16616fb201ddba457f73f010340f61af68e54b468d0eed288f1b08f997b391d20c5de4cdba84f97d1e6740eab52c47047b1529d067342817e173ee1c90651814c6e00e89119b1159d1e56bf97052220b1537f2baaded53dca0b8eac10304712b9ce2dd26efa73c2e70eb644591a4539ac21c050929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac000000000
tx3: 62c4ee8db462610f38f51ef6ca445ad43b45ae70ac6c5b7701027f9eca626e64 020000000001013edaa6c4e0740ae334dbb5857dd8c6faf6ea5196760652ad7033ed9031c261c00000000000fdffffff012823000000000000225120cf627a3c621e695cd239bfa1c76c6ec6b5c9ea7be16616fb201ddba457f73f0101407fe329753f5a3be9eb9113697f96888e2a72f6b4a6c80edfa76950b71de01c85104816f304fd4f3ebbd5d26c97a45c692b53c6bb68d415bb4a7a62505cc28fe500000000
*/
finian commented 1 year ago

@paulmillr Thanks for your replies.

The code looks very weird, since it will be 'A or A' (same key) multisig, which works as Key-spend and Script-spend at the same time.

It might be weird but it's a valid usage scenario.

Please use library API, instead of creating the script manually:

Actually I have removed extra opcodes in the script to keep the demo codes simple.

Because 'internalKey' is key before tweak and you need to tweak it to get real key.

Yes it's true for key path spending but NOT for the script path spending.

Say we have an internalKey P0, a publicKey P1 in script1 and a publicKey P2 in script2. When we spend using script2, we'll sign the tx with the privateKey corresponding to the P2. It's nothing to do with the internalKey P0, not to mention tweaking the P2.

So we should not tweak the key when dealing with the script path spending, just sign as is. https://github.com/paulmillr/scure-btc-signer/blob/e9fdfbc1fe8ce43efa384e1f433f72c21ce25562/index.ts#L2301-L2306

The third example codes you provided is actually using the key path spending. If we use script path spending, it will hit the issue: "No taproot scripts signed":

// Or even both (but kinda pointless, since same key): can be spent by using key or script
{
  const scriptP2tr = btc.p2tr(internalPublicKey, btc.p2tr_pk(internalPublicKey), regtest, true);
  const tx = new btc.Transaction({ allowUnknowInput: true });
  tx.addInput({
    txid: 'c061c23190ed3370ad5206769651eaf6fac6d87d85b5db34e30a74e0c4a6da3e',
    index: 0,
    witnessUtxo: {
      amount: 10000n,
      script: scriptP2tr.script,
    },
    ...scriptP2tr,
    tapInternalKey: undefined, // <-- here we force a `script path spending`
    sequence: 0xfffffffd,
  });
  tx.addOutputAddress(
    'bcrt1pea3850rzre54e53eh7suwmrwc66un6nmu9npd7eqrhd6g4lh8uqsxcxln8',
    9000n,
    regtest
  );
  tx.signIdx(internalPrivateKey, 0);
  tx.finalize();
  console.log('tx3:', tx.id, tx.hex);
}
paulmillr commented 12 months ago

It might be weird but it's a valid usage scenario.

    tapInternalKey: undefined, // <-- here we force a `script path spending`

The third example codes you provided is actually using the key path spending. If we use script path spending, it will hit the issue: "No taproot scripts signed":

It is because there is no reason to use script path spending if you can use key path spending:

So we should not tweak the key when dealing with the script path spending, just sign as is.

This specific code looks like a leftover from support of nested p2tr inputs (which is unsupported now), so we will remove it. But your use-case looks like a mistake, forcing library to spend more fees than required.

finian commented 12 months ago

My demo codes are from this article: https://dev.to/eunovo/a-guide-to-creating-taproot-scripts-with-bitcoinjs-lib-4oph (source codes: https://github.com/Eunovo/taproot-with-bitcoinjs/blob/main/src/index.ts) I've tested it with different libs including bitcoinjs-lib, tapscript etc. and I failed with scure-btc-signer.

It is because there is no reason to use script path spending if you can use key path spending:

  • Script path will require more fees and space on blockchain.
  • Code which re-uses internal key inside leaf scripts is either:
    • a mistake: leaf script can be removed without affecting actual logic, just creating outputs which will require more fee to spend.
    • a security error: if internal key is accidentally set as one of keys from multi-sig or other complex logic. In this case owner of internal key can spend whole output without any signatures from other wallets.

Yes, you are totally right. In practice, we may not construct scripts like that, but still, I think it's valid according to BIP-0341 (though it's discouraged).

paulmillr commented 12 months ago

We prevent users from doing weird things that degrade security. I think it's fine to not match BIP, esp when it's discouraged there.