Repository navigation
Conversation
This change will be reverted when units reverts to 1.63. For now, bitcoin-units 1.0 use rust version 1.74, so pulling in the compat layer in one crate requires the crate MSRV to raise for all dep graph.
Enable the compat feature of bitcoin 0.32.101 and patch bitcoin to the compat layer branch, on root workspace and in the embedded and bitcoind-tests. The branch lives on fork so its state stays under control.
Switch RelLockTime and the satisfier internals to use stable Sequence from the compat layer. Block conversions to and from the unstable sequence for a clean breaking major with no commitment to keep old type. Eventually LockTime will be upgraded and UnstableSequence cases associated with it will be cleared. Churn here is a tradeoff of bissectability and ease of review by clear commit separation. Disallow bitcoin::Sequence in lint.
Switch max_weight_to_satisfy return type and the witness script size check conditional weight type to stable through compat later. The api for Weight has not changed, no conversions needed. Disallow bitcoin::Weight force using new type on lint.
Switch every absolute lock time to stable. Convert at psbt boundaries with to_stable because of primitives. The stable type renames from_time to from_mtp. Disallow bitcoin::absolute::LockTime in lint.
Switch relative lock time to the compat relative::LockTime. Some inevitable churn related to Sequence already stable the satisfier and interpreter no longer dip in unstable, those allows go away. Kill conversions to the unstable locktime for callers. Disallow bitcoin::relative::LockTime in lint.
12eacdf to
9e8b3e6
Compare
|
FWIW from miniscript 14 onward we should just bump the MSRV. |
|
@tcharding just pinging |
| lock_time: absolute::LockTime::ZERO, | ||
| lock_time: bitcoin::absolute::LockTime::ZERO, |
There was a problem hiding this comment.
Why introduce SequenceUnstable but use bitcoin::absolute::LockTime?
This is @apoelstra's place but FWIW I'm currently leaning towards preference for the latter (ie using path to distinguish and not use an alias). Whichever we choose we should be uniform.
There was a problem hiding this comment.
Sequence is part of a Bitcoin transaction and there is only one kind.
vs LockTime where there are two kinds, absolute and relative, so I prefer using one layer of path.
The bitcoin:: part I have no opinion about.
There was a problem hiding this comment.
I don't know what I was talking about. The comment does not match the code context. My bad.
| Some(Sequence(1000).to_relative_lock_time().unwrap()), | ||
| Some(relative::LockTime::from_consensus(1000).unwrap()), |
There was a problem hiding this comment.
So this change is interesting. How you did it in the other place (changing to bitcoin::Sequence confused me, so I guess this is the clearer way. Not sure though.
There was a problem hiding this comment.
It was a little alarming to see the direct Sequence(1000) constructor which would bypass all invariants (though for Sequence, unlike the locktime types, I believe there are no invariants). So this is definitely clearer.
I'm not sure what "the other place" you're referring to is.
There was a problem hiding this comment.
'other place' I think is above but I cannot really tell what I was talking about. So please ignore.
Built on top of units 1.0 compat layer. Mirrors the rust-psbt and corepc demos.
Done against version 13.x, so that I can test on BDK which is in 13.x.
Changes:
bitcoin::compat.[patch.crates-io]or they resolve bitcoin 0.32.6 (todo: nothing in them was migrated here, awaiting primitives).I noticed that
TxIn,TxOutandTransaction(from primitives) are responsible for all lint exceptions, because they are using the unstable types, awaiting primitives compat layer.