Skip to content

Conversation

yacovm
Copy link
Contributor

@yacovm yacovm commented Apr 29, 2025

Why this should be merged

If the user sends a transaction with the wrong chain ID, it is returned with a cryptic error.

This commit adds a logging event that informs if a user tries to send a transaction with the wrong chain ID.

How this works

Adds a log entry in case the chain ID mismatches with the transaction's chain ID.

How this was tested

Ran it locally with error logging level and then reverted back to trace logging level.

Need to be documented?

Need to update RELEASES.md?

@yacovm yacovm requested review from a team and ceyonur as code owners April 29, 2025 15:26
opts.MinTip = new(big.Int)
}
if err := txpool.ValidateTransaction(tx, pool.currentHead.Load(), pool.signer, opts); err != nil {
if errors.Is(err, txpool.ErrInvalidSender) && pool.chainconfig.ChainID.Cmp(tx.ChainId()) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this log condition will only be met for EIP-155 type transactions that were signed using an incorrect chain ID (where the chain ID isn't explicitly included in the tx itself, but using the wrong chain ID results in an invalid signature).

Should we remove the "invalid sender" check, and instead just have the chain ID check? I think that way it will apply for any tx types using the wrong chain ID, including EIP-2930, EIP-1559, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this log condition will only be met for EIP-155 type transactions that were signed using an incorrect chain ID (where the chain ID isn't explicitly included in the tx itself, but using the wrong chain ID results in an invalid signature).

Should we remove the "invalid sender" check, and instead just have the chain ID check? I think that way it will apply for any tx types using the wrong chain ID, including EIP-2930, EIP-1559, etc.

Sure, done.

This commit adds a logging event that informs if a user tries to send a transaction with the wrong chain ID.

Signed-off-by: Yacov Manevich <[email protected]>
@yacovm yacovm force-pushed the logChainMismatch branch from b0f028f to e2b4962 Compare May 2, 2025 13:56
Comment on lines +686 to +688
if pool.chainconfig.ChainID.Cmp(tx.ChainId()) != 0 {
log.Trace("Transaction targets wrong chain", "chainID", tx.ChainId(), "expected", pool.chainconfig.ChainID)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the right place to put this (I think we are trying to minimize the diff with geth in the core package.. Right? cc: @ceyonur)

If we do go with this, we shouldn't log this for unprotected transactions.

Suggested change
if pool.chainconfig.ChainID.Cmp(tx.ChainId()) != 0 {
log.Trace("Transaction targets wrong chain", "chainID", tx.ChainId(), "expected", pool.chainconfig.ChainID)
}
if tx.Protected() && pool.chainconfig.ChainID.Cmp(tx.ChainId()) != 0 {
log.Trace("Transaction targets wrong chain", "chainID", tx.ChainId(), "expected", pool.chainconfig.ChainID)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do go with this, we shouldn't log this for unprotected transactions.

why?

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.

6 participants