Closed jbowens closed 7 years ago
PTAL
:eyes:
One thing I'd like to see in this PR, that will also help me review the rest of it: a comment (in protocol/doc.go
, perhaps) describing the various phases of block processing. I once understood the sequence but am now having trouble reconstructing it. The names ValidateBlock
, ApplyValidBlock
, and CommitAppliedBlock
were supposed to help illustrate the correct sequence, but those have changed, and there's also "saving" a block and "finalizing" a block (so the naming scheme was never very complete).
One thing I'd like to see in this PR, that will also help me review the rest of it: a comment (in protocol/doc.go, perhaps) describing the various phases of block processing. I once understood the sequence but am now having trouble reconstructing it.
Ok, I added some high-level documentation.
there's also "saving" a block and "finalizing" a block (so the naming scheme was never very complete).
Heh, yeah. FinalizeBlock should probably be called something like 'NotfyBlock.' I'm not 100% on this, but I think we'll eventually be able to remove FinalizeBlock from the Store interface, replacing it with some additional logic in chain/core. It really has more to do with intra-Core coordination than the protocol.
PTAL
:eyes:
LGTM
Previously, we only called CommitAppliedBlock from a single goroutine: either the core/generator or the core/fetch goroutine. This meant we could access c.State() and apply new blocks to it without the possiblity of another goroutine applying newer blocks first.
When we modify Chain to always have an up-to-date state tree regardless of leader status, the goroutine listening to txdb.ListenBlocks heights will need to also apply blocks. In preparation for this change, we need to make committing & applying a block idempotent.