[Refactor] 3: Switch legacy to coordinator #239

QuentinI · 2024-11-06T19:29:34Z

Closes #206

This PR:

Switches legacy builder to builder state coordinator.

This PR does not:

Key places to review:

coveralls · 2024-11-06T19:34:08Z

Changes unknown
when pulling d418ff6 on ag/refactor-legacy
into ** on main**.

QuentinI · 2024-11-23T16:56:02Z

crates/shared/src/state.rs

@@ -159,6 +159,24 @@ where
            <Types::BlockPayload as BlockPayload<Types>>::from_bytes(encoded_txns, metadata);
        let txn_commitments = block_payload.transaction_commitments(metadata);

+        let views_since_nonempty_block = if txn_commitments.is_empty() {


This is used by legacy builder to decide whether we want to build empty block to prioritize finalization.

QuentinI · 2024-11-23T16:58:52Z

crates/legacy/src/block_size_limits.rs

This was moved to its own file. The only change is making it atomic to reduce amount of locks we juggle in the legacy builder.

QuentinI · 2024-11-23T17:01:38Z

crates/legacy/src/block_store.rs

This file factors out the blocks and block_cache fields of legacy's GlobalState.
The only actual change made is instead of Lru this uses the tiered map, so that we can prune blocks we know we won't need instead of hoping our Lru size is just right and we don't evict blocks we may yet need but also don't keep around too many blocks we'll never need again.

QuentinI · 2024-11-23T17:02:32Z

crates/legacy/src/lib.rs

-/// `WaitAndKeep` is a helper enum that allows for the lazy polling of a single
-/// value from an unbound receiver.
-#[derive(Debug)]
-pub enum WaitAndKeep<T> {


This was moved to the shared crate and refactored a bit.

QuentinI · 2024-11-23T17:02:41Z

crates/legacy/src/lib.rs

-
-// TODO: Update commitment calculation with the new `commit`.
-// <https://github.com/EspressoSystems/marketplace-builder-core/issues/143>
-trait LegacyCommit<T: NodeType> {


We don't need this anymore!

QuentinI · 2024-11-23T17:04:30Z

crates/legacy/src/service.rs

Where most of the actual refactoring happened.

QuentinI · 2024-11-23T17:05:39Z

crates/marketplace/src/service.rs

@@ -41,6 +42,28 @@ pub use marketplace_builder_shared::utils::EventServiceStream;

 use crate::hooks::BuilderHooks;

+/// Configuration to initialize the builder
+#[derive(Debug, Clone)]
+pub struct BuilderConfig<Types: NodeType> {


Sneaked in a config for the service instead of passing a bajilion parameters to the constructor

QuentinI · 2024-11-23T17:06:51Z

crates/marketplace/src/testing/mod.rs

 pub mod basic_test;
 pub mod integration;
 pub mod order_test;
-
-pub(crate) struct SimulatedChainState {


Moved to shared crate, so that legacy tests can take advantage of this as well.

shenkeyao

This is a big one! I left many comments but structure-wise this looks really good.

crates/legacy/src/service.rs

crates/marketplace/src/lib.rs

crates/shared/src/coordinator/mod.rs

crates/marketplace/src/testing/basic_test.rs

crates/marketplace/src/testing/integration.rs

crates/shared/src/state.rs

crates/shared/src/block.rs

crates/legacy/src/testing/mod.rs

crates/legacy/src/service.rs

dailinsubjam · 2024-11-27T05:37:55Z

crates/legacy/src/block_size_limits.rs

+#[repr(C)]
+pub(crate) struct MutableState {
+    pub max_block_size: u64,
+    pub last_block_size_increment: u64,


Is it possible to have a comment on what last_block_size_increment is about?

dailinsubjam · 2024-11-27T05:55:34Z

crates/legacy/src/block_size_limits.rs

+    /// If increment period has elapsed or `force` flag is set,
+    /// increment [`Self::max_block_size`] by current value * [`Self::MAX_BLOCK_SIZE_CHANGE_DIVISOR`]
+    /// with [`Self::protocol_max_block_size`] as a ceiling
+    pub fn try_increment_block_size(&self, force: bool) {


What's the use case of this function? Just trying to get more understanding on the design motivation of try_increment_block_size and decrement_block_size.

I added some docs to the BlockSizeLimits struct itself, LMK if they don't clear it up enough

Thanks for the detailed comment!

dailinsubjam · 2024-11-27T06:33:18Z

crates/marketplace/src/hooks.rs

@@ -4,8 +4,29 @@ use async_trait::async_trait;
 use hotshot::types::Event;
 use hotshot_types::traits::node_implementation::NodeType;

+/// A trait for hooks into the builder service. Used to further customize
+/// builder behaviour in ways not possible in builder core.
+/// If you don't such customisation, use [`NoHooks`].


Nit: "If you don't use such"

shenkeyao · 2024-11-28T00:04:45Z

crates/shared/src/state.rs

+            self.parent_block_references.last_nonempty_view
+        } else {
+            // This block is non-empty
+            Some(quorum_proposal.view_number)


IIRC in the previous commit you updated self.parent_block_references.last_nonempty_view as well. Should we keep it?

Sorry, not sure I understand what you mean. Could you please clarify?

As discussed today this actually looks good!

crates/legacy/src/service.rs

crates/shared/src/testing/constants.rs

crates/shared/src/coordinator/tiered_view_map.rs

* cfg-gate the refactor

QuentinI changed the title ~~[Refactor] 3: Switch legacy to coordinator~~ [WIP] [Refactor] 3: Switch legacy to coordinator Nov 6, 2024

QuentinI force-pushed the ag/refactor-legacy branch 3 times, most recently from a685fff to 03e7677 Compare November 8, 2024 19:26

QuentinI changed the base branch from main to ag/refactor-marketplace November 8, 2024 19:26

QuentinI force-pushed the ag/refactor-legacy branch from 03e7677 to 7970184 Compare November 8, 2024 19:53

QuentinI force-pushed the ag/refactor-marketplace branch from ce9a7dd to 09a1df3 Compare November 8, 2024 19:58

QuentinI force-pushed the ag/refactor-legacy branch 3 times, most recently from 92455e6 to f361ed2 Compare November 11, 2024 16:52

QuentinI force-pushed the ag/refactor-marketplace branch from 09a1df3 to 4a09ecb Compare November 11, 2024 16:53

QuentinI force-pushed the ag/refactor-legacy branch from f361ed2 to a7fc02f Compare November 12, 2024 14:32

QuentinI force-pushed the ag/refactor-marketplace branch from 4a09ecb to 269a518 Compare November 12, 2024 14:32

QuentinI force-pushed the ag/refactor-legacy branch from a7fc02f to b037837 Compare November 12, 2024 15:07

QuentinI force-pushed the ag/refactor-marketplace branch 2 times, most recently from 28d258d to 25012a1 Compare November 12, 2024 15:15

QuentinI force-pushed the ag/refactor-legacy branch 2 times, most recently from 294d4e7 to 4454c92 Compare November 12, 2024 15:27

QuentinI force-pushed the ag/refactor-marketplace branch from 25012a1 to 26433c1 Compare November 12, 2024 15:27

QuentinI force-pushed the ag/refactor-legacy branch 3 times, most recently from 30842fa to b306471 Compare November 12, 2024 15:35

QuentinI force-pushed the ag/refactor-marketplace branch from 26433c1 to 25e9b7b Compare November 12, 2024 15:37

QuentinI force-pushed the ag/refactor-legacy branch from b306471 to fabaa54 Compare November 12, 2024 15:38

QuentinI force-pushed the ag/refactor-marketplace branch from 25e9b7b to bd92200 Compare November 12, 2024 15:40

QuentinI force-pushed the ag/refactor-legacy branch 4 times, most recently from 0bb5c40 to 9de7a38 Compare November 14, 2024 15:48

QuentinI commented Nov 23, 2024

View reviewed changes

crates/legacy/src/service.rs Outdated

Copy link

Contributor Author

QuentinI Nov 23, 2024

Choose a reason for hiding this comment

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

Where most of the actual refactoring happened.

QuentinI commented Nov 23, 2024

View reviewed changes

QuentinI changed the title ~~[WIP] [Refactor] 3: Switch legacy to coordinator~~ [Refactor] 3: Switch legacy to coordinator Nov 25, 2024

QuentinI added 2 commits November 25, 2024 15:51

add mutants

5883103

no test coverage for tests

bcd331e

shenkeyao reviewed Nov 27, 2024

View reviewed changes

dailinsubjam reviewed Nov 27, 2024

View reviewed changes

QuentinI added 2 commits November 27, 2024 13:54

Suggestions

9ed8fab

Merge remote-tracking branch 'origin/main' into ag/refactor-legacy

6c5aeb8

QuentinI force-pushed the ag/refactor-legacy branch from 1c00c95 to fb34e1b Compare November 27, 2024 19:20

Reduce channel size in overflow tests

1f1f21a

QuentinI force-pushed the ag/refactor-legacy branch from fb34e1b to 1f1f21a Compare November 27, 2024 19:50

shenkeyao reviewed Nov 28, 2024

View reviewed changes

Suggestions

64fb108

shenkeyao approved these changes Dec 2, 2024

View reviewed changes

QuentinI added 2 commits December 4, 2024 20:47

Merge remote-tracking branch 'origin/main' into ag/refactor-legacy

a1551a4

cfg-gate the legacy refactor (#269)

7cf96d9

* cfg-gate the refactor

QuentinI force-pushed the ag/refactor-legacy branch from f32ceb8 to 7cf96d9 Compare December 6, 2024 16:33

Bump HotShot

3a9ebc5

QuentinI force-pushed the ag/refactor-legacy branch from 663aeb0 to 11d282b Compare December 6, 2024 17:17

Merge remote-tracking branch 'origin/main' into ag/refactor-legacy

d418ff6

QuentinI force-pushed the ag/refactor-legacy branch from 11d282b to d418ff6 Compare December 6, 2024 17:22

QuentinI merged commit 95de223 into main Dec 6, 2024
10 checks passed

QuentinI deleted the ag/refactor-legacy branch December 6, 2024 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] 3: Switch legacy to coordinator #239

[Refactor] 3: Switch legacy to coordinator #239

QuentinI commented Nov 6, 2024 •

edited

Loading

coveralls commented Nov 6, 2024 •

edited

Loading

QuentinI Nov 23, 2024

QuentinI Nov 23, 2024

QuentinI Nov 23, 2024

QuentinI Nov 23, 2024

QuentinI Nov 23, 2024

QuentinI Nov 23, 2024

QuentinI Nov 23, 2024

QuentinI Nov 23, 2024

shenkeyao left a comment

dailinsubjam Nov 27, 2024

dailinsubjam Nov 27, 2024

QuentinI Nov 27, 2024 •

edited

Loading

dailinsubjam Nov 27, 2024

dailinsubjam Nov 27, 2024

shenkeyao Nov 28, 2024

QuentinI Nov 28, 2024 •

edited

Loading

shenkeyao Dec 2, 2024

[Refactor] 3: Switch legacy to coordinator #239

[Refactor] 3: Switch legacy to coordinator #239

Conversation

QuentinI commented Nov 6, 2024 • edited Loading

This PR:

This PR does not:

Key places to review:

coveralls commented Nov 6, 2024 • 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

Choose a reason for hiding this comment

shenkeyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuentinI Nov 27, 2024 • 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

QuentinI Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuentinI commented Nov 6, 2024 •

edited

Loading

coveralls commented Nov 6, 2024 •

edited

Loading

QuentinI Nov 27, 2024 •

edited

Loading

QuentinI Nov 28, 2024 •

edited

Loading