-
Notifications
You must be signed in to change notification settings - Fork 1.1k
doc: add shrex-sub spec #4541
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
doc: add shrex-sub spec #4541
Conversation
Co-authored-by: Wondertan <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## shrex_spec #4541 +/- ##
=============================================
Coverage ? 36.00%
=============================================
Files ? 304
Lines ? 20170
Branches ? 0
=============================================
Hits ? 7263
Misses ? 11957
Partials ? 950 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0af89d1 to
cba3aa9
Compare
Wondertan
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.
- Rendered link would help even with no diagram
- Misses some form of motivation section where we explain the purpose behind this protocol. Yes, it delivers notifications, but what's the point?
specs/src/shrex/shrex-sub.md
Outdated
|
|
||
| ## Abstract | ||
|
|
||
| **ShrEx/Sub** (Share Exchange/Subscribe) is a push-based notification protocol implementing the publish-subscribe pattern for Extended Data Square (EDS) hash distribution in the Celestia data availability network. The protocol enables efficient dissemination of new EDS availability notifications across different node types. |
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 wish we had a place to point to what EDS is
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.
Maybe core specs?
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 did not find anything specific. But probably we can use https://github.com/celestiaorg/celestia-app/blob/6ec2d1cab99a130cbfd9a2ddecf5d59e20084fa3/specs/src/data_structures.md the Erasure Coding Section..
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 this is a bit misleading to say that it distributes eds notifications. It is just new blocks being available. Notification mainly carries the height and eds root hash only to verify notification
specs/src/shrex/shrex-sub.md
Outdated
| // Publish publishes data hash and height to the topic | ||
| Publish(context, dataHash []byte, height uint64) -> error |
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.
Lets aim to be consistent with notations. Discovery detached from golang, even in implementation section, but here we start explaining the protocol directly with interfaces.
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 its fine to use golang in the spec generally, but in the some for of implementation detail section in the end.
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 reworked discovery. Please double check. No golang notations anymore.
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.
That's what I mean here by it's not consistent with the discovery spec
I just don't think stepping away from golang is necessary in the implementation section. Up to you tho, as long as it consistent in our specs
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 leave language agnostic if you are ok with it.
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.
if we are going language agnostic then context is not needed here or in other signature definitions.
specs/src/shrex/shrex-sub.md
Outdated
| **Lifecycle Management:** | ||
|
|
||
| - All components must be started before message processing | ||
| - Proper shutdown ensures cleanup of resources and connections | ||
|
|
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.
Is this AI generated?
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.
It was editing small parts and helping me to fix misspelling.
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.
Will remove this section.
specs/src/shrex/shrex-sub.md
Outdated
|
|
||
| ## Abstract | ||
|
|
||
| **ShrEx/Sub** (Share Exchange/Subscribe) is a push-based notification protocol implementing the publish-subscribe pattern for Extended Data Square (EDS) hash distribution in the Celestia data availability network. The protocol enables efficient dissemination of new EDS availability notifications across different node types. |
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 this is a bit misleading to say that it distributes eds notifications. It is just new blocks being available. Notification mainly carries the height and eds root hash only to verify notification
| ```text | ||
| Notification { | ||
| data_hash: bytes[32] // EDS root hash | ||
| height: uint64 // Block height | ||
| } | ||
| ``` |
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.
need to specify encoding schema (json / proto )
specs/src/shrex/shrex-sub.md
Outdated
| - **Message Distribution**: Flood-based (sends to all connected peers) | ||
| - **Overlay**: No mesh topology (unlike GossipSub) |
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.
Those two points looks like one. floodsub is no mesh
| **Properties:** | ||
|
|
||
| - Messages MUST have a fixed 40-byte payload (32 bytes hash + 8 bytes height) | ||
| - Serialization overhead SHOULD be minimal |
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.
Serialisation must be defined in spec
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.
Otherwise its like spec saying to itself that it should have a minimal overhead 😁
specs/src/shrex/shrex-sub.md
Outdated
|
|
||
| - Messages MUST have a fixed 40-byte payload (32 bytes hash + 8 bytes height) | ||
| - Serialization overhead SHOULD be minimal | ||
| - Each EDS at a specific height MUST generate only a single notification |
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 needs is not a MUST right now. Currently it is ok if some message is not send or duplicates are also acceptable. So for current state of implementation lets replace MUST with SHOULD.
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 would also double check grammar here. "generate" might need to be to replaced with "produce" or "send"
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.
produce sgtm
| ### Validation Interface | ||
|
|
||
| **MessageValidator Interface:** | ||
|
|
||
| ```text | ||
| // Validate validates a message from a peer | ||
| Validate(context, peerID PeerID, message []byte) -> ValidationResult | ||
| ``` |
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.
Need to add explanation why this is exposed and how it will be used by consumer of Validate (peer-manager)
Motivation section is still missing |
|
|
||
| - Messages MUST have a fixed 40-byte payload (32 bytes hash + 8 bytes height) | ||
| - Serialization overhead SHOULD be minimal | ||
| - Each EDS at a specific height SHOULD produce only a single notification |
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.
why SHOULD here?
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.
Because we don't have any restrictions about that. The message will be ignored in this case and the peer won't be added to the pool.
specs/src/shrex/shrex-sub.md
Outdated
| // Publish publishes data hash and height to the topic | ||
| Publish(context, dataHash []byte, height uint64) -> error |
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.
if we are going language agnostic then context is not needed here or in other signature definitions.
specs/src/shrex/shrex-sub.md
Outdated
|
|
||
| ShrEx/Sub implementations MUST implement the following validation process: | ||
|
|
||
| **Format Validation Requirements:** |
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.
we need to add here that the header corresponding to the given height + datahash must be fully verified + included as part of implementation's subjective chain in order for notification to be considered valid.
Maybe this was implied by
Height MUST be a valid block height
but we should be more concrete about what that means.
You could optionally also split this up into sanity checks vs verification check to make it more clear.
Co-authored-by: rene <[email protected]>
specs/src/shrex/shrex-sub.md
Outdated
|
|
||
| - `ACCEPT`: Message is valid and MUST be processed | ||
| - `REJECT`: Message is invalid and MUST be discarded | ||
| - `IGNORE`: Message is valid but duplicate/stale and SHOULD be ignored |
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.
we should only ignore a message if sender has already been seen for the exact same message.
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.
Peer manager detects duplication. Shrex-sub does not use IGNORE at all. Mistakenly added deduplication here.
specs/src/shrex/shrex-sub.md
Outdated
|
|
||
| ### Publisher Behavior (BN) | ||
|
|
||
| - **Trigger**: Publishers MUST publish when a new EDS becomes available locally |
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.
please distinguish "new EDS becomes available locally" from a syncing BN
publishers should only publish message if subjectively it understands that it is synced up and that this EDS is from a height at the tip of the network
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.
rephrased
specs/src/shrex/shrex-sub.md
Outdated
| - **Trigger**: Publishers MUST publish when a new EDS becomes available locally | ||
| - **Action**: Publishers MUST publish EDS hash and height to `/eds-sub/0.0.1` topic | ||
| - **Frequency**: Each EDS MUST be published exactly once per height | ||
| - **Validation**: Publishers MUST only publish hashes for locally validated and available EDS |
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.
should we define "available" at top of file?
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.
Removed available as it is already mentioned in Trigger
specs/src/shrex/shrex-sub.md
Outdated
| - **Subscription**: Subscribers MUST maintain an active subscription to `/eds-sub/0.0.1` | ||
| - **Processing**: Subscribers MUST validate received hash format and height | ||
| - **Action**: Subscribers MUST process notifications through registered listeners | ||
| - **Deduplication**: Subscribers MUST ignore duplicate notifications |
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.
need to specify what a duplicate is
also this feels like a SHOULD
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.
removed deduplication section
Added spec for the shree-sub sub-component
Rendered