consensus: check block parts don't exceed maximum block bytes by cmwaters · Pull Request #5431 · tendermint/tendermint

cmwaters · 2020-09-30T12:36:56Z

Description

Closes: #5423

codecov · 2020-09-30T12:43:16Z

Codecov Report

Merging #5431 into master will decrease coverage by 0.05%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #5431      +/-   ##
==========================================
- Coverage   61.40%   61.35%   -0.06%     
==========================================
  Files         259      259              
  Lines       23442    23452      +10     
==========================================
- Hits        14395    14389       -6     
- Misses       7588     7603      +15     
- Partials     1459     1460       +1

Impacted Files	Coverage Δ
types/part_set.go	`68.78% <66.66%> (-0.09%)`	⬇️
consensus/state.go	`68.25% <100.00%> (+0.11%)`	⬆️
consensus/reactor.go	`74.08% <0.00%> (-3.15%)`	⬇️
mempool/reactor.go	`82.57% <0.00%> (-2.28%)`	⬇️
blockchain/v0/pool.go	`78.59% <0.00%> (ø)`
statesync/syncer.go	`79.32% <0.00%> (+0.84%)`	⬆️
blockchain/v0/reactor.go	`62.56% <0.00%> (+0.98%)`	⬆️
statesync/snapshots.go	`93.27% <0.00%> (+1.68%)`	⬆️
p2p/switch.go	`68.32% <0.00%> (+2.48%)`	⬆️

consensus/state.go

melekes · 2020-09-30T13:24:21Z

types/part_set.go

 	count         uint32
+	// a count of the total size (in bytes). Used to ensure that the
+	// part set doesn't exceed the maximum block bytes
+	byteSize int64


it matches MaxBytes in the Block Params which is also int64. tbh it's still a relatively arbitrary number

tac0turtle · 2020-09-30T13:40:49Z

consensus/state.go

 	if err != nil {
 		return added, err
 	}
+	if cs.ProposalBlockParts.ByteSize() > cs.state.ConsensusParams.Block.MaxBytes {


Could the same thing be achieved by doing?

reader := io.LimitReader( cs.ProposalBlockParts.GetReader(), cs.state.ConsensusParams.Block.MaxBytes) bz, err := ioutil.ReadAll(reader)

Is this more performant?

I am not sure if its more performant but causes fewer changes and I believe mimics the previous behavior. I would have to check what amino was doing.

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

melekes · 2020-10-01T08:39:47Z

could u also cheer-pick 433bdf5 onto v0.34.0-rc5 branch?

erikgrinaker · 2020-10-01T09:48:10Z

types/part_set.go

 	count         uint32
+	// a count of the total size (in bytes). Used to ensure that the
+	// part set doesn't exceed the maximum block bytes
+	byteSize int64


It would've been even simpler to just calculate the size on the fly in ByteSize(), this isn't performance critical.

Yeah true but it's a relatively minor difference right?

Just less stuff that can go wrong :)

