[execution-pool] BlockRetrievalRequest struct -> enum #15812

hariria · 2025-01-25T03:59:32Z

Description

Deprecates the BlockRetrievalRequest struct in favor of a BlockRetrievalRequest enum. This is needed as execution pool plans to make BlockRetrievalRequests that include epoch and round information instead of target_block_id.

The plan is to phase this in three releases. The first release will introduce the new BlockRetrievalRequest enum and all corresponding structs / logic that use it. The second release will deprecate the BlockRetrievalRequestV1 struct and all related structs / logic.

Important

Some TODOs around deprecation are already marked in the code. Additional TODO in future PRs:

process_block_retrieval_v2 to be implemented in a future PR cc @bchocho @hariria
Once the new BlockRetrievalRequest enum is in the mainnet release, request_block should send ConsensusMsg::BlockRetrievalRequest instead of ConsensusMsg::DeprecatedBlockRetrievalRequest

Additional Questions

Why does consensus.yaml require TUPLEARRAY instead of TUPLE?

How Has This Been Tested?

trunk-io · 2025-01-25T03:59:35Z

⏱️ 5h 6m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	2h 30m	🟩 🟩 🟥 🟩 🟩 (+2 more)
execution-performance / test-target-determinator	30m	🟩 🟩 🟩 🟩 🟩 (+1 more)
check-dynamic-deps	28m	🟩 🟩 🟩 🟩 🟩 (+6 more)
test-target-determinator	26m	🟩 🟩 🟩 ⬜ 🟩 (+1 more)
rust-cargo-deny	18m	🟩 🟩 🟩 ⬜ 🟩 (+6 more)
fetch-last-released-docker-image-tag	9m	🟩 🟩 🟩 ⬜ 🟩 (+1 more)
rust-doc-tests	6m	🟩
rust-doc-tests	6m	🟩
rust-doc-tests	6m	🟩
rust-doc-tests	6m	🟩
general-lints	6m	🟩 🟩 🟩 ⬜ 🟩 (+6 more)
rust-doc-tests	6m	🟩
semgrep/ci	4m	🟩 🟩 🟩 🟩 🟩 (+6 more)
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+6 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

bchocho · 2025-01-27T17:40:17Z

consensus/consensus-types/src/block_retrieval.rs

