Propagate unreliable peers upwards from the trie accumulator to the block synchronizer #3594

alsrdn · 2023-01-20T19:18:19Z

The trie accumulator uses a snapshot of the peer list provided by the block synchronizer through the global state synchronizer in order to get the required tries.

Because we didn't provide the block synchronizer any feedback on what the peers that dind't have the global state data were, the block synchronizer may re-issue the global state sync request with the same peer set as for the previous failed attempt. This may lead to delays in getting the global state data.

This PR adds:

the ability to propagate the unreliable peers (that didn't have the requested data) upwards to the global state synchronizer and then to the block synchronizer. This gives feedback that the original peer list provided should be changed if another global sync request is made.
block synchronizer now demotes the peers unreliable received from the global state synchronizer
a test to verify that the peer list is changed based on the feedback from the global state sync components.
slight change into how PeerQuality is handled when demoting and promoting peers; now peers with Reliable or Unreliable quality will not be set as Unknown when demoting or promoting respectively.

These changes reduce the risk that the global state synchronizer gets stuck asking the same peers all over again.

Further potential improvements that can be made:

Promote peers that were successful when downloading global state. We only demote the peers now.
Use a reputation based peer list (similar to the one used in the block synchronizer) in the trie accumulator rather than going through peers one by one.

Fixes: #3563

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

The only time a peer has `Unknown` quality is when it was just added to the peer list and we haven't heard anything from it. If a peer that has `Unknown` quality is promoted we now make it `Reliable` since it means that an item we required was found on that node. Also if a peer with `Unknown` quality is demoted, we make it `Unreliable`. Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

The trie accumulator uses a snapshot of the peer list provided by the block synchronizer through the global state synchronizer in order to get the required tries. Propagate the unreliable peers (that didn't have the requested data) upwards to the global state synchronizer and then to the block synchronizer. This gives feedback that the original peer list provided should be changed if another global sync request is made. Demote the peers received from the global state synchronizer in the block synchronizer. Also add a test to verify that the peer list is changed based on the feedback from the global state sync components. Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

alsrdn · 2023-01-21T11:31:12Z

bors try

casperlabs-bors-ng · 2023-01-21T12:16:36Z

try

Build succeeded:

continuous-integration/drone/push

node/src/components/block_synchronizer.rs

node/src/components/block_synchronizer/peer_list.rs

rafal-ch · 2023-01-24T12:04:51Z

node/src/components/block_synchronizer/tests.rs

+#[tokio::test]
+async fn global_state_sync_wont_stall_with_bad_peers() {


Nice test! 👍

Fix a bug that was pointed out in code review related to the number of peers returned when getting qualified peers. Also add some unit tests for the peer list. Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

rafal-ch

LGTM 👍

Fraser999

One minor change requested, otherwise LGTM.

Fraser999 · 2023-01-27T15:31:27Z

node/src/components/block_synchronizer/global_state_synchronizer.rs

-    TrieAccumulator(TrieAccumulatorError),
-    #[error("ContractRuntime failed to put a trie into global state: {0}")]
-    PutTrie(engine_state::Error),
+    #[error("trie accumulator encountered an error while fetching a trie; unreliable peers {0:?}")]


I think we should avoid mixing debug output into display output. There's utils::DisplayIter which could probably help here.

Same comment for the PutTrie variant below.

Fixed in 18b802d

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

EdHastingsCasperAssociation · 2023-01-30T17:58:55Z

node/src/components/block_synchronizer/global_state_synchronizer.rs

+#[derive(Debug, Clone)]
+pub(crate) struct Response {
+    hash: Digest,
+    unreliable_peers: Vec<NodeId>,
+}
+
+impl Response {
+    pub(crate) fn new(hash: Digest, unreliable_peers: Vec<NodeId>) -> Self {
+        Self {
+            hash,
+            unreliable_peers,
+        }
+    }
+
+    pub(crate) fn hash(&self) -> &Digest {
+        &self.hash
+    }
+
+    pub(crate) fn unreliable_peers(self) -> Vec<NodeId> {
+        self.unreliable_peers
+    }
+}
+


you could do a two state enum for Success / Failure, and flatten the Error as a field on the failure variant; this would allow places that return Result<Response, Error> to be simplified to just return the enum variant (etc).

Yes, we could to that though I'm more partial to having separate types that are clearly delimited. What I think we can do is have a trait implemented for both the Response and Error that can extract the unreliable peers which would make the code looking at the response type potentially more nicer. WDYT?

EdHastingsCasperAssociation · 2023-01-30T18:04:26Z

node/src/components/block_synchronizer/peer_list/tests.rs

+    assert!(peer_list.is_peer_reliable(&test_peer));
+}
+
+#[test]


might as well add the unreliable_peer_remains_unreliable_if_demoted test case

Added in baa3461

EdHastingsCasperAssociation

a nit and a suggestion to consider, but LGTM, gj

EdHastingsCasperAssociation · 2023-01-30T18:07:52Z

bors try

casperlabs-bors-ng · 2023-01-30T18:48:06Z

try

Build succeeded:

continuous-integration/drone/push

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

alsrdn · 2023-02-02T18:54:02Z

bors try

casperlabs-bors-ng · 2023-02-02T19:36:36Z

try

Build succeeded:

continuous-integration/drone/push

alsrdn · 2023-02-02T19:45:28Z

bors merge

casperlabs-bors-ng · 2023-02-02T20:25:20Z

Build succeeded:

continuous-integration/drone/push

alsrdn added 3 commits January 20, 2023 19:07

trie_accumulator: remove unused error variant

c82054d

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

alsrdn marked this pull request as ready for review January 21, 2023 11:29

alsrdn self-assigned this Jan 21, 2023

alsrdn requested review from Fraser999, EdHastingsCasperAssociation, fizyk20, georgepisaltu and rafal-ch January 21, 2023 11:29

casperlabs-bors-ng bot added a commit that referenced this pull request Jan 21, 2023

Try #3594:

6bf44d1

rafal-ch reviewed Jan 24, 2023

View reviewed changes

block_synchronizer/peer_list: fix number of qualified peers

1122fd2

Fix a bug that was pointed out in code review related to the number of peers returned when getting qualified peers. Also add some unit tests for the peer list. Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

alsrdn force-pushed the global-state-sync-bug branch from 0ae4df5 to 1122fd2 Compare January 25, 2023 15:25

rafal-ch approved these changes Jan 26, 2023

View reviewed changes

Fraser999 reviewed Jan 27, 2023

View reviewed changes

alsrdn added 2 commits January 30, 2023 11:14

global state sync: use Display format for error messages

18b802d

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

Merge branch 'dev' into global-state-sync-bug

6f6ac00

Fraser999 approved these changes Jan 30, 2023

View reviewed changes

EdHastingsCasperAssociation reviewed Jan 30, 2023

View reviewed changes

EdHastingsCasperAssociation approved these changes Jan 30, 2023

View reviewed changes

casperlabs-bors-ng bot added a commit that referenced this pull request Jan 30, 2023

Try #3594:

e61cdc2

peer_list: add additional unit test for unreliable peers

baa3461

Signed-off-by: Alexandru Sardan <alexandru@casperlabs.io>

casperlabs-bors-ng bot added a commit that referenced this pull request Feb 2, 2023

Try #3594:

771282e

casperlabs-bors-ng bot merged commit 749cab9 into casper-network:dev Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate unreliable peers upwards from the trie accumulator to the block synchronizer #3594

Propagate unreliable peers upwards from the trie accumulator to the block synchronizer #3594

alsrdn commented Jan 20, 2023 •

edited

Loading

alsrdn commented Jan 21, 2023

casperlabs-bors-ng bot commented Jan 21, 2023

rafal-ch Jan 24, 2023

rafal-ch left a comment

Fraser999 left a comment

Fraser999 Jan 27, 2023

alsrdn Jan 30, 2023

EdHastingsCasperAssociation Jan 30, 2023 •

edited

Loading

alsrdn Jan 30, 2023

EdHastingsCasperAssociation Jan 30, 2023

alsrdn Jan 31, 2023

EdHastingsCasperAssociation left a comment

EdHastingsCasperAssociation commented Jan 30, 2023

casperlabs-bors-ng bot commented Jan 30, 2023

alsrdn commented Feb 2, 2023

casperlabs-bors-ng bot commented Feb 2, 2023

alsrdn commented Feb 2, 2023

casperlabs-bors-ng bot commented Feb 2, 2023

		#[tokio::test]
		async fn global_state_sync_wont_stall_with_bad_peers() {

Propagate unreliable peers upwards from the trie accumulator to the block synchronizer #3594

Propagate unreliable peers upwards from the trie accumulator to the block synchronizer #3594

Conversation

alsrdn commented Jan 20, 2023 • edited Loading

alsrdn commented Jan 21, 2023

casperlabs-bors-ng bot commented Jan 21, 2023

try

rafal-ch Jan 24, 2023

Choose a reason for hiding this comment

rafal-ch left a comment

Choose a reason for hiding this comment

Fraser999 left a comment

Choose a reason for hiding this comment

Fraser999 Jan 27, 2023

Choose a reason for hiding this comment

alsrdn Jan 30, 2023

Choose a reason for hiding this comment

EdHastingsCasperAssociation Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

alsrdn Jan 30, 2023

Choose a reason for hiding this comment

EdHastingsCasperAssociation Jan 30, 2023

Choose a reason for hiding this comment

alsrdn Jan 31, 2023

Choose a reason for hiding this comment

EdHastingsCasperAssociation left a comment

Choose a reason for hiding this comment

EdHastingsCasperAssociation commented Jan 30, 2023

casperlabs-bors-ng bot commented Jan 30, 2023

try

alsrdn commented Feb 2, 2023

casperlabs-bors-ng bot commented Feb 2, 2023

try

alsrdn commented Feb 2, 2023

casperlabs-bors-ng bot commented Feb 2, 2023

alsrdn commented Jan 20, 2023 •

edited

Loading

EdHastingsCasperAssociation Jan 30, 2023 •

edited

Loading