rfc: Banning peers based on ResponseCheckTx

[jmalicevic]

This RFC captures the changes required to implement the proposals in #7918 , #2185, #6523. The gist of the document is that the mempool should be able to trigger baning of peers if they either send us transactions that fail too often, or transactions that fail due to a particular code that indicates that the application has determined a transaction to never be valid, in any state.

It closes #9546 and if accepted will be followed by implementing the proposed changes.

📖 Rendered

[sergio-mena]

As I understand it, transactions with ResponseCheckTx.code > 1 should never be removed from the cache

[jmalicevic]

That is something we need to agree on. If we do not expect to have many nodes sending us such transactions, we might save space by just removing it and not storing the peer info, because this peer is immediately banned and this should be a one-off occurence. That is also why the condition to remove the tx from the cache from your previous comments removes the transaction if this is the reason for failure.

If we do think this is better stored and never removed, then the condition from your comment should be as you suggested and not as it is currently written.

[sergio-mena]

Yeah, I think it'd be best to discuss this synchronously

[jmalicevic]

Just to summarize our in person discussion which will also lead to a clarification in the doc. The debate we are having is whether we want the never-valid transactions in the cache or not.

• Yes because this means we will never run checkTx on them again and we know it will never be valid, so we may want it to stay there forever.
• No because they are very rare, the peer is banned so they take up space in the cache when they don't occur that often

[sergio-mena]

As mentioned above, my thought on this part is that I can't to see a valid example of an App that locally passed CheckTx on a TX, and then fails at reCheckTx time with "> 1"

[jmalicevic]

No, we would not expect the transaction to fail it here after it passed it the first time. What I meant here is that we check whether the transaction was marked as invalid with a particular code when it was added, and if yes we ban all the peers that sent it. So we only leverage the fact that we anyways loop through all the transactions and do not apply the normal recheck logic (which applies to only valid transactions).

In my opinion this solution is a bad one, but I decided to list it here in case people feel differently.

[sergio-mena]

Maybe mention this is a non-breaking interface change

[williambanfield]

I disagree, I believe that in a certain sense this is breaking. We are giving meaning to something that applications may have already given meaning too.

[sergio-mena]

Yeah, I saw your comment above. I hadn't thought about that. I think you are right

[williambanfield]

Thanks so much for the great write-up! I would say that, in my mind, this is effectively bringing a mechanism of the PEX reactor into the mempool. That reactor has logic to mark a peer as bad if it sends malformed network addresses. In that context, the bytes of the message may contain a valid proto but the values in that proto message are invalid. In that case, they do not represent a legal network address. I believe that this mechanism should just mimic the logic of the PEX reactor but with the needed input of the application.

Since the contents of the Tx are meaningless from the standpoint of Tendermint, we have no idea if they are well formed and/or contain clearly bogus data so we must ask the application. If the application responds that the data is malformed or never valid for some other reason, we should ban the peer. This mimics what we do in the PEX reactor: when we see garbage data we ban the peer.

I think that the case of banning a peer if it frequently sends transactions to our node that could have been valid at some point, either in the past or in the future, is much much more subtle and, without strong justification, we should likely avoid solving entirely. If we need something we could consider a rate-limit for such peers to prevent our resources from being completely tied up by peers that are not actively malicious but that are also being unhelpful.

[williambanfield]

They thus propose
to introduce a special response code for CheckTx to indicate this behaviour, and ban the peers...

I would also note that this may be breaking for old applications. We have never reserved response codes as meaningful to Tendermint, so giving semantic meaning to some response code, such as '44 = ban the peer' may break older applications that had used 44 for some internal reason. It seems unlikely that this is the case but it is a concern worth taking into consideration.

[williambanfield]

Since our p2p layer implements a cryptographic handshake procedure, we can be certain of the identity of the sending peer. This means a malicious peer could not force us to ban a different peer.

[williambanfield]

It appears to me that that's what's being suggested by the different response codes, either =1 or >1 @sergio-mena

[williambanfield]

Also, I'm not seeing it in the Background section, is this behavior that we definitely need? I think that banning peers that send us transactions that absolutely could never have been valid is mostly what's being asked for. A mechanism for banning peers that often send currently invalid - but not never-valid -transactions likely cannot be defined within Tendermint so I'm wondering how much effort we should allocate to doing it?

[williambanfield]

Some of the difficulty here is that this would be an application-produced issue that would first be noticed by operators but that operators would be unable to reasonably address.

Is there a possibility of (accidentally) introducing a bug in CheckTx, which would cause (most of) the P2P to loose connectivity? How would that look like?

It seems to me that, of course this is the case. If the app always returns an erroneous 'never-valid' response code, then the p2p network will completely disconnect.

[williambanfield]

Yeah, supposing some overlay network, as a node I can only influence my direct connections. I can force a peer to disconnect from me forever if, say, I wanted to lower the number of ways it has of getting messages to the rest of the network. However, I could have already done that by just disconnecting or by dropping its messages.

[williambanfield]

I disagree, I believe that in a certain sense this is breaking. We are giving meaning to something that applications may have already given meaning too.

[cmwaters]

I concur with what you're saying @williambanfield but there are definitely non-breaking options such as simply adding a boolean field like txCouldNeverHaveBeenValid to ResponseCheckTx

[cmwaters]

Do we want to discuss banning by node ID and IP vs banning by IP only?

[cmwaters]

Another way of phrasing this is stateless checks vs stateful checks. Because check tx can be called by validators across different heights i.e. different states, stateful checks are non deterministic and thus should never result in the ban of a peer, however stateless checks should be deterministic for every honest node irrespective of the height and thus should lead to a ban.

[jmalicevic]

In general, CheckTx should be stateful , in the sense that it compares the current transaction against the latest state. The application is the one that defines whether the failure is something that happened due to a difference in states (height etc.) or is the failure a reason for banning. THese two distinctions cover the case where the first scenario happens very often, while the other is immediate reason for banning.

[cmwaters]

In general, CheckTx should be stateful

Not necessarily. The SDK only has one stateful check in the antehandler (that I know of) which is to check the nonce numbers everything else about it is stateless (It is essentially one large ValidateBasic function which mostly ensures that the bytes match a recognised protobuf message).

It doesn't make sense to spend too much time checking against state because you don't know whether that's the state that the transactions will eventually execute against so you will need to do all those stateful checks again.

CheckTx should be seen as an initial gatekeeper for transactions but not the final gatekeeper. The final gatekeeper is PrepareProposal and ProcessProposal.

[jmalicevic]

This is the source for my comment btw: https://github.com/tendermint/tendermint/blob/main/spec/abci/abci%2B%2B_methods.md#checktx . We might need to rephrase this if you feel that's needed.

[cmwaters]

We're kind of jumping ahead to the implementation here, but I would be for introducing a new field in ResponseCheckTx that defaults to not banning the peer rather than modifying the semantics of the existing Code field.

[lasarojc]

Suggested change

	that is being gossiped was valid or not (assumint that invalid transactions could become valid).
	that is being gossiped was valid or not (assuming that invalid transactions could become valid).

[lasarojc]

Suggested change

As it is the mempool **reactor** that has access to the p2p layer, not the actual mempool implementation, the peer banning function will most likely have to send the peer ID to a channel to inform the reactor of the fact that this peer should be banned. This would require adding a channel for communication into the `CListMempool` on construction, and adding a routine in the mempool ractor that waits on a response via this channel. However, the actual implementation of this is yet to be defined.

As it is the mempool **reactor** that has access to the p2p layer, not the actual mempool implementation, the peer banning function will most likely have to send the peer ID to a channel to inform the reactor of the fact that this peer should be banned. This would require adding a channel for communication into the `CListMempool` on construction, and adding a routine in the mempool reactor that waits on a response via this channel. However, the actual implementation of this is yet to be defined.

[lasarojc]

Should we also consider using bogus transactions (those which could never be valid) as evidence of misbehavior?

[adizere]

Appreciate how self-contained the doc is. Without much prior knowledge I can grok it!

[adizere]

For completeness, this is the exact config name and link to it:

https://github.com/cosmos/gaia/blob/5db8fcc9a229730f5115bed82d0f85b6db7184b4/contrib/testnets/test_platform/templates/app.toml#L8-L11

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ValarDragon]

I'm very supportive for banning for txs that could never be valid

I think banning being defined as blocking the IP is fine to me personally, you should be unable to spoof other IP's due to the secret connection handshake

I do not share cited concerns around backwards compatability. This feels like something a chain who attempts the upgrade on testnet will catch

[tac0turtle]

Definitely support this and echo backwards comparability concern from valardragon. I don't think this is a large concern. Any additions to checktx message are backwards compatible and can be introduced in a minor release