fix(mempool): Move senders from reactor back to CList entries by hvanz · Pull Request #3131 · cometbft/cometbft

hvanz · 2024-05-27T08:52:27Z

This PR re-introduces the TxInfo parameter with sender information into the signature of CheckTx in the Mempool interface. The new signature is CheckTx(tx types.Tx, sender p2p.ID) (*abcicli.ReqRes, error), instead of CheckTx(tx types.Tx) (*abcicli.ReqRes, error).

PR #1010 moved the senders from the CList mempool entries to the reactor. The reason for the changes in this PR is that storing the sender (in the reactor, through a callback) after storing the transaction (in the CList struct) introduces a small delay during which the transaction doesn't have a sender, allowing, in a few cases, the gossip routines to send back the transaction to the sender. Now the transaction and the (first) sender are stored together when the transaction is added to the mempool.

Note that the changes in #1010 didn't reach v0.38.x, so this change will partially revert the mempool API in main and v1.x.

For reviewers, please see the github comments on the code.

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

hvanz · 2024-05-28T09:47:37Z

mempool/reactor.go

 		// https://github.com/tendermint/tendermint/issues/5796

-		if !memR.isSender(memTx.tx.Key(), peer.ID()) {
+		if !memTx.isSender(peer.ID()) {


This is the main reason of this PR. When reading the transaction to gossip, the sender must be available immediately so the transaction is not sent back to it. By first storing the transaction in CList and then storing the sender in the reactor, a small delay was introduced during which the transaction has no sender.

hvanz · 2024-05-28T09:50:38Z

mempool/clist_mempool.go

 		mp.cache = NopTxCache{}
 	}

-	proxyAppConn.SetResponseCallback(mp.globalCb)


The globalCb callback was only used for Recheck responses. Now the responses are simply handled by a callback in a ReqRes structure, right after calling CheckTxAsync during rechecking.

There are three structures with a callback:

proxyAppConn, implementing the Client interface, with a callback of type func(*types.Request, *types.Response). Before this PR, this callback was used only on CheckTx ABCI responses. It is set only once, during the creation of CListMempool.

ReqRes, with a callback of type func(*types.Response). This is more flexible because we can set it when CheckTxAsync returns the request/response, allowing us to customise the function by passing extra information, such as the sender.

the external callback passed to CListMempool.CheckTx, reintroduced in this PR, and invoked at the end of handling the CheckTx response. This is used for processing the response on the RPC endpoints broadcast_tx_sync and broadcast_tx_commit.

The globalCb callback was only used for Recheck responses.

I see there a case for CheckTx in addition to the one for ReCheckTx

Should this useful explanation be somewhere in the form of a comment?

…or)`

hvanz · 2024-05-29T07:07:11Z

mempool/clist_mempool.go

-	default:
-		// ignore other messages
+			// update metrics
+			mem.metrics.Size.Set(float64(mem.Size()))


Now, update metrics only when the mempool is modified.

rpc/core/mempool.go

mempool/reactor.go

Co-authored-by: Sergio Mena <sergio@informal.systems>

mempool/mempool.go

mempool/clist_mempool.go

mempool/mempoolTx.go

sergio-mena

Thanks for this!

Closes #3084 This PR re-introduces the `TxInfo` parameter with sender information into the signature of `CheckTx` in the `Mempool` interface. The new signature is `CheckTx(tx types.Tx, sender p2p.ID) (*abcicli.ReqRes, error)`, instead of `CheckTx(tx types.Tx) (*abcicli.ReqRes, error)`. PR #1010 moved the senders from the CList mempool entries to the reactor. The reason for the changes in this PR is that storing the sender (in the reactor, through a callback) after storing the transaction (in the CList struct) introduces a small delay during which the transaction doesn't have a sender, allowing, in a few cases, the gossip routines to send back the transaction to the sender. Now the transaction and the (first) sender are stored together when the transaction is added to the mempool. Note that the changes in #1010 didn't reach `v0.38.x`, so this change will partially revert the mempool API in `main` and `v1.x`. For reviewers, please see the github comments on the code.  --- #### PR checklist - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] 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: Sergio Mena <sergio@informal.systems> (cherry picked from commit 26cb788) # Conflicts: # rpc/client/rpc_test.go # rpc/core/mempool.go

…#3131)" This reverts commit e98ce85.

Closes #3084 This PR re-introduces the `TxInfo` parameter with sender information into the signature of `CheckTx` in the `Mempool` interface. The new signature is `CheckTx(tx types.Tx, sender p2p.ID) (*abcicli.ReqRes, error)`, instead of `CheckTx(tx types.Tx) (*abcicli.ReqRes, error)`. PR #1010 moved the senders from the CList mempool entries to the reactor. The reason for the changes in this PR is that storing the sender (in the reactor, through a callback) after storing the transaction (in the CList struct) introduces a small delay during which the transaction doesn't have a sender, allowing, in a few cases, the gossip routines to send back the transaction to the sender. Now the transaction and the (first) sender are stored together when the transaction is added to the mempool. Note that the changes in #1010 didn't reach `v0.38.x`, so this change will partially revert the mempool API in `main` and `v1.x`. For reviewers, please see the github comments on the code.  --- - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] 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: Sergio Mena <sergio@informal.systems>

#3131) (#3178) Closes #3084 This PR re-introduces the `TxInfo` parameter with sender information into the signature of `CheckTx` in the `Mempool` interface. The new signature is `CheckTx(tx types.Tx, sender p2p.ID) (*abcicli.ReqRes, error)`, instead of `CheckTx(tx types.Tx) (*abcicli.ReqRes, error)`. PR #1010 moved the senders from the CList mempool entries to the reactor. The reason for the changes in this PR is that storing the sender (in the reactor, through a callback) after storing the transaction (in the CList struct) introduces a small delay during which the transaction doesn't have a sender, allowing, in a few cases, the gossip routines to send back the transaction to the sender. Now the transaction and the (first) sender are stored together when the transaction is added to the mempool. Note that the changes in #1010 didn't reach `v0.38.x`, so this change will partially revert the mempool API in `main` and `v1.x`. For reviewers, please see the github comments on the code. --- #### PR checklist - [X] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [X] 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 #3131 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: Sergio Mena <sergio@informal.systems>

… (backport #3131) (#3178)" This reverts commit 9710d69.

…ync/commit` (#3193) This PR fixes a race condition generated when calling `broadcast_tx_sync` and `broadcast_tx_commit`. When closing the context, we want to also close the goroutine waiting for the response with `reqRes.Wait()`. We don't need to do `reqRes.Done()` because there is a chance it may become negative; instead, we just let the ABCI client to do it. This bug was recently introduced by #3131. --- #### PR checklist - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

…ync/commit` (#3193) This PR fixes a race condition generated when calling `broadcast_tx_sync` and `broadcast_tx_commit`. When closing the context, we want to also close the goroutine waiting for the response with `reqRes.Wait()`. We don't need to do `reqRes.Done()` because there is a chance it may become negative; instead, we just let the ABCI client to do it. This bug was recently introduced by #3131. --- #### PR checklist - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec (cherry picked from commit cd465c5) # Conflicts: # rpc/core/mempool.go

…ync/commit` (backport #3193) (#3194) This PR fixes a race condition generated when calling `broadcast_tx_sync` and `broadcast_tx_commit`. When closing the context, we want to also close the goroutine waiting for the response with `reqRes.Wait()`. We don't need to do `reqRes.Done()` because there is a chance it may become negative; instead, we just let the ABCI client to do it. This bug was recently introduced by #3131. --- #### PR checklist - [ ] 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 - [ ] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec <hr>This is an automatic backport of pull request #3193 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>

mempool/reactor.go

hvanz added 3 commits May 27, 2024 10:51

Move senders back from reactor to mempoolTx

11ac0c8

Merge branch 'main' into hvanz/mempool-fix-senders-3084

3d93672

make mockery

51e0182

hvanz commented May 28, 2024

View reviewed changes

pass *TxInfo instead of TxInfo

7302049

hvanz commented May 28, 2024

View reviewed changes

hvanz added 3 commits May 28, 2024 12:04

comments

d012e03

update ADR

d9e9a9a

Update UPGRADING.md

8313e08

hvanz self-assigned this May 28, 2024

hvanz added the mempool label May 28, 2024

hvanz changed the title ~~fix(mempool): Move senders back from reactor to clist_mempool entries~~ fix(mempool): Move senders from reactor back to CList entries May 28, 2024

fix tests

1b59dab

hvanz marked this pull request as ready for review May 28, 2024 13:49

hvanz requested a review from a team as a code owner May 28, 2024 13:49

hvanz requested a review from a team May 28, 2024 13:49

hvanz marked this pull request as draft May 28, 2024 15:06

hvanz added the wip Work in progress label May 28, 2024

Merge branch 'main' into hvanz/mempool-fix-senders-3084

1ef60a0

hvanz added backport-to-v1.x labels May 28, 2024

hvanz added 5 commits May 28, 2024 18:03

Change to `CheckTx(tx types.Tx, txInfo *TxInfo) (*abcicli.ReqRes, err…

485ff06

…or)`

fix test

727b2ca

fix rpc

0db1ef9

Update UPGRADING.md

ff0622d

pass nil instead of &TxInfo{}

89d795d

hvanz commented May 29, 2024

View reviewed changes

hvanz marked this pull request as ready for review May 29, 2024 07:20

hvanz marked this pull request as draft May 29, 2024 07:54

sergio-mena reviewed Jun 3, 2024

View reviewed changes

rpc/core/mempool.go Outdated Show resolved Hide resolved

sergio-mena reviewed Jun 3, 2024

View reviewed changes

mempool/reactor.go Show resolved Hide resolved

Apply suggestions from code review

9b4253f

Co-authored-by: Sergio Mena <sergio@informal.systems>

sergio-mena reviewed Jun 3, 2024

View reviewed changes

mempool/mempool.go Outdated Show resolved Hide resolved

sergio-mena reviewed Jun 3, 2024

View reviewed changes

mempool/clist_mempool.go Outdated Show resolved Hide resolved

gracefully close wait-for-response goroutine on rpc

ada8914

sergio-mena reviewed Jun 4, 2024

View reviewed changes

mempool/mempoolTx.go Outdated Show resolved Hide resolved

sergio-mena approved these changes Jun 4, 2024

View reviewed changes

hvanz and others added 5 commits June 4, 2024 11:02

In CheckTx signature, change *TxInfo for p2p.ID

ffc1918

replace switch by if

fa499b3

mark SetResponseCallback as deprecated

1a64450

changelog

e33f591

Merge branch 'main' into hvanz/mempool-fix-senders-3084

6b00a01

hvanz enabled auto-merge June 4, 2024 09:36

hvanz added this pull request to the merge queue Jun 4, 2024

Merged via the queue into main with commit 26cb788 Jun 4, 2024

hvanz deleted the hvanz/mempool-fix-senders-3084 branch June 4, 2024 09:43

mergify bot mentioned this pull request Jun 4, 2024

fix(mempool): Move senders from reactor back to CList entries (backport #3131) #3178

Merged

4 tasks

hvanz added a commit that referenced this pull request Jun 4, 2024

Revert "fix(mempool): Move senders from reactor back to CList entries (…

8693905

…#3131)" This reverts commit e98ce85.

andynog added a commit that referenced this pull request Jun 4, 2024

Revert "fix(mempool): Move senders from reactor back to CList entries…

0f76455

… (backport #3131) (#3178)" This reverts commit 9710d69.

hvanz mentioned this pull request Jun 5, 2024

fix(rpc): Race condition when waiting for response on broadcast_tx_sync/commit #3193

Merged

4 tasks

mergify bot mentioned this pull request Jun 5, 2024

fix(rpc): Race condition when waiting for response on broadcast_tx_sync/commit (backport #3193) #3194

Merged

4 tasks

jmalicevic reviewed Jul 30, 2024

View reviewed changes

mempool/reactor.go Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mempool): Move senders from reactor back to CList entries#3131

fix(mempool): Move senders from reactor back to CList entries#3131
hvanz merged 26 commits intomainfrom
hvanz/mempool-fix-senders-3084

hvanz commented May 27, 2024 •

edited

Loading

Uh oh!

hvanz May 28, 2024 •

edited

Loading

Uh oh!

hvanz May 28, 2024 •

edited

Loading

Uh oh!

hvanz May 28, 2024

Uh oh!

sergio-mena Jun 3, 2024

Uh oh!

sergio-mena Jun 3, 2024

Uh oh!

hvanz May 29, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

PR checklist

Uh oh!

hvanz May 28, 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 May 28, 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 May 28, 2024

Choose a reason for hiding this comment

Uh oh!

sergio-mena Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

sergio-mena Jun 3, 2024

Choose a reason for hiding this comment

Uh oh!

hvanz May 29, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hvanz commented May 27, 2024 •

edited

Loading

hvanz May 28, 2024 •

edited

Loading

hvanz May 28, 2024 •

edited

Loading