docs: add block validity rules specs

[evan-forbes]

Overview

Figured we could at least use this as a place holder as a high level summary of celestia-app, and to point to all the other portions of the application.

part of and closes #743
part of #650

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

[github-actions]

PR Preview Action v1.4.4
🚀 Deployed preview to https://celestiaorg.github.io/celestia-app/pr-preview/pr-1966/
on branch gh-pages at 2023-07-05 23:21 UTC

[codecov-commenter]

Codecov Report

Merging #1966 (71cac1e) into main (b2fc98c) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1966      +/-   ##
==========================================
+ Coverage   21.49%   21.54%   +0.04%     
==========================================
  Files         122      127       +5     
  Lines       13817    14293     +476     
==========================================
+ Hits         2970     3079     +109     
- Misses      10558    10922     +364     
- Partials      289      292       +3     

see 15 files with indirect coverage changes

[rootulp]

[question][no change needed]

1. Do the "fraud proofs" referenced exist yet? I think no.
2. Is there a more granular term used to describe these fraud proofs? If no such term exists, perhaps: "block validity fraud proofs"?

[evan-forbes]

BEFPs do, and blob inclusion will sooner or later. Not sure when we'll get state fraud proofs

this does bring up a good point, where I feel like all fraud proofs are actually block validity fraud proofs, and perhaps I should try to change the first paragraph to emphasize that

[rootulp]

🙌 fraud proof specs

[staheri14]

LGTM! added some suggestions.

[staheri14]

[Question] Do light nodes actually verify all the block validity rules? or just a subset? To me, it is more like the light client can only check the "Invalidity of the blocks" but not the validity. And they even do so with the aid of full nodes.

[staheri14]

[Question] Can you please elaborate on the reason behind including this section as part of the "Block validity rule" specs? It didn't seem to touch on any validation rule? Thanks 🙏

[evan-forbes]

hmm good point. I guess this was just a section for block validity rules that were directly related to data availability

this point is usually glossed over in most blockchains, as it is assumed that the data is downloaded entirely in order to verify it

this section describes the difference between consensus nodes and light clients to emphasize that point

The data for each block must be considered available before a given block can be
considered valid. For consensus nodes, this is done via an identical mechanism
to a normal CometBFT node, which involves downloading the entire block by each
node before considering that block valid.

Light clients however do not download the entire block.

and then uses this preface for the reasoning behind the rest of the rules

do you think we should remove this header? While I'm less confident about the header, I do think the three paragraphs after the header are a useful preface

[evan-forbes]

I will attempt to address this when I address the above comment

[staheri14]

I understand your point, and it does make sense. I would recommend consolidating all the relevant background into a dedicated section, such as "Background" or "Introduction".

[staheri14]

[Question] I think the story of light clients and how they consider a block valid actually deserves separate specs, and I guess it is not even part of the core/app implementation, right? which means it can live in the node repo. I think here it's better to focus on the entities that actually exist in the core/app layer e.g., full nodes, validators, consensus nodes, etc. While the content of this part adds additional great info I think is not directly relevant. Due to this, and in favor of brevity and conciseness, I'd suggest leaving this subsection outside of the current specs. Though, I'll leave it up to you.

[evan-forbes]

That is a good point that fraud proofs are not directly part of core/app atm, so including them in the spec might not make a lot of sense. I do think we should at least have a link to the relevant specs, since that will preserve our capability to send a newcomer a single link where they can find all the important portions.

The initial reason for including something here is that I feel that they have a large impact on the design and implementation. For example, the sole reason we have square layout/blob commitment rules are to accomodate blob inclusion proofs. If we eventually have at least a high level description of it, then we can see the end result, which I think makes the rest of the concepts a lot clearer.

[staheri14]

I do think we should at least have a link to the relevant specs, since that will preserve our capability to send a newcomer a single link where they can find all the important portions.

Good idea and agree!

The initial reason for including something here is that I feel that they have a large impact on the design and implementation. For example, the sole reason we have square layout/blob commitment rules are to accomodate blob inclusion proofs. If we eventually have at least a high level description of it, then we can see the end result, which I think makes the rest of the concepts a lot clearer.

