bitcoin-sv / ts-sdk

Other
51 stars 13 forks source link

[DISCUSSION] addMerklePath as a method on Transaction class #57

Closed sirdeggen closed 6 months ago

sirdeggen commented 6 months ago

Summary

We should add a new method to the Transaction class which is for formally adding a merkle path to it.

Motivation

Important because at the moment you just add it to the Transaction instance which allows you to add any random MerklePath, without checking it at all.

Description

At the moment we do:

const tx = Transaction.fromHex('...someTx')
tx.merklePath = MerklePath.fromHex('...someRandomBUMP')

// Then when you serialize everything works
tx.toHexBEEF()

It should throw error because the BUMP is not for the current Tx, but it never checks. So I propose we do something like:

tx.addMerklePath(MerklePath.fromHex('...someRandomBUMP'))

Where the method checks that the txid of the current Tx is contained within the BUMP data.

ty-everett commented 6 months ago

While I can understand the desire for this in some cases, doing it might also be trying to hand-hold the library consumers too much. In general, if you haven't checked everything already ahead of time, you should use tx.verify() to ensure it's all good, before trying to serialize or send previously untrusted data to peers or the network. This checks the current merkle path, and also any that may have been added to input transactions.

It's always possible to do this kind of member variable assignment in TypeScript, so this might just be adding another (slightly safer) way to set it, and we'd be relying on good documentation for people to use it. But we should encourage them to make use of .verify() for untrusted data regardless, and that could be all that's needed.