rust-bitcoin / rust-miniscript

Support for Miniscript and Output Descriptors for rust-bitcoin
Creative Commons Zero v1.0 Universal
343 stars 135 forks source link

Better error messages on satisfiers #721

Open futurechimp opened 1 month ago

futurechimp commented 1 month ago

I've been learning rust-miniscript, and when attempting to try out timelocks, I kept hitting a CouldNotSatisfy when finalizing my psbt. The reason turned out to be totally of my own doing - I had inadvertantly used a Sequence::MAX instead of a Sequence::ZERO when constructing my spend transaction. I had effectively disabled locktime usage, because I had done this:

 let txin = TxIn {
        previous_output,
        ..Default::default()
    }; 

instead of this:

 let txin = TxIn {
        previous_output,
        sequence: Sequence::ZERO,
        ..Default::default()
    }; 

I was so preoccupied trying to figure out how signing worked, how to use the plan module etc that I didn't notice this.

In order to figure out the problem, I ran my own local forks of bdk_wallet, rust-miniscript, and bitcoin crates, instrumenting them with printlns until I figure out what the (dumb, self-inflicted) problem was.

It would be supersonic if satisfier errors gave a more specific reason for failure - I wonder if it would be possible for instance to turn CouldNotSatisfy into something like CouldNotSatisfy("here is the reason")? Or perhaps something more typed like (in the case I just dealt with) CouldNotSatisfy(LockTimeDisabled)?

apoelstra commented 4 weeks ago

So, in general there is no single "reason" that satisfaction fails. You can imagine that instead of a and(pk, after) descriptor you had an or(and(pk, after), pk) descriptor. Now there are two branches. If one fails due to a bad sequence number and the other fails because the pubkey is missing, what is "the reason" that satisfaction failed?

Having said this, throughout the codebase we have dozens of somewhat-obscure reasons for failing satisfaction. In this case in src/psbt/mod.rs:318 we have a check if !self.psbt.unsigned_tx.input[self.index].enables_lock_time() which failed, meaning that we didn't even query the satisfier to see if the locktime might be met. Somehow we should communicate to the user that we did this.

A few observations: