fix: block include nonce mismatch tx issue #16
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
unclezoro
reviewed
Jul 17, 2023
mempool/v0/reactor.go
Outdated
| @@ -122,12 +124,14 @@ func (memR *Reactor) OnStart() error { | |||
| memR.Logger.Info("Tx broadcasting is disabled") | |||
| } | |||
| go memR.receiveRoutine() | |||
| go memR.checkTxRoutine() | |||
| return nil | |||
| } | |||
|
|
|||
| // OnStop implements p2p.BaseReactor. | |||
| func (memR *Reactor) OnStop() { | |||
| close(memR.recvCh) | |||
There was a problem hiding this comment.
I think we dont have to close these two channel.
Because the CI check is failed now: https://github.com/bnb-chain/greenfield-cometbft/actions/runs/5574353577/jobs/10189203589?pr=16.
It is ok leaving there.
unclezoro
reviewed
Jul 17, 2023
mempool/v0/reactor.go
Outdated
| if errors.Is(err, mempool.ErrTxInCache) { | ||
| memR.Logger.Debug("Tx already exists in cache", "tx", ntx.String()) | ||
| } else if err != nil { | ||
| memR.Logger.Info("Could not check tx", "tx", ntx.String(), "err", err) | ||
| } | ||
| } | ||
| close(errChan) |
j75689
force-pushed
the
fix/nonce_mismatch_issue
branch
from
9866402 to
c844fee
Compare
j75689
force-pushed
the
fix/nonce_mismatch_issue
branch
from
c844fee to
e926db4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
fix block include nonce mismatch tx issue
Rationale
Currently, there are two possible concurrent invocations of the CheckTx method. The first one is an RPC invocation, and the second one is the simultaneous invocation through different P2P connections.
However, in reality, CheckTx first calls ABCI for basic validation within Cosmos, and upon receiving the cometbft response, it writes to the mempool. The process is illustrated as follows:
mem.proxyAppConn.CheckTxAsynccall ABCIreqRes.SetCallback(mem.reqResCb(tx, txInfo.SenderID, txInfo.SenderP2PID, cb))writeTxinto mempoolHowever, this creates a problem. When the CPU is particularly busy, if concurrent operations occur here, the temporary scheduling of Goroutines may cause the order of writing to the mempool after ABCI check to be incorrect. Ultimately, this results in incorrect transaction order when packaging blocks.
Therefore, we must ensure that all transaction "checking" and "writing" are executed sequentially. In this PR modification, a channel is introduced to execute transaction requests received from both P2P and RPC in a sequential manner, guaranteeing the correct order of writing to the mempool.
Example
add an example CLI or API response...
Changes
Notable changes: