Remove `PackedLockTime` in favor of `absolute::LockTime` #1330

apoelstra · 2022-10-19T16:42:05Z

This is potentially a controversial PR, but hear me out. My argument is that right now absolute::LockTime and PackedLockTime are basically identical types; they can be converted between each other with From and they have exactly the same semantics except that PackedLockTime has Ord on it. This makes it very confusing to tell which one should be used, for example in PSBT2 where there are now extra locktime-related fields.

The motivation for having PackedLockTime independent of LockTime are:

PackedLockTime is theoretically more efficient because you don't need to unpack the field, don't need to store a enum discriminate, etc. I don't buy this. If you are trying to save individual bytes in your transaction-parsing there are lots of places you'd look before messing around with locktimes, so we shouldn't privilege this specific thing.
PackedLockTIme has an Ord impl, and removing that will have a cascading effect on transactions, blocks, etc., preventing them from being held in BTreeMaps etc. My proposal, implemented here, is to just manually impl Ord on Transaction and don't impl it on LockTime.

I recall some argument that we need to be able to sort miniscripts, and miniscripts might have locktimes in them, but I think this is wrong -- miniscripts always have explicitly either a Height or a Time, and there is no problem ordering these.

Closes #1455

apoelstra · 2022-10-19T16:42:16Z

cc @Kixunil @sanket1729 @tcharding

tcharding · 2022-10-19T18:39:15Z

Can you hack up a branch in rust-miniscript that depends on this branch to demo please, since that is where the drama with Ord came from. If there is no rush I can do it tomorrow for you, won't get to it today though.

Kixunil · 2022-10-19T19:05:45Z

Regarding optimization being kinda pointless I both agree with it and dislike it. :D

just manually impl Ord on Transaction and don't impl it on LockTime.

This sounds reasonable, let me sleep on it first but I'm leaning towards ACK here now.

Kixunil

It's sad we would have to change the API again. I guess it proves the value of my "didn't change for a while" requirement for 1.0.

Kixunil · 2022-10-19T19:08:40Z

bitcoin/src/blockdata/transaction.rs

+            .then(self.input.cmp(&other.input))
+            .then(self.output.cmp(&other.output))
+    }
+}


One thing I just realized: the fields are not ordered in the same order they are on the wire. Could it be a problem?

It's been this way since the beginning of this library and nobody has complained, but I'm happy to change this to match the wire ordering just in case.

Hard question since we could break someone's code by changing it. There isn't any standardized sort order of transactions, is there?

No, there isn't. If I had to guess, the only specific sort order that anybody is using is one based on TXIDs, for indexing purposes.

Our Ord impl is purely here to make supporting ordered collections easy.

Kixunil · 2022-10-19T19:10:07Z

bitcoin/src/blockdata/locktime/absolute.rs

-        write!(f, "{:X}", self.0)
-    }
-}
-


Is #[deprecated] type PackedLockTime = LockTime; missing here?

Oh, sure, I can add this (if this is one of the cases where deprecation works..)

FWIW the resulting type will not quite have the same API as the old one .. though they will match in a lot of cases.

Oh, yes, at least Ord will be missing which was originally one of the main points of having the type... The deprecation message would still help I guess. I think it works on type but I may be misremembering.

Kixunil · 2022-12-09T21:06:04Z

OK, I think it's concept ACK from me. But I do want some doc explaining all this. :) Will review after rebase.

apoelstra · 2022-12-11T17:09:29Z

I am in the process of rebasing this, but FYI we have explicit tests right now about serde de/serialization, and in particular we test that LockTime and PackedLockTime deserialize differently.

Since these will become the same type, I will need to unify them somehow, which is an unfortunate breaking change.

apoelstra · 2022-12-11T17:50:13Z

Rebased. Watch out for the new commit 1b26aac5dd0cce544f357126c5c459ad38ac5710 which unifies the serde impls, breakingly.

This will be tested in a later commit, when `PackedLockTime` is folded into this type so all its tests apply to `LockTime`.

The next commit will be a mechanical s/PackedLockTime/LockTime/; this commit seemed like the easiest way to facilitate that.

