feat(core/types): Header Copy hook

[qdm12]

Why this should be merged

Introduce header copy hooks to get rid of core/types.CopyHeader in coreth and subnet-evm

How this works

Header copy hooks 🎉

How this was tested

• Tested in chore(core/types): Header libevm PostCopy hook coreth#759
• Needs to be unit tested

[ARR4N]

This introduces a breaking change, which we should only do as an absolute last resort because it can have flow-on effects that require changes elsewhere now or in the future. This was a primary source of difficulty when performing upgrades under the old forking approach as I explained in the proposal for this project:

A core tenet of [the libevm] approach is that geth MUST be considered to be a “user” of libevm. As they update their own code, they assume that the rest of it is intact—this may be a tautology, but an important one nonetheless.

It also places the entire burden of copying on the libevm importer when all that's needed is for them to copy the registered extra.

What if the hook was PostCopy(dst, src *Header), called at the end of this function, and the NOOPHeaderHooks implementation was empty?

func CopyHeader(h *Header) *Header {
// ...
  h.hooks().PostCopy(cpy, h)
  return cpy
}

[qdm12]

It's PostCopy(dst) in the end, given the source is already accessible by the header hooks.

[ARR4N]

How is it already accessible? Can you share an example snippet, please?

[qdm12]

Never mind, I had too many dead brain cells when writing this.
PostCopy doesn't (at least for now) need to do anything with the source header, since it's meant to only copy the extra payload (aka HeaderExtra). For example in coreth:

https://github.com/ava-labs/coreth/pull/759/files#diff-c238f33a661d072dc51dd5a20ab81dbd341f59478b5c26d25db686822b492521R85

[ARR4N]

That's fair. We can always add it later as we don't make any interface-stability guarantees.

[ARR4N]

Why not merge into main? Is there a reason to batch PRs for a later re-review and merge?

[qdm12]

Why not merge into main?

I needed the fixes of darioush/libevm-phase-2.5 to make it work in coreth. How are we planning to handle the few fix commits in darioush/libevm-phase-2.5 in the end?

Is there a reason to batch PRs for a later re-review and merge?

Not at all, I would rather not 😉

[ARR4N]

Approved with minor suggestions that don't require re-review

[ARR4N]

Suggested change

	accessor pseudo.Accessor[*Header, *stubHeaderHooks]
	copied *stubHeaderHooks
	accessor pseudo.Accessor[*Header, *stubHeaderHooks] // from [RegisterExtras]
	toCopy *stubHeaderHooks

copied implies that it's the "got" / "actual" of the test whereas toCopy implies that it's an action for the test double.

[ARR4N]

Suggested change

	hdr := new(Header)
	stub := &stubHeaderHooks{
	accessor: extras.Header,
	copied: &stubHeaderHooks{
	suffix: []byte("copied"),
	},
	}
	extras.Header.Set(hdr, stub)
	cpy := CopyHeader(hdr)
	copiedStub := extras.Header.Get(cpy)
	assert.Equal(t, stub.copied, copiedStub)
	hdr := new(Header)
	stub := &stubHeaderHooks{
	accessor: extras.Header,
	toCopy: &stubHeaderHooks{
	suffix: []byte("copied"),
	},
	}
	extras.Header.Set(hdr, stub)
	got := extras.Header.Get(CopyHeader(hdr))
	assert.Equal(t, stub.toCopy, got)

My suggested changes aren't as broad as the GH diff suggests:

1. Changed copied to toCopy, in keeping with my earlier suggestion;
2. Spacing change to logically group operations as setup then assertion; and
3. Usage of "got" variable to signal that it's the value under test.

[ARR4N]

Why not merge into main?

I needed the fixes of darioush/libevm-phase-2.5 to make it work in coreth. How are we planning to handle the few fix commits in darioush/libevm-phase-2.5 in the end?

Is there a reason to batch PRs for a later re-review and merge?

Not at all, I would rather not 😉

Was it only the change to the legacy package that's absolutely required? If so, can you please open another PR and take a closer look re avoiding gas refunds I mentioned on DM?