fuel-core-sync #889

freesig · 2023-01-05T04:50:22Z

Closes [Sync] Sync task #881
Closes [Sync] Import task #882
Closes [Sync] Create sync service wrapper for sync tasks #880

…tdown longer.

…uel-core-sync

xgreenx

The change looks solid, so many tests=D I didn't review all files, will continue later. I left several questions and suggestion

crates/services/src/lib.rs

crates/services/src/service.rs

crates/services/sync/src/import.rs

crates/services/sync/src/ports.rs

xgreenx · 2023-01-17T15:28:09Z

crates/services/sync/src/state.rs

+/// This case should not occur but because we must handle
+/// it then the most resilient way is to assume that we
+/// should re-process the gap.
+fn commit_creates_processing(


I didn't get it. Why do we want to return Processing for the range that is committed before?

I guess if new_height < existing_height, we need to return Committed(existing_height), because it means that we already processed the new_height..existing_height range before.

So this is a bit of a hard one. Really by design we can't ever have this happen because the commits are run in order and there's no state that can lead to re-running a commit. However I can't handle this case as an error at the stream level because there's nothing you can do so for the sake of completeness I needed to do something here. If we get a commit that is lower then the current commit then something is wrong and my way of looking at this is we can't trust the higher commit value so it should reprocess the headers in between. But you could make an argument either way.

crates/services/sync/src/import.rs

crates/services/sync/src/service.rs

ControlCplusControlV

Read through changes but didn't have too much to say from my end, so defer to others as well. But looks good beyond Green's suggestions

crates/services/src/service.rs

crates/services/sync/src/service.rs

crates/services/sync/src/state.rs

xgreenx

Nice work! I left several comments. You can address them now or later=)

xgreenx · 2023-01-19T18:08:50Z

crates/services/sync/src/import.rs

