Skip to content
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

Blobstream docs feedback #1275

Closed
rootulp opened this issue Nov 17, 2023 · 10 comments · Fixed by #1261
Closed

Blobstream docs feedback #1275

rootulp opened this issue Nov 17, 2023 · 10 comments · Fixed by #1261
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@rootulp
Copy link
Contributor

rootulp commented Nov 17, 2023

I have some feedback from reading the first four page of Blobstream docs

  1. ✅ The links to these two ADRs seems out of place. The topics of: valsets and data commitments haven't been introduced yet so this assumes some prior knowledge.
    The specification of Blobstream `Valset`s, which track the Celestia validator set
    changes, can be found in [ADR 002](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-002-qgb-valset.md).
    Blobstream data commitments, which represent commitments over sets of blocks
    defined by a data commitment window, are discussed more in-depth in
    [ADR 003](https://github.com/celestiaorg/celestia-app/blob/main/docs/architecture/adr-003-qgb-data-commitments.md).
  2. ✅ The term ValSet is written with inconsistent casing. Sometimes it is Valset. Sometimes it is ValSet. We should use consistent casing. IMO the best form to use is ValidatorSet
  3. ✅ In this diagram, why is there an arrow from Celestia validator node to Blobstream relayer? That line isn't explained in the text.
    ![Blobstream-Orchestrator](/img/blobstream/blobstream-orchestrator.png)
  4. ✅ The Golang version for Blobstream claims 1.20.2 but the repo uses 1.21 so I think this constant needs to be bumped
    1. [Install Go](https://go.dev/doc/install) {{constants.golangBlobstream}}
  5. https://docs.celestia.org/nodes/blobstream-binary contains content copy + pasted from the README of blobstream. I don't think it should contain the Contributing section b/c that's applicable to readers of the Github README but not as applicable to readers of the docs (who just want to run the binary).
  6. ✅ The blockspace race is over and I don't think the network is still supported. Proposal to remove:
    Bootstrapper for the Blockspace Race is:
    - `/dns/bootstr-incent-1.celestia.tools/tcp/30000/p2p/12D3KooWSGZ2LXW2soQFHgU82uLfN7pNW5gMMkTnu1fhMXG43TvP`
  7. ✅ What does "This latter" mean?
    - Access to your EVM address private key. This latter doesn't need to be funded in any network. If yours is not yet set, check the [register an EVM address](#register-evm-address) section.
  8. ✅ "The team" is a bit vague here. Proposal to delete that sentence entirely.
    - A list of bootstrappers for the P2P network. These will be shared by the team for every network we plan on supporting.
  9. ✅ There are no flags listed in
    The orchestrator accepts the following flags:
    ```sh
    blobstream orchestrator start --help
    Starts the Blobstream orchestrator to sign attestations
    Usage:
    blobstream orchestrator start <flags> [flags]
    ```
  10. ✅ What does "find the latter" mean here? https://github.com/celestiaorg/docs/blob/756234bb6be524c2cc8f7ba674f645d7cd8b0a1e/nodes/blobstream-orchestrator.md?plain=1#L123C31-L123C31
  11. ✅ I think this section should come before the start command:
    ### Open the P2P port
  12. ✅ "This latter" doesn't make sense
    When creating a validator, a random EVM address corresponding to its operator is set in the Blobstream state. This latter will be used by the orchestrator to sign attestations. And since validators will generally not have access to its corresponding private key, that address needs to be edited with one whose private key is known to the validator operator.
  13. ✅ Remove this note about BSR
    Note: A validator set change is triggered if more than 5% of the total staking power of the network changes (0.5% for BSR). This means that even if you change your EVM address, and you don't see your orchestrator signing, it's alright. Just wait until the validator set changes, and then your orchestrator will automatically start signing.
  14. ✅ Multiple other instances of "This latter" which don't make sense.

cc: @sweexordious

@rootulp rootulp added the documentation Improvements or additions to documentation label Nov 17, 2023
@jcstein jcstein self-assigned this Nov 17, 2023
@jcstein
Copy link
Member

jcstein commented Nov 17, 2023

thank you for this feedback, super helpful! notes on current plan for blobstream for context:

  1. i’m working on consolidating these two pages and finding a home for the intro page, what do you think of that? at least, i’d like to make nodes/blobstream-intro more accurate based on this from github
  1. I'm thinking of keeping nodes/blobstream-* to be only things that are relevant to running the orchestrator-relayer. on that note, [the docs that are imported from orchestrator-relayer repo]

@rootulp
Copy link
Contributor Author

rootulp commented Nov 17, 2023

i’m working on consolidating these two pages and finding a home for the intro page, what do you think of that?

Makes sense to me.

the docs that are imported from orchestrator-relayer repo

ahh so does that mean the issues I suggested above should not be fixed in the docs repo but should be fixed in the orchestrator-relayer repo?

@jcstein
Copy link
Member

jcstein commented Nov 17, 2023

I'm going to pushed to a branch shortly with a reformatted blobstream-intro and updated blobstream-binary, addressing your points 1-5

ahh so does that mean the issues I suggested above should not be fixed in the docs repo but should be fixed in the orchestrator-relayer repo?

the others are still relevant to docs, but just need to be edited in orchestrator-relayer first, then pulled in (i'll get to this automation eventually, for now the script works)

i’m working on consolidating these two pages and finding a home for the intro page, what do you think of that?

Makes sense to me.

I think i'm going to consoildate blobstream-intro and blobstream-binary so people don't end up on the overview thinking it's the overview of blobstream for developers

jcstein added a commit that referenced this issue Nov 17, 2023
Resolves #1271
Resolves #1275 partially
@jcstein jcstein mentioned this issue Nov 17, 2023
7 tasks
@jcstein
Copy link
Member

jcstein commented Nov 17, 2023

I will make changes for 6-14 next week (if SweeXordious doesn't beat me to it), 1-5 addressed in #1261

@rach-id
Copy link
Member

rach-id commented Nov 20, 2023

👍 👍 @jcstein Let me know if you need any information about these changes or want me to handle any of them

@jcstein
Copy link
Member

jcstein commented Nov 20, 2023

I'll go ahead and update orch-relayer repo today and make a PR @sweexordious

@rach-id
Copy link
Member

rach-id commented Nov 20, 2023

Awesome, thanks a lot

@jcstein
Copy link
Member

jcstein commented Nov 20, 2023

you're welcome! for "10. What does "find the latter" mean here? https://github.com/celestiaorg/docs/blob/756234bb6be524c2cc8f7ba674f645d7cd8b0a1e/nodes/blobstream-orchestrator.md?plain=1#L123C31-L123C31" @sweexordious what is "latter" referring to here?

@rach-id
Copy link
Member

rach-id commented Nov 20, 2023

@jcstein the orchestrator's TOML config file

@jcstein
Copy link
Member

jcstein commented Nov 20, 2023

jcstein added a commit that referenced this issue Nov 20, 2023
* chore: update to mocha for blobstream

* chore: rework blobstream intros

Resolves #1271
Resolves #1275 partially

* fix: autformatting

* chore: pull in orch-relayer docs updates

* chore: add mocha bootstrapper

* chore: add import of blobstream-bootstrapper doc

* chore: copy edits

* chore: rearrange blobstream.md

* feat: add bootstrapper doc

* Update nodes/blobstream-binary.md

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>

* Apply suggestions from code review

Co-authored-by: Rootul P <rootulp@gmail.com>

* Update nodes/blobstream-orchestrator.md

Co-authored-by: Rootul P <rootulp@gmail.com>

* chore: pull in blobstream updates

Co-Authored-By: Rootul P <rootulp@gmail.com>

---------

Co-authored-by: CHAMI Rachid <chamirachid1@gmail.com>
Co-authored-by: Rootul P <rootulp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants