Add Tendermint max block size param to genesis config file #843

sug0 · 2022-11-30T09:47:50Z

We allow validators to configure the max transaction batch size at genesis (up to 90 MiB worth of tx data). Tendermint is configured with a hard-coded value of 100 MiB per serialized block; this is fine, because we enforce tx data size on an application basis, with the block space allocator abstraction.

NOTES: https://hackmd.io/DJVZdogGSuKjw3fjVKv7ag?view

TODO: Add maximum gas cap on blocks

apps/src/lib/config/genesis.rs

james-chf

lgtm though I think we should mark out Tendermint consensus params in their own config struct (either in this PR, or some later one). This PR should be targeting main

apps/src/lib/config/genesis.rs

sug0 · 2022-12-08T21:30:38Z

@james-chf can't target main yet, because the block space allocator is a hard dependency of this PR. without it, blocks are always configured to be 100 MiB in size, which isn't ideal. I guess I forgot to mention that in the description 😅 btw, I'm still missing the evidence config. will add that tomorrow

sug0 · 2022-12-09T09:15:00Z

so, actually, it looks like we do not configure evidence params right now, and they seem to be using 1 MiB of space at most, by default. that should be plenty of space, so I'm not going to touch that config.

james-chf · 2022-12-09T14:58:15Z

@james-chf can't target main yet, because the block space allocator is a hard dependency of this PR. without it, blocks are always configured to be 100 MiB in size, which isn't ideal. I guess I forgot to mention that in the description 😅 btw, I'm still missing the evidence config. will add that tomorrow

Wouldn't it just be setting a max cap of 100 MiB rather than having blocks always be 100 MiB in size? (I would have thought applying this PR to main, there would be no functional change, except the max possible block size would be different, but I could be wrong). In that case, the changes in this PR don't look like they depend on the block space allocator PRs.

…size-genesis-param

sug0 · 2022-12-12T09:04:49Z

@james-chf can't target main yet, because the block space allocator is a hard dependency of this PR. without it, blocks are always configured to be 100 MiB in size, which isn't ideal. I guess I forgot to mention that in the description 😅 btw, I'm still missing the evidence config. will add that tomorrow

Wouldn't it just be setting a max cap of 100 MiB rather than having blocks always be 100 MiB in size? (I would have thought applying this PR to main, there would be no functional change, except the max possible block size would be different, but I could be wrong). In that case, the changes in this PR don't look like they depend on the block space allocator PRs.

yeah, it'd just set a max block size of 100 MiB, but I'm not sure if it's a good idea to include this PR in main without a way to throttle the num of txs included in a block

batconjurer · 2022-12-12T09:38:56Z

core/src/ledger/parameters/mod.rs

@@ -111,6 +115,16 @@ impl Parameters {
            pos_inflation_amount,
        } = self;

+        // write max proposal bytes parameter


We should add validation somewhere that the input isn't insane. Here perhaps?

Or maybe it' crazy to validate genesis input. But we will need some mechanism for validation if it is changed by governance. Tomas mentioned something about guarding it with VPs. Maybe a separate pr.

we're grabbing this parameter from the toml config, whose deserialization already has some logic that checks if it is in the range [1 B, 90 MiB]

I'm not sure if that's enough?

Ah, I missed that. So good enough for now. For governance, I'm not so sure as it doesn't require a hard fork but just a change in storage. So we might need this VP idea. But that can be a separate PR.

btw I think I misinterpreted the Tendermint specs. https://github.com/tendermint/tendermint/blob/v0.34.x/spec/abci/apps.md#blockparamsmaxbytes

I think the hard cap for a block might be 100 mb - 1 b, instead of 100 mb, since they're using an open interval

james-chf · 2022-12-12T09:12:02Z

core/src/ledger/parameters/storage.rs

+    // TODO: improve this code; use some kind of prefix
+    // tree to efficiently match `key`
    is_epoch_duration_storage_key(key)
        || is_max_expected_time_per_block_key(key)
        || is_tx_whitelist_key(key)
        || is_vp_whitelist_key(key)
+        || is_implicit_vp_key(key)
+        || is_epochs_per_year_key(key)
+        || is_pos_gain_p_key(key)
+        || is_pos_gain_d_key(key)
+        || is_staked_ratio_key(key)
+        || is_pos_inflation_amount_key(key)
+        || is_max_proposal_bytes_key(key)


oic the efficiency issue now. It would be good I guess if all protocol parameters were under some subtree e.g. /parameters/protocol/... or something, then we could easily recognize them

I was playing around with trie codegen over the weekend for this specific use-case, but I still need to improve the generated code. https://gist.github.com/sug0/1bc632eb19ebdeb2d596c1617fba4138

btw, we do have the same internal address as a prefix in each of these keys

apps/src/lib/config/genesis.rs

