incentives: split unfinishedBlock/proposableBlock interfaces against #5757

[cce]

This is here to run CI checks on the interface changes in #5967 against the WIP PR in #5757

Changes:

• Adds a participatingAddresses []basics.Address argument to agreement.AssembleBlock()
• Adds a addrs []basics.Address argument to BlockEvalutor.GenerateBlock()
• BlockEvaluator.GenerateBlock(addrs) now looks up the end-of-block state of the accounts in addrs, and returns an ledgercore.UnfinishedBlock containing it (along with the block and deltas)
• Renames and moves AssembledBlock.WithProposer() to UnfinishedBlock.FinishBlock() which returns an agreement.ProposableBlock (not an agreement.ValidatedBlock)
• Gives TransactionPool access to a VotingAccountsForRound(basics.Round) []basics.Address method so it can look up what participating addresses need to be passed to GenerateBlock()
• Updates tests that used to call BlockEvaluator.GenerateBlock() and expected a ledgercore.ValidatedBlock returned (including deltas): now they get a ledgercore.UnfinishedBlock and can call UnfinishedBlock.UnfinishedDeltas() to access the deltas (name makes clear they are for simulation/test purposes only).

[jannotti]

I'm still trying to understand all the different block types and interfaces. Biggest areas of confusion are the differences between the various validated blocks and proposable blocks.

[jannotti]

This is the type I don't understand. Now that we have a ledgercore.UnfinishedBlock and I think you even removed Block() so it can't be mistaken for a proposable or validated block, why not use that type directly in node package?

[jannotti]

It seems to come down to this. A ledgercore.ValidatedBlock has this:

// FinishBlock completes the block and returns a proposable block.
func (ub UnfinishedBlock) FinishBlock(s committee.Seed, proposer basics.Address, eligible bool) bookkeeping.Block {
	return ub.blk.WithProposer(s, proposer, eligible)
}

but it doesn't really return a proposable block, it returns a bookeeping.Block. So this FinishBlock doesn't implement agreement.UnfinishedBlock. So we have a ledgercore.UnfinishedBlock that is not an agreement.UnfinishedBlock.

Seems like the root problem is having ledgercore and agreement wanting to both do a lot with blocks, but being unable to share anything amongst each other. So node creates all sort of similarly named, but subtly different shims to convince them to just do the obvious things - modify some fields in a block.

Would this be fixed by moving the various *Block interfaces into bookkeeping? agreement and ledgecore don't have to be declare their own interfaces, just to forced node to bend them to each others' will, do they?

[jannotti]

I tried moving the interfaces into bookkeeping in a PR on top of this one. cce#18
I think it's simpler because it requires few types and less wrapping, but I suppose it's debatable. What do you think?

[cce]

IDK ... it breaks the existing pattern in the agreement package that all the interfaces it needs from the rest of the codebase are defined in agreement/abstractions.go, and also is counter to the Go team's best practices advice "Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values.".

Since agreement is the only user of these types, IDK if it makes sense to move it to bookkeeping?

[cce]

Seems like the root problem is having ledgercore and agreement wanting to both do a lot with blocks, but being unable to share anything amongst each other. So node creates all sort of similarly named, but subtly different shims to convince them to just do the obvious things - modify some fields in a block.

Given the current design I'm not sure that ledger/ledgercore is "supposed" to be able to share anything with agreement unless expressly permitted by the integration package (the node implementation). There are pros and cons, but tight control of the integration logic between agreement and the rest of the systems (transaction pool, ledger lookups, block writing/validating) has historically been enforced via the node implementation.. breaking those walls would also break precedent.

[jannotti]

"Go interfaces generally belong in the package that uses values of the interface type, not the package that implements those values.".

That is good advice if your interface is designed to let others supply you with multiple implementations of an interface, maybe without even knowing you've defined such an interface. But as soon as you start intertwining your demands, by requiring an implementation to have a method that returns one of your other interfaces (as UnfinishedBlock requires of FinishBlock), I think you're in a different world. Your clients must know about you and the interfaces you have, or else they can't possibly implement them.

At that point, it seems to me that this advice is not helpful. You don't gain the cool property that implementors don't even know you structured your code with interfaces. Instead, they are forced to contort themselves to adapt every possible use, because they can't simply use a single type that works for them and for you. But if the interface is well known, like error or io.Writer, multiple users of the interface can pass the same types around.

Since agreement is the only user of these types

Would you feel ok if ledgercore imported agreement? In order to get at the necessary types to implement agreement.UnfinishedBlock? In the end, that's what I want to have happen. I don't see any advantage to node knowing how to make the agreement interface signatures happy, versus ledgercore doing it, instead of having its own UnfinishedBlock that can't implement agreement.UnfinishedBlock.

To my mind, all these wrappers just increase the chance we're doing something wrong by sharing internal pointers or similar.

[cce]

I am OK with that — I generally try to follow the existing pattern but reducing extra types/shims has its own benefit and I just noticed

go-algorand/node/impls.go

Line 34 in dfd95ff

// TODO these implementations should be pushed down into the corresponding structs or alternatively turned into new structs in the correct subpackages

[jannotti]

XXX in non-test code is confusing. Is this something that needs to be fixed?

[cce]

Not intended for merge, left a few in there because I wanted to get feedback about it — can remove the XXXs now that they've been spotted and mentioned!

[cce]

I was assuming reviewers would have an opinion on the test usages here — I was just doing the minimum to get the tests working again, but was curious since there are different ways to turn generated blocks into validated blocks for these tests.

[jannotti]

I was starting to learn toward some sort of fakeAgreement function for test code instead of stuff like this:

ledgercore.MakeValidatedBlock(ub.UnfinishedBlock(), ub.UnfinishedDeltas())

especially for the places that also fake a Seed and Proposer.
But I'm not sure what should be done in simulate. It feels a little wrong to supply an easy way to coerce an UnfinishedBlock to Proposable/Validated. But then again, that's what we want to do. As long as it's explicit, I suppose it's fine.

[jasonpaulos]

It doesn't seem like these accounts are really used in this PR. Is the intent that incentives will use them after this is merged to look up info on the proposer (e.g. if paying them wouldn't violate their min balance)?

[jannotti]

Yes, that's the intent. I'd prefer to merge this into the incentives PR, and perform that work before merging this to mainline.

[cce]

Yes, using the balances I left as a TBD for merge into #5757

[cce]

Merged as part of #5757