* docs: specify TM version in go tutorials (#5427) Closes #5425 * privval: allow passing options to NewSignerDialerEndpoint (#5434) Required for #5291 to set timeouts for remote signers. * config: set statesync.rpc_servers when generating config file (#5433) Required for #5291, to generate configuration files with state sync RPC servers. * consensus: check block parts don't exceed maximum block bytes (#5431) * ci: docker remvoe circleci and add github action (#5420) * privval: fix ping message encoding (#5441) Fixes #5371. * docs: revise ADR 56, documenting short term decision around amnesia evidence (#5440) * light: expand on errors and docs (#5443) * test: add end-to-end testing framework (#5435) Partial fix for #5291. For details, see [README.md](https://github.com/tendermint/tendermint/blob/erik/e2e-tests/test/e2e/README.md) and [RFC-001](https://github.com/tendermint/tendermint/blob/master/docs/rfc/rfc-001-end-to-end-testing.md). This only includes a single test case under `test/e2e/tests/`, as a proof of concept - additional test cases will be submitted separately. A randomized testnet generator will also be submitted separately, there a currently just a handful of static testnets under `test/e2e/networks/`. This will eventually replace the current P2P tests and run in CI. * changelog: add missing date to v0.33.5 release, fix indentation (#5454) I forgot to add the date when we cut 0.33.5. This fixes that. It also fixes a header indentation issue for 0.33.8. * test: add basic end-to-end test cases (#5450) Partial fix for #5291. This adds a basic set of test cases for core network invariants. Although small, it is sufficient to replace and extend the current set of P2P tests. Further test cases can be added later. * test: add GitHub action for end-to-end tests (#5452) Partial fix for #5291. * fix RPC blockresults reutrn (#5459) ## Description In blocks_results we use the proto definition of abciResponses: https://github.com/tendermint/tendermint/blob/2672b91ab099b8b02f3afabae4a0a745acd93c3f/rpc/core/blocks.go#L152-L155, this leads to the use of the proto definition of the pubkey which is an interface in go (oneof). The interface must be registered with the JSON encoder to have it work correctly. A clearer divide between proto types and native types is needed. Closes: #XXX * circleci: remove Gitian reproducible_builds job (#5462) * docs: fix links to adr 56 (#5464) ## Description fix broken link from a previous change * test: remove P2P tests (#5453) Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Erik Grinaker <erik@interchain.berlin> Co-authored-by: Callum Waters <cmwaters19@gmail.com> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Tess Rinearson <tess.rinearson@gmail.com>

* docs: specify TM version in go tutorials (#5427) Closes #5425 * privval: allow passing options to NewSignerDialerEndpoint (#5434) Required for #5291 to set timeouts for remote signers. * config: set statesync.rpc_servers when generating config file (#5433) Required for #5291, to generate configuration files with state sync RPC servers. * consensus: check block parts don't exceed maximum block bytes (#5431) * ci: docker remvoe circleci and add github action (#5420) * privval: fix ping message encoding (#5441) Fixes #5371. * docs: revise ADR 56, documenting short term decision around amnesia evidence (#5440) * light: expand on errors and docs (#5443) * makefile: config build-docs for branch and path prefix * update versions with new 0.33 branch Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: Erik Grinaker <erik@interchain.berlin> Co-authored-by: Callum Waters <cmwaters19@gmail.com> Co-authored-by: Marko <marbar3778@yahoo.com>

tac0turtle · 2020-10-09T15:30:35Z

types/part_set.go

 		parts:         parts,
 		partsBitArray: partsBitArray,
 		count:         total,
+		byteSize:      int64(len(data)),


i think this is culprit, you should be accounting for the parts not the entire struct

…mint#5431)

cmwaters added 2 commits September 29, 2020 15:01

create test for overflow block

4c67ec7

add byteSize to part set and check on receiving part

98ca9cb

cmwaters added the C:consensus Component: Consensus label Sep 30, 2020

linting and testing

947fdae

cmwaters marked this pull request as ready for review September 30, 2020 13:13

cmwaters requested review from ebuchman, erikgrinaker, melekes and tessr as code owners September 30, 2020 13:13

melekes reviewed Sep 30, 2020

View reviewed changes

tac0turtle reviewed Sep 30, 2020

View reviewed changes

melekes approved these changes Oct 1, 2020

View reviewed changes

cmwaters and others added 2 commits October 1, 2020 08:49

add space

1f31907

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Merge branch 'master' into callum/check-block-size

2e1814b

cmwaters merged commit 433bdf5 into master Oct 1, 2020

cmwaters deleted the callum/check-block-size branch October 1, 2020 07:59

cmwaters added a commit that referenced this pull request Oct 1, 2020

consensus: check block parts don't exceed maximum block bytes (#5431)

ca2b570

cmwaters mentioned this pull request Oct 1, 2020

consensus: block bytes into rc5 (#5431) #5436

Merged

erikgrinaker reviewed Oct 1, 2020

View reviewed changes

melekes pushed a commit that referenced this pull request Oct 9, 2020

consensus: check block parts don't exceed maximum block bytes (#5431)

b690b8c

tac0turtle reviewed Oct 9, 2020

View reviewed changes

kostko pushed a commit to oasisprotocol/tendermint that referenced this pull request Oct 12, 2020

consensus: check block parts don't exceed maximum block bytes (tender…

1bf14b1

…mint#5431)

kostko pushed a commit to oasisprotocol/tendermint that referenced this pull request Oct 12, 2020

consensus: check block parts don't exceed maximum block bytes (tender…

67bacc1

…mint#5431)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consensus: check block parts don't exceed maximum block bytes#5431

consensus: check block parts don't exceed maximum block bytes#5431
cmwaters merged 5 commits intomasterfrom
callum/check-block-size

cmwaters commented Sep 30, 2020

Uh oh!

codecov bot commented Sep 30, 2020 •

edited

Loading

Uh oh!

Uh oh!

melekes Sep 30, 2020

Uh oh!

cmwaters Sep 30, 2020

Uh oh!

tac0turtle Sep 30, 2020

Uh oh!

cmwaters Sep 30, 2020

Uh oh!

tac0turtle Sep 30, 2020

Uh oh!

melekes commented Oct 1, 2020

Uh oh!

erikgrinaker Oct 1, 2020

Uh oh!

cmwaters Oct 1, 2020

Uh oh!

erikgrinaker Oct 1, 2020

Uh oh!

tac0turtle Oct 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cmwaters commented Sep 30, 2020

Description

Uh oh!

codecov bot commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melekes commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Sep 30, 2020 •

edited

Loading