Modify locktime serde implementations - #4511
Conversation
github-actions
Bot
added
C-bitcoin
|
The CI failure looks real. I don't know exactly what is causing it. Can you take a look if you have a second please? My guess is its because of the way we |
|
|
||
| #[cfg(feature = "serde")] | ||
| impl<'de> serde::Deserialize<'de> for LockTime { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> |
There was a problem hiding this comment.
In 0aea1a0:
Why is this function implementation so big instead of symmetrically (to serialize) just calling u32::deserialize(deserializer)? then attempting to convert the result to a locktime?
There was a problem hiding this comment.
I'm skeptical that you'd need your last patch if you did this.
There was a problem hiding this comment.
I believe this is a serde bug/feature. I think we do have to have the visitor and the kludge to get things working on 32 bit machines.
There was a problem hiding this comment.
FTR I tried removing it and running it through CI just now.
There was a problem hiding this comment.
If this didn't work then the the implication is that serde doesn't work for u32. If true, I don't see why it'd be our problem.
Github has kinda hidden the CI failures but I think I found the failed run you are alluding to and it appears to be a compilation issue around a function called visit_u32 which wouldn't be needed if we were just going through u32.
There was a problem hiding this comment.
I think that is the correct implication. That is what I understand from the code comments already there. I'll remove it again and push it up so you can see the error.
42e6477 to
af4915f
Compare
|
For the record, since Github has completely erased this history, the commit before last was 42e6477 which I authored and pushed. |
|
@tcharding in your version why did you revert the implementations of |
af4915f to
7ebf25c
Compare
|
I'm confused because:
Anywho - I pushed a version without the kludge and using your combinators and refactor to |
|
And I'll put the kettle on, that should help ... |
|
Looks good now but needs rebase. |
Make test code use uniform spacing - twitch averted. Whitespace only, no logic change.
The `units::locktime` types are used for two things: - They are the inner types of `primitives` `LockTime`s - They are used ephemerally for checking satisfaction Neither of these use cases requires `serde` impls for the `units` types. Since we are trying to release 1.0 with minimal amounts of code we should remove them. For `LockTime`s that need to be stored on disk or go over the wire we can manually implement the `serde` traits. For `absolute::LockTime` this is done already and there is no reason the `relative::LockTime` impl cannot be the same [0]. This differs from the current `serde` trait impls but we have already decided that in 0.33 we are going to accept breakage and direct users to use 0.32 to handle it. - Remove `serde` stuff from `units::locktime` - Manually implement `serde` traits on `relative::LockTime` - Fix the regression test to use the new format While we are at it use a uniform terse call in `serialize`. [0] This is because there is an unambiguous encoding for the whole set of locktimes - consensus encoding.
7ebf25c to
4621d2b
Compare
2 participants
Patch 1 is preparatory clean up. Patch 2 is the meat and potatoes. See commit log there for full explanation.
Briefly: