refactor synchronizer by turuslan · Pull Request #2433 · qdrvm/kagome

turuslan · 2025-04-10T14:34:55Z

Referenced issues

Description of the Change

should also fix [Bug]: Ancestry is never removed #2420 ancestry_ not removed, each KnownBlock stores it's ancestry_ iterator
separate fetchGrandpaFork function
remove syncState peer arg, simplify syncState callback
remove syncByBlockInfo/syncByBlockHeader ambigous callback
use NewHead event in timeline to check catch up status
checkExtrinsicRoot
check --max-parallel-downloads at least 1

Sync

types of blocks
- finalized canonical - in db
- finalized forks - should be discarded
- unfinalized - in db
- "attached" - in memory, received headers
  - headers form multiple trees
  - root parent is unfinalized or last finalized block
  - root will be executed (full sync) or appended (fast sync) next
  - ready to be imported, may need to fetch body to execute (full sync)
  - when sync doesn't lag, received block announce is attached
- "detached" - in memory, received headers
  - headers form multiple trees
  - root parent has no known parent
  - fetch more headers down from root to reach known blocks, merge with detached or attached, or become attached
  - when sync lags, received block announce is detached
types of requests
- "gap" - fetch headers descending parent chain to known blocks
- "body" - fetch body to execute during full sync
- "range" - fetch peer best chain, by number, ascending from top
  - useful when there are no block announces or they are too far
  - optimistic version of "find common block"
    - if peer chain is not fork we insert that chain and continue fetching more from new max
    - even if that chain is fork, we fetch more from new max, and fetch "gap" when that chain diverges
synchronizer knows attached/detached roots of known blocks trees, so it can plan and prefetch header/body from different peers in parallel

Possible Drawbacks

validate seal and vrf for current and next session

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.

Files not reviewed (1)

test/core/network/CMakeLists.txt: Language not supported

Comments suppressed due to low confidence (3)

test/mock/core/network/synchronizer_mock.hpp:19

[nitpick] The refactor removes the boolean-based syncByBlockInfo and syncByBlockHeader methods in favor of void-returning notification methods. Please ensure that all tests and mocks depending on the old behavior are updated to use the new interface.

MOCK_METHOD(void, onBlockAnnounceHandshake, (const BlockInfo &, const PeerId &), (override));

test/core/network/synchronizer_test.cpp:1

The entire synchronizer test file has been removed. Confirm that synchronizer-related functionality remains adequately exercised by other tests or that this removal is intentional.

/* Entire file removed */

core/network/impl/synchronizer_impl.hpp:271

[nitpick] The new variables attached_roots_ and detached_roots_ are central to managing block roots. Consider adding inline documentation clarifying their specific roles and how they relate to the ancestry structure.

std::set<BlockInfo> attached_roots_;

kamilsa · 2025-04-11T07:05:53Z

    SL_WARN(log_, "sync hung up, restarting sync");
    known_blocks_.clear();
-    generations_.clear();
+    attached_roots_.clear();


Some random thoughts. Maybe instead of relying on timer, it would be more straightforward to implement ttl containers for these data?

This isn't TTL, it's workaround timeout which resets synchronizer state if it hangs, which isn't normal situation.

so, in normal situation this container (and maybe others) will grow indefinetely?

kamilsa · 2025-04-11T07:22:11Z

+            return VisitAncestryResult::STOP;
+          }
+          if (not block.data.body.has_value()) {
+            auto block_info = block.data.header.value().blockInfo();


Used in 3 lines below

kamilsa · 2025-04-11T11:59:17Z

-        auto remove_executing_blocks =
+      auto &known = known_blocks_.at(block_info.hash);
+      auto make_cb = [&] {
+        executing_blocks_.emplace(block_info);


block_info is captured by reference. make_cb is invoked after block execution, which happens in separate thread if am not mistaken. Can't we end up with dangling reference to block_info ?

make_cb returns cb to use as arg to append/execute functions, so it's called before them

kamilsa · 2025-04-11T12:20:53Z

+          continue;
+        }
+        if (auto known = entry(known_blocks_, block.hash)) {
+          if (not checkExtrinsicRoot(known->data.header.value(), *block.body)) {


Hopefully, we cannot skip extrinsic root validation if block was not known

loadBlocks doesn't insert body into known_blocks_,
it requires header with extrinsic root to be known.

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

xDimon · 2025-04-15T11:04:33Z

Please, provide new set of tests for new mechanism of sync.

xDimon · 2025-04-16T07:38:31Z

+      return;
    }
-    return res;
+    fetchTasks();


Seems new unknown block will just be ignored.

Handshake contains only block hash and number, not header.

If block is not in known_blocks_, it can either be already in db (block tree) or new unknown block.

xDimon · 2025-04-16T08:14:16Z

+    auto cb = [WEAK_SELF, peer_id, request, reason, busy{std::move(busy)}](
+                  outcome::result<BlocksResponse> response_res) mutable {
+      WEAK_LOCK(self);
+      busy.reset();


Should fetching be decremented here?

It's decremented.
busy.reset() -> ~MovableFinalAction -> --fetching

xDimon · 2025-04-16T08:15:05Z

-                   from);
-          if (handler) {
-            handler(Error::WRONG_ORDER);
+        if (block.body.has_value()) {


Should we handle body in case of header absence?

We add body to known_blocks_ if header is already there.
If response contains both header and body, header is added to known_blocks_ first

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

xDimon · 2025-04-24T09:06:47Z

      BlocksRequest request{
          .fields = BlockAttribute::HEADER | BlockAttribute::JUSTIFICATION,
-          .from = root.hash,
+          .from = known_blocks_.at(root.hash).data.header.value().parent_hash,


.at() potential source of exception. Does it guaranteed known_blocks_ contains root.hash? Use op[] if yes, or add check up otherwise.

.at() was used intentionally, because std::unordered_map::operator[] doesn't assert.
known_blocks_, attached_roots_, detached_roots_, and ancestry_ are assumed to be consistent,
but assert/exception will protect from breaking that consistency.

xDimon · 2025-04-24T09:10:45Z

-    if (request.direction == Direction::ASCENDING) {
-      std::ranges::reverse(blocks);
-    }


Are you sure this should be removed?

It was moved to other place

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Signed-off-by: turuslan <turuslan.devbox@gmail.com> # Conflicts: # core/application/impl/app_configuration_impl.cpp

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

refactor synchronizer

040c106

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

turuslan requested review from kamilsa and xDimon April 10, 2025 15:09

clang-tidy

474928d

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

kamilsa requested a review from Copilot April 11, 2025 05:34

Copilot AI reviewed Apr 11, 2025

kamilsa requested a review from Copilot April 11, 2025 06:04

Copilot AI reviewed Apr 11, 2025

View reviewed changes

kamilsa reviewed Apr 11, 2025

View reviewed changes

kamilsa and others added 8 commits April 11, 2025 17:25

Add comments

8fdfae3

Merge remote-tracking branch 'origin/master' into refactor/synchronizer

e177f98

rename

264740f

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

simplify reverse

ba70f79

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

const validate

584ca63

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

rename

e8e86d5

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

comment

d996024

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

comment

9cab364

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

turuslan requested a review from kamilsa April 14, 2025 16:08

xDimon reviewed Apr 16, 2025

View reviewed changes

turuslan added 10 commits April 21, 2025 15:44

const, reverse

e7ca55c

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

comment

c921cef

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

dont ignore announces above executing

591070f

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

unordered set

d21b19d

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

rename

0c3524d

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

rename

d8e0e3c

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

test

a6aa8ad

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

slot increasing

509dc23

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Merge remote-tracking branch 'origin/master' into refactor/synchronizer

b077f2c

debug print

6cc0e78

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

turuslan requested a review from xDimon April 23, 2025 03:49

comment

7b3ea9e

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

xDimon reviewed Apr 24, 2025

View reviewed changes

turuslan added 6 commits April 24, 2025 14:41

future block

e1874e8

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

wrong return

b1307f6

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

update roots on finalize

fd6b170

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

state sync choose peer and retry

dee82a5

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Merge remote-tracking branch 'origin/master' into refactor/synchronizer

3e6a320

Signed-off-by: turuslan <turuslan.devbox@gmail.com> # Conflicts: # core/application/impl/app_configuration_impl.cpp

fetch range if above best

befb8e3

Signed-off-by: turuslan <turuslan.devbox@gmail.com>

Conversation

turuslan commented Apr 10, 2025

Referenced issues

Description of the Change

Sync

Possible Drawbacks

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turuslan Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

turuslan Apr 24, 2025 •

edited

Loading