core/txpool: Improve error responses #30715
Conversation
piersy
requested review from
karalabe,
holiman and
rjl493456442
as code owners
I noticed that these two responses can make their way back to the transaction submitter when submitting unsupported tx types. Adding the extra context makes it easier for the submitter to understand what went wrong. Particularly in the case of the invalid sender response, which is returned when an unsupported transaction type is encountered.
piersy
force-pushed
the
improve-error-responses-for-invalid-transactions
branch
from
967c675
to
e44cfee
Compare
core/txpool/txpool.go
Outdated
| @@ -341,7 +341,7 @@ func (p *TxPool) Add(txs []*types.Transaction, local bool, sync bool) []error { | |||
| for i, split := range splits { | |||
| // If the transaction was rejected by all subpools, mark it unsupported | |||
| if split == -1 { | |||
| errs[i] = core.ErrTxTypeNotSupported | |||
| errs[i] = fmt.Errorf("%w (type %d)", core.ErrTxTypeNotSupported, txs[i].Type()) | |||
There was a problem hiding this comment.
Please change the format to "%w: received type %d", to keep it consistent with the rest of our error formatting.
core/txpool/validation.go
Outdated
| @@ -99,7 +99,7 @@ func ValidateTransaction(tx *types.Transaction, head *types.Header, signer types | |||
| } | |||
| // Make sure the transaction is signed properly | |||
| if _, err := types.Sender(signer, tx); err != nil { | |||
| return ErrInvalidSender | |||
| return fmt.Errorf("%w: %w", ErrInvalidSender, err) | |||
There was a problem hiding this comment.
The second %w should be %v, I'm not sure having 2 %w formatters is a good idea. It was definitely disallowed in the past. I think they relaxed it now, but it would go against our way of using the errors, so lets keep it the simpler form.
There was a problem hiding this comment.
Wrapping mulitple errors like this was introduced in go 1.20 see (https://tip.golang.org/doc/go1.20#errors). Happy to change it, but could you explain why having the two errors wrapped would be problematic for you?
There was a problem hiding this comment.
It's mostly about style. If the entire codebase operates on the first being the thing wrapped and the second is just a detail, it's less surprising what happens when you match on errors. If some errors wrap both parts, then a match might match on the end not the beginning. That's fine, if the code is architected to take that into account, which ours isn't really.
There was a problem hiding this comment.
Thanks for the explanation @karalabe.
I noticed that these two responses can make their way back to the transaction
submitter when submitting unsupported tx types.
Adding the extra context makes it easier for the submitter to understand what
went wrong. Particularly in the case of the invalid sender response, which is
returned when an unsupported transaction type is encountered.