-
Notifications
You must be signed in to change notification settings - Fork 235
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
feat: add tx status in mempool #1287
base: main
Are you sure you want to change the base?
feat: add tx status in mempool #1287
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Suggestions were mostly optional comment improvements
// Push adds the given raw transaction to the cache and returns true if it was | ||
// newly added. Otherwise, it returns false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Push adds the given raw transaction to the cache and returns true if it was | |
// newly added. Otherwise, it returns false. | |
// Push adds the given tx key to the cache and returns true if it was | |
// newly added. Otherwise, it returns false. |
@@ -18,14 +18,14 @@ type TxCache interface { | |||
|
|||
// Push adds the given raw transaction to the cache and returns true if it was | |||
// newly added. Otherwise, it returns false. | |||
Push(tx types.Tx) bool | |||
Push(tx types.TxKey) bool | |||
|
|||
// Remove removes the given raw transaction from the cache. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Remove removes the given raw transaction from the cache. | |
// Remove removes the given tx key from the cache. |
// Has reports whether tx is present in the cache. Checking for presence is | ||
// not treated as an access of the value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Has reports whether tx is present in the cache. Checking for presence is | |
// not treated as an access of the value. | |
// Has reports whether tx key is present in the cache. Checking for presence is | |
// not treated as an access of the value. |
key := tx.Key() | ||
func (c *LRUTxCache) Remove(key types.TxKey) { | ||
c.RemoveTxByKey(key) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Remove
is now redundant with RemoveTxByKey
. Should we remove one of them?
wtx := txmp.store.get(txKey) | ||
if wtx != nil { | ||
return wtx.tx, true | ||
} | ||
return types.Tx{}, false | ||
} | ||
|
||
// GetTxEvicted returns a bool indicating whether the transaction with | ||
// the specified key was recently evicted and is currently within the cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the specified key was recently evicted and is currently within the cache | |
// the specified key was recently evicted and is currently within the cache. |
@@ -183,6 +183,16 @@ func (mem *CListMempool) TxsFront() *clist.CElement { | |||
return mem.txs.Front() | |||
} | |||
|
|||
// GetTxByKey currently not enabled in v0 but it is required by the interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// GetTxByKey currently not enabled in v0 but it is required by the interface. | |
// GetTxByKey is not supported by the v0 mempool but it is required to satisfy the mempool interface. |
return nil, false | ||
} | ||
|
||
// GetTxEvicted currently not enabled in v0 but it is required by the interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// GetTxEvicted currently not enabled in v0 but it is required by the interface. | |
// GetTxEvicted is not supported by the v0 mempool but it is required to satisfy the mempool interface. |
} | ||
|
||
// GetTxEvicted returns a bool indicating whether the transaction with | ||
// the specified key was recently evicted and is currently within the cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// the specified key was recently evicted and is currently within the cache | |
// the specified key was recently evicted and is currently within the cache. |
|
||
// TxStatus retrieves the status of a transaction given its hash. It returns a ResultTxStatus | ||
// containing the height and index of the transaction within the block(if committed) | ||
// or whether the transaction is pending, evicted in the mempool otherwise unknown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// or whether the transaction is pending, evicted in the mempool otherwise unknown. | |
// or whether the transaction is pending, evicted from the mempool, or otherwise unknown. |
return &ctypes.ResultTxStatus{Status: "PENDING"}, nil | ||
} | ||
|
||
// Check if the tx is evicted | ||
isEvicted := env.Mempool.GetTxEvicted(txKey) | ||
if isEvicted { | ||
return &ctypes.ResultTxStatus{Status: "EVICTED"}, nil | ||
} | ||
|
||
// Check if the tx has been committed | ||
txInfo := env.BlockStore.LoadTxInfo(hash) | ||
if txInfo != nil { | ||
return &ctypes.ResultTxStatus{Height: txInfo.Height, Index: txInfo.Index, Status: "COMMITTED"}, nil | ||
} | ||
|
||
// If the tx is not in the mempool, evicted, or committed, return unknown | ||
return &ctypes.ResultTxStatus{Status: "UNKNOWN"}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] should we extract consts for PENDING
, EVICTED
, COMMITTED
, and UNKNOWN
?
Description
Fixes #1281
Opens #1381