Conflicting and/or duplicated information in specs: mining vs. expected-consensus

[shannonwells]

Description

There are some duplicated and seemingly conflicting passages between mining and expected-consensus documentation.

In particular there isn't full agreement between the two on the structure of types.Block and also with the source types/block.go. E.g. there's no NullBlocks count in the source.

Acceptance criteria

Documentation should be internally consistent and also reflect reality

Comment from @whyrusleeping copied to here since this is the appropriate repo (sorry for the n00b mistake):

As it says in the expected consensus document, what is described there is how to generally implement expected consensus. Expected consensus is an algorithm, much like 'merge sort'. Merge sort doesnt specify exactly what sort of things you will be sorting, or even how to compare elements in the list.

The mining document describes the filecoin implementation of expected consensus. This is what we should be aiming to implement.

I would love to make this more clear to the reader, could you suggest some changes in wording to help get that point across better?

[shannonwells]

Based on the assertion that a spec is a spec and not a mirror of the code, then it would seem that what's in data_structures is actually incorrect. Clearly if there is a code snippet in a spec that looks to the reader that it is telling you what to expect is in a Block, and this is very different to what is in a different document, that this not only confusing, but one of them is incorrect. That said, my suggestions are this:

1. In general aim to have a single, full canonical description of a given thing (algorithm , struct, process, etc) which all other specs refer back to rather than trying to keep multiple copies updated.

2. The data_structure spec would only contain the actual planned structure, and either notate what is currently implemented, or contain a link to the go-filecoin file for reference as to what is the current state of this implementation.

3. The mining spec would link to the data_structures#block section as a reference. This way the Block struct is documented in a single place and need only be edited there.

4. If there is what amounts to a roadmap for the spec, then perhaps there could additional documents in a folder named by version (?) which is sort of what you have with the drafts folder.

[shannonwells]

Another place where specifically what is expected in the Block struct is inconsistent is in expected_consensus. The fields listed in Block Generation don't match up with what is in data_structures. These need to match; they cannot refer to fields with different names. Implementers should not be in any doubt what field is what even if it seems obvious. For example there is Signature in the former, it's missing in the latter, and it's called BlockSig in mining.

[shannonwells]

Also, if "The mining document describes the filecoin implementation of expected consensus," what is the "expected consensus" document describing? So many questions!

[whyrusleeping]

@shannonwells to quote myself, and elaborate

As it says in the expected consensus document, what is described there is how to generally implement expected consensus. Expected consensus is an algorithm, much like 'merge sort'. Merge sort doesnt specify exactly what sort of things you will be sorting, or even how to compare elements in the list.

Think of it this way, expected consensus is a generic algorithm. Things described in a document specifically about expected consensus should describe how to implement the algorithm, think of this like a paper describing raft, or paxos, or some other consensus algorithm. Things described in such a document express the things needed by that protocol, as general abstract concepts. Such a document should go into more detail around the intuition and theory of the algorithm, and be very light on actual implementation details.

The fact that the structures described in the tendermint paper don't line up one to one with the fields in actual structs in their code is fine.