Skip to content

Return error if any in mempool CheckTx and fix callback #311

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.

Sign up for GitHub

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

Open
masih wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Return error if any in mempool CheckTx and fix callback #311

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
758743c
Return error if any in mempool `CheckTx` and fix callback
masih Sep 4, 2025
e139fc9
Increment pending size only if insertion is successful
masih Sep 4, 2025
7ae635d
Remove unnecessary error log add and fix nil check
masih Sep 5, 2025
d231364
Merge branch 'main' into masih/fix-checktx-err-handling-and-cb
masih Sep 24, 2025
0b921c0
Merge branch 'main' into masih/fix-checktx-err-handling-and-cb
masih Sep 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 20 additions & 21 deletions internal/mempool/mempool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,7 @@ func (txmp *TxMempool) CheckTx(
if err != nil {
removeHandler(true)
res.Log = txmp.AppendCheckTxErr(res.Log, err.Error())
return err
Copy link
Contributor

@yzang2019 yzang2019 Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break the existing broadcast logic if we return err instead of putting err in the response. Currently error is already being handled by adding to the res.log and passed into cb function back to the client

Copy link
Contributor

@yzang2019 yzang2019 Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a second though, I think it makes more sense to return error to the client rather than putting error in the response log, so the change LGTM. Probably need to evaluate whether there's any risk/impact to the existing client or not.

}

wtx := &WrappedTx{
Expand All @@ -377,28 +378,26 @@ func (txmp *TxMempool) CheckTx(
estimatedGas: res.GasEstimated,
}

if err == nil {
// only add new transaction if checkTx passes and is not pending
if !res.IsPendingTransaction {
err = txmp.addNewTransaction(wtx, res.ResponseCheckTx, txInfo)
if err != nil {
return err
}
} else {
// otherwise add to pending txs store
if res.Checker == nil {
return errors.New("no checker available for pending transaction")
}
if err := txmp.canAddPendingTx(wtx); err != nil {
// TODO: eviction strategy for pending transactions
removeHandler(true)
return err
}
atomic.AddInt64(&txmp.pendingSizeBytes, int64(wtx.Size()))
if err := txmp.pendingTxs.Insert(wtx, res, txInfo); err != nil {
return err
}
// only add new transaction if checkTx passes and is not pending
if !res.IsPendingTransaction {
err = txmp.addNewTransaction(wtx, res.ResponseCheckTx, txInfo)
if err != nil {
return err
}
} else {
// otherwise add to pending txs store
if res.Checker == nil {
return errors.New("no checker available for pending transaction")
}
if err := txmp.canAddPendingTx(wtx); err != nil {
// TODO: eviction strategy for pending transactions
removeHandler(true)
return err
}
if err := txmp.pendingTxs.Insert(wtx, res, txInfo); err != nil {
return err
}
atomic.AddInt64(&txmp.pendingSizeBytes, int64(wtx.Size()))
}

if cb != nil {
Expand Down