+pub struct BlockRetrievalRequestV2 {
+    block_id: HashValue,
+    num_blocks: u64,
+    // TODO: remove the Option, if it's not too painful


Ah, two things I now remember thinking to clean up:

Now that we're separating out the logic, can we remove the Option? It doesn't make sense to have a V2 request without the target

Is epoch needed here?

Sounds good. For posterity in case we come back to this, the option and (epoch, round) was included from the original PR

Will remove epoch and Option

ibalajiarun · 2025-01-27T21:00:25Z

consensus/src/epoch_manager.rs

+                        BlockRetrievalRequest::V1(v1) => v1,
+                        // TODO @bchocho @hariria implement after all nodes upgrade to release with enum BlockRetrievalRequest (not struct)
+                        BlockRetrievalRequest::V2(_) => {
+                            unimplemented!("Should not have received a BlockRetrievalRequestV2...")


It's better to log error with sender.

Changed to

bail!("Should not have received a BlockRetrievalRequestV2 {} from peer with id {}", v2, peer_id);

ibalajiarun · 2025-01-27T21:09:01Z

consensus/src/network.rs

-    BlockRetrieval(IncomingBlockRetrievalRequest),
+    /// NOTE: This is being phased out in two releases to accommodate `IncomingBlockRetrievalRequestV2`
+    /// TODO @bchocho @hariria can remove after all nodes upgrade to release with enum BlockRetrievalRequest (not struct)
+    DeprecatedBlockRetrieval(IncomingBlockRetrievalRequest),


I am wondering if we should append V0 to IncomingBlockRetrievalRequest and make it IncomingBlockRetrievalRequestV0. That way we can keep the new variant as BlockRetrieval instead?

Changed to

#[derive(Debug)] pub enum IncomingRpcRequest { /// NOTE: This is being phased out in two releases to accommodate `IncomingBlockRetrievalRequestV2` /// TODO @bchocho @hariria can remove after all nodes upgrade to release with enum BlockRetrievalRequest (not struct) DeprecatedBlockRetrieval(DeprecatedIncomingBlockRetrievalRequest), BatchRetrieval(IncomingBatchRetrievalRequest), DAGRequest(IncomingDAGRequest), CommitRequest(IncomingCommitRequest), RandGenRequest(IncomingRandGenRequest), BlockRetrieval(IncomingBlockRetrievalRequest), }

ibalajiarun · 2025-01-27T21:09:32Z

consensus/src/network_interface.rs

@@ -83,14 +86,17 @@ pub enum ConsensusMsg {
    OrderVoteMsg(Box<OrderVoteMsg>),
    /// RoundTimeoutMsg is broadcasted by a validator once it decides to timeout the current round.
    RoundTimeoutMsg(Box<RoundTimeoutMsg>),
+    /// RPC to get a chain of block of the given length starting from the given block id, using epoch and round.
+    BlockRetrievalRequestV2(Box<BlockRetrievalRequest>),


Same as for RPC request. V0 for the original variant and drop the V2 for the new variant?

Changed to

/// Network type for consensus #[derive(Clone, Debug, Deserialize, Serialize)] pub enum ConsensusMsg { /// DEPRECATED: Please use [`ConsensusMsg::BlockRetrievalRequest`](ConsensusMsg::BlockRetrievalRequest) going forward /// This variant was renamed from `BlockRetrievalRequest` to `DeprecatedBlockRetrievalRequest` /// RPC to get a chain of block of the given length starting from the given block id. /// Note: Naming here is `BlockRetrievalRequest` and not `DeprecatedBlockRetrievalRequest` /// to be consistent with the naming implementation in [`ConsensusMsg::name`](ConsensusMsg::name) DeprecatedBlockRetrievalRequest(Box<BlockRetrievalRequestV1>), ... /// RPC to get a chain of block of the given length starting from the given block id, using epoch and round. BlockRetrievalRequest(Box<BlockRetrievalRequest>), }

ibalajiarun

As discussed offline, let's implement process_block_retrieval_v2 in the next PR.

bchocho · 2025-01-28T18:14:55Z

consensus/src/epoch_manager.rs

+                            continue;
+                        }
+                        if let Err(e) = monitor!(
+                            "process_block_retrieval",


Change to process_block_retrieval_v2 so we can see the change in metrics while rolling out

github-actions · 2025-01-28T18:55:52Z

✅ Forge suite `compat` success on `55909aa300b714e37df1113a239332c334cb2896` ==> `6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23`

Compatibility test results for 55909aa300b714e37df1113a239332c334cb2896 ==> 6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23 (PR)
1. Check liveness of validators at old version: 55909aa300b714e37df1113a239332c334cb2896
compatibility::simple-validator-upgrade::liveness-check : committed: 12240.24 txn/s, latency: 2590.93 ms, (p50: 2600 ms, p70: 2800, p90: 3100 ms, p99: 4200 ms), latency samples: 399220
2. Upgrading first Validator to new version: 6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 4352.44 txn/s, latency: 7165.75 ms, (p50: 8000 ms, p70: 8500, p90: 8800 ms, p99: 9100 ms), latency samples: 92640
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4409.31 txn/s, latency: 7700.57 ms, (p50: 8600 ms, p70: 8700, p90: 9100 ms, p99: 9100 ms), latency samples: 155420
3. Upgrading rest of first batch to new version: 6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3766.22 txn/s, latency: 8072.08 ms, (p50: 8600 ms, p70: 9600, p90: 10300 ms, p99: 10500 ms), latency samples: 82680
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3732.33 txn/s, latency: 9205.33 ms, (p50: 10000 ms, p70: 10200, p90: 10500 ms, p99: 10600 ms), latency samples: 133700
4. upgrading second batch to new version: 6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 7701.94 txn/s, latency: 3948.86 ms, (p50: 4600 ms, p70: 4800, p90: 5000 ms, p99: 5300 ms), latency samples: 143820
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 7687.54 txn/s, latency: 4463.75 ms, (p50: 4900 ms, p70: 5000, p90: 5300 ms, p99: 5400 ms), latency samples: 256960
5. check swarm health
Compatibility test for 55909aa300b714e37df1113a239332c334cb2896 ==> 6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23 passed
Test Ok

github-actions · 2025-01-28T18:57:17Z

✅ Forge suite `realistic_env_max_load` success on `6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23`

two traffics test: inner traffic : committed: 14808.21 txn/s, latency: 2675.19 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3900 ms), latency samples: 5630420
two traffics test : committed: 99.99 txn/s, latency: 1406.88 ms, (p50: 1400 ms, p70: 1400, p90: 1600 ms, p99: 2700 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.452, avg: 1.414", "ConsensusProposalToOrdered: max: 0.298, avg: 0.288", "ConsensusOrderedToCommit: max: 0.410, avg: 0.401", "ConsensusProposalToCommit: max: 0.699, avg: 0.689"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.52s no progress at version 66242 (avg 0.19s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.65s no progress at version 5905605 (avg 0.64s) [limit 16].
Test Ok

hariria requested a review from bchocho January 25, 2025 03:59

hariria self-assigned this Jan 25, 2025

hariria force-pushed the andrew/exec-pool-block-retrieval-request branch 2 times, most recently from d98225a to be5e4c3 Compare January 25, 2025 04:19

[execution-pool] BlockRetrievalRequest deprecation

a682f31

hariria force-pushed the andrew/exec-pool-block-retrieval-request branch from be5e4c3 to 2c36388 Compare January 25, 2025 20:13

hariria marked this pull request as ready for review January 25, 2025 20:23

hariria requested review from zekun000, sasha8 and ibalajiarun as code owners January 25, 2025 20:23

hariria changed the title ~~[execution-pool] BlockRetrievalRequest deprecation~~ [execution-pool] BlockRetrievalRequest struct -> enum Jan 25, 2025

bchocho reviewed Jan 27, 2025

View reviewed changes

[execution-pool] BlockRetrievalRequest consensus.yaml

902382e

hariria force-pushed the andrew/exec-pool-block-retrieval-request branch from 2c36388 to 902382e Compare January 27, 2025 19:16

hariria requested a review from bchocho January 27, 2025 19:16

bchocho approved these changes Jan 27, 2025

View reviewed changes

ibalajiarun reviewed Jan 27, 2025

View reviewed changes

ibalajiarun added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jan 27, 2025

This comment has been minimized.

Sign in to view

[exec-pool] BlockRetrievalRequest address Balaji comments

ca4499f

hariria force-pushed the andrew/exec-pool-block-retrieval-request branch from d3516c9 to ca4499f Compare January 27, 2025 21:59

This comment has been minimized.

Sign in to view

hariria requested a review from ibalajiarun January 27, 2025 22:28

ibalajiarun approved these changes Jan 27, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

bchocho approved these changes Jan 28, 2025

View reviewed changes

[exec-pool] Add a TODO for process_block_retrieval_v2

6bd46ac

hariria force-pushed the andrew/exec-pool-block-retrieval-request branch from 8389258 to 6bd46ac Compare January 28, 2025 18:27

hariria enabled auto-merge (squash) January 28, 2025 18:28

This comment has been minimized.

Sign in to view

hariria merged commit 4e1eb2d into main Jan 28, 2025
67 of 89 checks passed

hariria deleted the andrew/exec-pool-block-retrieval-request branch January 28, 2025 21:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[execution-pool] BlockRetrievalRequest struct -> enum #15812

[execution-pool] BlockRetrievalRequest struct -> enum #15812

hariria commented Jan 25, 2025 •

edited

Loading

trunk-io bot commented Jan 25, 2025 •

edited

Loading

bchocho Jan 27, 2025

hariria Jan 27, 2025

ibalajiarun Jan 27, 2025

hariria Jan 27, 2025

ibalajiarun Jan 27, 2025

hariria Jan 27, 2025

ibalajiarun Jan 27, 2025

hariria Jan 27, 2025 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ibalajiarun left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bchocho Jan 28, 2025

hariria Jan 28, 2025

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 28, 2025

github-actions bot commented Jan 28, 2025

[execution-pool] BlockRetrievalRequest struct -> enum #15812

[execution-pool] BlockRetrievalRequest struct -> enum #15812

Conversation

hariria commented Jan 25, 2025 • edited Loading

Description

How Has This Been Tested?

trunk-io bot commented Jan 25, 2025 • edited Loading

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

hariria Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ibalajiarun left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 28, 2025

✅ Forge suite compat success on 55909aa300b714e37df1113a239332c334cb2896 ==> 6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23

github-actions bot commented Jan 28, 2025

✅ Forge suite realistic_env_max_load success on 6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23

hariria commented Jan 25, 2025 •

edited

Loading

trunk-io bot commented Jan 25, 2025 •

edited

Loading

hariria Jan 27, 2025 •

edited

Loading

✅ Forge suite `compat` success on `55909aa300b714e37df1113a239332c334cb2896` ==> `6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23`

✅ Forge suite `realistic_env_max_load` success on `6bd46ace468ed89d8838c61b1aa7bfa1cb2f8a23`