agreement: split ValidatedBlock and AssembledBlock interfaces

[cce]

Summary

Agreement has three main interfaces dealing with blocks: the BlockFactory (used to produce block proposals to send out, implemented by the transaction pool), the BlockValidator (used to verify incoming block proposals, implemented by the ledger), and the LedgerWriter (used to write blocks to the ledger after consensus has been reached).

These interfaces all share a common block type, ValidatedBlock, which is used when building proposals, validating proposals, and later writing blocks (if they have already been validated). To support building proposals, ValidatedBlock provides a WithSeed() method used to modify an assembled proposal's headers just before sending it out. To avoid re-evaluating winning proposals twice, ValidatedBlock also holds on to block evaluation outputs between calls to BlockValidator.Validate() and LedgerWriter.EnsureValidatedBlock().

This PR splits the AssembledBlock interface from the ValidatedBlock interface to split the block-building calls (AssembleBlock(), WithSeed()) from the block-validating-and-writing calls (Validate(), EnsureValidatedBlock()) more strictly.

Test Plan

Relatively small interface change, updated tests that implement BlockFactory so existing tests will pass.

[codecov]

Codecov Report

Attention: Patch coverage is 75.86207% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 55.77%. Comparing base (c2d7047) to head (5a19b26).
Report is 1 commits behind head on master.

Files	Patch %	Lines
node/node.go	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5967      +/-   ##
==========================================
+ Coverage   55.71%   55.77%   +0.06%     
==========================================
  Files         490      490              
  Lines       68125    68138      +13     
==========================================
+ Hits        37954    38006      +52     
+ Misses      27599    27565      -34     
+ Partials     2572     2567       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gmalouf]

Suggested a nit correction, but overall this makes sense and is very logical

[jannotti]

Since AssembledBlock is a subset of ValidatedBlock, ledgercore.ValidatedBlock is both. And there are many places that use ledgercore.ValidatedBlock directly, instead of an interface. Unfortunately, that means that things like GenerateBlock are expected to return ledgercore.ValidatedBlock which happens to be both an AssembledBlock and a ValidatedBlock. But I think we'd like it to be an AssembledBlock.
I'm not sure how much surgery it would be to make changes along those lines.

[cce]

I think there may be tests that use GenerateBlock and want the deltas, which is why I left GenerateBlock as is but changed only the return type of AssembleBlock.. I can modify GenerateBlock too if it turns out there are no tests that use the deltas

[jannotti]

I can modify GenerateBlock too if it turns out there are no tests that use the deltas

Would it be too crazy to give the Deltas() as different name in the two interfaces? I can see good reasons to look at the deltas, but that shouldn't force us to conflate the interfaces. TentativeDeltas() might be a little much, but that's how we should think of the deltas from AssembledBlock, I think.

[cce]

Looking at it now I'm not sure it's essential to modify the return type of BlockEvaluator.GenerateBlock(). Hiding the BlockEvaluator, dropping the deltas, and returning an agreement.AssembledBlock is the job of the node's implementation of the BlockFactory interface in AlgorandFullNode.AssembleBlock. Thinking about it more I don't think this PR needs to make any changes to the ledgercore.ValidatedBlock type, or anything other details invisible to agreement.

[jannotti]

Looking at it now I'm not sure it's essential to modify the return type of BlockEvaluator.GenerateBlock(). Hiding the BlockEvaluator, dropping the deltas, and returning an agreement.AssembledBlock is the job of the node's implementation of the BlockFactory interface in AlgorandFullNode.AssembleBlock. Thinking about it more I don't think this PR needs to make any changes to the ledgercore.ValidatedBlock type, or anything other details invisible to agreement.

I don't follow. GenerateBlock starts like this:

func (eval *BlockEvaluator) GenerateBlock() (*ledgercore.ValidatedBlock, error) {
	if !eval.generate {
		logging.Base().Panicf("GenerateBlock() called but generate is false")
	}

If eval.generate is true, as it must be, then I want the return type to prevent anyone from calling ledger.AddValidatedBlock with this thing. At the very least, it needs to receive a Seed, which is the distinction you're trying to create with Assembled/Validated, right?

And with my planned changes, I'd want to go even further: AssembledBlock becomes SeededBlock then it becomes a ValidatedBlock only after be Eval()d again, so that the incentive payout appears in the deltas.

I think the PR can and should be merged as is, because it makes things better, but I would still like to understand your reasoning, so I can think about how we might want to evolve later.

(I'm now seeing that we could potentially have an optimization in which the SeededBlock is not re-evaluated, we just tack on the incentive payout to create a ValidatedBlock. But I think we are currently performing the re-evaluation anyway, so it's not like we need such an optimization.)

[cce]

My point is that agreement controls the only way for validated blocks to get added to the ledger when calling agreement.EnsureValidatedBlock — which this PR addresses (just the types handled by agreement and agreement's AssembleBlock generated block now contains no deltas). Any other shortcut to take GenerateBlock's output and add it to a ledger is in simulation & test code.. it sounds like those tests may break, or not be testing realistic enough cases after your changes in #5757 now, though?

[cce]

Closing, merged into #5757 via jannotti#19

[jannotti]

I don't follow yet.

[jannotti]

Why do we need Round() now?

AssembledBlocks can't give out their Block()?

[cce]

The idea is not to allow access to the AssembledBlock, it is an opaque type that should not be used for serializing or access until Finalize is called

[cce]

Making this more opaque also helped me catch some agreement tests & test helpers that were using the AssembleBlock() value without calling WithSeed before — those tests were ignoring and not validating the seed anyway, but it was good to find where they were.

[jannotti]

It does seem good that an assembledBlock will no longer implement ValidatedBlock.

[jannotti]

So the idea is that the underlying type is not going to be a Block, it's going to be something that holds onto extra information about those addresses (balances, presumably) so that a later call to FinalizeBlock can erase the payout if it won't leave the account above min balance?

[cce]

Yes and whatever else you need to do (like updating the seed, assigning proposer etc) to finish/finalize the block

[cce]

Closing, replaced by #5973 and what has already been merged into #5757 via jannotti#19