apps/src/lib/node/ledger/tendermint_node.rs

core/src/types/chain.rs

…k size

james-chf

lgtm!

sug0 added ledger ethereum-bridge labels Nov 30, 2022

sug0 requested review from batconjurer and james-chf November 30, 2022 09:47

james-chf reviewed Nov 30, 2022

View reviewed changes

apps/src/lib/config/genesis.rs Outdated Show resolved Hide resolved

sug0 force-pushed the tiago/ethbridge/max-block-size-genesis-param branch from 414b6e9 to a937e00 Compare December 6, 2022 11:08

sug0 added 11 commits December 7, 2022 09:01

Add max block size genesis param

4b57678

Add max block size param to e2e and dev genesis files

e771a3e

Add Tendermint block size type

5144e29

Tendermint block size value constructor

cf8444c

Add missing import

1ecff45

Add max bytes per block storage key

d557635

Tendermint block size type fixes

8bdbbf7

Restrict unsafe to a smaller scope

ca6d785

Read/write block size param from/to storage

fadca13

Add custom serde serialization routines for tm block size

02f0970

Enforce tm block size bounds during (de)serialization

d1301b5

sug0 force-pushed the tiago/ethbridge/max-block-size-genesis-param branch from a937e00 to d1301b5 Compare December 7, 2022 09:02

sug0 added 7 commits December 7, 2022 09:59

WIP: Replace TendermintBytesPerBlock with ProposalSize

8bc924f

Proposal bytes genesis param

fafb161

Docstr fixes

0da5567

Small semantics fix on gas arithmetic

9daea84

Fix serde value range for ProposalBytes

08b6741

Write block params to Tendermint's genesis file

a98aae8

Add i64 visitor on ProposalBytes serde parser

fd7cc4c

sug0 marked this pull request as ready for review December 7, 2022 14:41

sug0 requested a review from james-chf December 7, 2022 14:41

Small docstr fix

2e93732

sug0 marked this pull request as draft December 7, 2022 16:31

james-chf suggested changes Dec 8, 2022

View reviewed changes

apps/src/lib/config/genesis.rs Outdated Show resolved Hide resolved

sug0 marked this pull request as ready for review December 9, 2022 09:15

sug0 requested a review from james-chf December 9, 2022 09:22

sug0 marked this pull request as draft December 9, 2022 13:12

Merge eth-bridge-integration

2c6baec

sug0 marked this pull request as ready for review December 9, 2022 15:19

Merge branch 'eth-bridge-integration' into tiago/ethbridge/max-block-…

f19e839

…size-genesis-param

batconjurer reviewed Dec 12, 2022

View reviewed changes

batconjurer approved these changes Dec 12, 2022

View reviewed changes

james-chf suggested changes Dec 12, 2022

View reviewed changes

sug0 added 5 commits December 12, 2022 15:17

Add NOTE regarding TOML deserialization bug

f0fc770

Use raw proposal bytes value in deserializer error msg

0e02760

Fix potential open range bug whilst configuring Tendermint's max bloc…

844f209

…k size

Convert regular comments to docstrs

878d2d3

Improve max_proposal_bytes docstr

da9f3aa

sug0 requested a review from james-chf December 12, 2022 16:16

sug0 mentioned this pull request Dec 12, 2022

Optimize block proposal #771

Merged

8 tasks

james-chf approved these changes Dec 12, 2022

View reviewed changes

sug0 merged commit efa9f34 into eth-bridge-integration Dec 12, 2022

sug0 deleted the tiago/ethbridge/max-block-size-genesis-param branch December 12, 2022 19:26

sug0 mentioned this pull request May 10, 2024

Harden max proposal bytes configuration and deserialization #3220

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tendermint max block size param to genesis config file #843

Add Tendermint max block size param to genesis config file #843

sug0 commented Nov 30, 2022 •

edited

Loading

james-chf left a comment

sug0 commented Dec 8, 2022

sug0 commented Dec 9, 2022

james-chf commented Dec 9, 2022

sug0 commented Dec 12, 2022

batconjurer Dec 12, 2022

batconjurer Dec 12, 2022

sug0 Dec 12, 2022

sug0 Dec 12, 2022

batconjurer Dec 12, 2022

sug0 Dec 12, 2022

james-chf Dec 12, 2022

sug0 Dec 12, 2022

sug0 Dec 12, 2022

james-chf left a comment

Add Tendermint max block size param to genesis config file #843

Add Tendermint max block size param to genesis config file #843

Conversation

sug0 commented Nov 30, 2022 • edited Loading

james-chf left a comment

Choose a reason for hiding this comment

sug0 commented Dec 8, 2022

sug0 commented Dec 9, 2022

james-chf commented Dec 9, 2022

sug0 commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-chf left a comment

Choose a reason for hiding this comment

sug0 commented Nov 30, 2022 •

edited

Loading