feat: add default block based ttl and resign expired txs #5302
Conversation
ninabarbakadze
added
the
warn:blocked
| if apperrors.IsNonceMismatchCode(resp.ExecutionCode) { | ||
| expired, err := client.waitForTransactionToExpire(ctx, txHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to wait for transaction to expire for resubmission: %w", err) | ||
| } | ||
| if expired { | ||
| fmt.Println("tx with hash", txHash, "expired") | ||
| // resign and resubmit | ||
| txInfo, exists := client.txTracker[txHash] | ||
| if !exists { | ||
| return nil, fmt.Errorf("transaction %s not found in tracker", txHash) | ||
| } | ||
|
|
||
| // check if it's a blob transaction or just a normal transaction | ||
| // decode the original transaction to extract messages | ||
| decodedTx, err := client.signer.DecodeTx(txInfo.txBytes) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode transaction: %w", err) | ||
| } | ||
|
|
||
| // extract messages from the decoded transaction | ||
| msgs := decodedTx.GetMsgs() | ||
|
|
||
| // Check if this is a PayForBlob transaction | ||
| if len(msgs) > 0 { | ||
| // Check if the first message is a PayForBlob message | ||
| if _, ok := msgs[0].(*types.MsgPayForBlobs); ok { | ||
| // This is a blob transaction, we need to extract the blobs | ||
| // and recreate the PayForBlob transaction | ||
| blobTx, isBlob, err := blobtx.UnmarshalBlobTx(txInfo.txBytes) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to unmarshal blob transaction: %w", err) | ||
| } | ||
| if !isBlob { | ||
| return nil, fmt.Errorf("expected blob transaction but got normal transaction") | ||
| } | ||
|
|
||
| // Resubmit as a PayForBlob transaction | ||
| resp, err := client.BroadcastPayForBlob(ctx, blobTx.Blobs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to resubmit blob tx after expiry: %w", err) | ||
| } | ||
| fmt.Println("resubmitted blob tx", txHash, "new hash", resp.TxHash) | ||
|
|
||
| // confirm the resubmitted transaction using the NEW hash | ||
| confirmResp, err := client.ConfirmTx(ctx, resp.TxHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to confirm resubmitted blob tx: %w", err) | ||
| } | ||
|
|
||
| return confirmResp, nil | ||
|
|
||
| } | ||
| // This is a normal transaction, resubmit with messages | ||
| resp, err := client.BroadcastTxWithoutMutex(ctx, msgs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to resubmit tx after expiry: %w", err) | ||
| } | ||
| fmt.Println("resubmitted tx", txHash, "new hash", resp.TxHash) | ||
|
|
||
| // confirm the resubmitted transaction using the NEW hash | ||
| confirmResp, err := client.ConfirmTx(ctx, resp.TxHash) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to confirm resubmitted tx: %w", err) | ||
| } | ||
|
|
||
| return confirmResp, nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
could we possible clean up this logic using the typical techniques?
I could be wrong, but looks like we avoid 4 layers nested if statements by exiting early with negative if statements and deduping logic
pkg/user/tx_client_test.go
Outdated
| func TestRejections(t *testing.T) { | ||
| ttlNumBlocks := int64(5) | ||
| _, txClient, ctx := setupTxClient(t, ttlNumBlocks, appconsts.DefaultMaxBytes) | ||
| // t.Run("should reset the signer nonce after tx is rejected with no subsequent txs to resubmit", func(t *testing.T) { |
There was a problem hiding this comment.
why comment this out? can we delete if not needed?
No worries! It should be close to done now. I will need to look over it but feedback is welcome at this point. Important to note: We can only merge this after we've released the recent cache changes we made on v0.38.x in core and bumped app |
ninabarbakadze
removed
warn:blocked
| latestHeight, err := client.getLatestBlockHeight(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| timeOutHeight := uint64(latestHeight) + client.ttlNumBlocks | ||
|
|
||
| txTimeoutHeight, err := GetTimeoutHeightFromOptions(client, opts...) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Do not override user set timeout height for the transaction | ||
| if txTimeoutHeight > 0 { | ||
| timeOutHeight = txTimeoutHeight | ||
| } | ||
|
|
||
| opts = append([]TxOption{SetGasLimit(gasLimit), SetFee(fee), SetTimeoutHeight(timeOutHeight)}, opts...) |
There was a problem hiding this comment.
| latestHeight, err := client.getLatestBlockHeight(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| timeOutHeight := uint64(latestHeight) + client.ttlNumBlocks | |
| txTimeoutHeight, err := GetTimeoutHeightFromOptions(client, opts...) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Do not override user set timeout height for the transaction | |
| if txTimeoutHeight > 0 { | |
| timeOutHeight = txTimeoutHeight | |
| } | |
| opts = append([]TxOption{SetGasLimit(gasLimit), SetFee(fee), SetTimeoutHeight(timeOutHeight)}, opts...) | |
| latestHeight, err := client.getLatestBlockHeight(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| timeOutHeight := uint64(latestHeight) + client.ttlNumBlocks | |
| opts = append([]TxOption{SetGasLimit(gasLimit), SetFee(fee), SetTimeoutHeight(timeOutHeight)}, opts...) |
Since you're prepending it, if a user has it set, it will override it
pkg/user/tx_client.go
Outdated
| if err := client.signer.SetSequence(signer, networkSequence); err != nil { | ||
| return nil, fmt.Errorf("setting sequence: %w", err) | ||
| } |
There was a problem hiding this comment.
This won't work, if a transaction is rejected and there are several subsequent transactions, each of those subsequent ones will all get set with the same network sequence and thus only the first thereafter will be accepted, the rest will be immediately rejected in CheckTx
There was a problem hiding this comment.
🥲🥲🥲🥲🥲🥲🥲🥲🥲🥲
I see it now the reason it worked in my tests was that i was confirming them 1 by 1
I'll work on better design
|
Putting my thoughts in this doc until I come up with a solution that can anticipate nonces and resign transactions in a way that won't get them rejected |
Currently prototyping this design |
|
Designing a resubmission engine that consistently passes all pressure testing scenarios has proven non trivial. The current implementation is still hitting nonce errors. It’s difficult to pinpoint exactly which edge cases are being hit in these failures so further investigation is needed to identify and account for all scenarios before we can fully resolve them. Another potential avenue to explore would be having a sort of a local mempool in the tx client that so that when we get nonce failures we will resubmit transactions that were rejected with that nonce and currently are waiting in local pool to be resubmitted. |
3 participants
Overview
Fixes #5133