Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error messages on satisfiers #721

Open
futurechimp opened this issue Aug 15, 2024 · 1 comment
Open

Better error messages on satisfiers #721

futurechimp opened this issue Aug 15, 2024 · 1 comment

Comments

@futurechimp
Copy link

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
Copy link
Member

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:

  • The Satisfier and AssetProvider traits currently return very weak error information. Maybe these errors could be extended. Though we should be mindful of users who don't want potentially large memory usage.
  • Having said this, it's not only developers who want this information ... actual users, when satisfaction fails, would like to know "why" and we should be able to tell them.
  • Alternately, Assets itself could track failed satisfactions somehow (similar to LoggerAssetProvider). Though this isn't nearly as nice because the user has to be aware of it in order to use it, and the default path will lead to poor error messages which may bubble up to the user seeing an opaque "could not satisfy".
  • In this case the asset provider did successfully make a plan (since the Descriptor::plan believes the locktimes you tell it and doesn't know about any actual transaction), and successfully updated the PSBT (since Plan::update_psbt_input looks only at the input and also doesn't know about the whole transaction), and a failure only occurs at the satisfy stage. At this point the library really ought to "know what is supposed to happen" and be able to tell the user. Also, the fact that this happened because of an interaction between Txin::default and an obscure BIP65 rule is pretty nasty.
  • In general, while there is no single reason for satisfaction to fail, in many common specific cases there are. Any top-level conditions, or conditions which are in a series of ands to the top level, must be satisfied and if they're not this is the "reason".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants