Skip to content

feat(consensus): additional sanity checks for the size of proposed blocks (backport #1408)#2140

Merged
melekes merged 10 commits intov0.37.xfrom
mergify/bp/v0.37.x/pr-1408
Feb 9, 2024
Merged

feat(consensus): additional sanity checks for the size of proposed blocks (backport #1408)#2140
melekes merged 10 commits intov0.37.xfrom
mergify/bp/v0.37.x/pr-1408

Conversation

@mergify
Copy link
Contributor

@mergify mergify bot commented Jan 26, 2024

This is an automatic backport of pull request #1408 done by Mergify.
Cherry-pick of 28ad4d2 has failed:

On branch mergify/bp/v0.37.x/pr-1408
Your branch is up to date with 'origin/v0.37.x'.

You are currently cherry-picking commit 28ad4d223.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   consensus/state.go
	modified:   crypto/merkle/proof.go
	modified:   evidence/pool_test.go
	modified:   state/execution_test.go
	modified:   types/part_set.go
	modified:   types/part_set_test.go

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   internal/consensus/errors.go
	deleted by us:   internal/consensus/state_test.go
	deleted by us:   internal/state/store_test.go
	deleted by us:   internal/store/store_test.go
	both modified:   types/event_bus_test.go

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

…ocks (#1408)

Hardens tests regarding the size of proposed blocks, namely:

- The byte size of a proposal block `Part` should be constant (`==
types.BlockPartSizeBytes`), except for the last part of a `PartSet` (`<=
types.BlockPartSizeBytes`)
- A valid `Proposal` should not enclose a `PartSet` enabling the
building of a `ProposalBlock` with size larger than the configured
`ConsensusParams.Block.MaxBytes`. Notice that building a `ProposalBlock`
larger than the allowed would fail in any case, but the proposed changes
also invalidate the associated `Proposal`.

---

#### PR checklist

- [x] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Andy Nogueira <me@andynogueira.dev>
(cherry picked from commit 28ad4d2)

# Conflicts:
#	internal/consensus/errors.go
#	internal/consensus/state_test.go
#	internal/state/store_test.go
#	internal/store/store_test.go
#	types/event_bus_test.go
@mergify mergify bot requested a review from a team as a code owner January 26, 2024 07:49
@mergify mergify bot added the conflicts label Jan 26, 2024
Daniel Cason and others added 6 commits January 26, 2024 10:25
…ocks (#1408)

Hardens tests regarding the size of proposed blocks, namely:

- The byte size of a proposal block `Part` should be constant (`==
types.BlockPartSizeBytes`), except for the last part of a `PartSet` (`<=
types.BlockPartSizeBytes`)
- A valid `Proposal` should not enclose a `PartSet` enabling the
building of a `ProposalBlock` with size larger than the configured
`ConsensusParams.Block.MaxBytes`. Notice that building a `ProposalBlock`
larger than the allowed would fail in any case, but the proposed changes
also invalidate the associated `Proposal`.

---

- [x] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Co-authored-by: Sergio Mena <sergio@informal.systems>
Co-authored-by: Andy Nogueira <me@andynogueira.dev>
Copy link

@cason cason left a comment

Choose a reason for hiding this comment

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

Changes in this backport are identical to the original PR, with the following exceptions:

  • we don't have consensus/errors.go in this branch, so the new error was added to consensus/state.go
  • fillStore is not on state/store_test.go in this branch
  • TestSaveBlockWithExtendedCommitPanicOnAbsentExtension and TestLoadBlockExtendedCommit are not present on store/store_test.go
  • TestPruningService are not on store/store_test.go in this branch
  • TestMain in store/store_test.go, not in main, had to be fixed

@sergio-mena
Copy link
Collaborator

Checked diff-of-diffs.
All good except the changes to TestLoadBlockPart. I see you didn't take them from the original PR. It looks like TestLoadBlockPart is still passing without those changes, but I think I left those changes to make the test case clearer, could you bring them to this backport?

@cason
Copy link

cason commented Jan 31, 2024

TestLoadBlockPart

Restored these changes in the last commit.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants