Make CommonTx have TransactionMisc as pointer

[ImTei]

This is a follow-up of the previous PR: #9667

As described in the previous PR, we must not copy atomic.Value, but TransactionMisc has atomic.Value, and it is implicitly copied in such codes (like #9667 (comment)).

So I made CommonTx have TransactionMisc as a pointer to prevent implicit copy.
And added store/load methods for atomic.Value to prevent nil pointer error at the initial time.

The concern about this change is there could be two TX objects which are sharing a same TransactionMisc pointer. This could make performance improvement(because cache is preserved), but may make bugs. I wanna ask your opinion. Thanks!

[yperbasis]

W/o this PR, are the implicit copies of TransactionMisc theoretical or do you have concrete examples?

[ImTei]

@yperbasis This issue was encountered when we were developing op-erigon.
We were trying to add a rollup-related field to TransactionMisc, but the linter said it must not be copied. See the following CI logs:

• changes
• CI logs

This CI error occurs because of atomic.Pointer, which is not in the upstream erigon, but atomic.Value is also not allowed to be copied. I have no idea why golang linter cannot catch the copy of atomic.Value, it seems to be an issue of golang linter. Anyway you can see the implicit copies in the CI logs.

This CI error was fixed after this PR is applied (testinprod-io#1)

[ImTei]

@yperbasis @somnathb1 Hi team, could you please let me know when it might be reviewed?

[somnathb1]

Shouldn't the receivers be for TransactionMisc instead

[ImTei]

If then, we can't wrap initializeMisc()

[somnathb1]

Rather than wrapping, checking nil should be a part of the accessing code

[ImTei]

So you prefer like this?

if cpy.TransactionMisc == nil {
	cpy.TransactionMisc = &TransactionMisc{}
}
cpy.from.Store(address)

I think it's easy to make nil pointer errors if developer forget to check nil. So I wrap store/load methods like this. Can you tell me why you prefer manual nil check?

[somnathb1]

Because it's never intended to be used with a nil, and if the developer forgets to check nil, it must be a bug before that point

[ImTei]

@somnathb1 Got it. I have some questions:

1. Are you talking about only this method(FakeSign)? if so, I agree that it's never called with nil TransactionMisc because it's initialized in copy().
2. Or are you talking about every code using the new Load/Store methods in this PR?

I made these new Load/Store methods because I changed the TransactionMisc in TX structs to a pointer, so TransactionMisc won't be initialized automatically when TX instances are initialized. Actually, I saw the nil pointer error before I made these methods.

As you know, TransactionMisc is used as cache in the current code, so I believe calling load/store methods before creating it is possible.
What do you think? Please let me know if I'm missing something!

[somnathb1]

All code, in general.
I think, though, that making TransactionMisc a pointer isn't the right way to go. It should rather be that all places that make use of "creating" a xyz_Tx object should do it with a store() call. Thus, this PR, I don't fully agree with

[ImTei]

@somnathb1 May I ask you why you don't think making TransactionMisc as a pointer is the right way?

[yperbasis]

Superseded by PR #11512