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

Slash block proposer polluting the state #8192

Closed
4 tasks
robert-zaremba opened this issue Dec 17, 2020 · 15 comments
Closed
4 tasks

Slash block proposer polluting the state #8192

robert-zaremba opened this issue Dec 17, 2020 · 15 comments

Comments

@robert-zaremba
Copy link
Collaborator

robert-zaremba commented Dec 17, 2020

Summary

In SDK we don't have a slashing logic for a validator putting rubbish transactions in a block.

Problem Definition

There are many types of invalid transaction. We can group them into 2 categories:

  • Category A: tx which don't pass the initial checks through CheckTx: unmarshaling (check if transaction is properly serialized), signatures.... without running the transaction.
  • Category B: tx is invalidated only during transaction execution (double spend, insufficient funds etc...).

Transactions from Category A are completely rubbish because nobody will pay for them. These transaction will pollute the blockchain (both the block space and long term storage) and the processing power of the whole network (validators will still need to process the block).

Transaction author will still need to pay a fee for processing an invalid transaction of Category B. These transaction won't make any effect except being recorded as a part of a block and fee charging. They also pollute a chain and at least someone is paying for them.

Related problem: voters don't see transactions when voting on a block.

In Tendermint, voters don't see the block transaction when voting on a proposed block. Hence they can't oppose to accept a block if the block is rubbish. However, if such block is committed, everyone can provide an evidence of a misbehave and an honest validator should include a slashing transaction in a block at some point.

Proposal

Including transactions of category A should be slashable. The block producer doesn't have any "on-chain" benefit from it and it consumes a space. Transactions for category B is a misbehave, but it's hard to judge if it should be slashable.

  1. Create evidence and slashing conditions for misbehave of category A.
  2. Research sound conditions for slashing misbehave of category B.

NOTE: if accepted in SDK, this should be configurable.

Further discussion

