Skip to content

Commit

Permalink
congestion: glue together congestion info bootstrapping (#11361)
Browse files Browse the repository at this point in the history
We still had an unresolved issue: Where in the code will the first
congestion info be computed? This is referred to as bootstrapping the
congestion info.

In this PR, I suggest we let the runtime compute it inside `apply`,
which means it is done on the shard level, rather than the chain level.
I believe this makes sense, since we don't need anything other than the
state trie to make this computation. And I'd rather not dig into the
trie from the chain level.

As a side-effect of this, the `ContextError` introduced for congestion
control is no longer possible. Implementation errors, which would have
previously caused this error followed by a crash, would now cause an
expensive recomputation of the congestion info and print a warning.

This PR also suggest a semantic change to how CongestionControl and
CongestionInfo are used. I think we should clearly distinguish between:

1) Read-only usage of congestion info (through CongestionControl) when
making congestion control decisions. 2) A congestion info we are still
building.

Thus, I changed the semantic of `CongestionControl` to only make sense
for looking at previous chunks' congestion info. Consequently,
ReceiptSink now only holds an instance of `CongestionInfo`.

With this semantic change, the `CongestionControl` no longer has a need
for add/remove function to modify internals. And the finalization of the
allowed shard no longer makes sense on that struct, this should happen
on the CongestionInfo only.

But now we need a way to compute the congestion level on the
`CongestionInfo` struct (without missed chunks) and one on
`CongestionControl` (with missed chunks). At this point, the somewhat
object-oriented approach to store the config inside `CongestionControl`
no longer works as smoothly. Thus, a few more function need the config
object injected at the parameter level. I have to admit, it's a bit more
clutter. But I think it makes sense to use apply_state.config as source
of truth rather than a copy of the config.

I even tried to add another struct (e.g. CongestionInfoBuilder) to
further make the distinction between mutable and read-only congestion
info. But for my taste, it adds more clutter than justified. We already
have too many different types.

With all this justification for the semantic changes, I want to stress
that I am still open to different semantics and code structures. Getting
this right is important to me, to not make the runtime code a bigger
mess than it has to be. This is just one proposal how it could be done.
  • Loading branch information
jakmeier committed May 21, 2024
1 parent f7617f0 commit 7d08c8b
Show file tree
Hide file tree
Showing 15 changed files with 364 additions and 363 deletions.
2 changes: 0 additions & 2 deletions chain/chain/src/runtime/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,6 @@ impl NightshadeRuntime {
// TODO(#2152): process gracefully
RuntimeError::ReceiptValidationError(e) => panic!("{}", e),
RuntimeError::ValidatorError(e) => e.into(),
// TODO(#2152): process gracefully
RuntimeError::ContextError(e) => panic!("{}", e),
})?;
let elapsed = instant.elapsed();

Expand Down
12 changes: 7 additions & 5 deletions chain/chain/src/runtime/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1634,26 +1634,28 @@ fn prepare_transactions(
transaction_groups: &mut dyn TransactionGroupIterator,
storage_config: RuntimeStorageConfig,
) -> Result<PreparedTransactions, Error> {
// TODO(congestion_info)
let congestion_info_map = HashMap::new();
let shard_id = 0;
let block = chain.get_block(&env.head.prev_block_hash).unwrap();
let congestion_info = block.shards_congestion_info();

env.runtime.prepare_transactions(
storage_config,
PrepareTransactionsChunkContext {
shard_id: 0,
shard_id,
gas_limit: env.runtime.genesis_config.gas_limit,
},
PrepareTransactionsBlockContext {
next_gas_price: env.runtime.genesis_config.min_gas_price,
height: env.head.height,
block_hash: env.head.last_block_hash,
congestion_info: congestion_info_map,
congestion_info,
},
transaction_groups,
&mut |tx: &SignedTransaction| -> bool {
chain
.chain_store()
.check_transaction_validity_period(
&chain.get_block_header(&env.head.prev_block_hash).unwrap(),
&block.header(),
tx.transaction.block_hash(),
chain.transaction_validity_period,
)
Expand Down
23 changes: 12 additions & 11 deletions core/primitives/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,17 +596,18 @@ impl Block {

for chunk in self.chunks().iter() {
let shard_id = chunk.shard_id();
// TODO(congestion_control): default is not always appropriate!
let congestion_info = chunk.congestion_info().unwrap_or_default();
let height_included = chunk.height_included();
let height_current = self.header().height();
let missed_chunks_count = height_current.checked_sub(height_included);
let missed_chunks_count = missed_chunks_count
.expect("The chunk height included must be less or equal than block height!");

let extended_congestion_info =
ExtendedCongestionInfo::new(congestion_info, missed_chunks_count);
result.insert(shard_id, extended_congestion_info);

if let Some(congestion_info) = chunk.congestion_info() {
let height_included = chunk.height_included();
let height_current = self.header().height();
let missed_chunks_count = height_current.checked_sub(height_included);
let missed_chunks_count = missed_chunks_count
.expect("The chunk height included must be less or equal than block height!");

let extended_congestion_info =
ExtendedCongestionInfo::new(congestion_info, missed_chunks_count);
result.insert(shard_id, extended_congestion_info);
}
}
result
}
Expand Down

0 comments on commit 7d08c8b

Please sign in to comment.