-
Notifications
You must be signed in to change notification settings - Fork 930
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
fix(gossipsub): signed messages use monotonically increasing seq numbers #3551
Conversation
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.
Thanks!
I think this deserves a changelog entry. Can you please add one?
Yep sure, added (feel free to change format) |
Co-authored-by: Thomas Eizinger <thomas@eizinger.io>
This pull request has merge conflicts. Could you please resolve them @AgeManning? 🙏 |
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.
Thanks for the work here.
@@ -190,6 +192,7 @@ impl From<MessageAuthenticity> for PublishConfig { | |||
keypair, | |||
author: public_key.to_peer_id(), | |||
inline_key: key, | |||
last_seq_no: rand::random(), |
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.
Out of curiosity, I don't think it actually matters: Why start with a random number and not the current timestamp as the go implementation does?
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.
Not sure really. I think I have some innate aversion to giving out more information about ourselves than needed. Maybe also because I was lazy about finding out which timestamp to use in go, ms vs s etc.
Happy to change this, no real reason tbh, random just felt slightly safer, with no logical reason to back it up. :)
@@ -1,3 +1,10 @@ | |||
# 0.44.2 - unreleased |
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.
Missing Cargo.toml
version bump. Thus a "ghost version". Will be fixed with #3668.
Previously, we only mutably borrowed the `last_seq_no` in the current scope but did not modify the underlying number. This is because `u64` is copy and calling `wrapping_add` consumes `self` so the compiler just copied it. We introduce a new-type instead that is not `Copy`. Additionally, `wrapping_add` and initializing with a random u64 might actually warp the number and thus not give us sequential numbers as intended in #3551. To solve this, we initialize with the current unix timestamp in nanoseconds. This allows a node to publish 1000000 messages a second and still not reuse sequence numbers even after a restart / re-initialization of the configuration. This is also what the go implementation does. Resolves #3714. Co-authored-by: Thomas Eizinger <thomas@eizinger.io> Pull-Request: #3716.
Description
This modifies the gossipsub implementation to use monotonically increasing sequence numbers for signed messages (as dictated by the specification). There is a discussion about this in #3453. This change will make rust-libp2p gossipsub align with the go-implementation when messages are signed.
Messages will however still use randomized sequence numbers when messages are unsigned for security reasons (as discussed in the issue linked).
This shouldn't change any user-level API, only the seqno behavior. It is fully backwards compatible.
Resolves #3453.