feat(gossipsub): fallible sequence number generation and error handling #6211
Conversation
| fn new() -> Result<Self, SequenceNumberError> { | ||
| let unix_timestamp = SystemTime::now() | ||
| .duration_since(SystemTime::UNIX_EPOCH) | ||
| .expect("time to be linear") | ||
| .as_nanos(); | ||
| .map_err(|_| SequenceNumberError::ClockBeforeUnixEpoch)? | ||
| .as_millis(); | ||
|
|
||
| Self(unix_timestamp as u64) | ||
| let timestamp = u64::try_from(unix_timestamp).map_err(|_| SequenceNumberError::Overflow)?; | ||
|
|
||
| Ok(Self(timestamp)) | ||
| } | ||
|
|
||
| fn next(&mut self) -> u64 { | ||
| self.0 = self | ||
| .0 | ||
| .checked_add(1) | ||
| .expect("to not exhaust u64 space for sequence numbers"); | ||
| fn next(&mut self) -> Result<u64, SequenceNumberError> { | ||
| self.0 = self.0.checked_add(1).ok_or(SequenceNumberError::Overflow)?; | ||
|
|
||
| self.0 | ||
| Ok(self.0) |
There was a problem hiding this comment.
Generally I agree that we should avoid all possible panics.
That said, I think in both cases here it's safe to assume that they won't panic. If the system time of a user is this far of they'll' run into major issues anyway. And I don't think the sequence number can ever go above 2**64.
There was a problem hiding this comment.
I am aware that panicking is unlikely here. I only added it as an improvement. The real issue was that the doc comment referred to milliseconds but the code was actually using nanoseconds.
There was a problem hiding this comment.
Ah yeah, you are right. Changing to milliseconds sounds good to me, or alternatively just fix the docs.
Technically, we do violate backwards compatibility because with this change the sequence number of all peers will "jump" back to a much lower value. By linearity increasing, a peer could eventually start re-using numbers that have been used in the past, which violates the specs: https://github.com/libp2p/specs/blob/69c4fdf5da3a07d2f392df6a892c07256c1885c0/pubsub/README.md?plain=1#L136-L142
That said, I don't think it will ever happen in practice. The old ns-based sequence numbers are a such huge fraction larger than the new ms-based ones will ever be. cc @jxs
Sequence number creation and incrementing are now fallible instead of panicking. Also, previous implementation had multiple issues such as casting u128 into u64 directly and using nanoseconds instead of milliseconds. Signed-off-by: Onur Özkan <[email protected]>
Signed-off-by: Onur Özkan <[email protected]>
onur-ozkan
force-pushed
the
better-sequence-numbering
branch
from
f41febf to
d9ca9f7
Compare
2 participants
Description
Sequence number creation and incrementing are now fallible instead of panicking.
Also, previous implementation had multiple issues such as casting u128 into u64 directly and using nanoseconds instead of milliseconds.
Change checklist