Skip to content

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

Merged
hvanz merged 3 commits intov1.xfrom
mergify/bp/v1.x/pr-3131
Jun 4, 2024
Merged

fix(mempool): Move senders from reactor back to CList entries (backport #3131)#3178
hvanz merged 3 commits intov1.xfrom
mergify/bp/v1.x/pr-3131

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented Jun 4, 2024

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

  • 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
This is an automatic backport of pull request #3131 done by [Mergify](https://mergify.com).

@hvanz @mergify
fix(mempool): Move senders from reactor back to CList entries (#3131)
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.

<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

#### 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
@mergify mergify bot requested a review from a team as a code owner June 4, 2024 09:44
@mergify mergify bot requested a review from a team June 4, 2024 09:44
@mergify mergify bot added the conflicts label Jun 4, 2024
@mergify
Copy link
Contributor Author

mergify bot commented Jun 4, 2024

Cherry-pick of 26cb788 has failed:

On branch mergify/bp/v1.x/pr-3131
Your branch is up to date with 'origin/v1.x'.

You are currently cherry-picking commit 26cb78807.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	deleted:    .changelog/unreleased/breaking-changes/1010-mempool-interface.md
	new file:   .changelog/unreleased/breaking-changes/3084-deprecate-set-response-callback.md
	new file:   .changelog/unreleased/breaking-changes/3084-mempool-interface.md
	modified:   UPGRADING.md
	modified:   abci/client/client.go
	modified:   abci/client/grpc_client.go
	modified:   abci/client/local_client.go
	modified:   abci/client/socket_client.go
	modified:   abci/client/unsync_local_client.go
	modified:   docs/references/architecture/adr-105-refactor-mempool-senders.md
	modified:   internal/consensus/mempool_test.go
	modified:   internal/consensus/reactor_test.go
	modified:   internal/consensus/replay_stubs.go
	modified:   internal/consensus/replay_test.go
	modified:   internal/consensus/state_test.go
	modified:   mempool/bench_test.go
	modified:   mempool/cache_test.go
	modified:   mempool/clist_mempool.go
	modified:   mempool/clist_mempool_test.go
	modified:   mempool/mempool.go
	modified:   mempool/mempoolTx.go
	modified:   mempool/mocks/mempool.go
	modified:   mempool/nop_mempool.go
	modified:   mempool/nop_mempool_test.go
	modified:   mempool/reactor.go
	modified:   mempool/reactor_test.go
	modified:   node/node_test.go
	modified:   proxy/app_conn.go
	modified:   test/fuzz/mempool/checktx.go
	modified:   test/fuzz/tests/mempool_test.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   rpc/client/rpc_test.go
	both modified:   rpc/core/mempool.go

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

hvanz and others added 2 commits June 4, 2024 13:13
@hvanz
Revert "fix(mempool): Move senders from reactor back to CList entries (…
8693905 
…#3131)"

This reverts commit e98ce85.
@hvanz @sergio-mena
fix(mempool): Move senders from reactor back to CList entries (#3131)
f723573 
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.

<!--

Please add a reference to the issue that this PR addresses and indicate
which
files are most critical to review. If it fully addresses a particular
issue,
please include "Closes #XXX" (where "XXX" is the issue number).

If this PR is non-trivial/large/complex, please ensure that you have
either
created an issue that the team's had a chance to respond to, or had some
discussion with the team prior to submitting substantial pull requests.
The team
can be reached via GitHub Discussions or the Cosmos Network Discord
server in
the #cometbft channel. GitHub Discussions is preferred over Discord as
it
allows us to keep track of conversations topically.
https://github.com/cometbft/cometbft/discussions

If the work in this PR is not aligned with the team's current
priorities, please
be advised that it may take some time before it is merged - especially
if it has
not yet been discussed with the team.

See the project board for the team's current priorities:
https://github.com/orgs/cometbft/projects/1

-->

---

- [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>
@hvanz hvanz merged commit 9710d69 into v1.x Jun 4, 2024
@hvanz hvanz deleted the mergify/bp/v1.x/pr-3131 branch June 4, 2024 11:33
andynog added a commit that referenced this pull request Jun 4, 2024
@andynog
Revert "fix(mempool): Move senders from reactor back to CList entries…
0f76455 
… (backport #3131) (#3178)"

This reverts commit 9710d69.
@jchappelow jchappelow mentioned this pull request Jun 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@hvanz hvanz hvanz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hvanz @sergio-mena