Fix batch tx send encoding #11500
Fix batch tx send encoding #11500
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
amit-momin
force-pushed
the
bug/fix-batch-tx-encoding
branch
from
083a17a to
3ad6f6f
Compare
amit-momin
force-pushed
the
bug/fix-batch-tx-encoding
branch
from
3ad6f6f to
9a344d1
Compare
amit-momin
requested review from
a team,
RyanRHall,
samsondav,
prashantkumar1982 and
jmank88
as code owners
core/chains/evm/client/errors.go
Outdated
| @@ -424,6 +424,7 @@ func ClassifySendError(err error, lggr logger.Logger, tx *types.Transaction, fro | |||
| return commonclient.Fatal, err | |||
| } | |||
| if sendError.IsNonceTooLowError() || sendError.IsTransactionAlreadyMined() { | |||
| lggr.Debugw("Transaction already confirmed for this nonce: %d", tx.Nonce(), "err", sendError) | |||
There was a problem hiding this comment.
For some of these messages we already log on the caller side, so you might want to aggregate those.
There was a problem hiding this comment.
Wouldn't it be alright to keep both? The caller would be able to provide more info potentially and it'd also make tracing the call easier. And keeping it the logs in the ClassifySendError ensures we get a log for all of these even if the caller side doesn't
There was a problem hiding this comment.
I'm not against it, but if we keep them let's try to differentiate the 2 messages to get more context.
There was a problem hiding this comment.
I've updated the error logging in the broadcaster and confirmer to avoid re-logging the same thing as the ClassifySendError function
core/chains/evm/client/errors.go
Outdated
| @@ -413,20 +413,21 @@ func ExtractRPCError(baseErr error) (*JsonError, error) { | |||
| return &jErr, nil | |||
| } | |||
|
|
|||
| func ClassifySendError(err error, lggr logger.Logger, tx *types.Transaction, fromAddress common.Address, isL2 bool) (commonclient.SendTxReturnCode, error) { | |||
| func ClassifySendError(err error, lggr logger.Logger, tx *types.Transaction, fromAddress common.Address, isL2 bool) commonclient.SendTxReturnCode { | |||
There was a problem hiding this comment.
nice cleanup here :)
There was a problem hiding this comment.
out of curiosity is there any reason we dont grab the fromAddress from the tx?
There was a problem hiding this comment.
Good question. I think it's because this Transaction type isn't our internal one but the go-ethereum one which doesn't expose the from address.
| return | ||
| } | ||
| codes[i], txErrs[i] = client.ClassifySendError(reqs[i].Error, lggr, tx, attempts[i].Tx.FromAddress, c.client.IsL2()) | ||
| sendErr := reqs[i].Error | ||
| codes[i] = client.ClassifySendError(sendErr, lggr, tx, attempts[i].Tx.FromAddress, c.client.IsL2()) |
There was a problem hiding this comment.
what scenarios can the attempt.FromAddress differ from the tx.FromAddress?
There was a problem hiding this comment.
I think we're just resorting to getting the from address from the attempt here because you can't get it through the go-ethereum Transaction tx
There was a problem hiding this comment.
sorry didnt see you previous message :)
There was a problem hiding this comment.
Don't you also need to make the change to this logic for batch Txs:
chainlink/core/chains/evm/txmgr/common.go
Line 43 in bd7e233
The other places that you've changed, don't seem to be used for batched sending, just regular sending of unbatched Txs.
| if decodeErr != nil { | ||
| return reqs, now, successfulBroadcast, fmt.Errorf("failed to decode signed raw tx into Transaction object: %w", decodeErr) | ||
| } | ||
| txBytes, marshalErr := signedTx.MarshalBinary() |
There was a problem hiding this comment.
Could you explain via a comment, why do we have to decode the raw tx and encode it again before sending in batched req?
|
SonarQube Quality Gate
|
MarshalBinary. The encoding was causing an invalid RLP error when resending tx's in batch. The client was unable to properly decode the tx which was usingUnmarshalBinaryunder the hood.types.NewTransactionwhich has been deprecated. It was replaced withtypes.NewTxcalled on atypes.LegacyTxobject.