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

Don't allow undecodable bytes to be accepted in ProcessProposal #2663

Closed
4 tasks
cmwaters opened this issue Oct 11, 2023 · 3 comments · Fixed by #3006
Closed
4 tasks

Don't allow undecodable bytes to be accepted in ProcessProposal #2663

cmwaters opened this issue Oct 11, 2023 · 3 comments · Fixed by #3006
Assignees
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules
Milestone

Comments

@cmwaters
Copy link
Contributor

cmwaters commented Oct 11, 2023

Currently, if a tx as an array of bytes fails to be decoded, we skip over the tx but don't actually reject it:

if err != nil {
// we don't reject the block here because it is not a block validity
// rule that all transactions included in the block data are
// decodable
continue
}

This effectively means that a proposer is able to include arbitrary bytes in the block and leverage data availability without incurring any cost. If we plan to have a minimal fee market in place for v2, then part of this requires adding a new block validity rule that rejects any tx that is not decodable

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@cmwaters cmwaters added the enhancement New feature or request label Oct 11, 2023
@cmwaters cmwaters added this to the v2 milestone Oct 11, 2023
@cmwaters cmwaters removed the enhancement New feature or request label Oct 12, 2023
@evan-forbes evan-forbes added proposal item is not yet actionable and is suggesting a change that must first be agreed upon consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules and removed proposal item is not yet actionable and is suggesting a change that must first be agreed upon labels Oct 15, 2023
@evan-forbes
Copy link
Member

I think this is a good idea reguardless of implementing a mechanism to stop proposers from including 0 fee transactions

@evan-forbes
Copy link
Member

iirc, the reasoning for keeping this, was that we didn't explicitly discuss adding this block validity rule and its block validity rule that is not easily fraud provable (while also bounding fraud proof size to something smaller than the block)

@rootulp
Copy link
Collaborator

rootulp commented Jan 11, 2024

Interesting, even though we don't already reject proposals that include a non-decodable tx. We have a test case for it. The test case rejects the proposal block here with the error:

"normal tx at index 3 can not be appended after blob tx"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus breaking modifies block validity rules in a way that will break consensus unless all nodes update their rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants