fix: never read reserved txs from the mempool #1267

evan-forbes · 2024-03-13T16:28:17Z

see details there.

evan-forbes · 2024-03-13T17:11:04Z

mempool/cat/store.go

 func (s *store) has(txKey types.TxKey) bool {
 	s.mtx.RLock()
 	defer s.mtx.RUnlock()
-	_, has := s.txs[txKey]
+	tx, has := s.txs[txKey]
+	if has && tx.isReserved() {
+		return false
+	}
 	return has
 }


wasn't sure about this one @cmwaters

do we want to return true even if a tx is reserved? does this defeat the purpose of reserving in the first place?

I think this is correct. We don't want to mark the node as having the transaction until we have verified it. We might want to revisit this solution however when we add the tx status endpoint. There could be several states:

Processing

Pending

Proposed

Committed

Evicted

Rejected

rootulp

CI is failing here: https://github.com/celestiaorg/celestia-core/actions/runs/8268178282/job/22620430005?pr=1267#step:6:124

rootulp · 2024-03-13T20:07:23Z

mempool/cat/tx.go

@@ -34,3 +34,5 @@ func newWrappedTx(tx types.Tx, key types.TxKey, height, gasWanted, priority int6

 // Size reports the size of the raw transaction in bytes.
 func (w *wrappedTx) size() int64 { return int64(len(w.tx)) }
+
+func (w *wrappedTx) isReserved() bool { return w.height == -1 }


[question] what is a reserved transaction? How does it relate to height == -1? Any links to docs would be helpful.

Also this function could use a Go doc.

reserved tx is a cat thing, I'm not sure yet if we need it as afaik it's an optimization

celestia-core/mempool/cat/pool.go

Line 299 in 5cfce0b

if !txmp.store.reserve(key) {

It's basically a way of not processing the same transaction in parallel. Say I receive transaction x and while processing it I receive tx x again I will verify it again because I haven't yet added it to the mempool

Rephrasing that: isReserved is true when a transaction has been received but hasn't been added to the mempool yet because it's still being processed? Does that mean isReserved == isProcessing?

Are these the exhaustive states a transaction could be in: https://github.com/celestiaorg/celestia-core/pull/1267/files#r1526067986

If yes, would it make sense to consolidate on one term either: isProcessing or isReserved?

evan-forbes · 2024-03-15T02:55:00Z

CI is failing here

yeah thanks, I'm waiting on this comment as I'm not sure if this is correct property or not #1267 (comment) and the test is failing because its testing for that property

cmwaters

Looks good to me

cmwaters · 2024-03-15T10:27:42Z

mempool/cat/store.go

 func (s *store) has(txKey types.TxKey) bool {
 	s.mtx.RLock()
 	defer s.mtx.RUnlock()
-	_, has := s.txs[txKey]
+	tx, has := s.txs[txKey]
+	if has && tx.isReserved() {
+		return false
+	}
 	return has
 }


I think this is correct. We don't want to mark the node as having the transaction until we have verified it. We might want to revisit this solution however when we add the tx status endpoint. There could be several states:

Processing

Pending

Proposed

Committed

Evicted

Rejected

staheri14

Looks good to me, though I have a question regarding the reasoning behind storing reserved transactions.

staheri14 · 2024-03-15T19:25:55Z

mempool/cat/store.go

@@ -27,7 +27,7 @@ func (s *store) set(wtx *wrappedTx) bool {
 	}
 	s.mtx.Lock()
 	defer s.mtx.Unlock()
-	if tx, exists := s.txs[wtx.key]; !exists || tx.height == -1 {
+	if tx, exists := s.txs[wtx.key]; !exists || tx.isReserved() {


[Question]Why should we store a reserved transaction if it will not be processed due to being a duplicate?

idk that's a great question cc @cmwaters

this was just adding a method for syntactic sugar, but it does seem like we should remove that check entirely

staheri14 · 2024-03-15T19:26:26Z

mempool/cat/store.go

+	if tx != nil && tx.isReserved() {
+		return nil
+	}


[Question] In which cases a reserved tx exists, while the original tx does not?

if its currently being processed by the mempool

Does it mean then there are situations in which the original tx is removed from the mempool, but the reserved one is still in the mempool?

The original tx and the reserved tx are the same barring the fact that the height we received the transaction has not yet been set.

If a transaction fails to be added it is removed. If it succeeds the height is added and it's no longer considered reserved

evan-forbes · 2024-03-18T15:41:14Z

while separate from this change, I'm fine with not merging until we resolve #1267 (comment)

rootulp · 2024-03-18T18:45:27Z

I don't understand this change enough to approve.

evan-forbes · 2024-03-18T19:10:24Z

I don't understand this change enough to approve.

@rootulp Does this sentence in the original issue help, or perhpaps we can sync on it quickly to get to the bottom it?

CAT will return txs that are reserved but not yet stored. This can result in weird behavior, such as passing the application a zero length transaction during PrepareProposal.

The fundemental problem is just that we can't pass len 0 txs to the application, as it will panic. Since the mempool processes things async, atm its possible to reap a reserved tx (a tx that is in the process of being added to the mempool, but only has a hash)

rootulp

LGTM. Sync'ed with @evan-forbes offline. My rough notes:

isReserved is unique to the CAT pool. It doesn't exist in other mempool implementations.
isReserved is a mechanism used to label transactions that are in the process of being added to the mempool.
We don't want a block proposer to reap an isReserved transaction and include it in a block yet. Therefore, methods like getAllKeys and getAllTxs were modified in this PR to return all non-reserved transactions.
Evan and I discussed two potential refactors:

Modify the conditional in

celestia-core/mempool/cat/pool.go

Line 299 in f95a434

if !txmp.store.reserve(key) {

to avoid a double negative.
Consider adding a separate map for reserved transactions

cmwaters · 2024-03-19T09:21:57Z

Consider adding a separate map for reserved transactions

This seems plausible as well. We essentially just want to check this map when a new transaction comes in, in case we are already processing it. It could just be a map of the tx hashes

evan-forbes · 2024-03-20T19:19:18Z

closing in favor of #1272

@rootulp

) This is an alternative to #1267 that solves the current problem with reserving transactions by using a separate lookup map as suggested by @rootulp.<hr>This is an automatic backport of pull request #1272 done by [Mergify](https://mergify.com). Co-authored-by: Callum Waters <[email protected]>

fix: never read reserved txs from the mempool

aa37422

evan-forbes added the T:Bug Type: Bug (confirmed) label Mar 13, 2024

evan-forbes self-assigned this Mar 13, 2024

evan-forbes requested review from cmwaters, staheri14, rach-id, ninabarbakadze and rootulp as code owners March 13, 2024 16:28

evan-forbes added the S:backport-to-v0.34.x label Mar 13, 2024

evan-forbes commented Mar 13, 2024

View reviewed changes

rootulp reviewed Mar 13, 2024

View reviewed changes

cmwaters previously approved these changes Mar 15, 2024

View reviewed changes

staheri14 reviewed Mar 15, 2024

View reviewed changes

fix: test

62c76d8

evan-forbes dismissed cmwaters’s stale review via 62c76d8 March 18, 2024 15:30

evan-forbes requested review from rootulp, cmwaters and staheri14 March 18, 2024 15:35

evan-forbes enabled auto-merge (squash) March 18, 2024 15:35

evan-forbes disabled auto-merge March 18, 2024 15:41

rootulp approved these changes Mar 18, 2024

View reviewed changes

rootulp mentioned this pull request Mar 18, 2024

CAT can return reserved txs #1266

Closed

cmwaters mentioned this pull request Mar 20, 2024

fix: use a separate lookup map for reserving txs #1272

Merged

evan-forbes closed this Mar 20, 2024

mergify bot mentioned this pull request Mar 22, 2024

fix: use a separate lookup map for reserving txs (backport #1272) #1274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: never read reserved txs from the mempool #1267

fix: never read reserved txs from the mempool #1267

evan-forbes commented Mar 13, 2024

evan-forbes Mar 13, 2024

cmwaters Mar 15, 2024

rootulp left a comment

rootulp Mar 13, 2024

evan-forbes Mar 15, 2024

cmwaters Mar 15, 2024

rootulp Mar 18, 2024

evan-forbes commented Mar 15, 2024 •

edited

Loading

cmwaters left a comment

cmwaters Mar 15, 2024

staheri14 left a comment

staheri14 Mar 15, 2024

evan-forbes Mar 18, 2024

staheri14 Mar 15, 2024

evan-forbes Mar 18, 2024

staheri14 Mar 18, 2024

cmwaters Mar 19, 2024

evan-forbes commented Mar 18, 2024

rootulp commented Mar 18, 2024

evan-forbes commented Mar 18, 2024

rootulp left a comment

cmwaters commented Mar 19, 2024

evan-forbes commented Mar 20, 2024

fix: never read reserved txs from the mempool #1267

fix: never read reserved txs from the mempool #1267

Conversation

evan-forbes commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Mar 15, 2024 • edited Loading

cmwaters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Mar 18, 2024

rootulp commented Mar 18, 2024

evan-forbes commented Mar 18, 2024

rootulp left a comment

Choose a reason for hiding this comment

cmwaters commented Mar 19, 2024

evan-forbes commented Mar 20, 2024

evan-forbes commented Mar 15, 2024 •

edited

Loading