timestamp timelocks are not satisfied

[LLFourn]

It looks like the logic of finalize_psbt only checks if height based timelocks are satisfied:

                  match desc.satisfy(
                        &mut tmp_input,
                        (
                            PsbtInputSatisfier::new(psbt, n),
                            After::new(current_height, false),
                            Older::new(current_height, create_height, false),
                        ),
                    ) {

In addition, we should check satisfaction of timestamp based timelocks. I will fix this in bdk_core but I thought this was worth noting and fixing ahead of then.

[wszdexdrf]

Maybe I could work on this.

[LLFourn]

Sure. To do this strictly correctly you need to know the "median time past": https://github.com/bitcoin/bitcoin/blob/a4e066af8573dcefb11dff120e1c09e8cf7f40c2/src/chain.h#L290-L302

I think an implementation on latest block timestamp would be better than nothing though?

[LLFourn]

Maybe relevant: https://github.com/rust-bitcoin/rust-miniscript/pull/408

[wszdexdrf]

I have reached a small hiccup. I was going to get blocktimes (for median time past) from the blockchain object, but then realised there is no way of actually accessing the blockchain inside finalize_psbt (no reference passed as an argument). What should we do here?

[LLFourn]

I have reached a small hiccup. I was going to get blocktimes (for median time past) from the blockchain object, but then realised there is no way of actually accessing the blockchain inside finalize_psbt (no reference passed as an argument). What should we do here?

I think just use the last sync height rather than the median time past thingo. Should give pretty good approximation and very rarely make something valid when it is not (and it would not be invalid for long).

(this implies we need to store the time as well as height in DB)

[afilini]

Wait, I'm a bit lost on this: the PR says that this is actually not a problem because miniscript correctly checks for the timestamps as well, so I guess we could close this issue, right?

Or did anybody ever encounter some actual issue trying to finalize a tx with timestamps?

[notmandatory]

Added to alpha release, need to confirm miniscript correctly checks for the timestamps and if so close. Otherwise need to see if/how to fix.

[nondiremanuel]

Need a check to see if it remains in alpha @notmandatory

[notmandatory]

@ValuedMammal per your note in discord. I found a "threshold" test that includes timelocks in the rust-miniscript plan module, but we're not yet using this. It's possible we don't have any tests right now that confirm timestamp timelocks are satisfied. But it should be possible to create a test similar to the one in the plan module: https://github.com/rust-bitcoin/rust-miniscript/blob/45904881e0d6f27f8b81ab310c4739cc14429a9b/src/plan.rs#L863