mirrors/tendermint
1
0
Fork 0
mirror of https://github.com/tendermint/tendermint.git synced 2026-09-21 07:24:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

Applied PR comments and discussion feedback

Browse Source
This commit is contained in:
Jasmina Malicevic
2022-11-11 12:01:05 +01:00
parent c3fde54e2a
commit a5ce4ac589
1 changed files with 141 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/rfc/rfc-026-p2p-bad-peers-checktx.md
+141 -84
View File
@@ -1,13 +1,18 @@
# RFC 26: Banning peers based on ResponseCheckTx
## Changelog
- Nov 4, 2022: Initial draft (jmalicevic)
- Nov 8, 2022: Updated draft (jmalicevic)
- Nov 4, 2022: Initial draft (jmalicevic)
- Nov 8, 2022: Updated draft (jmalicevic)
- Nov 11, 2022: Updated based on PR comments (jmalicevic)
## Abstract
Before adding a transaction to its mempool, a node runs `CheckTx` on it. `CheckTx` returns a non-zero code
in case the transaction fails an application specific check.
In Tendermint, nodes receive transactions either from external clients via RPC,
or from their peers via p2p. Upon receiving a transaction, a node runs `CheckTx` on it. This is
an application specific check whose return code with a zero value indicates the transaction
has passed this check, and can be added into the mempool. Any non-zero code indicates the transaction
is not valid. Thus, the main role of `CheckTx` is to, as early as possible, prevent invalid transactions
from entering the mempool.
Note that there are valid scenarios in which a transaction will not
pass this check (including nodes getting different transactions at different times, meaning some
@@ -44,11 +49,11 @@ this behaviour and explaining their needs.
- Celestia: [blacklisting peers that repeatedly send bad tx](https://github.com/celestiaorg/celestia-core/issues/867)
and investigating how [Tendermint treats large Txs](https://github.com/celestiaorg/celestia-core/issues/243)
- BigChainDb: [Handling proposers who propose bad blocks](https://github.com/bigchaindb/BEPs/issues/84)
- IBC relayer: Simulate transactions in the mempool and detect whether they could ever have been valid to
- IBC-go: Simulate transactions in the mempool and detect whether they could ever have been valid to
[avoid overloading the p2p/mempool system](https://github.com/cosmos/ibc-go/issues/853#issuecomment-1032211020)
(ToDo check with Adi what is this about, what are valid reasons this can happen and when should we disconnect)
(relevant code [here](https://github.com/cosmos/ibc-go/blob/a0e59b8e7a2e1305b7b168962e20516ca8c98fad/modules/core/ante/ante.go#L23))
- From Adi: Nodes allow transactions with a wrong `minGas` transaction and gossip them, and other nodes keep rejecting them. (Problem seen with relayers)
- IBC relayers: Nodes allow transactions with a wrong `minGas` transaction and gossip them, and other nodes keep rejecting them. (Problem seen with relayers)
### Current state of mempool/p2p interaction
@@ -85,29 +90,32 @@ If this feature is to be implemented we need to clearly define the following:
1. A peer can be simply disconnected from.
2. Peer is disconnected from and banned.
3. Conditions for the peer to be banned.
2. If `CheckTx` signals a peer should be banned, how do we know which peer(s) to ban?
3. Are there possible attack scenarios by allowing this.
2. If `CheckTx` signals a peer should be banned, retrieve the ID of peers to ban.
3. Are there possible attack scenarios or unexpected behaviours by allowing this.
Any further mentions of `banning` will be agnostic to the actual way banning is implemented by the p2p layer.
### 1. What does banning a peer mean
Tendermint recognizes that peers can accept transactions into their mempool as valid but then when the state changes, they can become invalid. There are also transactions that are received that could never have been valid (for examle due to misconfiguration on one node).
Tendermint recognizes that peers can accept transactions into their mempool as valid but then when the state changes, they can become invalid.
There are also transactions that are received that could never have been valid (for examle due to misconfiguration on one node).
We thus differentiate two scenarios - a) where `CheckTx` fails due to reasons already
known and b) where `CheckTx` deems a transactions could never have been valid.
known and b) where `CheckTx` deems a transaction could never have been valid.
For the sake of simplicity, in the remainder of the text we will distinguish the failures due to a) as failures
signaled with `ResponseCheckTx.code = 1` and the failures described in b), failures with `ResponseCheckTx.code > 1`.
For the sake of simplicity , in the remainder of the text we will distinguish the failures due to a) as failures
signaled with `ResponseCheckTx.code = 1` and the failures described in b), failures with `ResponseCheckTx.code > 1`, even though
the way we actually mark them in the end might differ.
For a), a peer sends transactions that **repeatedly** fail CheckTx with `ResponseCheckTx.code = 1`, and is banned or disconnected from to avoid this. In this case we need to define what repeatedly means.
For a), a peer sends transactions that **repeatedly** fail CheckTx with `ResponseCheckTx.code = 1`, and is banned or disconnected from to avoid this.
In this case we need to define what repeatedly means.
For b) we need to understand what is the potential reason a transaction could never have been valid on one node, but passes `CheckTx` on another node.
We need to understand all the possible scenarios in which this can happen:
1. What happens if a node is misconfigured and allows, for example, very large transactions into the mempool.
This node would then gossip these transactions and they would always fail on other nodes.
Is this a scenario where we want nodes to disconnect from this peer and ban it without this peer being neccessarily malicious?
Is this a scenario where we want nodes to disconnect from this peer and ban it but do not consider it malicious?
2. Are all other reasons for this to happen sign of malicious behaviour where a node explicitly lies? How can `CheckTx` pass on a valid node, but fail on another valid node with a `ResponseCheckTx.code > 1`?
If such behaviour is only possible when a peer is malicious, should this peer be punished or banned forever? Note that
we cannot know whether a node is a validator in order for it to be punished. Gossiping this behaviour to other peers pro-actively
@@ -116,29 +124,42 @@ also entails a different set of problems with it - how do we know we can trust p
For now, we will disconnect and ban the peer regardless of the exact reason a transaction
is considered to never be valid.
#### **Banning in case of ResponseCheckTx.code = 1**
#### **Banning for frequent CheckTx failures**
If a node sends transactions that fail `CheckTx` with a response code of 1, there might be a valid reason for this behaviour.
A peer should not be banned the first time this happens, but rather if this happens a predefined number
of times (`numFailures`) within a time interval (`lastFailure`). This time interval should be reset every `failureResetInterval`ms.
If a node sends transactions that fail `CheckTx` but could be valid at some point, a peer should not be banned the first time this happens.
Only if this happens frequently enough should this be considered as spam. To define this behaviour we keep track how many times (`numFailures`) a peer
sent us invalid transactions within a time interval (`lastFailure`). This time interval should be reset every `failureResetInterval`ms.
For each peer, we should have a separate `numFailures` and `lastFailure` variable. There is no need to have one per transaction. Whenever a transaction fails, if the `now - lastFailure <= failureResetInterval`, then we increment the `numFailures` for this particular peer and set the `lastFailure` to `now`. Otherwise, we set `lastFailure` to `now` but do not increment the `numFailures` variable. Once the value for `numFailures` for a peer reaches `maxAllowedFailures`, the peer is disconnected from and banned.
For each peer, we should have a separate `numFailures` and `lastFailure` variable. There is no need to have one per transaction.
Whenever a transaction fails, if the `now - lastFailure <= failureResetInterval`, we increment the `numFailures` for this particular peer and set the `lastFailure` to `now`.
Otherwise, we set `lastFailure` to `now` and set `numFailures` to 1.
Once the value for `numFailures` for a peer reaches `maxAllowedFailures`, the peer is disconnected from and banned.
The reason for this logic is as follows: We deem it acceptable if every now and then a peer sends us an invalid transaction. But if this happens very frequently, then this behaviour can be considered as spamming and we want to disconnect from the peer. The actual values of these parameter are yet to be defined and should be based on the time it usually takes for a transacation to travel between two nodes, the time for a block to be agreed on and committed. [ToDo] What other factors can impact their value?
The reason for this logic is as follows: We deem it acceptable if every now and then a peer sends us an invalid transaction.
But if this happens very frequently, then this behaviour can be considered as spamming and we want to disconnect from the peer.
**Discussion**
The problem with supporting this scenario is the definition of the above mentioned parameters. It is very hard to estimate, at the Tendermint level, what these parameters should be. A possible solution is
to allow the application to set these parameters. What is unclear, how will the application know that these parameters are not well set if, due to a bug or network problems, transactions start to fail?
The network could end up with all nodes banning everyone. How would an application developer know to debug this, what to look for?
A possible solution is to ban peers temporarily. In addition to the question on how long is temporarily, setting specific time limits for banning on a peer basis
is currently not supported by the p2p layer.
*Banning a peer in case of duplicate transactions*
Currently, a peer can send the same valid (or invalid) transaction [multiple times](https://github.com/tendermint/tendermint/blob/ff0f98892f24aac11e46aeff2b6d2c0ad816701a/mempool/v0/clist_mempool.go#L247). Peers do not
gossip transactions to peers that have sent them that same transaction. But there is no check on whether
a node has already sent the same transaction to this peer before. There is also no check whether the transaction
that is being gossiped was valid or not (assumint that invalid transactions could become valid).
that is being gossiped is currently valid or not (assumint that invalid transactions could become valid).
The transaction broadcast logic simply loops through the mempool and tries to send the transactions currently in the pool.
If we want to ban peers based on duplicate transactions, we should either add additional checks for the cases above, or
not ban peers for this behaviour at the moment. It would be useful to gather metrics on how often a peer gossips the same
transaction and whether this is cause of significant traffic.
#### **Banning in case of ResponseCheckTx.code > 1**
#### **Banning for sending *never-valid* transactions**
If a transaction fails since it could never have been valid, `CheckTx` returns a `ResponseCheckTx.code`
value greater than 1. In this case, the peer should be disconnected from and banned immediately without keeping count on how often
@@ -158,6 +179,8 @@ a node saves the peer ID of the peer(s) that have sent it. As each peer had to h
run `CheckTx` on this transaction before adding it to its own mempool, we can assume this peer
can be held accountable for the validity of transactions it gossips. Invalid transactions are kept only in the mempool
cache and thus not gossiped.
As nodes have to complete a cryptographic handshake at the p2p layer, Tendermint guarantees that a malicious peer
cannot lie about who the sender of the transaction is.
*Transactions received from users*
@@ -172,25 +195,48 @@ While an attack by simply banning peers on failing `CheckTx` is hard to imagine,
Should we keep transactions that could never have been valid in the cache? Assuming that receiving such transactions is rare, and the peer that sent them is banned, do we need to occupy space in the mempool cache with these transactions?
- What if nodes run different versions of Tendermint and banning is not supported in one of the versions?
- Reserving response codes can be problematic for existing applications that may have reserved these codes for internal purposes withtou being aware that this causes a ban now.
## Implementation considerations
**Indicating a new type of `CheckTx` failure**
The initial proposal is to reserve a special response code to indicate that the transaction could never have been valid.
Due to concerns of this being a breaking change for applications that have already reserved this code for internal
purposes, there is an alternative implementation: expanding `ResponseCheckTx` with an additional field.
This field `neverValidTx` would be `false` by default. If a transaction could never have been valid,
in addition to indicating this with a non-zero response code from `CheckTx`, the application would set this field value.
**Adding support for peer banning**
When a transaction fails `CheckTx`, it is not stored in the mempool but **can** be stored in the cache. If it is in the cache, it cannot be resubmitted again (as it will be discovered in the cache and not checked again). These two scenarios require a different implementation of banning in case `CheckTx` failed.
In both cases we need to keep track of the peers that sent invalid transactions. If invalid transactions are cached, we also need to keep track of the `CheckTx` response code for each transaction. Currently the `ResponseCheckTx` code is checked in `resCbFirstTime` of the mempool. If invalid transactions are kept in the cache, the check is ran only when a transaction is
In both cases we need to keep track of the peers that sent invalid transactions. If invalid transactions are cached,
we also need to keep track of the `CheckTx` response code for each transaction. Currently the `ResponseCheckTx` code is checked in `resCbFirstTime` of the mempool.
If invalid transactions are kept in the cache, the check is ran only when a transaction is
seen for the first time. Afterwards, the transaction is cached, to avoid running `CheckTx` on transactions already checked.
Thus when a transaction is received from a peer, if it is in the cache,
`CheckTx` is not ran again, but the peers' ID is addded to the list of peers who sent this particular transaction.
These transactions are rechecked once a block is committed to verify that they are still valid.
If invalid transactions are not kept in the cache, they can be resubmitted multiple times, and `CheckTx` will be executed on them upon submission therefore we do not need to remember the previous response codes for these transactions.
If invalid transactions are not kept in the cache, they can be resubmitted multiple times, and `CheckTx` will be executed on them upon submission.
Therefore we do not need to remember the previous response codes for these transactions.
Currently there are two ways to implement peer banning based on the result of `CheckTx`:
In summary, if we want to support banning peers based on the frequency with which they submit invalid transactions, we need to have **additional datastructures**:
1. One to keep track of past invalid transactions
2. A datastructure to differentiate between valid and invalid *cached* transactions. If the `KeepInvalidTxsInCache` configuration parameter is not set, this datastructure
is not needed.
1. Introduce banning when transactions start to be processed
We propose two ways to implement peer banning based on the result of `CheckTx`:
1. Introduce banning when transactions are received
2. Adapt the recheck logic to support this
**Peer banning when transactions are received**
If a transaction fails `CheckTx` the [first time it is seen](https://github.com/tendermint/tendermint/blob/ff0f98892f24aac11e46aeff2b6d2c0ad816701a/mempool/v0/clist_mempool.go#L409), the peer can be banned right there:
@@ -198,42 +244,50 @@ If a transaction fails `CheckTx` the [first time it is seen](https://github.com/
>>mempool/v0/clist_mempool.go#L409
```golang
if (r.CheckTx.Code == abci.CodeTypeOK) && postCheckErr == nil {
// Check Tx passed
} else {
// ignore bad transaction
mem.logger.Debug(
"rejected bad transaction",
"tx", types.Tx(tx).Hash(),
"peerID", peerP2PID,
"res", r,
"err", postCheckErr,
)
mem.metrics.FailedTxs.Add(1)
if (r.CheckTx.Code == abci.CodeTypeOK) && postCheckErr == nil {
// Check Tx passed
} else {
// ignore bad transaction
mem.logger.Debug(
"rejected bad transaction",
"tx", types.Tx(tx).Hash(),
"peerID", peerP2PID,
"res", r,
"err", postCheckErr,
)
mem.metrics.FailedTxs.Add(1)
mem.banPeer(peerP2PID)
mem.banPeer(peerP2PID)
if !mem.config.KeepInvalidTxsInCache {
// remove from cache (it might be good later)
mem.cache.Remove(tx)
} else {
// If transactins stay in the cache, remember they failed
mem.cache.invalidCachedTx.Store(tx.Key(), true)
}
}
if !mem.config.KeepInvalidTxsInCache {
// remove from cache (it might be good later)
mem.cache.Remove(tx)
} else {
// If transactins stay in the cache, remember they failed
mem.cache.invalidCachedTx.Store(tx.Key(), true)
}
}
```
The `KeepInvalidTxsInCache` configuration parameter defines whether an invalid transaction stays in cache.
Currently, even transactions that never could have been valid are kept in the cache. This can be mitigated by
expanding the condititon in the code above with:
The `KeepInvalidTxsInCache` configuration parameter defines whether an invalid transaction stays in cache. For *never-valid*
transactions, we could apply a different approach based on what we deem to be the bigger gain:
- As we do not expect to receive frequently and from many peers, and we ban the peer that sent it immediately, we do not store it in the cache to save space. This would mean
that if we did see it again, we'd ran `CheckTx` on it again.
```golang
if !mem.config.KeepInvalidTxsInCache || r.CheckTx.Code == abci.NeverValid {
// remove from cache (it might be good later)
// remove from cache (it might be good later or is never valid, we'll most likely don't see it again)
mem.cache.Remove(tx)
}
```
- We do keep it in the cache as long as possible to avoid running `CheckTX` on it because we know, for sure, that it will never be valid. As it is rare enough, it
might not take that much space. In this case though, as we ban the sending peer immediately, we can save space by not storing peer information for this transaction.
The question is which one is more costly, doing `CheckTx` more then once, or keeping an extra entry in the cache?
As said, this code will [never be executed](https://github.com/tendermint/tendermint/blob/ff0f98892f24aac11e46aeff2b6d2c0ad816701a/mempool/v0/clist_mempool.go#L239) for transactions whose signature is found
in the cache.
Instead of remembering the cached transactions, we could have had a valid/invalit bit per transaction within the cache. As transactions themselves do not
@@ -266,47 +320,47 @@ type CListMempool struct {
```golang
if !mem.cache.Push(tx) { // if the transaction already exists in the cache
// Record a new sender for a tx we've already seen.
// Note it's possible a tx is still in the cache but no longer in the mempool
// (eg. after committing a block, txs are removed from mempool but not cache),
// so we only record the sender for txs still in the mempool.
if e, ok := mem.txsMap.Load(tx.Key()); ok {
memTx := e.(*clist.CElement).Value.(*mempoolTx)
memTx.senders.LoadOrStore(txInfo.SenderID, true)
// TODO: consider punishing peer for dups,
// its non-trivial since invalid txs can become valid,
// but they can spam the same tx with little cost to them atm.
}
// Record a new sender for a tx we've already seen.
// Note it's possible a tx is still in the cache but no longer in the mempool
// (eg. after committing a block, txs are removed from mempool but not cache),
// so we only record the sender for txs still in the mempool.
if e, ok := mem.txsMap.Load(tx.Key()); ok {
memTx := e.(*clist.CElement).Value.(*mempoolTx)
memTx.senders.LoadOrStore(txInfo.SenderID, true)
// TODO: consider punishing peer for dups,
// its non-trivial since invalid txs can become valid,
// but they can spam the same tx with little cost to them atm.
}
// If transaction was invalid, we need to remember the peer information
if _, ok := mem.cache.invalidCachedTx.Load(tx.Key); ok {
mem.banPeer(peerID)
}
return mempool.ErrTxInCache
}
// If transaction was invalid, we need to remember the peer information
if _, ok := mem.cache.invalidCachedTx.Load(tx.Key); ok {
mem.banPeer(peerID)
}
return mempool.ErrTxInCache
}
```
```golang
func (mem* ClistMempool) banPeer(peerID NodeID) {
numFails := 0
// Check whether this peer has sent us transactions that fail
if val, ok := mem.peerFailureMap[peerID]; ok {
lastFailureT := val.lastFailure
numFails = val.numFails
// if the failure was recent enough, update the number of failures and
// ban peer if applicable
if time.Since(lastFailureT) <= failureResetInterval {
if numFails == maxAllowedFailures - 1 {
// Send Ban request to p2p
}
}
func (mem* ClistMempool) banPeer(peerID NodeID) {
numFails := 0
// Check whether this peer has sent us transactions that fail
if val, ok := mem.peerFailureMap[peerID]; ok {
lastFailureT := val.lastFailure
numFails = val.numFails
// if the failure was recent enough, update the number of failures and
// ban peer if applicable
if time.Since(lastFailureT) <= failureResetInterval {
if numFails == maxAllowedFailures - 1 {
// Send Ban request to p2p
}
// Update the time of the last failure
mem.peerFailureMap[peerID] = { time.Now(), numFailures + 1}
}
}
// Update the time of the last failure
mem.peerFailureMap[peerID] = { time.Now(), numFailures + 1}
}
```
If transactions with `ResponseCheckTx.code > 1` are not deleted from the cache and we want to ban them on the first offence,
we can skip the second `if` in the code above but call `banPeer` immediately at the end of the function. The function `banPeer` should simply forward the `peerID` to the p2p layer. In fact, we do not need to store any information on this peer as the node will remove it from its peer set.
@@ -327,6 +381,8 @@ us transactions the entire time between two blocks.
- If we want to keep track of when peers sent us a traansaction and punish them only if the misbehaviour happens
frequently enough, this approach makes it hard to keep track of when exactly was a transaction submitted.
- Rechecking if optional and node operators can disable it.
- Furthermore, rechecking is a node local configuration parameter. This means that, some nodes might be performing this check,
while others will be unaware of this.
On the plus side this would avoid adding new logic to the mempool caching mechanism and keeping additional information about
transaction validity. But we would still have to keep the information on peers and the frequency at which they send us bad transactions.
@@ -346,6 +402,7 @@ Those checks, have been mentioned in the user issues described at the beginning
- Transaction size
- Proto checks
- Receiving unknown messages via the mempool channel
The previous code snippets do not incroporate these in peer banning. If we adopt those as valid reasons for banning, we should put the corresponding logic in place.
@@ -358,7 +415,7 @@ The previous code snippets do not incroporate these in peer banning. If we adopt
### Impact on ABCI
- Introduction of new response codes for CheckTx
- Introduction of new response codes for CheckTx. As previously noted, this might break existing applications if they reserved codes for internal purposes.
- Altering the specification to reflect this change
### References