doc: specs #116

gupadhyaya · 2023-10-06T15:27:54Z

Overview

Rendered

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

codecov-commenter · 2023-10-11T14:39:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (09c272d) 67.81% compared to head (9bd3962) 63.66%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
- Coverage   67.81%   63.66%   -4.15%     
==========================================
  Files          38       39       +1     
  Lines        3079     3366     +287     
==========================================
+ Hits         2088     2143      +55     
- Misses        843     1058     +215     
- Partials      148      165      +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

p2p/p2p.md

sync/sync.md

MSevey

lgtm, some small clean up comments.

… relative

Wondertan

High-Level Q: What kind of spec is this and who is it targeted for?

It looks pretty Golang implementation-specific, and AFAIK, specs are meta descriptions of protocols and behaviors decoupled from implementation details. Usually, their target is a client implementer who needs to understand how things work. From my understanding, this is excellent code documentation, but not the spec. I wonder if we should aim to extract all the protocol information into an independent doc separating docs from specs or at least make the protocol description part of the documentation.

Anyway, this is awesome, and I am very grateful for this ❤️

I will review this in batches, and the first one is P2P doc review

specs/src/README.md

p2p/p2p.md

Wondertan · 2023-10-23T11:53:05Z

p2p/p2p.md

+
+The new peers are tracked by subscribing to `event.EvtPeerConnectednessChanged{}`.
+
+The peer tracker also runs garbage collector (gc) that removes the disconnected peers (determined as disconnected for more than `maxAwaitingTime` or connected peers whose scores are less than or equal to `defaultScore`) from the tracked peers list once every `gcPeriod`.


We should either link to those constants or mention their values here is some legend.

This comment applies to all other constants throughout the spec

Wondertan · 2023-10-23T11:55:15Z

p2p/p2p.md

+
+## Metrics
+
+Currently only following metrics are collected:


Note that the list is already extended for Subscriber, and more will be added soon.

I'll take care of updating the spec

p2p/p2p.md

Wondertan · 2023-10-23T12:10:31Z

p2p/p2p.md

+ // GetRangeByHeight returns the given range of Headers.
+ GetRangeByHeight(ctx context.Context, from, amount uint64) ([]H, error)
+
+ // GetVerifiedRange requests the header range from the provided Header and
+ // verifies that the returned headers are adjacent to each other.
+ GetVerifiedRange(ctx context.Context, from H, amount uint64) ([]H, error)


These methods were recently unified, and this needs to be updated.

Wondertan · 2023-10-23T12:19:49Z

p2p/p2p.md

+```
+message HeaderRequest {
+  oneof data {
+    uint64 origin = 1;
+    bytes hash = 2;
+  }
+  uint64 amount = 3;
+}
+```


[blocking]
Let's separate the protocol from implementation. I suggest adding ### Protocol paragraph, which contains all the messages and explains the basic flow. The method description of components can then link to it.

gupadhyaya · 2023-10-24T12:11:00Z

High-Level Q: What kind of spec is this and who is it targeted for?

It looks pretty Golang implementation-specific, and AFAIK, specs are meta descriptions of protocols and behaviors decoupled from implementation details. Usually, their target is a client implementer who needs to understand how things work. From my understanding, this is excellent code documentation, but not the spec. I wonder if we should aim to extract all the protocol information into an independent doc separating docs from specs or at least make the protocol description part of the documentation.

Anyway, this is awesome, and I am very grateful for this ❤️

I will review this in batches, and the first one is P2P doc review

thanks for your review @Wondertan
i am writing this specs from the perspective of the consumer (e.g. rollkit, in future other frameworks). i agree that it includes interface definitions in a specific language (golang), however i don't think it will prevent a non-golang adopter from understanding and consuming it. the whole idea here is to clearly specify the interfaces and implicit/explicit assumptions made. we have followed similar strategy in specifying rollkit.

i agree that we can add a protocol section which tries to summarize the components without getting into the details. but i don't think we should try to make two separate documents (specs and impl details) imho.

if we want, we can convert the golang interface definitions to language independent. let me know.

gupadhyaya · 2023-10-24T12:19:50Z

@Wondertan let me revise this based on your comments and try to make certain parts golang independent. Will request review soon.

Co-authored-by: Hlib Kanunnikov <[email protected]>

