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.

Sign up for GitHub

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

Jump to bottom

Height and Time derive Deserialize without checks #2559

Closed
Kixunil opened this issue Mar 9, 2024 · 8 comments · Fixed by #2582
Closed

Height and Time derive Deserialize without checks #2559

Kixunil opened this issue Mar 9, 2024 · 8 comments · Fixed by #2582
Labels
bug
Milestone
0.32.0

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Mar 9, 2024

This is obviously wrong and would be great to fix for next release. It's also a backport candidate.
@Kixunil Kixunil added the bug label Mar 9, 2024
@Kixunil Kixunil added this to the 0.32.0 milestone Mar 9, 2024
@apoelstra
Copy link
Member

Ouch. I think these De/serialize implementations are really weird and we shouldn't have released them.

For the two LockTime types I think we should de/serialize both as consensus-encoded u32s with checks. For Height and Time we should just use #[serde(from = LockTime)] and #[serde(try_into = LockTime)] (and we should implement the appropriate From/TryInto to make this work).

And for relative::LockTime we should in turn use #[serde(try_from = Sequence)] and #[serde(into = Sequence)].

In other words everything just passes through to the "real" type that's part of a transaction.

@apoelstra
Copy link
Member

apoelstra commented Mar 9, 2024 •
edited
Loading

Other options are:

  • Keep the current situation, which for relative::LockTime is actually fine (both Height and Time are represented as u16s and all values are legal). For absolute we have to add error checking as you say.
  • Drop Deserialize and Serialize for Height and Time in both modules, since it's weird to be using these types outside of a LockTime.

tcharding added a commit to tcharding/rust-bitcoin that referenced this issue Mar 13, 2024
@tcharding
fix: Manually implement serde traits
e7135d6 
Currently we are deriving the serde traits for the `absolute::{Height,
Time}` types, this is incorrect because we maintain an invariant on
the inner `u32` of both types that it is above or below the threshold.

Manually implement the serde traits and pass the deserialized `u32` to
`from_consensus` to maintain the invariant.

Close: rust-bitcoin#2559
@tcharding tcharding mentioned this issue Mar 13, 2024
Merged
@tcharding
Copy link
Member

Flagging that, as mentioned above, this issue should only relate to absolute::{Height, Time} types.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Mar 13, 2024

OK, I suggest serde(try_from = "LockTime") for the absolute types and don't touch others.

@apoelstra
Copy link
Member

Yeah, that seems reasonable to me, if a little inconsistent between the two locktime types.

My preference would be to either to try_from = "LockTime" for both relative and absolute, or drop Deserialize/Serialize for Height and Time for both relative and absolute.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Mar 13, 2024

a little inconsistent between the two locktime types.

Oh, I now realized why. It's a good question which is better.

@tcharding
Copy link
Member

OK, I suggest serde(try_from = "LockTime") for the absolute types and don't touch others.

I don't understand this statement, I changed the issue description to be only about Height and Time - are you saying you disagree with the change?

IMO the serde stuff for both LockTime structs is correct as is.

tcharding added a commit to tcharding/rust-bitcoin that referenced this issue Mar 18, 2024
@tcharding
fix: Manually implement serde traits
feff559 
Currently we are deriving the serde traits for the `absolute::{Height,
Time}` types, this is incorrect because we maintain an invariant on
the inner `u32` of both types that it is above or below the threshold.

Manually implement the serde traits and pass the deserialized `u32` to
`from_consensus` to maintain the invariant.

Close: rust-bitcoin#2559
@tcharding
Copy link
Member

Could be considered as part of #2612

apoelstra added a commit that referenced this issue Mar 20, 2024
@apoelstra
Merge #2582: fix: Manually implement serde traits
51bb6a2 
911f8cb fix: Manually implement serde traits (Tobin C. Harding)

Pull request description:

  Currently we are deriving the serde traits for the `absolute::{Height, Time}` types, this is incorrect because we maintain an invariant on the inner `u32` of both types that it is above or below the threshold.

  Manually implement the serde traits and pass the deserialized `u32` to `from_consensus` to maintain the invariant.

  Close: #2559

ACKs for top commit:
  apoelstra:
    ACK 911f8cb
  sanket1729:
    ACK 911f8cb.

Tree-SHA512: 28f9f3e23b2016e00216e5e94f35e22a5bfa6b29e1dd30fa3351575eafc2e47dbd98aa8e51631de2f09eec8d881b0e5e79231185c7a293c1ba0170a90fbbd19d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
0.32.0
Development

Successfully merging a pull request may close this issue.

fix: Manually implement serde traits tcharding/rust-bitcoin
3 participants
@Kixunil @apoelstra @tcharding