feat: nonce (sequence number) based mempool #13645

kocubinski · 2022-10-25T13:36:23Z

Description

This simple mempool implementation keeps transactions sorted by nonce (sequence number). Transactions with the lowest nonce globally are prioritized. Transactions from different senders with the same nonce are prioritized by sender address. Fee/gas based prioritization is not supported.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

types/mempool/nonce.go

tac0turtle · 2022-10-25T14:30:04Z

baseapp/baseapp.go


+	"github.com/cosmos/cosmos-sdk/types/mempool"
+	"github.com/cosmos/gogoproto/proto"
+


Suggested change

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

"github.com/cosmos/gogoproto/proto"

"github.com/cosmos/gogoproto/proto"

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

@kocubinski do you use VSCode? If so, you can add the following to your SDK workspace settings:

"gopls": { "local": "github.com/cosmos/cosmos-sdk" }

I'm sure there is one equivalent for vim or whatever

codecov · 2022-10-25T15:01:15Z

Codecov Report

Merging #13645 (c0ce7d1) into main (ad08ec4) will increase coverage by 0.15%.
The diff coverage is 96.73%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13645      +/-   ##
==========================================
+ Coverage   54.69%   54.85%   +0.15%     
==========================================
  Files         645      655      +10     
  Lines       55745    56239     +494     
==========================================
+ Hits        30491    30851     +360     
- Misses      22764    22866     +102     
- Partials     2490     2522      +32

Impacted Files	Coverage Δ
baseapp/baseapp.go	`75.12% <0.00%> (ø)`
x/auth/tx/builder.go	`81.65% <92.30%> (+1.45%)`	⬆️
types/mempool/nonce.go	`100.00% <100.00%> (ø)`
x/distribution/simulation/operations.go	`80.64% <0.00%> (-9.68%)`	⬇️
x/group/keeper/keeper.go	`56.25% <0.00%> (-0.40%)`	⬇️
tx/textual/valuerenderer/timestamp.go	`89.28% <0.00%> (ø)`
tx/textual/valuerenderer/valuerenderer.go	`72.41% <0.00%> (ø)`
tx/textual/valuerenderer/int.go	`50.00% <0.00%> (ø)`
tx/textual/valuerenderer/string.go	`66.66% <0.00%> (ø)`
tx/textual/valuerenderer/message.go	`64.22% <0.00%> (ø)`
... and 4 more

alexanderbez

Left a few comments, but otherwise LGTM!

alexanderbez · 2022-10-25T14:47:11Z

baseapp/baseapp.go


+	"github.com/cosmos/cosmos-sdk/types/mempool"
+	"github.com/cosmos/gogoproto/proto"
+


@kocubinski do you use VSCode? If so, you can add the following to your SDK workspace settings:

"gopls": { "local": "github.com/cosmos/cosmos-sdk" }

I'm sure there is one equivalent for vim or whatever

alexanderbez · 2022-10-25T14:48:21Z

types/mempool/mempool.go

+
+type Factory func() Mempool
+
+func IsEmpty(mempool Mempool) error {


rather than type cast. I would just define NumTxs() on the mempool interface.

alexanderbez · 2022-10-25T14:48:51Z

x/auth/tx/builder.go

@@ -1,6 +1,7 @@
 package tx

 import (
+	"github.com/cosmos/cosmos-sdk/types/mempool"


nit: formatting

alexanderbez · 2022-10-25T15:02:10Z

types/mempool/nonce.go

+// are prioritized.  Transactions with the same nonce are prioritized by sender address. Fee/gas based
+// prioritization is not supported.
+type nonceMempool struct {
+	txQueue *huandu.SkipList


Just want to point out that using a skip list this for this type of implementation might be overkill. You could just use a sorted []Tx array, or a wrapper around Tx so you don't have to do anything extra to get the sender and nonce, etc..

You could just use a sorted []Tx array

I think this is an efficient way to maintain a sorted list. Using an array we'd still need to sort on insert, what are you proposing for maintaining the sort?

Yeah, its the same semantically. With an array, sort on insert. Both are logarithmic. Not blocking though. Just pointing it out.

alexanderbez · 2022-10-25T15:05:40Z

types/mempool/nonce.go

+		selectedTxs = append(selectedTxs, mempoolTx)
+		if txBytes += mempoolTx.Size(); txBytes >= maxBytes {
+			return selectedTxs, nil
+		}


I think you need to re-write this slightly. You only want to add the current tx s.t. if when added remains under or equal to maxBytes. I.e. txBytes + mempoolTx.Size() <= maxBytes { // append }

alexanderbez

🎉

nice work!

facundomedica

LGTM, added some very very silly nits (sorry)

facundomedica · 2022-10-25T19:20:44Z

types/mempool/mempool_test.go

+func (t testPubKey) Reset() { panic("implement me") }
+
+func (t testPubKey) String() string { panic("implement me") }
+
+func (t testPubKey) ProtoMessage() { panic("implement me") }
+
+func (t testPubKey) Address() cryptotypes.Address { return t.address.Bytes() }
+
+func (t testPubKey) Bytes() []byte { panic("implement me") }
+
+func (t testPubKey) VerifySignature(msg []byte, sig []byte) bool { panic("implement me") }
+
+func (t testPubKey) Equals(key cryptotypes.PubKey) bool { panic("implement me") }
+
+func (t testPubKey) Type() string { panic("implement me") }


nit:

Suggested change

func (t testPubKey) Reset() { panic("implement me") }

func (t testPubKey) String() string { panic("implement me") }

func (t testPubKey) ProtoMessage() { panic("implement me") }

func (t testPubKey) Address() cryptotypes.Address { return t.address.Bytes() }

func (t testPubKey) Bytes() []byte { panic("implement me") }

func (t testPubKey) VerifySignature(msg []byte, sig []byte) bool { panic("implement me") }

func (t testPubKey) Equals(key cryptotypes.PubKey) bool { panic("implement me") }

func (t testPubKey) Type() string { panic("implement me") }

func (t testPubKey) Reset() { panic("not implemented") }

func (t testPubKey) String() string { panic("not implemented") }

func (t testPubKey) ProtoMessage() { panic("not implemented") }

func (t testPubKey) Address() cryptotypes.Address { return t.address.Bytes() }

func (t testPubKey) Bytes() []byte { panic("not implemented") }

func (t testPubKey) VerifySignature(msg []byte, sig []byte) bool { panic("not implemented") }

func (t testPubKey) Equals(key cryptotypes.PubKey) bool { panic("not implemented") }

func (t testPubKey) Type() string { panic("not implemented") }

facundomedica · 2022-10-25T19:23:14Z

types/mempool/mempool_test.go

+func (tx testTx) GetSigners() []sdk.AccAddress { panic("implement me") }
+
+func (tx testTx) GetPubKeys() ([]cryptotypes.PubKey, error) { panic("GetPubKeys not implemented") }


consistency nit:

Suggested change

func (tx testTx) GetSigners() []sdk.AccAddress { panic("implement me") }

func (tx testTx) GetPubKeys() ([]cryptotypes.PubKey, error) { panic("GetPubKeys not implemented") }

func (tx testTx) GetSigners() []sdk.AccAddress { panic("not implemented") }

func (tx testTx) GetPubKeys() ([]cryptotypes.PubKey, error) { panic("not implemented") }

facundomedica · 2022-10-25T19:24:00Z

types/mempool/mempool_test.go

+				require.NoError(t, err)
+			}
+
+			//mempool.DebugPrintKeys(pool)


Suggested change

//mempool.DebugPrintKeys(pool)

facundomedica · 2022-10-25T19:32:04Z

types/mempool/nonce.go

+// nonceMempool is a mempool that keeps transactions sorted by nonce.  Transactions with the lowest nonce globally
+// are prioritized.  Transactions with the same nonce are prioritized by sender address. Fee/gas based
+// prioritization is not supported.


Suggested change

// nonceMempool is a mempool that keeps transactions sorted by nonce. Transactions with the lowest nonce globally

// are prioritized. Transactions with the same nonce are prioritized by sender address. Fee/gas based

// prioritization is not supported.

// nonceMempool is a mempool that keeps transactions sorted by nonce. Transactions with the lowest nonce globally

// are prioritized. Transactions with the same nonce are prioritized by sender address. Fee/gas based

// prioritization is not supported.

facundomedica · 2022-10-25T19:32:17Z

types/mempool/nonce.go

+	return sp
+}
+
+// Insert adds a tx to the mempool.  It returns an error if the tx does not have at least one signer.


Suggested change

// Insert adds a tx to the mempool. It returns an error if the tx does not have at least one signer.

// Insert adds a tx to the mempool. It returns an error if the tx does not have at least one signer.

facundomedica · 2022-10-25T19:32:40Z

types/mempool/nonce.go

+	return nil
+}
+
+// Select returns txs from the mempool with the lowest nonce globally first.  A sender's txs will always be returned


Suggested change

// Select returns txs from the mempool with the lowest nonce globally first. A sender's txs will always be returned

// Select returns txs from the mempool with the lowest nonce globally first. A sender's txs will always be returned

facundomedica · 2022-10-25T19:32:52Z

types/mempool/nonce.go

+	return sp.txQueue.Len()
+}
+
+// Remove removes a tx from the mempool.  It returns an error if the tx does not have at least one signer or the tx


