fix(dot/sync): execute p2p handshake when there is no target #3695

EclesioMeloJunior · 2024-01-15T23:33:56Z

Changes

Gossamer is not able to run bootstrap sync without a block target, the target is calculated given the peer's block announcements handshake (which contains the peer best block and best hash), so Gossamer waits for peers to send the data.
This PR introduces a change in the waitEnoughPeersAndTarget, basically this function after X seconds will, pro-actively, execute a block announcement handshake with the connected bootnodes, given the responses Gossamer can calculates the target and start syncing

Tests

WIP

Issues

Closes Gossamer sometimes don't start syncing when starting #3622

Primary Reviewer

@timwu20 @q9f

dot/sync/syncer.go

dimartiro · 2024-01-16T01:32:46Z

Why do we have to wait instead of just start syncing from bootnodes? I guess it is to prevent overload the bootnodes and being a "good ~~samaritan~~ client" but I want to be sure 😅

dimartiro

Very good idea to have introduced the peerViewSet struct, I added a couple of suggestions to take advantage of that structure but they are not blockers

dot/network/light.go

dot/sync/chain_sync.go

…method

…hainSafe/gossamer into eclesio/start-sync-from-block-announce

EclesioMeloJunior · 2024-01-16T15:28:55Z

Why do we have to wait instead of just start syncing from bootnodes? I guess it is to prevent overload the bootnodes and being a "good ~~samaritan~~ client" but I want to be sure 😅

Actually, this is a good option, we will not overload the network because this is just a handshake, which means we will not act as bad actors since this action will only take place at the start of the node. Will change to don't wait for peers/target but request those informations from bootnode peers and if there is no peers then we should wait

…hainSafe/gossamer into eclesio/start-sync-from-block-announce

…ions`

codecov · 2024-01-16T20:08:13Z

Codecov Report

Attention: 57 lines in your changes are missing coverage. Please review.

Comparison is base (e03fa13) 50.55% compared to head (44c08f0) 50.28%.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #3695      +/-   ##
===============================================
- Coverage        50.55%   50.28%   -0.27%     
===============================================
  Files              229      230       +1     
  Lines            28605    28663      +58     
===============================================
- Hits             14462    14414      -48     
- Misses           12624    12730     +106     
  Partials          1519     1519

dot/network/service.go

dot/sync/chain_sync_test.go

dimartiro

LGTM, agree with Axay's comment to remove / uncomment the test cases

dot/sync/chain_sync.go

jimjbrettj

nice work, lgtm

axaysagathiya

LGTM

# [0.9.0](v0.8.0...v0.9.0) (2024-3-1) ### Bug Fixes * add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89)) * **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32)) * don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de)) * **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec)) * fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384)) * fix non deterministic panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243)) * **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9)) * **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e)) * **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d)) * segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa)) * **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14)) * update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28)) * use last finalized block on startup ([#3737](#3737)) ([c262642](c262642)) ### Features * **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9)) * **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa)) * **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))

github-actions · 2024-03-01T16:43:41Z

🎉 This PR is included in version 0.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

# [0.9.0](v0.8.0...v0.9.0) (2024-3-1) ### Bug Fixes * add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89)) * **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32)) * don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de)) * **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec)) * fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384)) * fix non deterministic panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243)) * **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9)) * **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e)) * **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d)) * segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa)) * **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14)) * update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28)) * use last finalized block on startup ([#3737](#3737)) ([c262642](c262642)) ### Features * **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9)) * **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa)) * **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))

chore: execute a block announce if nothing arrives at time

f244fcd

EclesioMeloJunior requested review from edwardmack, timwu20, jimjbrettj, kishansagathiya, dimartiro and axaysagathiya as code owners January 15, 2024 23:33

Merge branch 'development' into eclesio/start-sync-from-block-announce

4a59ca7

dimartiro reviewed Jan 16, 2024

View reviewed changes

dot/sync/syncer.go Outdated Show resolved Hide resolved

dimartiro previously approved these changes Jan 16, 2024

View reviewed changes

dot/network/light.go Outdated Show resolved Hide resolved

dot/sync/chain_sync.go Outdated Show resolved Hide resolved

dot/sync/chain_sync.go Outdated Show resolved Hide resolved

dot/sync/chain_sync.go Outdated Show resolved Hide resolved

q9f added the P-high this should be addressed ASAP. label Jan 16, 2024

chore: update mocks and fix broken tests

46b3ceb

EclesioMeloJunior changed the title ~~fix(dot/sync): execute a block announce when there is no target~~ fix(dot/sync): execute p2p handshake when there is no target Jan 16, 2024

EclesioMeloJunior added 4 commits January 16, 2024 10:18

chore: moving peerset view to its own file and introduce getTarget …

746d6ad

…method

Merge branch 'eclesio/start-sync-from-block-announce' of github.com:C…

c28fb42

…hainSafe/gossamer into eclesio/start-sync-from-block-announce

chore: make peersetview.update simpler

f43b9de

Merge branch 'development' into eclesio/start-sync-from-block-announce

a409b03

EclesioMeloJunior added 5 commits January 16, 2024 11:58

chore: dont wait if there is bootnodes available

92cb283

Merge branch 'eclesio/start-sync-from-block-announce' of github.com:C…

6b31457

…hainSafe/gossamer into eclesio/start-sync-from-block-announce

Merge branch 'development' into eclesio/start-sync-from-block-announce

5b3420d

chore: fix broken tests

06b8aae

chore: fix broken tests, rename isBootstrap to `currentSyncInformat…

