bitcoindevkit / bitcoin-ffi

Other
12 stars 7 forks source link

`Txid` type is currently kind of a footgun #20

Open thunderbiscuit opened 1 week ago

thunderbiscuit commented 1 week ago

After trying to use the Txid type in the testing of this pr, I realized a few things I had not yet internalized.

  1. The Txid type is just a typealias on String until you use it in a method that requires an actual Txid. At that point it attempts the conversion, and panics if it can't do it.
  2. This panic is not catchable by the target languages.

Example

If you use a valid Txid, the new type works perfectly, and allows type-safe API on a method like get_tx(txid: Txid). Does the conversion, provides it to the Esplora client in this case, and everything is dandy.

pub fn get_tx(&self, txid: Txid) -> Result<Option<Arc<Transaction>>, EsploraError> {
    let tx_opt = self.0.get_tx(&txid)?;
    Ok(tx_opt.map(|inner| Arc::new(Transaction::from(inner))))
}
// This works
val txid: Txid = "9d0a83df4a556d61338ff18cbc20bf49a3c732805e404a66e25e1579271597a9"
val transaction: Transaction? = esploraClient.getTx(txid)
println("Transaction: ${transaction?.computeTxid()}")

But if your Txid is not valid, you'll get a panic which is not thrown on the JVM side, and therefore cannot be caught.

This crashes your test/app
try {
    val txid: Txid = "imatxidiswear"
    val transaction: Transaction? = esploraClient.getTx(txid)
    println("Transaction: ${transaction?.computeTxid()}")
} catch (t: Throwable) {
    println("Panic: $t") // you'll never get here
}

I just realized this a few hours ago and haven't looked at how/whether we could solve it using the

[Custom]
typedef string Txid;

construction with a Result or if the only way is to have a proper constructor on the Txid type. I'll take a look at this next week!