Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use pointer receiver type because of atomic.Value #9667

Merged
merged 2 commits into from
Mar 25, 2024

Conversation

pcw109550
Copy link
Contributor

@pcw109550 pcw109550 commented Mar 10, 2024

One line summary: We must use pointer receiver type for tx methods because if not, value receivers will copy the struct containing atomic.Value field, which is prohibited to be copied. TransactionMisc struct is embedded to to every tx type, so we must use pointer receiver for every tx methods.

For more context, struct TransactionMisc is defined as below:

type TransactionMisc struct {
	// caches
	hash atomic.Value //nolint:structcheck
	from atomic.Value
}

TransactionMisc is embedded to struct AccessListTx, BlobTx, BlobTxWrapper, DynamicFeeTransaction, CommonTx, LegacyTx. Methods for these structs tend to use value receiver, not a pointer receiver.

When value receiver method is used, the program copies the struct value and uses it. TransactionMisc is embedded, so its fields hash and from are also copied.

However these fields' types are atomic.Value, which are not allowed to be copied after first use. This guideline is also mentioned at Google's golang style guide.

Therefore we must use pointer receiver to avoid synchronization issues. Also, using pointer receivers may be more efficient if receiver is large struct.

We must use receiver type for tx methods because if not,
value receivers will copy the struct containing atomic.Value field.
TransactionMisc struct is embedded to to every tx type, so
we must use pointer receiver for every tx methods.
@pcw109550
Copy link
Contributor Author

@AskAlexSharov Thanks for reviewing/approving the PR. To add more context, golang linter could not detect this issue because of golang legacy issues.

Which means there are other places to fix to not copy atomic.Pointer.

For example, at https://github.com/ledgerwatch/erigon/blob/e9a8555be262717da38e4aa9b3fdf2e2b02a9057/core/types/transaction_test.go#L696, you can see that blob tx is copied, which should be not copied but redefined.

Will open a follow PR to fix this issues across the codebase.

@AskAlexSharov
Copy link
Collaborator

I will merge it after current release done.

@yperbasis yperbasis enabled auto-merge (squash) March 25, 2024 12:18
@yperbasis yperbasis merged commit 74abef8 into erigontech:devel Mar 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants