fix: mempool locking mechanism in v1 and cat #1582
Conversation
cmwaters
requested review from
rootulp and
rach-id
and removed request for
a team
ninabarbakadze
changed the title
rootulp
changed the title
| @@ -58,6 +63,9 @@ func (s *store) remove(txKey types.TxKey) bool { | |||
| return false | |||
| } | |||
| s.bytes -= tx.size() | |||
| if err := s.deleteOrderedTx(tx); err != nil { | |||
| panic(err) | |||
There was a problem hiding this comment.
I'm concerned about the panic here. Should we consider:
- logging this error and
return falseinstead? - modifying the
removefunction signature to propagate the error?
There was a problem hiding this comment.
The panic is in place because this would imply a developer error i.e. this shouldn't error so long as the developer has correctly implemented this method (the tx should always be in the ordered list if it exists in the map)
| if idx >= len(s.orderedTxs) || s.orderedTxs[idx] != tx { | ||
| return fmt.Errorf("transaction %X not found in ordered list", tx.key) | ||
| } | ||
| s.orderedTxs = append(s.orderedTxs[:idx], s.orderedTxs[idx+1:]...) |
There was a problem hiding this comment.
if the idx is the last transaction in the ordered list, I expect s.orderedTxs[idx+1:] to fail.
Perhaps this needs a unit test where deleteOrderedTx is invoked on the last tx in the ordered list to verify there is no bug here.
There was a problem hiding this comment.
TestStoreSimple actually tests this by adding a tx and removing it. Since it's the only transaction, it is defacto the last tx in the ordered list
There was a problem hiding this comment.
if the idx is the last transaction, s.orderedTxs[idx+1:] will be empty.
| require.Empty(t, store.txs) | ||
| } | ||
|
|
||
| func TestStoreOrdering(t *testing.T) { |
There was a problem hiding this comment.
should we be testing this with higher transaction count?
There was a problem hiding this comment.
What's the advantage
| if idx >= len(s.orderedTxs) || s.orderedTxs[idx] != tx { | ||
| return fmt.Errorf("transaction %X not found in ordered list", tx.key) | ||
| } | ||
| s.orderedTxs = append(s.orderedTxs[:idx], s.orderedTxs[idx+1:]...) |
There was a problem hiding this comment.
if the idx is the last transaction, s.orderedTxs[idx+1:] will be empty.
Yes I did (locally)
Upstream doesn't have a version of cat or the v1 priority mempool. This locking mechanism isn't an issue in v0
No I don't think so. The test can catch a wider net of bugs in celestia-app |
|
Removed the auto-merge by accident |
Upgrade to https://github.com/celestiaorg/celestia-core/releases/tag/v1.44.2-tm-v0.34.35 to pick up the fix in celestiaorg/celestia-core#1582 (cherry picked from commit 885c317) # Conflicts: # test/interchain/go.mod # test/interchain/go.sum
This is residual of #1553
The problem is now even more subtle. Because the mempool mutexes weren't over both CheckTx and the actual adding of the transaction to the mempool we occasionally hit a situation as follows:
This PR fixes this pattern, however this won't be watertight for the CAT pool until we can order both on gas fee (priority) and nonce.