This can be replicated by deleting the `type PackedLockTime = LockTime' line, and then running find . -type f | xargs sed -i 's/PackedLockTime/LockTime/g at the root of the repo.

tcharding · 2022-12-12T00:16:33Z

The code looks good to me, I wanted to mention that with this applied deriving Ord for Terminal in rust-miniscript becomes impossible and implementing it manually is painful because Terminal is a big enum and the manual implementation requires a match statement which calls cmp on each variant.

Kixunil · 2022-12-12T09:54:25Z

Terminal is a big enum and the manual implementation requires a match statement which calls cmp on each variant.

Sounds like a job for a macro?

(I'm low on time rn so reviewing will take a while :( )

apoelstra · 2022-12-12T13:39:02Z

Yes, we can macro-ize it, or just remove the impl entirely in favor of PartialOrd since this type is kinda-sorta internal, or create our own OrdLockTime wrapper just for Terminal.

Good to be aware of, but this seems like a problem that's both solvable and not the responsibility of this library.

tcharding · 2022-12-12T22:33:01Z

Cool, I'm happy with that.

tcharding

Added the packed_lock_time comment because I'm requesting changes to remove Ord.

tcharding · 2022-12-12T22:39:18Z

bitcoin/src/blockdata/locktime/absolute.rs

@@ -179,8 +62,7 @@ impl fmt::UpperHex for PackedLockTime {
 /// ```
 #[allow(clippy::derive_ord_xor_partial_ord)]
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
-#[cfg_attr(feature = "serde", serde(crate = "actual_serde"))]
+#[derive(Ord)] // will be removed in next commit


This never gets removed, claimed as part of 51f8b9a3 drop Ord on absolute::LockTime; add Ord to Transaction but not done.

Oops, sorry! I rebased this like 20 times and must have lost it.

tcharding · 2022-12-12T22:40:32Z

bitcoin/src/blockdata/locktime/absolute.rs

+        let packed_lock_time = crate::parse::hex_u32(s)?;
+        Ok(Self::from_consensus(packed_lock_time))


Perhaps this local variable could be better name, packed_lock_time is an artifact of PackedLockTime, right?

No, I think it's fine ... we are parsing a u32, which can reasonably be described as a "packed locktime" then we're unpacking it into an enum type.

This still has the line let lock_time = absolute::LockTime::from_height(psbt.unsigned_tx.lock_time.to_consensus_u32() + lock_time_delta).unwrap(); I'm unsure whether this "adding height to a locktime" concept is a meaningful thing or just the sort of thing that shows up in example code. Maybe we should have first-class support for it. Note that the line, as written, depends on the fact that the original locktime was a small blockheight. A proper function for this would handle the exceptional case gracefully.

sanket1729

concept ACK and code review ACK. modulo Tobin's comment about removal of Ord

apoelstra · 2022-12-13T15:05:12Z

Rebased to remove the derive(Ord).

sanket1729

code review ACK 1b15a13

tcharding

ACK 1b15a13

apoelstra · 2022-12-14T18:08:38Z

@Kixunil can you say more about what documentation you want for this?

I'm going to merge this as-is, I"m happy to do a followup documenting things. But the LockTime type itself is already heavily documented, and the conversion to/from u32 that occurs during consensus encoding is self-contained and "documented" to the same degree that anything related to bitcoin transaction serialization formats are.

tcharding · 2022-12-14T21:02:08Z

But the LockTime type itself is already heavily documented

ooooooo yeah, that was all me, writing docs like its my job :)

Kixunil · 2022-12-14T22:04:39Z

@apoelstra something in the changelog to help people understand it and some paragraph in both Transaction documentation pointing out that the ordering is based on bytes not semantics. And some paragraph on LockTime as well with mentions of each-other.

apoelstra · 2022-12-14T22:21:56Z

@Kixunil there is no byte-based ordering. I'm not sure what you want me to add to LockTime.

I will add a changelog entry, yes.

Kixunil · 2022-12-14T22:23:23Z

Huh? How are transactions ordered then?

apoelstra · 2022-12-14T22:31:26Z

@Kixunil ah, yes, transactions are ordered using the locktime as sort field and in this case the locktimes are compared using their consensus encoding.

I'm happy to document this, but this is the same ordering that Transaction has ever had, and I'm not aware of any semantic meaning to the Transaction sort order, nor can I imagine people expecting one to exist.

Kixunil · 2022-12-14T22:36:27Z

Basically I just want to have a heads up along the lines "this contains a non-Ord field and changes the ordering semantics of it in the PartialOrd implementation" Yeah, it's not something super-weird but having it visible so people can have it in the back of their minds when doing things with ordering is nice.

…rust-bitcoin#1330

…shenanigans in #1330 02c1cd6 add some documentation clarifying the locktime ordering shenanigans in #1330 (Andrew Poelstra) Pull request description: Updates the CHANGELOG and also the doccomment on `Transaction`. ACKs for top commit: tcharding: ACK 02c1cd6 Kixunil: ACK 02c1cd6 sanket1729: ACK 02c1cd6 Tree-SHA512: e2d23a90fb1e53758449fe49a3db7ae1497a260ce7efcade4b50265fa70840db273609019590d9d0a69e1272607a6bcf37924b805b4f09909487eb0c3b91a3cd

