Add TXMv2 fixes - #540
Conversation
|
11488b8 to
adafa6a
Compare
| broadcastTimer.Reset(broadcastWithBackoff.Duration()) | ||
| } else { | ||
| broadcastWithBackoff.Reset() | ||
| broadcastCh = time.After(timeutil.JitterPct(0.1).Apply(broadcastInterval)) | ||
| broadcastTimer.Reset(timeutil.JitterPct(0.1).Apply(broadcastInterval)) |
There was a problem hiding this comment.
While you're at it, maybe add a comment about the reason for doing this? Kinda hard to understand why
There was a problem hiding this comment.
This simply re-uses a single timer instead of creating a new one each time.
There was a problem hiding this comment.
That makes sense, but from reading it it's hard to see the reason for the timer and why we reset it with a duration in one place while resetting it with the broadcast interval in the other place
There was a problem hiding this comment.
Since we got at it, I moved around a few things to make the loop method cleaner, including the backoff mechanism and why it's there.
| if tx.LastBroadcastAt == nil || | ||
| time.Since(*tx.LastBroadcastAt) > (time.Duration(t.config.RetryBlockThreshold)*t.config.BlockTime) || | ||
| tx.IsPurgeable { | ||
| t.lggr.Info("Rebroadcasting attempt for txID: ", tx.ID) |
There was a problem hiding this comment.
Would be great if we could add our transactionLifecycleID tracing here, but I guess that's not easily done since that's specific to the dualContractTransmitter, right?
There was a problem hiding this comment.
Yes, but I can still add it anyway.
| // RPC calls. The upper limit is always MaxInFlightTransactions regardless of the pending nonce. | ||
| if unconfirmedCount >= MaxInFlightSubset { | ||
| if unconfirmedCount > MaxInFlightTransactions { | ||
| if unconfirmedCount >= MaxInFlightTransactions { |
There was a problem hiding this comment.
I would've expected there to be a test to be updated for this change, don't we have unit tests for this functionality?
There was a problem hiding this comment.
The test was checking for MaxInFlightTransactions + 1 although the limit was at MaxInFlightTransactions, hence why it still passes. Now it checks exactly at the limit.
| for { | ||
| start := time.Now() | ||
| bo, err := t.BroadcastTransaction(ctx, address) | ||
| shouldBackoff, err := t.BroadcastTransaction(ctx, address) // use a backoff if transmission is being throttled. |
This PR:
Best if it gets merged after the StuckTxDetection fixes: