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

Docs : ADR-xxx Proposer Slashing #11968

Closed
wants to merge 3 commits into from
Closed

Conversation

zmanian
Copy link
Member

@zmanian zmanian commented May 16, 2022

ADR to modify the slashing module to slash proposers for creating blocks that misuse the network's resources.

@zmanian zmanian requested a review from a team as a code owner May 16, 2022 12:06
@zmanian zmanian changed the title Docs : ADR-xxx Docs : ADR-xxx Proposer Slashing May 16, 2022
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general gist of this is good. Do you mind if I beef this up a bit and commit directly to your branch?

@avahowell
Copy link

How does this integrate with ABCI++? ABCI++ grants the ability for the state machine to determine the validity of a proposal before it is comitted:

https://github.com/tendermint/spec/blob/master/rfc/004-abci%2B%2B.md

@alexanderbez
Copy link
Contributor

How does this integrate with ABCI++? ABCI++ grants the ability for the state machine to determine the validity of a proposal before it is comitted:

https://github.com/tendermint/spec/blob/master/rfc/004-abci%2B%2B.md

That's a great question! Does each process' (validator) have to yield the same result of pre-process proposal, or just the proposer process?

@amaury1093 amaury1093 self-assigned this May 18, 2022
@mattdf
Copy link

mattdf commented May 19, 2022

This ADR should become standard practice on all Cosmos chains that have Turing-Complete execution environments and is really needed for the SDK to become a robust platform for VM execution. Not having the proposer be accountable to the resources their blocks actually use vs what they declare leaves open too many griefing vectors. Even with deferred execution, the proposer should always be able to deterministically pre-execute the block such that the outcome of the execution won't change once the block is committed.

In the ante handler, we should track if the total gas wanted has exceeded the max gas per block and fail any txs after that and slash the proposer. This should be done with a new decorator.

### Slashing
The slashing parameter should slash once per infraction. The jail should remove the validator from the validator set for a fixed time that is not per infraction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to jail here, or should it be a slash only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend jail and also recommend comparing a default osmo node to "some others" results may be surprising.

Copy link
Contributor

@faddat faddat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strong approval

@faddat
Copy link
Contributor

faddat commented Jun 9, 2022

@mattdf I think @zmanian's adr....

is applicable beyond Terra and I suspect is presently applicable on Osmosis. Per our studies, sans naming names, there are some surprising differences, that seem to matter.

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Some sections are missing (eg status). Let's update to follow the template
  • Consequences should be broken down into positive, neutral, negative
  • I think we should define default values for slashing and jailing


## Abstract

Block Proposers can construct blocks at their discretion but they are free to fill blocks with invalid transactions or exceed the block gas limit. This
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last sentence is no finished.

2. Submitting transactions with invalid signatures
3. Creating a block where the total of Gas_wanted > max gas for a block.

If during block processing, the application track these events and slash the block producer.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If during block processing, the application track these events and slash the block producer.
During block processing, the application track must track events and slash the block producer.


1. Submitting transactions with invalid sequence numbers
2. Submitting transactions with invalid signatures
3. Creating a block where the total of Gas_wanted > max gas for a block.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also could add:

  • including malformed transaction
  • wrong chain_id etc...

@robert-zaremba
Copy link
Collaborator

Related to / Solves: #8192

@alexanderbez alexanderbez marked this pull request as draft July 1, 2022 16:30
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot added the stale label Aug 16, 2022
@github-actions github-actions bot closed this Aug 22, 2022
@tac0turtle tac0turtle deleted the zaki-adr-xxx-proposer-slashing branch February 13, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants