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.

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

mempool: tx could be sent back to sender #3084

Closed
hvanz opened this issue May 15, 2024 · 0 comments · Fixed by #3131
Closed

mempool: tx could be sent back to sender #3084

hvanz opened this issue May 15, 2024 · 0 comments · Fixed by #3131
Labels

Comments

@hvanz
Copy link
Member

hvanz commented May 15, 2024

On ADR-105, it was decided to move the list of senders from the mempoolTx entries in CList to the mempool reactor. This was implemented in #1010. The interface of CheckTx was changed from

CheckTx(tx types.Tx, cb func(*abci.ResponseCheckTx), txInfo TxInfo) error

to

CheckTx(tx types.Tx) (abcicli.ReqRes, error)

where TxInfo used to contain the sender information. With this modification, the reactor now calls CheckTx and record the sender like this:

reqRes, err := memR.mempool.CheckTx(tx)
...
reqRes.SetCallback(func(res *abci.Response) {
	if res.GetCheckTx().Code == abci.CodeTypeOK {
        	memR.addSender(tx.Key(), sender.ID())
        }
})

The problem with this design is that the callback may introduce a delay when recording the sender. broadcastTxRoutine reads the transactions as soon as they are added to clist, so when it tries to read the list of senders in the reactor, the sender is not there yet, so it sends back the transaction to the sender.

A potential solution would be to move back the sender to the CList entries. Then the sender would be stored together with the transaction in mempoolTx and would be available at the moment the transaction is broadcasted. The drawback of this solution is that it reintroduces the mixing of the pool implementation and the gossip protocol.

Also note that:

  • Removing the cb parameter from the original CheckTx signature does not pose a problem and it's still useful to keep it out of CheckTx. This callback was only used by the RPC endpoints broadcast_sync and broadcast_commit to handle the CheckTx response. Now this is done with abcicli.ReqRes.
  • ADR-105 was not backported to v0.38.x.
@hvanz hvanz added the mempool label May 15, 2024
github-merge-queue bot pushed a commit that referenced this issue 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.

<!--

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 <[email protected]>
mergify bot pushed a commit that referenced this issue 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.

<!--

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 <[email protected]>
(cherry picked from commit 26cb788)

# Conflicts:
#	rpc/client/rpc_test.go
#	rpc/core/mempool.go
hvanz added a commit that referenced this issue 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.

<!--

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 <[email protected]>
hvanz added a commit that referenced this issue Jun 4, 2024
#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 <[email protected]>
Co-authored-by: hvanz <[email protected]>
Co-authored-by: Sergio Mena <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

1 participant