Review of POS mempool (remove _connectFailingTxn)

[diamondhands0]

No description provided.

[diamondhands0]

Connecting the txn twice is definitely going to slow us down. I almost wish there was a way to do this in such a way that we just update the pointer to point to the copiedView.

Also I'm not 100% sure we need this _connectTransactions primitive on the view vs doing this logic in the mempool one time but will keep reading.

[diamondhands0]

I don't see how we would ever get past this line since totalConnectedTxns isn't incremented until AFTER this line. Could this be a bug?

[diamondhands0]

Love this deletion.

[diamondhands0]

I wish there was a comment here explaining why we need this. You obviously added it for a reason and it would be super helpful to have that reason here so that when I review I can internalize what drove the addition quickly instead of having to figure it out. poolcoke has recently internalized this advice and started writing extremely long comments on everything. I love this and would like to see more of it from everyone. See here as an example all throughout this file:

• core/lib/block_view_lockups.go

  Line 114 in 3dc58f3

  type LockedBalanceEntryKey struct {

[diamondhands0]

Love this deletion. I assume the logic is now embedded in one of the other parts of the code but will keep reading.

[diamondhands0]

All of these deletions are amazing. There was so much complexity driven by the changing block header that is now no longer needed. I really love it.

[diamondhands0]

I assume this is to make it so that you can recognize when a validator is reusing a nonce? If yes I would bump it to a higher number like 100k since we could have a lot of validator connections coming in and a lot of nonces to track.

[diamondhands0]

Ok cool I see this is what you did with the handshake manager. I love this consolidation.

[diamondhands0]

Nice. I think this should prevent us from sending unvalidated txns on the first sync which is good.

[diamondhands0]

Love this change. Just skipping txns that don't connect. Easy.

[diamondhands0]

Here again I wish you had super detailed comments that explained what these control and all of the design decisions around them.

[diamondhands0]

I was thinking maybe you don't need the nonce tracker anymore but I think you actually do need it to power replace-by-fee for txns that are beyond the 10k limit of our validation window. So good to keep it.

[diamondhands0]

Normally when you have more than one line in a Lock/RLock we like to break it up into a function like:

func getThings() {
mp.RLock()
defer mp.RLock()

validationView := mp.readOnlyLatestBlockView
mempoolTxns := mp.getTransactionsNoLock()

return validationView, mempoolTxns

}

[diamondhands0]

This prevents disasters as people modify the code in the future.

[diamondhands0]

I think I get why you made this function a member of the view but since we only really use it here, I would just embed the logic here and try to get it down to only one Connect call. I think it's slightly better from a readability standpoint, but from an efficiency standpoint the double Connect is definitely going to be a bottleneck so it fixes that.

[diamondhands0]

To clarify: I'm suggesting you delete ConnectTransactionsWithLimit and embed the logic that it's running in here, but get rid of the second Connect.

[diamondhands0]

Interesting. How would ii ever be more than mempoolTxns if you're passing len(mempoolTxns) number of txns into the big COnnect above? A comment here would be good.

[diamondhands0]

It looks like you have a double-locking bug here. You lock on the outside of updateTransactionValidationStatus and within the function as well.

[diamondhands0]

Since this is a write-lock, I'm worried this could cause a bottleneck. Just to max efficiency, I would try and do something more like this if you can:

// 1. With no lock, or just with RLock, get all the txns you want to mark as validated.
txnsToMarkAsValidated = // whatever
mp.Lock()
// 2. Inside the lock blitz through these txns and call txn.SetValidated() on all of them in one go
mp.Unlock()

[diamondhands0]

It almost feels like you shouldn't even need to lock the whole mempool to update a single txn like this but if you're going to do it I'd just try to minimize the time spent holding the lock a little bit more, as I mentioned above.

[diamondhands0]

Great deletion.

[diamondhands0]

I could have sworn we deleted this in an earlier PR. Not sure why it's showing up here but hopefully I didn't mess anything up with my branch.

[diamondhands0]

I had to stare at this for a while and trace through things but I think it achieves everything we want:
0. We will not broadcast a txn to our peers until after it has passed validation, which is great.

1. We are waiting to bundle txns at regular intervals thanks to TransactionValidationRefreshIntervalMillis
2. If a user submits N txns to a single node in rapid succession, those should get bundled together IN ORDER thanks to the fact that the InvLis is an ordered list.

Do you see any reason why we wouldn't be achieving all of these properties with this approach? Also I would add all of this in a comment here to explain the full dynamics of what adding this line does.

[diamondhands0]

The logic in this function seems to implement almost 1:1 what you're doing in ConnectTransactions[WithLimit]. Since you have to do some flavor of connect all these transactions to a view, I would consider a change as follows:
Instead of having ConnectTransactions ON the view, move it to pos_mempool.go over here with a signature something like the following:

func ConnectTransactionsToViewWithLimit(oldView *UtxoView, txnsToConnect []*MempoolTxn, numTxnsLimit int, ignoreFailing bool, etc...)

Then have this function simply return a new view that has connected everything to a copy of oldView. And you can use that function here and in the place where you're currently using COnnectTransactions. The upshot of this approach is you avoid adding code to the view and, critically, you only do ONE connect per txn because you don't need to update the internal state of a view.

[diamondhands0]

Below this line you call removeTransactionNoLock(). Is that OK to do here given that you're not holding any kind of lock in the outer function? It feels like maybe you need to hold a mp.Lock() while you do it. Or call the RemoveTransaction function or something.