Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make CheckTx response indicate if a tx could never have been valid #7918

Closed
4 tasks
ValarDragon opened this issue Mar 23, 2021 · 9 comments
Closed
4 tasks
Assignees
Labels
community-call This issue is to be discussed during a community call stale for use by stalebot

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Mar 23, 2021

Protocol Change Proposal

Problem definition

Currently peers are never punished for sending you invalid transactions. (See here) This is because its possible that for many txs that are marked as bad, there is a possible application state in which they would be good. (E.g. a tx being invalid due to wrong sequence numbers) Two full nodes can have a divergence in whether a tx is valid either due to prior txs in their mempool or from simply being on different blocks.

However it is useful to mark peers as invalid, as it increases the complexity a dishonest node must do in other to attempt to attack the network. We can't black-box mark a peer as bad for sending a tx that may have been valid from one block ago, or in the next block. However we can safely mark them as bad and disconnect from them if they sent a tx that could never be valid. This would stop malicioous nodes from being able to spam random bytes along the mempool tx channel. Furthermore, it is conceivable that for some chains, the especially burdensome to verify txs are such that they can always be statelessly verified.

Proposal

We make a CheckTx response code to allow indicating that a tx would never be valid under any state. Then the mempool would mark a peer as bad if CheckTx returns this code.

Currently the semantics of a Check Tx code is:

  • 0 Passed CheckTx
  • >0 Failed CheckTx

The proposed new semantics are then:

  • 0 Passed CheckTx
  • 1 Failed CheckTx, and the mempool should reject the Tx.
  • >= 2 Failed CheckTx, the mempool should reject the Tx, and punish the peer who sent it.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@josef-widder
Copy link
Contributor

I am wondering about the punishing. What is the evidence data supposed to look like? Is there supposed to be a new evidence type that consist of the rejected transaction signed by the peer who sent it? Do peers sign individual transactions in the mempool?

@alexanderbez
Copy link
Contributor

ACK. I don't think there is any evidence @josef-widder. In the go implementation, upon receiving a CheckTx response with code >= 2, the mempool reactor would signal to the PeerManager to either: (1) ban the peer outright or (2) decrease their peer score.

@josef-widder
Copy link
Contributor

OK, so I guess I was just confused by the term "punish". There is actually no punishment happening then, just a more informed peer selection.

@github-actions github-actions bot added the stale for use by stalebot label Jul 16, 2021
@cmwaters cmwaters removed the stale for use by stalebot label Jul 22, 2021
@cmwaters cmwaters reopened this Jul 22, 2021
@cmwaters
Copy link
Contributor

Is this something we might still be interested in implementing @alexanderbez ?

@alexanderbez
Copy link
Contributor

Correct @josef-widder. @cmwaters, yes absolutely.

@ValarDragon
Copy link
Contributor Author

I honestly think this is huge, and am very happy to help support in PR review etc.

@jmalicevic
Copy link
Contributor

Is there a particular example of a transaction that could never have been valid? The reason I am asking is because I am trying to understand whether such behaviour is always a sign of malice?

We can ban peers that gossip transactions that keep failing CheckTx and this is a different case. But if we consider transactions that never could have been valid: can such a transaction ever be added into the mempool of an honest node? If not, then in addition of banning such peers, we could very well have this as proof of malice?

@jmalicevic
Copy link
Contributor

One example I can think of is a peer sending us too big transactions due to misconfiguration for example?

@thanethomson thanethomson added the community-call This issue is to be discussed during a community call label Nov 7, 2022
@lasarojc
Copy link
Contributor

Sort of sideways to this issue, but if adding a flag saying that it will always be invalid, would it be useful to also add one stating that it will always be valid and no recheck is needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-call This issue is to be discussed during a community call stale for use by stalebot
Projects
Status: Done/Merged
Development

No branches or pull requests

7 participants