Spec migration
Ref: #650
Staging branch for specs migration. Draft PR so it can be tracked.
hmm I can't merge main into this branch
I wanted to merge main to pick up the new CODEOWNERS file
What's the status of this?
@musalbas apologies for delayed response. @adlerjohn and I triaged issues in https://github.com/celestiaorg/celestia-app/issues/650 today.
I think that the specs-staging branch is already a more accurate representation of the specs than https://celestiaorg.github.io/celestia-specs/latest/index.html so I'd like to merge specs-staging to main ASAP. But if we want to block this merge on updating specs to match the existing implementation, we can block on all the issues in the "Pure specs" section.
What's the latest with this and generally adding specs to celestia-app/core now?
This branch is the most up to date version of the specs but it's not publicly visible yet. IMO we should merge this branch to main, make it publicly visible, and take down the existing specs site at https://celestiaorg.github.io/celestia-specs/latest/index.html
The remaining specs issues are tracked under https://github.com/celestiaorg/celestia-app/issues/650 . We intend on prioritizing specs and documentation work after the core/app feature freeze for incentivized testnet.
I have some general comments that can possible be followed up at a later point:
- Can we break up data structures into smaller more self-explanatory files so that parts can be easily accessed in the table of contents. I also think it's less intuitive to explain a spec by just dumping all the data structures that are included in it. Would prefer more of a hierarchical function-orientated tree.
- Can we add a section explaining how the "celestia app" fits in with the rest of the system and include links so people can be easily redirected if they're interested with something in "celestia node" for example. Furthermore, I think calling it "celestia app" can be better worded. As this isn't the specification of the entire system, rather a component of it, we should look to distinguish it functionally. Call it "celestia data publication specification" (to complement "celestia data availability specfication") if we want to be deliberate about having two networks.
- I don't think
Rationaleshould be a separate section but should be embedded right next to the part in the spec it refers to. I should read what a certain protocol is and directly after why it is so. - It's evident that the specification includes not just novel work specific to Celestia but the components that it is built on top of. There is a lot of work on Tendermint's consensus algorithm (such as block validity, leader selection) and how the validator set is controlled. What about other components like governance, IBC, vesting, upgrades that still seem to be missing. It seems that not all state transition messages have been documented.
- Is QGB spec somewhere else?
- Can we have some preamble/context in the top level pages to each of the subpages.
- Are we sure we want a copy of many of the proto files in the spec as well? These are not the canonical ones that get used in generation and will constantly become outdated.
Overall I'm on-board with basically all of the suggestions you proposed. I think the feedback could be spun out into individual issues that we add to the epic rather than blocking feedback on this PR.
Can we break up data structures into smaller more self-explanatory files
I'm on-board. I agree that the data structures page is long and difficult to read top to bottom so I think multiple logically contained data structures pages would be helpful for readability.
I don't think Rationale should be a separate section but should be embedded right next to the part in the spec it refers to.
Agreed. I think we should move the rationale/message_block_layout section to block_proposer#laying-out-transactions-and-messages
Is QGB spec somewhere else?
Perhaps https://github.com/celestiaorg/celestia-app/blob/main/x/qgb/README.md
Are we sure we want a copy of many of the proto files in the spec as well?
Good question. I think we should not duplicate the proto definitions in the specs unless we have an automated way to update them if the canonical ones change.
are the markdown linter complaints valid?
They are valid but they don't appear when make lint is run locally. They appear resolved after the most recent commit.
Docker / hadolint fails but that occurs on other branches too (potentially b/c Github outage).
Will merge this by EOD tomorrow if no more feedback.
its weird that the docker builds are still failing here despite passing in other PRs, we didn't change anything related here afaict
edit: Rootul fixed it