We can also consider the following events as a misbehave leading to slashing events :

  • empty (or x% empty) blocks (it's hard to prove if a block producer has valid transactions in a mempool)

References


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba
Copy link
Collaborator Author

@sunnya97 - could be related to you Tendermint 2.0 ideas.

@ebuchman
Copy link
Member

Yes Category A would be addressed by the "Process Proposal" stage of ABCI++ proposed by Sunny/Dev where txs would be executed before voting.

I'm not sure anything meaningful can or should be done about Category B - they're already paying fees so it shouldn't matter :)

@tac0turtle
Copy link
Member

Yes Category A would be addressed by the "Process Proposal" stage of ABCI++ proposed by Sunny/Dev where txs would be executed before voting.

This doesn't necessarily solve the issue. Preprocess can be used to aggregate the signatures for faster verification but txs being valid is not needed for this. Tx execution here would increase latency but the trade off being less to no invalid txs.

Note: if this trade off is willing to be made then this can be solved today with no changes to abci. Just make checktx check state validity of a transaction not only if it adheres to the msg format.

@robert-zaremba
Copy link
Collaborator Author

@marbar3778 what do you mean with "processing"? For category A we don't need to do transaction execution. Only basic checks, which are cheap:

  • is it possible to decoded it (is it properly marshaled or it's just rubbish bytes)
  • Does it have required signatures
  • are the signatures valid
  • is the message registered
  • maybe a bytes limit?

I assume this checks are already possible (or even done) through ABCI / Tendermint, correct? So the task is mostly about creating an evidence transaction and penalizing misbehaving validator.

@tac0turtle
Copy link
Member

tac0turtle commented Dec 19, 2020

@marbar3778 what do you mean with "processing"? For category A we don't need to do transaction execution. Only basic checks, which are cheap:

  • is it possible to decoded it (is it properly marshaled or it's just rubbish bytes)

  • Does it have required signatures

  • are the signatures valid

  • is the message registered

  • maybe a bytes limit?

This is already done in checktx is called, msg.validatbasic is called in the sdk. This doesn't prevent spam/invalid transactions from being included in blocks though

I assume this checks are already possible (or even done) through ABCI / Tendermint, correct? So the task is mostly about creating an evidence transaction and penalizing misbehaving validator

Heavily against this because the sdk allows invalid transactions. This should be changed on the sdk's if you want to add evidence otherwise everyone will get slashed

@robert-zaremba
Copy link
Collaborator Author

This issue is in SDK ;). We can make it configurable - if someone is OK with keeping rubbish in a block then s/he will set that evidence option to false. Why you wrote that everyone will get slashed? We will penalize only a validator who includes transactions which are of category A.

@alexanderbez
Copy link
Contributor

In ABCI 1.0, if all nodes execute CheckTx as part of process proposal, then that does solve category A @marbar3778. I don't think we should be concerned about category B issues - that's why we have fees.

@tac0turtle
Copy link
Member

Why you wrote that everyone will get slashed? We will penalize only a validator who includes transactions which are of category A.

Meant that in the current design of Tendermint every validator would get slashed at a certain point in time because there isn't a way to avoid this. I thought category A txs is what checktx does?

In ABCI 1.0, if all nodes execute CheckTx as part of process proposal, then that does solve category A @marbar3778. I don't think we should be concerned about category B issues - that's why we have fees.

huh? I dont believe checktx has anything to do with process proposal. Process proposal would just send what's in the block (txs, evidence, etc..) to the app and the app would decide to vote for it or not.

@robert-zaremba
Copy link
Collaborator Author

Correct me if I'm wrong (I didn't check in the codebase):

  1. During the proposal, a validator MAY run CheckTx
  2. During voting, validators DON'T check CheckTx
  3. During voting, validators MAY vote for a block containing a Tx which violates CheckTx (because they don't check it by default)

@sunnya97
Copy link
Member

@robert-zaremba that is correct.

In ABCI 1.0, if all nodes execute CheckTx as part of process proposal, then that does solve category A

Yep. One thing that I think is making this unclear is that we're using the terms CheckTx and antehandler interchangeably because currently, the CheckTx just runs the antehandler. However, they're not the same thing. The proper thing is that "with ABCI++, all nodes should run the antehandlers as part of process proposal".

I'm not sure anything meaningful can or should be done about Category B - they're already paying fees so it shouldn't matter :)

Yep trying to prevent category B txs doesn't work. It would open up spam attacks.


Just want to note that slashing for inclusion of txs that fail checkTx doesn't work right now. During CheckTx, the SDK's ante handler is run. During DeliverTx, each tx is run in order, first the ante handler and then the rest of the tx execution.

Problem is that some txs that passed antehandler during checktx, might fail during delivertx, because they were affected by the execution of a previous tx.

The obvious example is that if AddrA has two txs in this block, tx1 and tx2. It needs to pay fees for both txs, and it has the balance to do both of them, so it passes checktxs. But now if during the DeliverTx of tx1, it sends the entire balance of the account to a new address, it will fail the tx2's DeliverTx antehandler.

What we will want to do is execute all the antehandlers of all txs in order first, and then the rest of the execution. This can take the form of executing the antehandlers immediately as part of ProcessProposal. Or it can be done during DeliverBlock.

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 22, 2020

Yep. One thing that I think is making this unclear is that we're using the terms CheckTx and antehandler interchangeably because currently, the CheckTx just runs the antehandler. However, they're not the same thing. The proper thing is that "with ABCI++, all nodes should run the antehandlers as part of process proposal".

Yeah, exactly. This is what @ebuchman and I were trying to get at.

huh? I dont believe checktx has anything to do with process proposal. Process proposal would just send what's in the block (txs, evidence, etc..) to the app and the app would decide to vote for it or not.

That is the entire point -- all validators execute the necessary state machine steps in order to determine the validity of txs during this phase. If any tx is deemed invalid (i.e. ante-handler fails), then honest validators won't vote on the block in question.

@robert-zaremba
Copy link
Collaborator Author

So, just to wrap up, can anyone confirm that currently events of Category A are now possible to detect during the block proposal phase (we don't need to change tendermint code, because it already calls CheckTx), and we can make a slashing evidence for it?

@alexanderbez
Copy link
Contributor

Yes, you can detect events from category A. I also suppose a node could broadcast evidence of a proposer including a faulty tx in a block.

@tac0turtle
Copy link
Member

close in favour of #11965

@robert-zaremba
Copy link
Collaborator Author

Shouldn't we close the other issue and keep this open? Usually we keep the earlier issue or the one with more context. Here this one is earlier and has more context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants