miner: Add block building interruption on payload resolution (getPayload)

[sebastianst]

Description

• Block building is interrupted on a getPayload engine api call.
• Empty block is now only built for NoTxPool FCU calls, and as before, FCU blocks on building the empty block. This has the advantage that if generating a lot of empty blocks, getPayload can immediately be called after forkchoiceUpdated to return that empty block.
• If using the tx pool, FCU doesn't block on any block building but returns immediately after creating the payload promise.
  • Resolution of the payload promise (getPayload) now waits for the first full block to be built, as there is no empty block to return. If at least one block has already been built, as before, it is returned immediately, while block building is interrupted in the background.

Tests

Extended the TestBuildPayload test to cover both cases of NoTxPool.

Added an ugly global bool to indicate that a test is running, which guarantees one full block is built and block building is not interrupted. This seemed to be the least-invasive way to make all tests still pass. This was necessary because a couple of test call Resolve or getPayload immediately after a FCU, but this would now interrupts the block building process, which made tests fail.

Additional context

Metadata

• Fixes https://github.com/ethereum-optimism/client-pod/issues/173
• Fixes https://github.com/ethereum-optimism/client-pod/issues/187
• Fixes https://github.com/ethereum-optimism/client-pod/issues/163

[sebastianst]

notes from call with proto

[ajsutton]

When talking about returning the "full" block, are we literally waiting for it to be packed with as many tx as possible, or does the interrupt stop adding tx to the block?

The ideal behaviour when getPayload is called is basically:

• If a valid block has already been built and is ready, return that and discard the in-progress block.
• Otherwise, stop adding tx from the pool immediately and finalise the block in progress so it can be returned.

Potentially you could be even smarter and compare the value of the existing block to the value of the block in progress and finalized the block in progress if it's already worth more.

That may mean that we return a half-full block because that's all we had time to build but that's a lot better than being late delivering the block. I wonder if that potentially simplifies some of the logic here as well since FCU would always defer building to the worker thread and getPayload would always have a block to return.

[sebastianst]

When talking about returning the "full" block, are we literally waiting for it to be packed with as many tx as possible, or does the interrupt stop adding tx to the block?

The interrupt stops adding to that "full" block that is currently being built.

The ideal behaviour when getPayload is called is basically:

• If a valid block has already been built and is ready, return that and discard the in-progress block.
• Otherwise, stop adding tx from the pool immediately and finalise the block in progress so it can be returned.

@ajsutton It is implemented like this 👍🏻

I now realize that there's one thing that can be optimized: in worker.generateWork, return immediately after fillTransactions if an interrupt is set and one block is already available. The current implementation still calls FinalizeAndAssemble even if the result is gonna be discarded. Will add this.

Potentially you could be even smarter and compare the value of the existing block to the value of the block in progress and finalized the block in progress if it's already worth more.

That may mean that we return a half-full block because that's all we had time to build but that's a lot better than being late delivering the block. I wonder if that potentially simplifies some of the logic here as well since FCU would always defer building to the worker thread and getPayload would always have a block to return.

This is exactly the implementation that I had up until last week. We then discussed last week that we rather want getPayload to instantly return if at least one block is already done, instead of it blocking on the currently ongoing block building process. This convinced me because my investigations showed that applying a single transaction, or calling FinalizeAndAssemble, might itself take several seconds in some instances. This isn't something that can easily be interrupted, since this is blocking somewhere down in some database access path.

The process could also happen with a timeout. Say, wait up to 10ms for the interrupted block building process to complete and the comparison to be done, and then return the existing block already if this times out. However, I remember that we discussed that an instant getPayload return is preferred.

If we want to revisit that decision, I suggest to do that in a follow-up.

[ajsutton]

Ok, went through this carefully and it looks good to me. Only question is really around the global var and as mentioned, I think we can avoid it but you may have seen something I missed.

[sebastianst]

@ajsutton thanks for review, I'm happy with your test workaround as well, will merge it into this pr

[protolambda]

Reviewed the payload-interrupt code, and the discussion about the half-full block, but I think we are not interrupting correctly on the first payload currently:

• If resolve(false), and if there's no block yet, it currently does not interrupt the block building at all: it waits for the payload with payload.cond.Wait(), and only then calls payload.stopBuilding().
• If an interrupt is received by the worker, during fillTransactions, then it doesn't seal the work, and returns an error instead. The error is propagated to payload.update(), and would set the payload.cond, but due to order of operations it'll be waiting for this cond before firing the interrupt.

The above two things combined mean we can't fire stopBuilding() early (due to lack of sealing on interrupt error)), and are forcing ourselves to build the full 30M gas block (due to lack of interrupt, since we wait for the cond if it's the first block).

I think we should add a flag to generateParams that defines whether or not we want to seal the half-full block after interrupt, or return an interrupt error.
And then run stopBuilding() before running the payload.cond.Wait(), to get that half-full block result, rather than not interrupting the first block. (This should work fine, since the interrupt is clean (we stop adding more txs, it doesn't leave the block-building state in an inconsistent place).

That way we don't get hung up on large 30M block building attempts if it's the first block building attempt.

Edit: I missed the isUpdate flag, it's already there. So it's mostly just the stopBuilding call we may want to move up, before the cond.Wait.

[sebastianst]

Thanks Proto, I moved the interrupt now before potentially waiting on the conditional, so we interrupt even the first block building routine. It was kind of a deliberate decision at the time to build the first block fully, but it's indeed better to even interrupt the first block, because even that one may take a long time.

As you wrote in the edit, I already added the isUpdate flag to generateParams which signals whether we seal an interrupted block or not.

I added another test that confirms that interruption actually works.

I now had to add more sleeps to the eth/catalyst/api_test.go tests, which now lets the tests run for ~143 s (measured with time go test ./eth/catalyst -count 1) instead of 2.7s! This was the reason in the first place why I just disabled interrupts completely in tests with the global var hack, because all geth tests assume that resolve doesn't interrupt. I personally would still favor just disabling it for the tests in eth/catalyst rather than adding sleeps.

[ajsutton]

hmm, so there's a failure in TestSimulatedBeaconSendWithdrawals which is using GetPayload straight after starting block building (

op-geth/eth/catalyst/simulated_beacon.go

Lines 153 to 172 in a8a9c8e

	var random [32]byte
	rand.Read(random[:])
	fcResponse, err := c.engineAPI.ForkchoiceUpdatedV2(c.curForkchoiceState, &engine.PayloadAttributes{
	Timestamp: tstamp,
	SuggestedFeeRecipient: feeRecipient,
	Withdrawals: withdrawals,
	Random: random,
	})
	if err != nil {
	return err
	}
	if fcResponse == engine.STATUS_SYNCING {
	return errors.New("chain rewind prevented invocation of payload creation")
	}
	envelope, err := c.engineAPI.getPayload(*fcResponse.PayloadID, true)
	if err != nil {
	return err
	}
	payload := envelope.ExecutionPayload

) so that's another place we'd need to wait for the building to complete.

I've exposed a couple of methods to deterministically wait for the block building to complete rather than needing a fixed timeout in #204 and hooked that up in the simulated beacon as well. We may need to add appropriate calls to wait in more tests yet which will be an annoying source of flakiness for a while but I think this is the right approach to deterministically wait. Definitely open to better ways to expose the ability to wait though.

[sebastianst]

Refined @ajsutton's solution and fixed a buggy test (TestNilWithdrawals)

[protolambda]

Looks good! Sorry I missed a few minor things in my last review, the PR looks ready otherwise