Suggested change

// Remove removes a tx from the mempool. It returns an error if the tx does not have at least one signer or the tx

// Remove removes a tx from the mempool. It returns an error if the tx does not have at least one signer or the tx

simple default mempool implementation Co-authored-by: Jeancarlo <jeancarlobarrios@gmail.com>

kocubinski requested a review from a team as a code owner October 25, 2022 13:36

github-actions bot added the C:x/auth label Oct 25, 2022

kocubinski and others added 13 commits October 25, 2022 08:41

mempool sender priority implementation

cb21a86

Fix tests

90fa7cf

remove priority impl

0de3547

fix IsEmpty

1859fd6

simplify refactor

dc60fef

add select godoc

f735bed

tx size

9016e96

nonce mempool implementation

e4c7f47

one test passing

55b8b79

remove sender priority implementation

505dc5f

Fix tests and add godocs

30ab950

formatting

034a85c

mod tidy

0f4c82e

kocubinski force-pushed the kocubinski/mempool-nonce branch from b5ff8a8 to 0f4c82e Compare October 25, 2022 13:42

tac0turtle reviewed Oct 25, 2022

View reviewed changes

types/mempool/nonce.go Show resolved Hide resolved

tac0turtle reviewed Oct 25, 2022

View reviewed changes

reorder imports

f6df50b

alexanderbez reviewed Oct 25, 2022

View reviewed changes

txBytes counting

14805d6

alexanderbez approved these changes Oct 25, 2022

View reviewed changes

kocubinski added 2 commits October 25, 2022 12:39

Merge branch 'main' into kocubinski/mempool-nonce

1654d11

Merge branch 'main' into kocubinski/mempool-nonce

fe36b38

facundomedica reviewed Oct 25, 2022

View reviewed changes

kocubinski added 4 commits October 26, 2022 07:05

Merge branch 'main' into kocubinski/mempool-nonce

2db6e6b

minor reformatting

d1e37ed

import formatting

0ae7966

fix import order

c0ce7d1

kocubinski merged commit 2418c3e into main Oct 26, 2022

kocubinski deleted the kocubinski/mempool-nonce branch October 26, 2022 14:04

kocubinski mentioned this pull request Oct 26, 2022

chore: cleanup mempool tests #13658

Merged

19 tasks

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

feat: nonce (sequence number) based mempool (cosmos#13645)

603c9d7

simple default mempool implementation Co-authored-by: Jeancarlo <jeancarlobarrios@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: nonce (sequence number) based mempool #13645

feat: nonce (sequence number) based mempool #13645

kocubinski commented Oct 25, 2022

tac0turtle Oct 25, 2022

alexanderbez Oct 25, 2022

codecov bot commented Oct 25, 2022 •

edited

Loading

alexanderbez left a comment

alexanderbez Oct 25, 2022

alexanderbez Oct 25, 2022

alexanderbez Oct 25, 2022

alexanderbez Oct 25, 2022

kocubinski Oct 25, 2022 •

edited

Loading

alexanderbez Oct 25, 2022

alexanderbez Oct 25, 2022

alexanderbez left a comment

facundomedica left a comment

facundomedica Oct 25, 2022

facundomedica Oct 25, 2022

facundomedica Oct 25, 2022

facundomedica Oct 25, 2022

facundomedica Oct 25, 2022

facundomedica Oct 25, 2022

facundomedica Oct 25, 2022


		"github.com/cosmos/cosmos-sdk/types/mempool"
		"github.com/cosmos/gogoproto/proto"


		type Factory func() Mempool

		func IsEmpty(mempool Mempool) error {

		func (tx testTx) GetSigners() []sdk.AccAddress { panic("implement me") }

		func (tx testTx) GetPubKeys() ([]cryptotypes.PubKey, error) { panic("GetPubKeys not implemented") }

	// Insert adds a tx to the mempool. It returns an error if the tx does not have at least one signer.
	// Insert adds a tx to the mempool. It returns an error if the tx does not have at least one signer.

	// Select returns txs from the mempool with the lowest nonce globally first. A sender's txs will always be returned
	// Select returns txs from the mempool with the lowest nonce globally first. A sender's txs will always be returned

	// Remove removes a tx from the mempool. It returns an error if the tx does not have at least one signer or the tx
	// Remove removes a tx from the mempool. It returns an error if the tx does not have at least one signer or the tx

feat: nonce (sequence number) based mempool #13645

feat: nonce (sequence number) based mempool #13645

Conversation

kocubinski commented Oct 25, 2022

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2022 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kocubinski Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

facundomedica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2022 •

edited

Loading

kocubinski Oct 25, 2022 •

edited

Loading