Isolate transaction broadcasting latency from regas logic #465
Conversation
ian-shim
force-pushed
the
broadcasting-timeout
branch
from
d240d87 to
e4fed18
Compare
ian-shim
changed the title
ian-shim
force-pushed
the
broadcasting-timeout
branch
from
e4fed18 to
d5cf05c
Compare
| for { | ||
| for _, tx := range txs { | ||
| _, err := t.wallet.GetTransactionReceipt(ctx, tx.TxID) | ||
| if err == nil || errors.Is(err, walletsdk.ErrReceiptNotYetAvailable) { |
There was a problem hiding this comment.
Checking error types seems like it's a bit brittle. I guess there's no other option here?
There was a problem hiding this comment.
We can change the implementation in eigensdk, but I think this signature should be kept.
One thing we can do is make the error type more expressive and return Fireblocks status as part of the error, but not sure if that will be less brittle
There was a problem hiding this comment.
Maybe add a comment: "Before the transaction is broadcast, this wallet method will return error of type walletsdk.Whatever. Once the transaction has been broadcast, it will either successfully return the receipt or return an error of type walletsdk.ErrReceiptNotYetAvailable."
I think we should just set a long timeout for this. If the mpc operation latency is extended, it's probably not transient and the retry will likely encounter similar issue. The current behavior is that it will "cancel" the transaction and fail. |
ian-shim
force-pushed
the
broadcasting-timeout
branch
2 times, most recently
from
9e834ae to
5e41c1c
Compare
| // Ensure transactions are broadcasted to the network before querying the receipt. | ||
| // This is to avoid querying the receipt of a transaction that hasn't been broadcasted yet. | ||
| // For example, when Fireblocks wallet is used, there may be delays in broadcasting the transaction due to latency from cosigning and MPC operations. | ||
| err = t.ensureAnyTransactionBroadcasted(ctxWithTimeout, req.txAttempts) |
There was a problem hiding this comment.
We don't retry here because there wouldn't be any advantage over just waiting longer?
There was a problem hiding this comment.
Yeah as explained in this comment
ian-shim
force-pushed
the
broadcasting-timeout
branch
from
5e41c1c to
f036cfe
Compare
ian-shim
force-pushed
the
broadcasting-timeout
branch
from
f036cfe to
ab140d0
Compare
Why are these changes needed?
Currently, we send a replacement transaction if an existing transaction doesn't get confirmed within predetermined timeout. This duration is measured from the time
TxnManagersends the transaction. If the transaction is published directly on eth network, this would have been a good measure to detect if the transaction is stuck on mempool and if a replacement transaction with higher gas is necessary.However, when a transaction is submitted via Fireblocks, this is not a good measure to determine if the transaction is stuck on mempool because it takes non-trivial amount of time for Fireblocks to publish the txn to mempool due to multiparty computation.
This PR breaks down this part of latency (pre-broadcast), and excludes this duration from the timeout watched to create a replacement transaction. For example, if it takes a long time to broadcast the transaction to mempool (due to MPC operation from Fireblocks), no replacement transactions should be created. Only if it takes a long time to confirm transaction onchain after it's been published in mempool, it creates a replacement transaction with higher gas.
Checks