-
Notifications
You must be signed in to change notification settings - Fork 77
Replace SystemTime with bitcoin::absolute::Time #1021
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.
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
Conversation
Convert timestamp handling throughout the v2 implementation to use bitcoin::absolute::Time instead of std::time::SystemTime for better consistency with Bitcoin ecosystem conventions. Add helper functions now() and now_as_unix_seconds() in uri::v2 module to centralize time operations. Update SessionContext, error types, and all related functionality to work with the new Time type. This change eliminates potential conversion issues between different time representations and provides better integration with Bitcoin's timestamp handling patterns used elsewhere in the codebase."
nothingmuch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review very thoroughly, since I have some feedback on the approach.
I would prefer if this wrapped bitcoin::absolute::Time in our own struct, so that the bitcoin Time isn't part of our public API but just an implementation detail, and so that we can support typesafe time calculations
| let now_seconds = crate::uri::v2::now_as_unix_seconds(); | ||
| let expiry_seconds = now_seconds + TWENTY_FOUR_HOURS_DEFAULT_EXPIRY.as_secs() as u32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since part of the goal of this change is using the typesystem to represent the time values we support more accurately, I think would prefer if instead of a allowing raw u32s for this, you defined a newtype Time(bitcoin::absolute::Time) which supports addition with our own Duration(u32) type, which has TryFrom<std::time::Duration> that forbids subsecond resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
|
|
||
| /// Get the current time as a bitcoin::absolute::Time with second precision. | ||
| pub(crate) fn now() -> Time { | ||
| Time::from_consensus(now_as_unix_seconds()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with a newtype of our own, we this could be SystemTime::now().into() given a From<SystemTime> impl
| Ok(Self(session_context)) | ||
| } | ||
|
|
||
| pub fn with_expiry(self, expiry: Duration) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a suggestion, at least not yet, but perhaps this could take an expiry: IntoTime where IntoTime is a sealed trait like IntoUri, and then Duration could convert to Time by being added to now(). Having such a conversion seems unwise for general time handling but i think since the only purpose is specifying timeouts in our API that should be fine. Then the conversion and use of the clock can be feature gated for nostd builds without requiring two different APIs, in std builds you can pass a duration or an absolute time, and in e.g. wasm builds you have to provide an abs time.
@DanGould what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather get this across the line with the simplest type (Time?) in the API instead of Duration and then am open to a follow up with a sealed trait that lets you pass Duration as IntoTime for std builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think that would be fine from a semver PoV as strictly speaking it would only expand the set of types that are allowed to appear in that argument position
I could go either way on this. We've already got rust-bitcoin types all over the place in our public API with no plans to remove it. Can we not support typesafe time calculations without this? |
we could but only with an extension trait, and arguably it's not typesafe in the same sense, you could inadvertantly take a transaction's nlocktime value parsed into a Seconds(Time) and then pass the Time value in as an expiry time, even though that probably doesn't make sense. this might happen accidentally due to variable shadowing or something like that, e.g. using the newtype pattern means we specify a BIP 77 expiry time that only happens to overlap in definition with the seconds based bitcoin consensus version, but they are statically distinguishable from each other. additionally since we introduce it, we can impl whatever we want for it, whereas for the bitcoin units' Time struct we can only define an extension trait, that seems more complex not simpler to me |
|
Replaced by #1047 |
Fixes #893
Problem
When roundtrip testing a new concrete
v2::PjParamsstruct,SystemTime's nanosecond precision causes test failures. After serializingSystemTimeinto the EX1 u32 expiration parameter and deserializing back, precision is lost andassert_eqfails. This highlights a fundamental mismatch betweenSystemTime's nanosecond precision and the protocol's second-precision requirements.Additionally,
SystemTimeis not available in WASM environments, limiting the crate's portability.Solution
Replace
std::time::SystemTimewithbitcoin::absolute::Timethroughout the v2 payjoin implementation for the following reasons:Why
bitcoin::absolute::Timeover alternatives:bitcoin::absolute::Time (Chosen)
Changes
SystemTimeusage withbitcoin::absolute::Timenow()andnow_as_unix_seconds()utility functions inuri::v2moduleSessionContextand error types to useTimeSystemTimeis unavailableThis change eliminates potential conversion issues between different time representations and provides better integration with Bitcoin's timestamp handling patterns used elsewhere in the codebase."
##AI disclosure
i used clause to understand the problem better i also used it to cross check the system::Time implementations in code base i also consulted Claude if the direction is okay
Please confirm the following before requesting review:
AI
in the body of this PR.