d972609

…ions`

chore: fix bootnodes handshake

4d2458d

P1sar added the S-network issues related to the dot/network package. label Jan 17, 2024

EclesioMeloJunior dismissed dimartiro’s stale review via 4d2458d January 17, 2024 10:36

EclesioMeloJunior requested a review from dimartiro January 17, 2024 13:06

axaysagathiya reviewed Jan 17, 2024

View reviewed changes

dot/network/service.go Outdated Show resolved Hide resolved

dot/network/service.go Outdated Show resolved Hide resolved

dot/sync/chain_sync_test.go Outdated Show resolved Hide resolved

dimartiro previously approved these changes Jan 17, 2024

View reviewed changes

chore: uncomment test cases

cd0a4e4

EclesioMeloJunior dismissed dimartiro’s stale review via cd0a4e4 January 17, 2024 14:46

EclesioMeloJunior added 2 commits January 17, 2024 10:48

chore: address reviews

4d596c3

Merge branch 'development' into eclesio/start-sync-from-block-announce

a05a3bb

EclesioMeloJunior requested review from axaysagathiya and dimartiro January 17, 2024 15:25

dimartiro approved these changes Jan 17, 2024

View reviewed changes

Merge branch 'development' into eclesio/start-sync-from-block-announce

44c08f0

jimjbrettj reviewed Jan 17, 2024

View reviewed changes

dot/sync/chain_sync.go Show resolved Hide resolved

jimjbrettj approved these changes Jan 17, 2024

View reviewed changes

axaysagathiya approved these changes Jan 18, 2024

View reviewed changes

EclesioMeloJunior merged commit a9db0ec into development Jan 18, 2024
23 of 24 checks passed

EclesioMeloJunior deleted the eclesio/start-sync-from-block-announce branch January 18, 2024 12:06

github-actions bot added the released label Mar 1, 2024

timwu20 pushed a commit that referenced this pull request Apr 18, 2024

fix(dot/sync): execute p2p handshake when there is no target (#3695)

ffef22a

timwu20 pushed a commit that referenced this pull request Apr 19, 2024

fix(dot/sync): execute p2p handshake when there is no target (#3695)

4c0145b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dot/sync): execute p2p handshake when there is no target #3695

fix(dot/sync): execute p2p handshake when there is no target #3695

EclesioMeloJunior commented Jan 15, 2024

dimartiro commented Jan 16, 2024 •

edited

Loading

dimartiro left a comment •

edited

Loading

EclesioMeloJunior commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

dimartiro left a comment

jimjbrettj left a comment

axaysagathiya left a comment

github-actions bot commented Mar 1, 2024

fix(dot/sync): execute p2p handshake when there is no target #3695

fix(dot/sync): execute p2p handshake when there is no target #3695

Conversation

EclesioMeloJunior commented Jan 15, 2024

Changes

Tests

Issues

Primary Reviewer

dimartiro commented Jan 16, 2024 • edited Loading

dimartiro left a comment • edited Loading

Choose a reason for hiding this comment

EclesioMeloJunior commented Jan 16, 2024 • edited Loading

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

dimartiro left a comment

Choose a reason for hiding this comment

jimjbrettj left a comment

Choose a reason for hiding this comment

axaysagathiya left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 1, 2024

dimartiro commented Jan 16, 2024 •

edited

Loading

dimartiro left a comment •

edited

Loading

EclesioMeloJunior commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading