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

Jump to bottom

fix: align signer extraction adapter for mempool remove #19759

Merged
merged 5 commits into from
Mar 18, 2024
Merged

fix: align signer extraction adapter for mempool remove #19759

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ab1cc53
align signer extraction adapter for mempool remove
mmsqe Mar 15, 2024
a88eb00
add test
mmsqe Mar 15, 2024
7762759
update doc
mmsqe Mar 15, 2024
d6abbbe
Apply suggestions from code review
mmsqe Mar 15, 2024
184a013
Merge remote-tracking branch 'origin/main' into fix_signer
mmsqe Mar 15, 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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (types) [#18768](https://github.com/cosmos/cosmos-sdk/pull/18768) Add MustValAddressFromBech32 function.
* (gRPC) [#19049](https://github.com/cosmos/cosmos-sdk/pull/19049) Add debug log prints for each gRPC request.
* (x/consensus) [#19483](https://github.com/cosmos/cosmos-sdk/pull/19483) Add consensus messages registration to consensus module.
* (types) [#19759](https://github.com/cosmos/cosmos-sdk/pull/19759) Align SignerExtractionAdapter in PriorityNonceMempool Remove.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please specify the version or release section for this changelog entry to ensure it's correctly categorized. This helps in maintaining the clarity and organization of the changelog, making it easier for users to understand the changes in each release.


### Improvements

Expand Down
6 changes: 2 additions & 4 deletions types/mempool/priority_nonce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ import (

"github.com/huandu/skiplist"

"cosmossdk.io/x/auth/signing"

sdk "github.com/cosmos/cosmos-sdk/types"
)

Expand Down Expand Up @@ -433,7 +431,7 @@ func (mp *PriorityNonceMempool[C]) CountTx() int {
func (mp *PriorityNonceMempool[C]) Remove(tx sdk.Tx) error {
mp.mtx.Lock()
defer mp.mtx.Unlock()
sigs, err := tx.(signing.SigVerifiableTx).GetSignaturesV2()
sigs, err := mp.cfg.SignerExtractor.GetSigners(tx)
if err != nil {
return err
}
Expand All @@ -442,7 +440,7 @@ func (mp *PriorityNonceMempool[C]) Remove(tx sdk.Tx) error {
}

sig := sigs[0]
sender := sdk.AccAddress(sig.PubKey.Address()).String()
sender := sig.Signer.String()
nonce := sig.Sequence

scoreKey := txMeta[C]{nonce: nonce, sender: sender}
Expand Down
82 changes: 82 additions & 0 deletions types/mempool/priority_nonce_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,88 @@ func TestOutOfOrder(t *testing.T) {
require.Error(t, validateOrder(rmtxs))
}

type signerExtractionAdapter struct {
UseOld bool
}

func (a signerExtractionAdapter) GetSigners(tx sdk.Tx) ([]mempool.SignerData, error) {
if a.UseOld {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we revert the if, to make code more readable

if !a.UseOld {
    return mempool.NewDefaultSignerExtractionAdapter().GetSigners(tx)
}

... more boilerplate code

sigs, err := tx.(signing.SigVerifiableTx).GetSignaturesV2()
if err != nil {
return nil, err
}
signerData := make([]mempool.SignerData, len(sigs))
for _, sig := range sigs {
signerData = append(signerData, mempool.SignerData{
Signer: sig.PubKey.Address().Bytes(),
Sequence: sig.Sequence,
})
}
return signerData, nil
}
signerData, err := mempool.NewDefaultSignerExtractionAdapter().GetSigners(tx)
return signerData, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of signerExtractionAdapter and its method GetSigners introduces a flexible way to switch between signer extraction methods based on the UseOld flag. However, there are a few points to consider:

  • The use of signing.SigVerifiableTx directly in the GetSigners method assumes that all transactions passed to this method will implement this interface. While this is likely the case, it's a good practice to check the type assertion's success to avoid potential runtime panics.
  • The method appends to signerData without initializing it with a specific length, which can be less efficient than allocating the exact required size upfront.
  • The fallback to mempool.NewDefaultSignerExtractionAdapter().GetSigners(tx) when UseOld is false is a good approach, but it creates a new instance of DefaultSignerExtractionAdapter every time. Consider if there's a more efficient way to handle this, perhaps by reusing an instance.

Consider checking the type assertion's success and initializing slices with the exact required size for efficiency. Also, evaluate the possibility of reusing the DefaultSignerExtractionAdapter instance to avoid unnecessary allocations.


func (s *MempoolTestSuite) TestPriorityNonceTxOrderWithAdapter() {
t := s.T()
ctx := sdk.NewContext(nil, false, log.NewNopLogger())
accounts := simtypes.RandomAccounts(rand.New(rand.NewSource(0)), 5)
sa := accounts[0].Address
sb := accounts[1].Address

tests := []struct {
txs []txSpec
order []int
fail bool
}{
{
txs: []txSpec{
{p: 21, n: 4, a: sa},
{p: 8, n: 3, a: sa},
{p: 6, n: 2, a: sa},
{p: 15, n: 1, a: sb},
{p: 20, n: 1, a: sa},
},
order: []int{4, 3, 2, 1, 0},
},
}
for i, tt := range tests {
t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) {
adapter := signerExtractionAdapter{}
pool := mempool.NewPriorityMempool(mempool.PriorityNonceMempoolConfig[int64]{
TxPriority: mempool.NewDefaultTxPriority(),
SignerExtractor: adapter,
})

// create test txs and insert into mempool
for i, ts := range tt.txs {
tx := testTx{id: i, priority: int64(ts.p), nonce: uint64(ts.n), address: ts.a}
c := ctx.WithPriority(tx.priority)
err := pool.Insert(c, tx)
require.NoError(t, err)
}

orderedTxs := fetchTxs(pool.Select(ctx, nil), 1000)

var txOrder []int
for _, tx := range orderedTxs {
txOrder = append(txOrder, tx.(testTx).id)
}

require.Equal(t, tt.order, txOrder)
require.NoError(t, validateOrder(orderedTxs))

adapter.UseOld = true
for _, tx := range orderedTxs {
require.NoError(t, pool.Remove(tx))
}

require.NoError(t, mempool.IsEmpty[int64](pool))
})
}
}

func (s *MempoolTestSuite) TestPriorityNonceTxOrder() {
t := s.T()
ctx := sdk.NewContext(nil, false, log.NewNopLogger())
Expand Down
Loading