CAT Part 2: write pool for handling CRUD-like operations

[cmwaters]

Ref: #935

[MSevey]

The comment for the TxPool struct says that txns are gossiped in the order that they arrive, however the combination of the channel and map doesn't preserve the order.

See simplified example https://go.dev/play/p/KSy7W4u4Fwc

If the order doesn't matter, I would update the comment. If the order does matter, then we should use a list or something to ensure FIFO ordering.

[cmwaters]

That's a good point. I was too focused on ensuring that transactions aren't dropped on their way to the reactor. Ordering isn't so important so long as transactions arrive in a timely manner (they are reordered by priority when it comes to block construction).

[MSevey]

should we check the connection before running the preCheck function? Or does the ordering of these two checks not matter much? If so, why?

[MSevey]

to me the error with a connection would trump the error from the preCheck which is why i would think it connection check would be first.

[cmwaters]

They're unrelated in the sense that different components are validating it.

We could call txmp.proxyAppConn.Error() earlier in the method if we wanted to. I believe it just needs to run before CheckTxSync to check that the application hasn't for some reason already errored and stopped.

Tbh, I think it's also possible that the application errors after Error() is called but before CheckTx so I think the application needs to handle this anyway and I'm not sure why Error() exists in the first place

[cmwaters]

I think the problem with making a direct copy of the priority mempool is that there are a lot of changes here that are just inherited from previous authors. Not that we shouldn't go over the work in it's entirety it's just that I've made it difficult to identify the changed that I've actually made and the ones that previously existed. I think what I would do next time is to have the first PR just be directly copying the v1 mempool as cat and then build the rest of the PR's of that. I guess I could still do that if people wanted. The nice thing about having the complete change is people can learn more about the workings of the mempool in its entirety.

[cmwaters]

That's a good point. I was too focused on ensuring that transactions aren't dropped on their way to the reactor. Ordering isn't so important so long as transactions arrive in a timely manner (they are reordered by priority when it comes to block construction).

[cmwaters]

They're unrelated in the sense that different components are validating it.

We could call txmp.proxyAppConn.Error() earlier in the method if we wanted to. I believe it just needs to run before CheckTxSync to check that the application hasn't for some reason already errored and stopped.

Tbh, I think it's also possible that the application errors after Error() is called but before CheckTx so I think the application needs to handle this anyway and I'm not sure why Error() exists in the first place

[evan-forbes]

no blocking feedback other than that from the original PR! LGTM 👍 👍

we might want to add a test for BlobTxs similar to

celestia-core/mempool/v1/mempool_test.go

Lines 658 to 681 in ab4ef71

	func TestRemoveBlobTx(t *testing.T) {
	txmp := setup(t, 500)
	originalTx := []byte{1, 2, 3, 4}
	indexWrapper, err := types.MarshalIndexWrapper(originalTx, 100)
	require.NoError(t, err)
	// create the blobTx
	b := tmproto.Blob{
	NamespaceId: []byte{1, 2, 3, 4, 5, 6, 7, 8},
	Data: []byte{1, 2, 3, 4, 5, 6, 7, 8, 1, 2, 3, 4, 5, 6, 7, 8, 9},
	ShareVersion: 0,
	}
	bTx, err := types.MarshalBlobTx(originalTx, &b)
	require.NoError(t, err)
	err = txmp.CheckTx(bTx, nil, mempool.TxInfo{})
	require.NoError(t, err)
	err = txmp.Update(1, []types.Tx{indexWrapper}, abciResponses(1, abci.CodeTypeOK), nil, nil)
	require.NoError(t, err)
	assert.EqualValues(t, 0, txmp.Size())
	assert.EqualValues(t, 0, txmp.SizeBytes())
	}

[cmwaters]

@MSevey do you mind giving this a second pass when you've got a moment

[MSevey]

LGTM, happy to approve once the open conversations are resolved.

[staheri14]

I have made some final suggestions that are non-blocking and everything looks good to me.

[staheri14]

It only applies to the
// first created block. After that, Update() overwrites the existing value.

What does the first created block mean? You mean f(tx) is applied to all the transaction in the Genesis block?
Also, can you please expand on the second part i.e.,

After that, Update() overwrites the existing value.

It is not clear immediately.

[cmwaters]

Yes all the txs before height 1 will use this preCheck and postCheck function. Afterwards, the Update function includes new preCheck and postCheck functions that will overwrite the ones initially provided in the constructor. Does that make sense? Shall I modify the comments as so (for the record those weren't my comments to begin with)

[staheri14]

Thanks for the clarification!

Shall I modify the comments as so (for the record those weren't my comments to begin with)

I'd say yes, that would help the readability of the code

[staheri14]

No further comments from my side, Looks good to me!