Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SubmitEthereumTxConfirmation bug #558

Merged
merged 3 commits into from
Feb 27, 2024
Merged

Fix SubmitEthereumTxConfirmation bug #558

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
454fe3c
Revert "Remove CompletedOutgoingTxs from unsigned batch and contract …
cbrit Feb 20, 2024
1f3506f
Revert "Patch broken unit tests"
cbrit Feb 20, 2024
a9e2276
Check completed otxs in SubmitEthereumTxConfirmation handler
cbrit Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions module/x/gravity/keeper/batch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,17 @@ func (k Keeper) getLastOutgoingBatchByTokenType(ctx sdk.Context, token common.Ad
// GetUnsignedBatchTxs returns all batches for which the specified validator has not submitted confirmations in ascending nonce order
func (k Keeper) GetUnsignedBatchTxs(ctx sdk.Context, val sdk.ValAddress) []*types.BatchTx {
var unconfirmed []*types.BatchTx
k.IterateCompletedOutgoingTxsByType(ctx, types.BatchTxPrefixByte, func(_ []byte, cotx types.OutgoingTx) bool {
sig := k.getEthereumSignature(ctx, cotx.GetStoreIndex(), val)
if len(sig) == 0 {
batch, ok := cotx.(*types.BatchTx)
if !ok {
panic(sdkerrors.Wrapf(types.ErrInvalid, "couldn't cast to batch tx for completed tx %s", cotx))
}
unconfirmed = append(unconfirmed, batch)
}
return false
})
Comment on lines +166 to +176
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the changes in contract_call.go, the addition of iterating over completed outgoing transactions in GetUnsignedBatchTxs is essential for the fix. However, the use of panic for error handling (line 171) should be reconsidered for the same reasons mentioned earlier: it's generally discouraged unless for truly unrecoverable errors. Consider returning an error instead.

Also, be mindful of the performance implications of iterating over a potentially large number of transactions. If performance issues arise, exploring optimizations for the data structure or query mechanism might be necessary.

k.IterateOutgoingTxsByType(ctx, types.BatchTxPrefixByte, func(_ []byte, otx types.OutgoingTx) bool {
sig := k.getEthereumSignature(ctx, otx.GetStoreIndex(), val)
if len(sig) == 0 {
Expand Down
9 changes: 3 additions & 6 deletions module/x/gravity/keeper/batch_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,7 @@ func TestGetUnconfirmedBatchTxs(t *testing.T) {

blockheight := uint64(ctx.BlockHeight())
sig := []byte("dummysig")
//gk.SetCompletedOutgoingTx(ctx, &types.BatchTx{
gk.SetOutgoingTx(ctx, &types.BatchTx{
gk.SetCompletedOutgoingTx(ctx, &types.BatchTx{
BatchNonce: 1,
Height: uint64(ctx.BlockHeight()),
})
Expand Down Expand Up @@ -438,14 +437,12 @@ func TestGetUnconfirmedBatchTxs(t *testing.T) {

addressA := "0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
addressB := "0xBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"
//gk.SetCompletedOutgoingTx(ctx, &types.BatchTx{
gk.SetOutgoingTx(ctx, &types.BatchTx{
gk.SetCompletedOutgoingTx(ctx, &types.BatchTx{
TokenContract: addressB,
BatchNonce: 3,
Height: blockheight,
})
//gk.SetCompletedOutgoingTx(ctx, &types.BatchTx{
gk.SetOutgoingTx(ctx, &types.BatchTx{
gk.SetCompletedOutgoingTx(ctx, &types.BatchTx{
TokenContract: addressA,
BatchNonce: 4,
Height: blockheight,
Expand Down
12 changes: 12 additions & 0 deletions module/x/gravity/keeper/contract_call.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,18 @@ import (

func (k Keeper) GetUnsignedContractCallTxs(ctx sdk.Context, val sdk.ValAddress) []*types.ContractCallTx {
var unconfirmed []*types.ContractCallTx
k.IterateCompletedOutgoingTxsByType(ctx, types.ContractCallTxPrefixByte, func(_ []byte, cotx types.OutgoingTx) bool {
sig := k.getEthereumSignature(ctx, cotx.GetStoreIndex(), val)
if len(sig) == 0 {
call, ok := cotx.(*types.ContractCallTx)
if !ok {
panic(sdkerrors.Wrapf(types.ErrInvalid, "couldn't cast to contract call for completed tx %s", cotx))
}
unconfirmed = append(unconfirmed, call)
}
return false
})
Comment on lines +15 to +25
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addition of iterating over completed outgoing transactions to include them in the list of unconfirmed contract call transactions is a crucial fix for the issue described. However, using panic for error handling (line 20) is generally discouraged in Go unless it's a truly unrecoverable error. Consider returning an error to the caller instead, which allows for more graceful error handling and logging.

Regarding performance, ensure that the iteration over completed transactions does not introduce significant overhead, especially as the number of transactions grows. If performance becomes a concern, consider optimizing the data structure used for storing and querying these transactions.


k.IterateOutgoingTxsByType(ctx, types.ContractCallTxPrefixByte, func(_ []byte, otx types.OutgoingTx) bool {
sig := k.getEthereumSignature(ctx, otx.GetStoreIndex(), val)
if len(sig) == 0 {
Expand Down
3 changes: 1 addition & 2 deletions module/x/gravity/keeper/contract_call_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,7 @@ func TestGetUnconfirmedContractCallTxs(t *testing.T) {
fees := []types.ERC20Token{}
sig := []byte("dummysig")
gk.CreateContractCallTx(ctx, 1, scope, address, payload, tokens, fees)
//gk.SetCompletedOutgoingTx(ctx, &types.ContractCallTx{
gk.SetOutgoingTx(ctx, &types.ContractCallTx{
gk.SetCompletedOutgoingTx(ctx, &types.ContractCallTx{
InvalidationNonce: 2,
InvalidationScope: scope,
Address: address.Hex(),
Expand Down
8 changes: 2 additions & 6 deletions module/x/gravity/keeper/grpc_query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,7 @@ func TestKeeper_UnsignedBatchTxs(t *testing.T) {
res, err := gk.UnsignedBatchTxs(sdk.WrapSDKContext(ctx), req)
require.NoError(t, err)
require.NotNil(t, res)
//require.Len(t, res.Batches, 3)
// Test broken by completed tx workaround to SubmitEthereumTxComfiration bug
require.Len(t, res.Batches, 1)
require.Len(t, res.Batches, 3)
}
})
}
Expand Down Expand Up @@ -346,9 +344,7 @@ func TestKeeper_UnsignedContractCallTxs(t *testing.T) {
res, err := gk.UnsignedContractCallTxs(sdk.WrapSDKContext(ctx), req)
require.NoError(t, err)
require.NotNil(t, res)
//require.Len(t, res.Calls, 3)
// Test broken by completed tx workaround to SubmitEthereumTxComfiration bug
require.Len(t, res.Calls, 2)
require.Len(t, res.Calls, 3)
}
})
}
Expand Down
17 changes: 10 additions & 7 deletions module/x/gravity/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,16 @@ func (k msgServer) SubmitEthereumTxConfirmation(c context.Context, msg *types.Ms
return nil, err
}

otx := k.GetOutgoingTx(ctx, confirmation.GetStoreIndex())
if otx == nil {
k.Logger(ctx).Error(
"no outgoing tx",
"store index", fmt.Sprintf("%x", confirmation.GetStoreIndex()),
)
return nil, sdkerrors.Wrap(types.ErrInvalid, "couldn't find outgoing tx")
var otx types.OutgoingTx
if otx = k.GetOutgoingTx(ctx, confirmation.GetStoreIndex()); otx == nil {
if otx = k.GetCompletedOutgoingTx(ctx, confirmation.GetStoreIndex()); otx == nil {
k.Logger(ctx).Error(
"no outgoing tx",
"store index", fmt.Sprintf("%x", confirmation.GetStoreIndex()),
)

return nil, sdkerrors.Wrap(types.ErrInvalid, "couldn't find outgoing tx")
}
}

gravityID := k.getGravityID(ctx)
Expand Down
Loading