apoelstra force-pushed the 2022-10--no-packedlocktime branch from 7605f57 to 8a6d5a7 Compare October 19, 2022 17:06

Kixunil reviewed Oct 19, 2022

View reviewed changes

apoelstra mentioned this pull request Oct 26, 2022

Add the "planning" module rust-bitcoin/rust-miniscript#481

Closed

apoelstra mentioned this pull request Dec 9, 2022

PackedLockTime is missing From<Height> and From<Time> #1455

Closed

apoelstra force-pushed the 2022-10--no-packedlocktime branch from 8a6d5a7 to e4f6fc6 Compare December 11, 2022 17:49

apoelstra force-pushed the 2022-10--no-packedlocktime branch from e4f6fc6 to d446105 Compare December 11, 2022 18:35

apoelstra added 4 commits December 11, 2022 18:48

locktime: unify serde impls

74ff494

locktime: add FromHexStr impl for LockTime

fa81568

This will be tested in a later commit, when `PackedLockTime` is folded into this type so all its tests apply to `LockTime`.

delete PackedLockTime by aliasing it to LockTime

4dee116

The next commit will be a mechanical s/PackedLockTime/LockTime/; this commit seemed like the easiest way to facilitate that.

remove PackedLockTime type

5b7d801

This can be replicated by deleting the `type PackedLockTime = LockTime' line, and then running find . -type f | xargs sed -i 's/PackedLockTime/LockTime/g at the root of the repo.

apoelstra force-pushed the 2022-10--no-packedlocktime branch from d446105 to 74ee243 Compare December 11, 2022 19:18

tcharding requested changes Dec 12, 2022

View reviewed changes

apoelstra added 3 commits December 13, 2022 14:52

drop Ord on absolute::LockTime; add Ord to Transaction

821842e

run cargo clippy and fmt

1b15a13

sanket1729 reviewed Dec 13, 2022

View reviewed changes

apoelstra force-pushed the 2022-10--no-packedlocktime branch from 74ee243 to 1b15a13 Compare December 13, 2022 15:05

sanket1729 approved these changes Dec 13, 2022

View reviewed changes

tcharding approved these changes Dec 14, 2022

View reviewed changes

apoelstra merged commit f231617 into rust-bitcoin:master Dec 14, 2022

apoelstra deleted the 2022-10--no-packedlocktime branch December 14, 2022 18:21

This was referenced Dec 14, 2022

Implemented unsized Script #1155

Merged

Add weight utilities to TxIn and TxOut #1467

Merged

apoelstra mentioned this pull request Dec 15, 2022

add some documentation clarifying the locktime ordering shenanigans in #1330 #1475

Merged

apoelstra added a commit to apoelstra/rust-bitcoin that referenced this pull request Dec 15, 2022

add some documentation clarifying the locktime ordering shenanigans in …

02c1cd6

…rust-bitcoin#1330

tcharding added the release notes mention label Feb 27, 2023

		let packed_lock_time = crate::parse::hex_u32(s)?;
		Ok(Self::from_consensus(packed_lock_time))

Remove PackedLockTime in favor of absolute::LockTime #1330

Remove PackedLockTime in favor of absolute::LockTime #1330

Uh oh!

Conversation

apoelstra commented Oct 19, 2022 • edited by Kixunil Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Oct 19, 2022

Uh oh!

tcharding commented Oct 19, 2022

Uh oh!

Kixunil commented Oct 19, 2022

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Dec 11, 2022

Uh oh!

apoelstra commented Dec 11, 2022

Uh oh!

tcharding commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Dec 12, 2022

Uh oh!

apoelstra commented Dec 12, 2022

Uh oh!

tcharding commented Dec 12, 2022

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Dec 13, 2022

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Dec 14, 2022

Uh oh!

tcharding commented Dec 14, 2022

Uh oh!

Kixunil commented Dec 14, 2022

Uh oh!

apoelstra commented Dec 14, 2022

Uh oh!

Kixunil commented Dec 14, 2022

Uh oh!

Remove `PackedLockTime` in favor of `absolute::LockTime` #1330

Remove `PackedLockTime` in favor of `absolute::LockTime` #1330

apoelstra commented Oct 19, 2022 •

edited by Kixunil

Loading

Kixunil commented Dec 9, 2022 •

edited

Loading

tcharding commented Dec 12, 2022 •

edited

Loading