Agree that such information, at a high level, will provide readers with a clear understanding of the motivations behind the design choices and the purpose of the validity rules.
Following my previous comment, I recommend incorporating all the motivational information into a single section. Additionally, it would be beneficial to explicitly mention that the design of the Celestia block is influenced by these reasons, resulting in an extensive list of validity rules.

[cmwaters]

I do think that wherever validity rules are described should be were fraud proofs which define the failure to comply with the aforementioned validity rules should reside. Therefore I think fraud proofs should be described in the specification here.

[staheri14]

[Suggestion] I see there is a missing link between the block validity rules and the supplied subsections. My suggestion revolves around making this link a bit clear (feel free to rephrase it as you see fit, I got inspiration from the subsections you already included in the specs):
Below is my suggested addition to the text:

In Celestia, the block validity rules encompass two main aspects: the rules governing the validity of individual transactions and the rules dictating how to construct the data hash.

• Transaction validity rules: Similar to any normal block, Celestia blocks are composed of a set of transactions. Therefore, the transaction validity rules defined by CometBFT (I guess part of the rules should be in the app not just CometBFT, so please revise it the way you think is correct) form an integral part of the block validity rules in Celestia. (a reference to the appropriate documentation for the specific transaction validity rules.)

• Blob transaction: The Blob transaction is unique to Celestia, and its validity rules are covered in the corresponding [specifications](either link the specs or a subsection in this spec). ( I am assuming this is something we need to explain separately, that is why I have put it as a separate item)

• Data hash calculation steps: The following steps are specific to the calculation of the data hash:

  • Encoding/laying out transactions into a data square: Transactions within the block are serialized and organized into a square format. This process involves reorganizing the transactions, converting them into a series of bytes, splitting them into fixed-size shares, arranging them in a square structure, and erasure coding them. All these steps adhere to specific rules, which are also considered part of the block validity rules. (Links to the respective specifications, OR subsections of the current doc, should be provided for further details.)

  • Constructing the data hash: The data hash is computed from the data square mentioned earlier. Detailed information on the construction of the data hash can be found in the provided specifications OR a subsection in this doc. The correct construction of the data hash is also within the block validity rules.

[evan-forbes]

In Celestia, the block validity rules encompass two main aspects: the rules governing the validity of individual transactions and the rules dictating how to construct the data hash.

I like this a lot for the celestia specific rules and have updated accordingly. Note that I have simplified further by deleting a lot of the original descriptions and instead relying on the other portions of the spec that already cover it

[evan-forbes]

updated this to be significantly smaller, and instead just rely on pointing to other portions of the spec for the time being

[rootulp]

Suggested change

	All remaining transaction types do not have to by valid if included in a block. For a complete list of modules see [state machine modules](./state_machine_modules.md).
	All remaining transaction types do not have to be valid if included in a block. For a complete list of modules see [state machine modules](./state_machine_modules.md).

[staheri14]

LGTM!

[staheri14]

I guess checking the validity of the transaction nonces goes under the CometBFT validity rules, right?

[evan-forbes]

good question! I don't think the ante handlers are really specified sufficiently anywhere, especially our usage of them, so I added #2044

that might involve updating the above! the reasoning behind saying "all remaining transactions types do not have to be valid" were these lines

celestia-app/app/process_proposal.go

Lines 55 to 61 in 65046dd

	sdkTx, err := app.txConfig.TxDecoder()(tx)
	if err != nil {
	// we don't reject the block here because it is not a block validity
	// rule that all transactions included in the block data are
	// decodable
	continue
	}

in the past, the rules were that validators could include invalid transactions in the block. It's very reasonable to see this as incorrect now since the most recent fix to use the entire antehandler instead of only incrementing the nonce!

celestia-app/app/process_proposal.go

Lines 72 to 79 in 65046dd

	// we need to increment the sequence for every transaction so that
	// the signature check below is accurate. this error only gets hit
	// if the account in question doens't exist.
	sdkCtx, err = handler(sdkCtx, sdkTx, false)
	if err != nil {
	logInvalidPropBlockError(app.Logger(), req.Header, "failure to incrememnt sequence", err)
	return reject()
	}

we might want to say something along the lines of "random tx data is permitted, but decodable sdk.Txs must pass all antehandler checks in order for the block to be valid".

[evan-forbes]

linked this convo as part of that issue to ensure we address this specifically