all: refactor so `NewBlock`, `WithBody` take `types.Body` #29482

lightclient · 2024-04-07T21:50:46Z

Something we've discussed a bit in the past; thought I would see if there is interest in it now before we look into merging PRs for 6110 and 7002 -- they both add new quantities to the body.

core/types/block.go

rjl493456442 · 2024-04-08T03:11:19Z

generally i think the idea is pretty good.

core/types/block.go

lightclient · 2024-04-08T20:31:14Z

Thanks @rjl493456442 and @fjl. PR updated based on your feedback.

karalabe · 2024-04-11T08:47:53Z

core/types/block.go

@@ -220,10 +221,20 @@ type extblock struct {
 // The values of TxHash, UncleHash, ReceiptHash and Bloom in header
 // are ignored and set to values derived from the given txs, uncles
 // and receipts.
-func NewBlock(header *Header, txs []*Transaction, uncles []*Header, receipts []*Receipt, hasher TrieHasher) *Block {
-	b := &Block{header: CopyHeader(header)}
+func NewBlock(header *Header, body *Body, receipts []*Receipt, hasher TrieHasher) *Block {


@rjl493456442 noted that in WithBody, you passed a non-pointer Body. Either case might be fine, but maybe it should be consistent across the two methods.

Given that a Body is fairly small containing only slices, and you also have this strange check below to handle nil,it might be simpler to go with the struct.

Unrelated: it's a bit confusing when you should use NewBlock and when NewBlockWithHeader.WithBody. Perhaps we could consider tweaking the names of one or the other a bit to make it clearer that they do different things and serve different purposes. Otherwise IMO it's asking for it.

Since we already handle Body as a pointer value in other places, I think making it a struct in NewBlock is going to be a bit annoying. This is seen mostly in implementations of consensus.Engine where FinalizeAndAssemble takes a pointer Body.

If you think it is worth turning all uses of Body over to struct values, I can do it. It just seems like it might actually be better to keep the pointer value given this.

Similarly with NewBlock and WithBody interfaces matching, if they need to match, they should probably both be *Body.

cc @fjl since I think it was his thought to convert to struct val.

Also agree on it being confusing when to use NewBlock vs. NewBlockWithHeader.WithBody. I think the answer is you use NewBlock when you need to compute the header accumulators with the body values and NewBlockWithHeader.WithBody when you need to recreate the block which has been disassembled (written to disk, received over the wire, etc).

Naming isn't perfect, but struggling to come up with something markedly better.

I still think it's a bit non-obvious, but it might suffice to update some method-docs.

The difference with NewBlock is that it would deep copy all inputs. It's meant to be used in tests, and that's also why it has a short name. Across production code, very few places actually create blocks, so we can live with a longer function name and we can also avoid some of the copying.

core/types/block.go

karalabe · 2024-04-11T08:50:52Z

beacon/types/exec_payload.go

-	block := types.NewBlockWithHeader(&header)
-	block = block.WithBody(transactions, nil)
-	block = block.WithWithdrawals(withdrawals)
+	block := types.NewBlockWithHeader(&header).WithBody(types.Body{Transactions: transactions, Withdrawals: withdrawals})


Why not the new NewBlock (with everything)?

I guess you don't have the receipts to recalculate header fields.

Exactly. Plus it might be a little weird to recalc the header fields when here we're trying to tease out our block def from a beacon block's execution payload. Should probably 1:1 the data and do a hash comparison somewhere to ensure integrity.

core/rawdb/accessors_chain.go

karalabe · 2024-04-11T08:55:31Z

PR and general directions seems ok, just some open ended questions to debate.

holiman

LGTM

holiman · 2024-04-15T12:32:17Z

core/types/block.go

@@ -220,10 +221,20 @@ type extblock struct {
 // The values of TxHash, UncleHash, ReceiptHash and Bloom in header
 // are ignored and set to values derived from the given txs, uncles
 // and receipts.
-func NewBlock(header *Header, txs []*Transaction, uncles []*Header, receipts []*Receipt, hasher TrieHasher) *Block {
-	b := &Block{header: CopyHeader(header)}
+func NewBlock(header *Header, body *Body, receipts []*Receipt, hasher TrieHasher) *Block {


I still think it's a bit non-obvious, but it might suffice to update some method-docs.

holiman · 2024-04-15T12:35:15Z

core/types/block.go

 // WithBody returns a copy of the block with the given transaction and uncle contents.
-func (b *Block) WithBody(transactions []*Transaction, uncles []*Header) *Block {
-	block := &Block{
-		header:       b.header,
-		transactions: make([]*Transaction, len(transactions)),
-		uncles:       make([]*Header, len(uncles)),
-		withdrawals:  b.withdrawals,
-	}
-	copy(block.transactions, transactions)
-	for i := range uncles {
-		block.uncles[i] = CopyHeader(uncles[i])
-	}
-	return block
-}
-
-// WithWithdrawals returns a copy of the block containing the given withdrawals.
-func (b *Block) WithWithdrawals(withdrawals []*Withdrawal) *Block {
+func (b *Block) WithBody(body Body) *Block {


Method doc should expand a bit. E.g.

// WithBody returns a new block, consisting of the orginal header (not copied) along // with the body contents (which are deep-copied). // OBS: The header-fields based on body derivatives (e..g txhash) are _not_ updated, and will // mismatch if the body does not match the header.

It's a bit funny / curious that this method avoids copying the original header, but conscientiously copies the incoming body.

So the idea here is, header fields inside types.Block can't leak out because we copy their values in block accessors. We do need to copy the fields in the passed Body here because a later modification of the input data (e.g. uncle header values) could mess up the internal caches in Block.

holiman · 2024-04-17T07:09:23Z

This PR has a pretty subtle but scary bug, surfaced by this test (well, and many others):

state_processor_test.go:263: test 0:
        have "missing withdrawals in block body"
        want "could not apply tx 1 [0x0026256b3939ed97e2c4a6f3fce8ecf83bdcfa6d507c47838c308a1fb0436f62]: nonce too low: address 0x71562b71999873DB5b286dF957af199Ec94617F7, tx: 0 state: 1"

Empty withdrawals become nil withdrawals on b, here, in block.go:

	if withdrawals == nil {
		b.header.WithdrawalsHash = nil
	} else if len(withdrawals) == 0 {
		b.header.WithdrawalsHash = &EmptyWithdrawalsHash
	} else {
		hash := DeriveSha(Withdrawals(withdrawals), hasher)
		b.header.WithdrawalsHash = &hash
		b.withdrawals = slices.Clone(withdrawals)
	}

Previously, it would (after some call dispatching), come to func (b *Block) WithWithdrawals and do

	block.withdrawals = make([]*Withdrawal, len(withdrawals))
	copy(block.withdrawals, withdrawals)

holiman

un-approved for now :)

lightclient · 2024-04-22T20:44:31Z

That was a sneaky one! I updated so that NewBlock(..) now explicitly sets the body's withdrawals to an empty, non-nil slice of withdrawals. I looked into WithBody(..) to see if there might also be an issue there (like nil withdrawals ends up as non-nil somehow) but it looks like slices.Clone retains the nil-ness of the input. So cloning a nil withdrawals list will still return nil.

holiman · 2024-04-29T13:22:32Z

Needs a rebase

holiman · 2024-04-30T07:03:26Z

core/types/block.go

-		b.header.WithdrawalsHash = &h
+		hash := DeriveSha(Withdrawals(withdrawals), hasher)
+		b.header.WithdrawalsHash = &hash
+		b.withdrawals = slices.Clone(withdrawals)


Can't you just do this unconditioally? IIUC, slices.Clone respects nil vs empty

holiman

Seems ok to me

…9482) * all: refactor so NewBlock(..) and WithBody(..) take a types.Body * core: fixup comments, remove txs != receipts panic * core/types: add empty withdrawls to body if len == 0

lightclient requested review from gballet, karalabe, holiman and rjl493456442 as code owners April 7, 2024 21:50

lightclient changed the title ~~all: refactor so NewBlock, NewBody take types.Body~~ all: refactor so NewBlock, WithBody take types.Body Apr 7, 2024

lightclient force-pushed the types-newblock-refactor branch from 3f67698 to 23edbea Compare April 7, 2024 21:52

rjl493456442 reviewed Apr 8, 2024

View reviewed changes