Since the current spec is tied to specific types like PayloadAttributesV3and engine_getPayloadV5does this implicitly mean that for each fork which would change these types (PayloadAttributesV4) we thus also need a new testing_buildBlockVx instance? (Which is fine).

Another main motivation of this method is bypassing the txpool. We could start a client and only inject specific txs into txpool, but if we then ask it to build a block via normal route (engine API) then it is not guaranteed all txs are in it, and the order of the txs is also not guaranteed.

Another feature I'd like here is a gas limit. Besides the obvious reason to create bigger blocks for benchmarks, there might also be other reasons to control the gas limit, for instance instead of going up, bringing it down (this has influence on the fee market).
The suggestion here is to add an optional parameter "gas target" or something similar, and the client will attempt to move to that gas target (within the bounds of increasing/decreasing the gas limit of a block)

This can obviously be done via client config, but with EELS as driver for the test/benchmark generator it is handy to be able to control this from EELS directly, without having to edit a config to set a specific gas target.

This is not really a good description, are we returning the method? or the same response as getPayloadV5

I read this as defining the schema of the response object to the the same as engine_getPayloadV5 response object?

It might be a useful (optional?) feature to allow for selectiung txpool transactions in the case where the transaction array is empty.

I would keep it as simple as possible in V1 to make it as easy as possible to implement for all EL teams. The use case is simple here - to build a block from provided txs. Your proposition is interesting, but:

1. It can break some test cases e.g. empty payload tests
2. It will add complexity - in current form we can totally ignore mempool and it's transactions

Also here, if majority of people will opt to have such feature, I'm fine with adding it.

I think my wording keeps the option open without specifying what the client should do in the case of empty transaction array.

In Besu's case, we actually have a similar RPC method we use for internal testing, engine_preparePayload_debug see besu-eth/besu#4427
There are enough similarities that we would probably modify it to fit this spec, but the main use case for ours is to test txpool selection.

I think if an empty array is passed then an empty transaction list should be used, otherwise it's non-deterministic, in the case of test chains and devnets, you may very well be trying to generate empty blocks. And to that effect, only the txs in the array should be included.

What if we make an empty array be "build an empty block" and null be "use your mempool if you want to"? Or is that too much?

I'd be fine with that, as long as the option is possible in some form that is spec-compliant, don't mind about the syntax.

The transactions parameter MUST accept an array, not null.

Accepting mempool transactions contradicts the intent:

"It is intended to replace the multi-step workflow of sending transactions"

With null + mempool, we are back to multi-step: eth_sendRawTransaction + testing_buildBlockV1.[1]

This endpoint will be heavily used for benchmarking. We should keep the underlying logic straightforward and slim.

As always, we can extend if there is pressing need for the mempool path for testing later. Lets keep v1 simple for now.

[1]: mempool test case

Then don't send null for your use case

The extraData field is a bit weird, since we didn't use it before, it needs to be piped through to the miner

extraData is part of the blockhash though, is it not? I think it's important that all the fields that would contribute to the blockHash of a real block are properly represented otherwise the returned blockhash wouldn't match the actual/expected blockhash (which is very useful for testing).

With extraData edits you can generate the same block, but with a different hash, which is a nice property. All other fields will edit something in the block which is observable from EVM.

What if invalid txs get added?

I have a test case for that, it's a defined error in my PR

Would this fit better under debug_ namespace?

The idea of creating a new namespace is to prevent enabling this method by mistake, together with other debug ones.
It was specified here: ethpandaops/gas-lighting-tracker#5
This new endpoint is sensitive and should exist under a new, non-exposed namespace (i.e., not debug_ or admin_ which are sometimes exposed by RPC providers).

@spencer-tb has another idea - to make it _debug so it will be different than current debug, but will not introduce quite exotic testing namespace.
I'm open for every option.

Besu actually already has a similar endpoint called engine_preparePayload_debug so would support the _debug suffix 👍

I think debug_ prefix is also fine because there are already debug_ methods that can do quite a lot of damage so it would be a misconfiguration to leave them publicly accessible IMO.

The problem with the debug namespace is that it contains the tracing methods, and basically everyone needs/wants the tracing methods, so debug is very often forced to be on, even for people who don't want the expose "dangerous methods"

I have no problem with a new namespace called testing being created, but I also have no problem with it going under debug since debug has been treated like a catchall and one more dangerous method isn't going to break the mess that is the debug namespace any more than it already is.

it's probably the trace ones that are most dangerous, in terms of resource consumption at least 😆
But yeh I take your point, I imagine there's ppl who want trace but not this.

Should we also hide it behind the jwt?

I'm not opposed to hiding behind jwt but there are a few things to consider:

1. Not all clients allow enabling/disabling methods specifically on the auth rpc port
2. It makes tooling an automation lot more difficult

Since this is a new namespace, I think it's overkill to require it to be hidden behind auth