bitcoinerlab / descriptors

A TypeScript library for parsing Bitcoin Descriptors, including Miniscript-based ones. Streamlines creating Partially Signed Bitcoin Transactions (PSBTs) from Descriptors. Features BIP32, single-signature, and Hardware Wallet signing capabilities, and facilitates finalizing transactions.
https://bitcoinerlab.com/modules/descriptors
41 stars 14 forks source link

`updatePsbt` throws an Error when attempting to add another UTXO belonging to the same timelocked descriptor/address #15

Closed 0xbrito closed 1 year ago

0xbrito commented 1 year ago

This is what will be thrown: https://github.com/bitcoinerlab/descriptors/blob/48b47b1d6dbd1a1eb5df5f7c3a6abf864637dced/src/psbt.ts#L202-L206

Is this somewhat intended? If locktime isn't undefined for the next UTXO it will always trigger the condition above.

https://github.com/bitcoinerlab/descriptors/blob/48b47b1d6dbd1a1eb5df5f7c3a6abf864637dced/src/descriptors.ts#L664

Possible Solution

Ensure that psbt.locktime and this.getLockTime are not equal before passing a value.

// src/descriptors.ts

   ...(psbt.locktime !== this.getLockTime() ? { locktime: this.getLockTime()  } : {}), // <-- L#664

And also check for psbt.locktime inside src/psbt.ts

// src/psbt.ts

  if (locktime !== undefined || psbt.locktime !== 0) { // <-- L#210

Please, let me know if I'm getting something wrong.

landabaso commented 1 year ago

The timelock is set at the tx level. It can only be set once. If the timelock has already been set already then an Error is thrown as a protection. It will throw even when trying to add the same timelock because users may have been unintentionally adding inputs now knowing about this limitation.

I'm not sure how you hit this problem. Did you try adding 2 inputs that require a timelock? Then this is the reason.

0xbrito commented 1 year ago

Yes, agree. That's why I'm suggesting to skip that subsequent locktime update if it were already set before.

And yes, I was trying to add 2 inputs that spends from the same descriptor (time-locked) hence they both require a timelock.

landabaso commented 1 year ago

The idea is that the library protects users from messing up with timelocks.

The timelock is set at the tx level, which means that all the inputs should agree on it.

If one of your input descriptors requires setting up a timelock, then the rest should not try to set it up again.

And yes, I was trying to add 2 inputs that spends from the same descriptor (time-locked) hence they both require a timelock.

Could you clarify a bit? It's not possible to have 2 inputs that try to spend the same output.

Do you have some code I could take a look at?

0xbrito commented 1 year ago

Oh sorry, @landabaso for late response. I see that you just some changes related to this.