pm: Only mark failed tx as redeemed for check tx err #2018
Conversation
yondonfu
force-pushed
the
yf/non-retryable-ticket-res
branch
from
f47dac1
to
b7e902a
Compare
yondonfu
force-pushed
the
yf/non-retryable-ticket-res
branch
from
b7e902a
to
4c5daef
Compare
|
|
||
| func isNonRetryableTicketErr(err error) bool { | ||
| // The latter check depends on logic in eth.client.CheckTx() | ||
| return err == errIsUsedTicket || strings.Contains(err.Error(), "transaction failed") |
There was a problem hiding this comment.
I don't feel great about this string check, but I ran into issues implementing an error type check since the tx failure check occurs in the eth package and importing an error type from the eth package in the pm package would result in an import cycle. There may be a better way to structure this check, but I went with the simple solution of a string check right now accepting the downside of exposing the pm package to the implementation details of eth.client.CheckTx().
| @@ -25,8 +25,6 @@ var unixNow = func() int64 { | |||
| return time.Now().Unix() | |||
| } | |||
|
|
|||
| type errCheckTx error | |||
There was a problem hiding this comment.
Removed this error type because we really care about whether the error when checking the tx is the result of a tx failure and this is accomplished by the string check in this PR making this error type unnecessary.
| ethcommon.Hash{}, | ||
| err, | ||
| } | ||
| // FIXME: If there are replacement txs then tx.Hash() could be different |
There was a problem hiding this comment.
Note: Create an issue to track this.
This was a problem before this PR and more substantial changes will be required to address it (i.e. return the tx that was mined from CheckTx()). The negative impact is relatively minimal though - the DB will just end up tracking the wrong tx hash for a redeemed ticket if a replacement tx was mined.
| @@ -424,7 +426,8 @@ func (sm *LocalSenderMonitor) redeemWinningTicket(ticket *SignedTicket) (*types. | |||
| if monitor.Enabled { | |||
| monitor.TicketRedemptionError() | |||
| } | |||
| return nil, errCheckTx(err) | |||
| // Return tx so caller can utilize the tx if it fails | |||
| return tx, err | |||
There was a problem hiding this comment.
This gives the caller access to the hash of a failed tx.
What does this pull request do? Explain your changes. (required)
At the moment, if an error is encountered while calling
eth.client.CheckTx()for a ticket redemption tx, the ticket is marked as redeemed with the tx hash set to the null hash (i.e. 0x000...). This behavior is a problem becauseeth.client.CheckTx()could return an error even if the tx did not fail. For example, an error is returned in the scenario where the tx needs to be replaced, but the required gas price for a replacement exceeds the node's configured max gas price. The consequence of this behavior is that the node incorrectly marks the ticket as redeemed in the node's DB which prevents the node from retrying the ticket and also makes it more difficult to query the DB for redeemable tickets in other tools such as the script in #2014.This PR introduces logic to only mark tickets for failed redeemed (i.e. revert, OOG) txs as redeemed.
Specific updates (required)
See commit history and in-line code comments.
How did you test each of these updates (required)
Ran unit tests.
Does this pull request close any open issues?
Fixes #2016
Checklist:
makeruns successfully./test.shpass