gupadhyaya · 2023-10-27T02:49:25Z

@Wondertan tried to generalize as much as i can. as discussed, we can revisit regularly and keep improving. wdyt?

MSevey · 2023-10-30T11:08:30Z

p2p/p2p.md

+Exchange defines a client for requesting headers from the P2P network. An exchange client is initialized using self [host.Host][host], a list of peers in the form of slice [peer.IDSlice][peer], and a [connection gater][gater] for blocking and allowing nodes. Optional parameters like `ChainID` and `NetworkID` can also be passed. The exchange client also maintains a list of trusted peers via a peer tracker. The peer tracker will continue to discover peers until:
+
+* the total peers (connected and disconnected) does not exceed [`maxPeerTrackerSize`][maxPeerTrackerSize] or
+* connected peer count does not exceed disconnected peer count.


Really? Connected peer count should always be less than disconnected peer count?

Feels like this needs some explanation.

MSevey

overall lgtm

Wondertan · 2023-11-22T15:36:40Z

@gupadhyaya, getting back to this finally

Wondertan · 2023-12-01T10:52:03Z

So, I added a rendered link to the PR description with rendered specs readme and realized that linking between symlinks defined in the specs folder and the actual specs markdown does not work. Do we really need this symlink redirection? I think we can directly point root specs README to the relevant md files.

cc @MSevey (as I see you set it up this way)

MSevey · 2023-12-01T16:33:50Z

So, I added a rendered link to the PR description with rendered specs readme and realized that linking between symlinks defined in the specs folder and the actual specs markdown does not work. Do we really need this symlink redirection? I think we can directly point root specs README to the relevant md files.

cc @MSevey (as I see you set it up this way)

@Wondertan what is the actual issue? The rendered readme works fine for me, plus you can see the rendered version in the diff.

The symlinks were a way to keep the specs close to the code to encourage keeping them up to date.

Wondertan · 2023-12-01T16:38:10Z

@MSevey, try clicking links to Components on the rendered page and you will see

MSevey · 2023-12-01T18:28:50Z

@MSevey, try clicking links to Components on the rendered page and you will see

oh right, yea we've talked about this internally. The Github UI doesn't support symlinks, but that is fine since we are optimizing for the rendered website and keeping the specs close to the code.

Wondertan · 2023-12-01T18:39:09Z

@MSevey, which rendered website?

Wondertan · 2023-12-01T18:52:37Z

I think we should make it work for both GH and the rendered website(or whatever it is) by removing symlinks and linking directly.

MSevey · 2023-12-01T19:53:49Z

@Wondertan the specs site https://celestiaorg.github.io/go-header/

to make it work with both I would just not link to the specs folder. the specs folder is essentially a wrapper to enable a deployed specs website.

The original discussion the team had came down to whether or not we want the spec md files in the specs folder or close to the code. Since there was a preference for keeping the specs md files close to the code, we set up the specs folder with symlinks to make the website work.

So to make it work on the GH UI we just shouldn't reference the specs folder outside the specs folder.

Wondertan · 2023-12-08T14:05:08Z

So to make it work on the GH UI we just shouldn't reference the specs folder outside the specs folder.

@MSevey, Do you mean pointing to the code directly like in here?

MSevey · 2023-12-08T16:35:39Z

So to make it work on the GH UI we just shouldn't reference the specs folder outside the specs folder.

@MSevey, Do you mean pointing to the code directly like in here?

Yea, if a the spec in pkgA wants to reference the spec in pkgB, it should link directly to pkgB not the spec/src/spec/pkgB symlink location.

gupadhyaya changed the title ~~initial specs~~ doc: specs Oct 6, 2023

gupadhyaya force-pushed the specs branch from 81680cc to 4bc23ab Compare October 11, 2023 14:37

gupadhyaya force-pushed the specs branch from d185091 to e9b5909 Compare October 18, 2023 00:06

gupadhyaya marked this pull request as ready for review October 18, 2023 00:06

gupadhyaya requested review from tzdybal, renaynay, Wondertan and vgonkivs as code owners October 18, 2023 00:06

gupadhyaya requested review from nashqueue, Manav-Aggarwal and MSevey October 18, 2023 00:07

vgonkivs reviewed Oct 18, 2023

View reviewed changes

p2p/p2p.md Show resolved Hide resolved