refactor(mempool): Add `Iterator` to replace `TxsFront` and `TxsWaitChan` methods by hvanz · Pull Request #3459 · cometbft/cometbft

hvanz · 2024-07-08T16:14:25Z

This PR adds:

the Entry interface, implemented by the existing mempoolTx data type,
the Iterator interface, with one method WaitNextCh() <-chan Entry.

On the routine that sends transactions to a peer, we can now use an Iterator to select the next mempool entry to send, instead of the methods TxsFront and TxsWaitChan in CListMempool. These two methods leak implementation details about the CList and are now marked as deprecated. Now all the logic needed to wait for and pick the next available entry is implemented in CListMempool.WaitNextCh.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments
Title follows the Conventional Commits spec

.changelog/unreleased/deprecations/3303-mempool-deprecate-txsfront-txswaitchan.md

mempool/clist_mempool.go

mempool/mempool.go

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

mempool/clist_mempool.go

melekes

👍 Not sure about channel closing and retrying semantics, otherwise LGTM

melekes · 2024-07-09T09:57:44Z

mempool/reactor.go

+		select {
+		case entry = <-iter.WaitNextCh():
+			// If the entry we were looking at got garbage collected (removed), try again.
+			if entry == nil {


If we reach the end of the list, do we also retry here?

In principle, yes, that's what would happen.

Here, entry is nil either because Clist.Front or CElement.Next returned nil. If Next returns nil because it's at the end of the list and the list is not empty, we shouldn't set the cursor to nil because we will start iterating from the beginning of the list, sending the same transactions more than once. The other reason I see that both Front and Next return nil is when the entry got removed just before calling these functions. I've seen these scenarios happen in unit tests, but they are difficult to reproduce.

I think it's better to investigate these scenarios in another issue; this PR should be just a refactoring.

melekes · 2024-07-09T09:59:16Z

mempool/clist_mempool.go

+		if iter.cursor != nil {
+			ch <- iter.cursor.Value.(Entry)
+		}
+		close(ch)


Remember that closing a channel also sends a nil value

case entry = <-iter.WaitNextCh():

will fire with nil and we will execute continue

The receiver will get nil when closing the channel without sending any message on it. This happens when iter.cursor == nil. When iter.cursor != nil, it first send the message on the channel and then it closes it, but the receiver (line 227) already got the message. (I checked this behaviour on the code.)

To make it more clear I can change it to:

if iter.cursor != nil { ch <- iter.cursor.Value.(Entry) } else { close(ch) }

In the first case the garbage collector will take care of closing the channel.

mempool/clist_mempool.go

mempool/mempool.go

sergio-mena

🎸🎸

… fails (#3647) Fixes a bug introduced in #3459 where instead of retrying the same tx when the peer is lagging, it was skipping it and moving on to the next entry. The same used to happen when sending the transaction failed: instead of retrying to send, it skipped it and proceeded to the next tx. This PR also reverts #3430, which was not working as expected. Note that the first commit introduces a new test `TestMempoolReactorSendLaggingPeer` that initially fails and then it's fixed: https://github.com/cometbft/cometbft/actions/runs/10300181146/job/28509056466?pr=3647 --- #### PR checklist - [X] Tests written/updated - ~~[ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)~~ - ~~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~~ - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

… fails (#3647) Fixes a bug introduced in #3459 where instead of retrying the same tx when the peer is lagging, it was skipping it and moving on to the next entry. The same used to happen when sending the transaction failed: instead of retrying to send, it skipped it and proceeded to the next tx. This PR also reverts #3430, which was not working as expected. Note that the first commit introduces a new test `TestMempoolReactorSendLaggingPeer` that initially fails and then it's fixed: https://github.com/cometbft/cometbft/actions/runs/10300181146/job/28509056466?pr=3647 --- #### PR checklist - [X] Tests written/updated - ~~[ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)~~ - ~~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~~ - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 1ac8832) # Conflicts: # mempool/reactor.go

… fails (backport #3647) (#3657) Fixes a bug introduced in #3459 where instead of retrying the same tx when the peer is lagging, it was skipping it and moving on to the next entry. The same used to happen when sending the transaction failed: instead of retrying to send, it skipped it and proceeded to the next tx. This PR also reverts #3430, which was not working as expected. Note that the first commit introduces a new test `TestMempoolReactorSendLaggingPeer` that initially fails and then it's fixed: https://github.com/cometbft/cometbft/actions/runs/10300181146/job/28509056466?pr=3647 --- #### PR checklist - [X] Tests written/updated - ~~[ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)~~ - ~~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~~ - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3647 done by [Mergify](https://mergify.com). --------- Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> Co-authored-by: hvanz <hernan.vanzetto@gmail.com>

hvanz · 2024-09-12T10:28:26Z

@Mergifyio backport v1.x

mergify · 2024-09-12T10:28:35Z

backport v1.x

✅ Backports have been created

Details

#4089 refactor(mempool): Add Iterator to replace TxsFront and TxsWaitChan methods (backport #3459) has been created for branch v1.x but encountered conflicts

…han` methods (#3459) Closes #3303 This PR adds: - the `Entry` interface, implemented by the existing `mempoolTx` data type, - the `Iterator` interface, with one method `WaitNextCh() <-chan Entry`. On the routine that sends transactions to a peer, we can now use an `Iterator` to select the next mempool entry to send, instead of the methods `TxsFront` and `TxsWaitChan` in `CListMempool`. These two methods leak implementation details about the CList and are now marked as deprecated. Now all the logic needed to wait for and pick the next available entry is implemented in `CListMempool.WaitNextCh`. --- #### PR checklist - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> (cherry picked from commit d4a82b7) # Conflicts: # mempool/clist_mempool_test.go # mempool/reactor.go

… fails (#3647) Fixes a bug introduced in #3459 where instead of retrying the same tx when the peer is lagging, it was skipping it and moving on to the next entry. The same used to happen when sending the transaction failed: instead of retrying to send, it skipped it and proceeded to the next tx. This PR also reverts #3430, which was not working as expected. Note that the first commit introduces a new test `TestMempoolReactorSendLaggingPeer` that initially fails and then it's fixed: https://github.com/cometbft/cometbft/actions/runs/10300181146/job/28509056466?pr=3647 --- #### PR checklist - [X] Tests written/updated - ~~[ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)~~ - ~~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~~ - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit 1ac8832) # Conflicts: # mempool/reactor_test.go

… fails (backport #3647) (#4109) Fixes a bug introduced in #3459 where instead of retrying the same tx when the peer is lagging, it was skipping it and moving on to the next entry. The same used to happen when sending the transaction failed: instead of retrying to send, it skipped it and proceeded to the next tx. This PR also reverts #3430, which was not working as expected. Note that the first commit introduces a new test `TestMempoolReactorSendLaggingPeer` that initially fails and then it's fixed: https://github.com/cometbft/cometbft/actions/runs/10300181146/job/28509056466?pr=3647 --- #### PR checklist - [X] Tests written/updated - ~~[ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog)~~ - ~~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~~ - [X] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3647 done by [Mergify](https://mergify.com). --------- Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> Co-authored-by: hvanz <hernan.vanzetto@gmail.com>

…han` methods (backport #3459) (#4089) Closes #3303 This PR adds: - the `Entry` interface, implemented by the existing `mempoolTx` data type, - the `Iterator` interface, with one method `WaitNextCh() <-chan Entry`. On the routine that sends transactions to a peer, we can now use an `Iterator` to select the next mempool entry to send, instead of the methods `TxsFront` and `TxsWaitChan` in `CListMempool`. These two methods leak implementation details about the CList and are now marked as deprecated. Now all the logic needed to wait for and pick the next available entry is implemented in `CListMempool.WaitNextCh`. --- #### PR checklist - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3459 done by [Mergify](https://mergify.com). --------- Co-authored-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> Co-authored-by: hvanz <hernan.vanzetto@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

hvanz added 2 commits July 8, 2024 18:00

add mempool Iterator

1dcdd03

mark TxsFront and TxsWaitChan as deprecated

719e624

hvanz added the mempool label Jul 8, 2024

hvanz self-assigned this Jul 8, 2024

hvanz added 2 commits July 8, 2024 18:27

add changelog files

de48133

update comment

782f51b

hvanz marked this pull request as ready for review July 8, 2024 16:37

hvanz requested a review from a team as a code owner July 8, 2024 16:37

hvanz requested a review from a team July 8, 2024 16:37

melekes reviewed Jul 9, 2024

View reviewed changes

hvanz and others added 2 commits July 9, 2024 09:24

Apply suggestions from code review

e8af3fd

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

update comments

c02a493

sergio-mena reviewed Jul 9, 2024

View reviewed changes

mempool/clist_mempool.go Outdated Show resolved Hide resolved

melekes approved these changes Jul 9, 2024

View reviewed changes

sergio-mena reviewed Jul 9, 2024

View reviewed changes

mempool/clist_mempool.go Show resolved Hide resolved

Add test

bb80743

sergio-mena reviewed Jul 9, 2024

View reviewed changes

mempool/mempool.go Show resolved Hide resolved

sergio-mena approved these changes Jul 9, 2024

View reviewed changes

comments; clarify code

004b388

hvanz added this pull request to the merge queue Jul 9, 2024

Merged via the queue into main with commit d4a82b7 Jul 9, 2024

hvanz deleted the hvanz/mempool-iterator-3303 branch July 9, 2024 19:27

hvanz mentioned this pull request Aug 8, 2024

fix(mempool): tx is skipped when sending to a lagging peer or sending fails #3647

Merged

2 tasks

mergify bot mentioned this pull request Aug 9, 2024

fix(mempool): tx is skipped when sending to a lagging peer or sending fails (backport #3647) #3657

Merged

2 tasks

mergify bot mentioned this pull request Sep 12, 2024

refactor(mempool): Add Iterator to replace TxsFront and TxsWaitChan methods (backport #3459) #4089

Merged

4 tasks

mergify bot mentioned this pull request Sep 16, 2024

fix(mempool): tx is skipped when sending to a lagging peer or sending fails (backport #3647) #4109

Merged

2 tasks

hvanz mentioned this pull request Dec 20, 2024

mempool: Separate reactor from implementation #1042

Closed

Conversation

hvanz commented Jul 8, 2024 • edited by sergio-mena Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

melekes Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanz Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

melekes Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

hvanz Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

hvanz Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

hvanz commented Sep 12, 2024

Uh oh!

mergify bot commented Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hvanz commented Jul 8, 2024 •

edited by sergio-mena

Loading

melekes Jul 9, 2024 •

edited

Loading

hvanz Jul 9, 2024 •

edited

Loading

mergify bot commented Sep 12, 2024 •

edited

Loading