Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
4b57678
Add max block size genesis param
sug0 Nov 30, 2022
e771a3e
Add max block size param to e2e and dev genesis files
sug0 Nov 30, 2022
5144e29
Add Tendermint block size type
sug0 Nov 30, 2022
cf8444c
Tendermint block size value constructor
sug0 Nov 30, 2022
1ecff45
Add missing import
sug0 Nov 30, 2022
d557635
Add max bytes per block storage key
sug0 Nov 30, 2022
8bdbbf7
Tendermint block size type fixes
sug0 Nov 30, 2022
ca6d785
Restrict unsafe to a smaller scope
sug0 Nov 30, 2022
fadca13
Read/write block size param from/to storage
sug0 Nov 30, 2022
02f0970
Add custom serde serialization routines for tm block size
sug0 Nov 30, 2022
d1301b5
Enforce tm block size bounds during (de)serialization
sug0 Nov 30, 2022
8bc924f
WIP: Replace TendermintBytesPerBlock with ProposalSize
sug0 Dec 7, 2022
fafb161
Proposal bytes genesis param
sug0 Dec 7, 2022
0da5567
Docstr fixes
sug0 Dec 7, 2022
9daea84
Small semantics fix on gas arithmetic
sug0 Dec 7, 2022
08b6741
Fix serde value range for ProposalBytes
sug0 Dec 7, 2022
a98aae8
Write block params to Tendermint's genesis file
sug0 Dec 7, 2022
fd7cc4c
Add i64 visitor on ProposalBytes serde parser
sug0 Dec 7, 2022
2e93732
Small docstr fix
sug0 Dec 7, 2022
2c6baec
Merge eth-bridge-integration
sug0 Dec 9, 2022
f19e839
Merge branch 'eth-bridge-integration' into tiago/ethbridge/max-block-…
sug0 Dec 9, 2022
f0fc770
Add NOTE regarding TOML deserialization bug
sug0 Dec 12, 2022
0e02760
Use raw proposal bytes value in deserializer error msg
sug0 Dec 12, 2022
844f209
Fix potential open range bug whilst configuring Tendermint's max bloc…
sug0 Dec 12, 2022
878d2d3
Convert regular comments to docstrs
sug0 Dec 12, 2022
da9f3aa
Improve `max_proposal_bytes` docstr
sug0 Dec 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions apps/src/lib/config/genesis.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use namada::ledger::pos::{GenesisValidator, PosParams};
use namada::types::address::{wnam, Address};
#[cfg(not(feature = "dev"))]
use namada::types::chain::ChainId;
use namada::types::chain::ProposalBytes;
use namada::types::ethereum_events::EthAddress;
use namada::types::key::dkg_session_keys::DkgPublicKey;
use namada::types::key::*;
Expand All @@ -36,6 +37,7 @@ pub mod genesis_config {
use namada::ledger::parameters::EpochDuration;
use namada::ledger::pos::{GenesisValidator, PosParams};
use namada::types::address::Address;
use namada::types::chain::ProposalBytes;
use namada::types::key::dkg_session_keys::DkgPublicKey;
use namada::types::key::*;
use namada::types::time::Rfc3339String;
Expand Down Expand Up @@ -232,6 +234,8 @@ pub mod genesis_config {

#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct ParametersConfig {
// Max payload size, in bytes, for a tx batch proposal.
pub max_proposal_bytes: ProposalBytes,
// Minimum number of blocks per epoch.
// XXX: u64 doesn't work with toml-rs!
pub min_num_of_blocks: u64,
Expand Down Expand Up @@ -597,6 +601,7 @@ pub mod genesis_config {
parameters.max_expected_time_per_block,
)
.into(),
max_proposal_bytes: parameters.max_proposal_bytes,
vp_whitelist: parameters.vp_whitelist.unwrap_or_default(),
tx_whitelist: parameters.tx_whitelist.unwrap_or_default(),
implicit_vp_code_path,
Expand Down Expand Up @@ -822,6 +827,8 @@ pub struct ImplicitAccount {
BorshDeserialize,
)]
pub struct Parameters {
// Max payload size, in bytes, for a tx batch proposal.
pub max_proposal_bytes: ProposalBytes,
/// Epoch duration
pub epoch_duration: EpochDuration,
/// Maximum expected time per block
Expand Down Expand Up @@ -904,6 +911,7 @@ pub fn genesis() -> Genesis {
min_duration: namada::types::time::Duration::seconds(600).into(),
},
max_expected_time_per_block: namada::types::time::DurationSecs(30),
max_proposal_bytes: Default::default(),
vp_whitelist: vec![],
tx_whitelist: vec![],
implicit_vp_code_path: vp_implicit_path.into(),
Expand Down
2 changes: 2 additions & 0 deletions apps/src/lib/node/ledger/shell/init_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ where
// Initialize protocol parameters
let genesis::Parameters {
epoch_duration,
max_proposal_bytes,
max_expected_time_per_block,
vp_whitelist,
tx_whitelist,
Expand Down Expand Up @@ -106,6 +107,7 @@ where
}
let parameters = Parameters {
epoch_duration,
max_proposal_bytes,
max_expected_time_per_block,
vp_whitelist,
tx_whitelist,
Expand Down
10 changes: 9 additions & 1 deletion apps/src/lib/node/ledger/tendermint_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use tokio::io::{AsyncReadExt, AsyncWriteExt};
use tokio::process::Command;

use crate::config;
use crate::facade::tendermint::Genesis;
use crate::facade::tendermint::{block, Genesis};
use crate::facade::tendermint_config::net::Address as TendermintAddress;
use crate::facade::tendermint_config::{
Error as TendermintError, TendermintConfig,
Expand Down Expand Up @@ -454,6 +454,14 @@ async fn write_tm_genesis(
genesis.genesis_time = genesis_time
.try_into()
.expect("Couldn't convert DateTimeUtc to Tendermint Time");
genesis.consensus_params.block = Some(block::Size {
// maximum size of a serialized Tendermint block
// cannot go over 100 MiB
max_bytes: 100 << 20,
// gas is metered app-side, so we disable it
// at the Tendermint level
max_gas: -1,
});
#[cfg(feature = "abcipp")]
{
genesis.consensus_params.timeout.commit =
Expand Down
57 changes: 47 additions & 10 deletions core/src/ledger/parameters/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use super::storage::types::{decode, encode};
use super::storage::{types, Storage};
use crate::ledger::storage::{self as ledger_storage};
use crate::types::address::{Address, InternalAddress};
use crate::types::chain::ProposalBytes;
use crate::types::storage::Key;
use crate::types::time::DurationSecs;

Expand All @@ -32,6 +33,8 @@ pub struct Parameters {
pub epoch_duration: EpochDuration,
/// Maximum expected time per block (read only)
pub max_expected_time_per_block: DurationSecs,
/// Max payload size, in bytes, for a tx batch proposal.
pub max_proposal_bytes: ProposalBytes,
/// Whitelisted validity predicate hashes (read only)
pub vp_whitelist: Vec<String>,
/// Whitelisted tx hashes (read only)
Expand Down Expand Up @@ -101,6 +104,7 @@ impl Parameters {
let Self {
epoch_duration,
max_expected_time_per_block,
max_proposal_bytes,
vp_whitelist,
tx_whitelist,
implicit_vp,
Expand All @@ -111,6 +115,16 @@ impl Parameters {
pos_inflation_amount,
} = self;

// write max proposal bytes parameter
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if that's enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

let max_proposal_bytes_key = storage::get_max_proposal_bytes_key();
let max_proposal_bytes_value = encode(&max_proposal_bytes);
storage
.write(&max_proposal_bytes_key, max_proposal_bytes_value)
.expect(
"Max proposal bytes parameter must be initialized in the \
genesis block",
);

// write epoch parameters
let epoch_key = storage::get_epoch_duration_storage_key();
let epoch_value = encode(epoch_duration);
Expand Down Expand Up @@ -382,6 +396,17 @@ where
DB: ledger_storage::DB + for<'iter> ledger_storage::DBIter<'iter>,
H: ledger_storage::StorageHasher,
{
// read max proposal bytes
let (max_proposal_bytes, gas_proposal_bytes) = {
let key = storage::get_max_proposal_bytes_key();
let (value, gas) =
storage.read(&key).map_err(ReadError::StorageError)?;
let value: ProposalBytes =
decode(value.ok_or(ReadError::ParametersMissing)?)
.map_err(ReadError::StorageTypeError)?;
(value, gas)
};

// read epoch duration
let (epoch_duration, gas_epoch) = read_epoch_duration_parameter(storage)
.expect("Couldn't read epoch duration parameters");
Expand Down Expand Up @@ -464,10 +489,31 @@ where
decode(value.ok_or(ReadError::ParametersMissing)?)
.map_err(ReadError::StorageTypeError)?;

let total_gas_cost = [
gas_epoch,
gas_tx,
gas_vp,
gas_time,
gas_implicit_vp,
gas_epy,
gas_gain_p,
gas_gain_d,
gas_staked,
gas_reward,
gas_proposal_bytes,
]
.into_iter()
.fold(0u64, |accum, gas| {
accum
.checked_add(gas)
.expect("u64 overflow occurred while doing gas arithmetic")
});

Ok((
Parameters {
epoch_duration,
max_expected_time_per_block,
max_proposal_bytes,
vp_whitelist,
tx_whitelist,
implicit_vp,
Expand All @@ -477,15 +523,6 @@ where
staked_ratio,
pos_inflation_amount,
},
gas_epoch
+ gas_tx
+ gas_vp
+ gas_time
+ gas_implicit_vp
+ gas_epy
+ gas_gain_p
+ gas_gain_d
+ gas_staked
+ gas_reward,
total_gas_cost,
))
}
28 changes: 28 additions & 0 deletions core/src/ledger/parameters/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const POS_GAIN_P_KEY: &str = "pos_gain_p";
const POS_GAIN_D_KEY: &str = "pos_gain_d";
const STAKED_RATIO_KEY: &str = "staked_ratio_key";
const POS_INFLATION_AMOUNT_KEY: &str = "pos_inflation_amount_key";
const MAX_PROPOSAL_BYTES_KEY: &str = "max_proposal_bytes";

/// Returns if the key is a parameter key.
pub fn is_parameter_key(key: &Key) -> bool {
Expand All @@ -20,10 +21,19 @@ pub fn is_parameter_key(key: &Key) -> bool {

/// Returns if the key is a protocol parameter key.
pub fn is_protocol_parameter_key(key: &Key) -> bool {
// 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)
Comment on lines +24 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

}

/// Returns if the key is an epoch storage key.
Expand Down Expand Up @@ -106,6 +116,14 @@ pub fn is_pos_inflation_amount_key(key: &Key) -> bool {
] if addr == &ADDRESS && pos_inflation_amount == POS_INFLATION_AMOUNT_KEY)
}

/// Returns if the key is the max proposal bytes key.
pub fn is_max_proposal_bytes_key(key: &Key) -> bool {
matches!(&key.segments[..], [
DbKeySeg::AddressSeg(addr),
DbKeySeg::StringSeg(max_proposal_bytes),
] if addr == &ADDRESS && max_proposal_bytes == MAX_PROPOSAL_BYTES_KEY)
}

/// Storage key used for epoch parameter.
pub fn get_epoch_duration_storage_key() -> Key {
Key {
Expand Down Expand Up @@ -205,3 +223,13 @@ pub fn get_pos_inflation_amount_key() -> Key {
],
}
}

/// Storage key used for the max proposal bytes.
pub fn get_max_proposal_bytes_key() -> Key {
Key {
segments: vec![
DbKeySeg::AddressSeg(ADDRESS),
DbKeySeg::StringSeg(MAX_PROPOSAL_BYTES_KEY.to_string()),
],
}
}
1 change: 1 addition & 0 deletions core/src/ledger/storage/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1295,6 +1295,7 @@ mod tests {
..Default::default()
};
let mut parameters = Parameters {
max_proposal_bytes: Default::default(),
epoch_duration: epoch_duration.clone(),
max_expected_time_per_block: Duration::seconds(max_expected_time_per_block).into(),
vp_whitelist: vec![],
Expand Down
Loading