[TxnManager] Check if replaced txns are confirmed #228
Conversation
ian-shim
force-pushed
the
check-old-txns
branch
from
26d826c to
65209bf
Compare
|
Is this an nice-to-have improvement? Transaction B will be rejected since it doesn't have a higher nounce. |
No, this is a must have. Transaction A & B have the same nonce because one replaces the other. |
disperser/batcher/txn_manager.go
Outdated
| _, receiptErr := t.ethClient.TransactionReceipt(ctx, tx.Hash()) | ||
| if receiptErr == nil { | ||
| // tx has been mined, update the request with the mined tx and continue | ||
| req.Tx = tx |
There was a problem hiding this comment.
remove the tx that was attempted from the list?
There was a problem hiding this comment.
Not sure if i understand. Why should it be removed?
There was a problem hiding this comment.
because it is one of tx from the list and has succeeded. was just trying to understand
ian-shim
force-pushed
the
check-old-txns
branch
from
65209bf to
67279fe
Compare
disperser/batcher/txn_manager.go
Outdated
| // tx has been mined, update the request with the mined tx and continue | ||
| req.Tx = tx | ||
| continue | ||
| } | ||
| } | ||
| } | ||
| t.logger.Error("[TxnManager] failed to send txn", "tag", req.Tag, "txn", req.Tx.Hash().Hex(), "err", err) | ||
| t.metrics.IncrementTxnCount("failure") | ||
| return nil, err |
There was a problem hiding this comment.
If the newTx is failed to sent, this is erroring out directly. Would any of the existing pending transaction still be able to succeed in such case?
There was a problem hiding this comment.
Any of the existing pending transactions may still succeed even in this case.
This was meant to catch non-transient errors, so that it doesn't get blocked on a transaction with potential issues. Maybe we should have a retry count here and return error only if it fails n times.
Thoughts?
There was a problem hiding this comment.
Yeah that sounds reasonable.
Instead of conditioning on if we can send a new txn, I wonder if it makes sense to:
- periodically (every txnRefreshInterval) send a new txn (unless a txn is mined)
- whether it succeeds or not, in each cycle we examine all the pending txns for a mined txn
- if this is getting N pending txns, or we have looped M txnRefreshInterval cycles (M may be different than N, because sending a new txn may fail like this), then we return error
- if we find a mined txn, then all other txns cannot succeed since they have same nounce, then we just wait for this mined txn to get enough confirmation, and then return success
I may have missed something, but it seems to be more robust and hopefully the logic in this monitor function is a bit clear?
There was a problem hiding this comment.
Yes
whether it succeeds or not, in each cycle we examine all the pending txns for a mined txn
This makes sense
if this is getting N pending txns, or we have looped M txnRefreshInterval cycles (M may be different than N, because sending a new txn may fail like this), then we return error
Note that when we send a new txn here, it's not a new txn but it's replacing the existing txn. Therefore, even if we replace the txn N times, once all validators are in sync, there would be only 1 pending txn. I think treating successful replacement txn vs. failed txn separately still makes sense. If we can successfully increase gas for a pending txn N times (or looped M txnRefreshInterval cycles), that should be allowed to continue being retried. However, if it fails to replace the txn more than N times, I think it's safe to deem it failed. Does it make sense?
ian-shim
force-pushed
the
check-old-txns
branch
from
67279fe to
a81de76
Compare
| // If the context times out but the receipt is available, it returns both receipt and error, noting that the transaction is confirmed but has not accumulated the required number of confirmations. | ||
| // Taken from https://github.com/ethereum/go-ethereum/blob/master/accounts/abi/bind/util.go#L32, | ||
| // but added a check for number of confirmations. | ||
| func (c *EthClient) waitMined(ctx context.Context, tx *types.Transaction) (*types.Receipt, error) { | ||
| func (c *EthClient) waitMined(ctx context.Context, txs []*types.Transaction) (*types.Receipt, error) { |
There was a problem hiding this comment.
It looks there is no constraints on what those txns should be based on impl below?
There was a problem hiding this comment.
Yeah, this method doesn't put any constraints on which txns it's waiting on. It just returns anything that is confirmed first.
Why are these changes needed?
Consider a scenario where transaction A is sent, and it's replaced by transaction B because it hasn't been confirmed within the timeout.
There is a possibility that transaction A gets confirmed instead, even though it's been replaced by B. In this case, transaction B is dropped from the mempool.
TxnManager needs to detect this case (the "replaced" transactions are getting confirmed) and return the receipt for the confirmed transaction.
Checks