+            let p2p = p2p.clone();
+            let consensus_port = consensus.clone();
+            move |result| {
+                let p2p = p2p.clone();
+                let consensus_port = consensus_port.clone();
+                async move {


Hmm, it seems we can create this function one time during the creation of the task and use it here. Instead of creating it on each iteration.

But it will affect your current code, so I guess it is enough for now to create it at the begin of the function.

Is that really a concern though? Very likely this just gets optimized away by the compiler.

Also we can't do that because this takes the closure owned so you would need to clone the closure anyway.

xgreenx · 2023-01-19T18:11:15Z

crates/services/sync/src/import.rs

+            let state = state.clone();
+            let executor = executor.clone();
+            move |block| {


The same here, we can create it only one time

xgreenx · 2023-01-19T18:30:46Z

crates/services/sync/src/import/back_pressure_tests.rs

+        max_get_header_requests: 10,
+        max_get_txns_requests: 10,
+    }
+    => is less_or_equal_than Count{ headers: 10, transactions: 10, consensus_calls: 10, executes: 1, blocks: 21 }


Hmm, how does comparation work? It will compare each field and only if all fields <= then true?

I'm assuming it just uses the existing derivation of Ord/PartialOrd for the Count type

Yeh exactly. It's just checking the tests never exceed those maximum values

xgreenx · 2023-01-19T18:37:42Z

crates/services/sync/src/import.rs

+}
+impl<P, E, C> Import<P, E, C>
+where
+    P: PeerToPeerPort + Send + Sync + 'static,


[nit] I guess you can make Send + Sync default bound for all ports like PeerToPeerPort: Send + Sync

There's no reason for these traits to be send and sync it's only in this usecase that it's required.

xgreenx · 2023-01-19T18:43:25Z

crates/services/sync/src/service.rs

+where
+    P: ports::PeerToPeerPort + Send + Sync + 'static,
+    E: ports::BlockImporterPort + Send + Sync + 'static,
+    C: ports::ConsensusPort + Send + Sync + 'static,


Hmmm, do we need those constraints here? Maybe we can remove them like:

Suggested change

where

P: ports::PeerToPeerPort + Send + Sync + 'static,

E: ports::BlockImporterPort + Send + Sync + 'static,

C: ports::ConsensusPort + Send + Sync + 'static,

where

P: ports::PeerToPeerPort,

And for types down, we can remove all constraints, because we don't need them during creation of tasks=)

@xgreenx we actually do because you can't add extra constraints to the run on RunnableTask so we are forced to put them on the type.

You can add constraints on the impl section itself, we did that for all other services.

Voxelot · 2023-01-19T21:26:10Z

crates/types/src/services/p2p.rs

+
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+/// Opaque peer identifier.
+pub struct PeerId(Vec<u8>);


We should make a tech-debt cleanup task for updating the other gossip data types here to use this new PeerId type.

freesig force-pushed the freesig/fuel-core-sync branch from b121512 to 7b101ae Compare January 10, 2023 03:48

freesig and others added 5 commits January 11, 2023 11:21

add state watcher to all runs

e30edab

Updated all services to handle watcher

3125885

Changed the default behavior of relayer to not lose the state and shu…

b9d9100

…tdown longer.

start on fuel-core-services

e063cdd

change vis

68ea3cb

freesig force-pushed the freesig/fuel-core-sync branch from 7b101ae to 68ea3cb Compare January 11, 2023 02:14

freesig added 4 commits January 11, 2023 18:14

wire up sync to service runner

c8db586

add typed state machine and testing

28c26cd

Added 100 unit tests and rewrote state

a3b769f

error handling and unit tests for import task

738fdd4

Voxelot linked an issue Jan 16, 2023 that may be closed by this pull request

[Sync] Sync task #881

Closed

freesig added 5 commits January 17, 2023 14:08

Add looping tests and new service function

4cf8dba

add back pressure tests

e7aebc8

add service test

6157891

comments and docs

01c47ad

Merge branch 'master' of github.com:FuelLabs/fuel-core into freesig/f…

b770f64

…uel-core-sync

freesig marked this pull request as ready for review January 17, 2023 07:29

freesig requested a review from a team January 17, 2023 07:31

xgreenx reviewed Jan 17, 2023

View reviewed changes

ControlCplusControlV assigned freesig Jan 17, 2023

ControlCplusControlV added the fuel-sync label Jan 17, 2023

ControlCplusControlV requested a review from a team January 17, 2023 16:59

ControlCplusControlV reviewed Jan 17, 2023

View reviewed changes

crates/services/src/service.rs Outdated Show resolved Hide resolved

crates/services/sync/src/service.rs Outdated Show resolved Hide resolved

crates/services/sync/src/state.rs Outdated Show resolved Hide resolved

ControlCplusControlV requested a review from a team January 17, 2023 19:30

freesig added 2 commits January 18, 2023 11:58

pr fixes

0c99fc7

change task handling to service runner

9529fc6

freesig requested a review from xgreenx January 18, 2023 06:28

freesig added 2 commits January 19, 2023 09:42

pr suggestions

65f1b21

remove execution result

f65ae05

freesig added 2 commits January 19, 2023 11:41

fix flakey test

a519b2d

move where clause

42b5aea

xgreenx approved these changes Jan 19, 2023

View reviewed changes

Voxelot reviewed Jan 19, 2023

View reviewed changes

Voxelot approved these changes Jan 19, 2023

View reviewed changes

freesig merged commit 2301e15 into master Jan 19, 2023

freesig deleted the freesig/fuel-core-sync branch January 19, 2023 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fuel-core-sync #889

fuel-core-sync #889

freesig commented Jan 5, 2023 •

edited

xgreenx left a comment

xgreenx Jan 17, 2023

freesig Jan 19, 2023

ControlCplusControlV left a comment

xgreenx left a comment

xgreenx Jan 19, 2023

freesig Jan 19, 2023

freesig Jan 19, 2023

xgreenx Jan 19, 2023

xgreenx Jan 19, 2023

Voxelot Jan 19, 2023

freesig Jan 19, 2023

xgreenx Jan 19, 2023

freesig Jan 19, 2023

xgreenx Jan 19, 2023

xgreenx Jan 19, 2023

freesig Jan 19, 2023

xgreenx Jan 19, 2023

Voxelot Jan 19, 2023

fuel-core-sync #889

fuel-core-sync #889

Conversation

freesig commented Jan 5, 2023 • edited

xgreenx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ControlCplusControlV left a comment

Choose a reason for hiding this comment

xgreenx left a 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

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

freesig commented Jan 5, 2023 •

edited