feat(spice): making congestion control work under SPICE. txs backpressure.#15960
feat(spice): making congestion control work under SPICE. txs backpressure.#15960ssavenko-near wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #15960 +/- ##
=======================================
Coverage 72.60% 72.61%
=======================================
Files 952 952
Lines 205109 205157 +48
Branches 205109 205157 +48
=======================================
+ Hits 148914 148965 +51
+ Misses 51218 51217 -1
+ Partials 4977 4975 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d14022d to
896cf80
Compare
01da9fb to
885e8e1
Compare
8cc01be to
27f19a2
Compare
There was a problem hiding this comment.
Pull request overview
Ports congestion-control integration tests from the legacy TestEnv framework to test-loop so they can exercise the real (including spice) transaction admission and execution pipeline.
Changes:
- Add
test-loopversions of congestion-control limit/filter/RPC-rejection tests with a custom runtime config. - Wire spice transaction-admission to derive per-shard congestion from executed
ChunkExtras (via certified execution results) in both RPC tx submission and chunk production. - Remove the legacy
integration-testscongestion-control module.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test-loop-tests/src/utils/transactions.rs | Clarifies docs for run_txs_parallel_on. |
| test-loop-tests/src/tests/spice/congestion.rs | Tightens module-level and inline comments for the spice congestion/bandwidth lagging-execution test. |
| test-loop-tests/src/tests/mod.rs | Registers the new congestion_control_limits test module. |
| test-loop-tests/src/tests/congestion_control.rs | Comment-only clarification in existing congestion-control test. |
| test-loop-tests/src/tests/congestion_control_limits.rs | New test-loop test suite covering tx inclusion limits and RPC shard-congestion rejection. |
| integration-tests/src/tests/features/mod.rs | Removes the legacy congestion-control feature test module entry. |
| integration-tests/src/tests/features/congestion_control.rs | Deletes the legacy TestEnv congestion-control tests. |
| chain/client/src/rpc_handler.rs | For spice, derive receiver-shard congestion from executed ChunkExtras (certified) for RPC tx rejection. |
| chain/client/src/chunk_producer.rs | For spice, supply per-shard congestion info (from executed ChunkExtras) into PrepareTransactionsBlockContext. |
| chain/chain/src/spice/core.rs | Extracts a helper to read certified execution results from DBCol::execution_results. |
| chain/chain/src/spice/chunk_application.rs | Adds spice_block_congestion_info helper to build tx-admission congestion info from executed ChunkExtras. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
27f19a2 to
80ff3a0
Compare
80ff3a0 to
7a86081
Compare
Port the four congestion-control integration tests (transaction inclusion limits, filtering, and RPC rejection) from the TestEnv framework to test-loop, as phase 1 (non-spice parity) of the migration. TestEnv stubs spice's execution actors and so can never exercise spice; test-loop runs the real pipeline. Faithful port: single node tracking all shards, custom RuntimeConfig (zeroed wasm op cost + pinned congestion-control params), synchronous process_tx for pool-fill bursts and the ShardCongested assertion, and per-chunk tx-count measurement via one-block stepping. Tests are ignored under spice for now (congestion gating reads a lagging signal there — phase 2). The integration-tests originals are kept as the parity reference.
7a86081 to
d80b3da
Compare
| ) -> Option<Arc<ChunkExecutionResult>> { | ||
| let key = get_execution_results_key(block_hash, shard_id); | ||
| self.chain_store.store().caching_get_ser(DBCol::execution_results(), &key) | ||
| get_execution_result_from_store(&self.chain_store, block_hash, shard_id) |
There was a problem hiding this comment.
seems this method could be inlined now.
| let mut prev_block_context = | ||
| PrepareTransactionsBlockContext::new(prev_block, &*self.epoch_manager)?; |
There was a problem hiding this comment.
nit: I kind of dislike mutating variables just returned from a constructor. I think we could modify new to take the header and congestion info and pass the block's congestion info in the non-spice case.
| // time the live parameters change. | ||
| fn set_default_congestion_control(config_store: &RuntimeConfigStore, config: &mut RuntimeConfig) { | ||
| #[allow(deprecated)] | ||
| let cc_protocol_version = ProtocolFeature::_DeprecatedCongestionControl.protocol_version(); |
There was a problem hiding this comment.
let's preserve the TODO about updating this.
| node.client().runtime_adapter.get_runtime_config(protocol_version).congestion_control_config | ||
| } | ||
|
|
||
| // ---- transaction builders (mirror the integration-test helpers) ---- |
There was a problem hiding this comment.
now that integration test is gone, let's remove references to it.
|
|
||
| /// Congestion info of the shard's last executed chunk, from its `ChunkExtra` at | ||
| /// `last_executed()` (the execution head, which trails consensus under spice). | ||
| fn head_congestion_info(node: &TestLoopNode, shard_id: ShardId) -> CongestionInfo { |
There was a problem hiding this comment.
in the old test non-spice would use something like:
head_block.chunks()[shard].congestion_info() → congestion from block H - 1
New test-loop test:
get_chunk_extra(head_hash, shard).congestion_info() → congestion from block H
| fn head_height(env: &TestLoopEnv) -> BlockHeight { | ||
| env.validator().head().height | ||
| } | ||
|
|
||
| /// Advance the chain by exactly one block. | ||
| fn produce_one_block(env: &mut TestLoopEnv) { | ||
| env.validator_runner().run_for_number_of_blocks(1); | ||
| } |
There was a problem hiding this comment.
nit: I would suggest to inline these simple methods.
Part 2 (of 3) of making congestion control work under SPICE.
The congestion inputs that gate transaction admission aren't available in the SPICE chunk header.
This PR derives them from the last certified block's executed
ChunkExtrasand migrates the congestion tx-limit tests to test-loop and enables them under spice.Production
BlockCongestionInforeconstructed from a block's executedChunkExtras. Prefers the locally executedChunkExtrafrom execution, falling back to the chain-widecertified result when not tracking the relevant shard.
PrepareTransactionsBlockContext.congestion_info.DBCol::execution_resultsread into a sharedget_execution_result_from_store.SpiceCoreReaderand the new path now share it.Tests
#[cfg_attr(spice, ignore)]gates: the four local/remote tx-limit, filtering, and RPC-rejection tests now run under spice and non-spice.ChunkExtraat the execution head (last_executed(), which trails consensus under spice) so the same assertions hold in both modes.