Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unknown list should get header from headermap and db #3702

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

driftluo
Copy link
Collaborator

@driftluo driftluo commented Nov 14, 2022

What problem does this PR solve?

Thanks to @eval-exec for pointing out these problems

ref: #3668

unknown list bug

unknown_list is the key to enabling multi-node parallel download in ibd phase. Its implementation is the get locator algorithm:

ckb/sync/src/types/mod.rs

Lines 2032 to 2035 in 1ce2b34

if index != 0 {
locator.push(self.shared.consensus().genesis_hash());
}
break;

Note this line in the algorithm, all results are added to the genesis hash again at the end

If insert_peer_unknown_header_list can find the corresponding value at the beginning of a new connection, this node can be used as one of the download sources:

ckb/sync/src/types/mod.rs

Lines 1875 to 1889 in 1ce2b34

pub fn insert_peer_unknown_header_list(&self, pi: PeerIndex, header_list: Vec<Byte32>) {
// update peer's unknown_header_list only once
if self.peers.unknown_header_list_is_empty(pi) {
// header list is an ordered list, sorted from highest to lowest,
// so here you discard and exit early
for hash in header_list {
if let Some(header) = self.header_map.get(&hash) {
self.peers.may_set_best_known_header(pi, header);
break;
} else {
self.peers.insert_unknown_header_hash(pi, hash)
}
}
}
}

Otherwise, try_update_best_known_with_unknown_header_list will get stuck in a dead loop on the genesis hash and the node will not be able to become a download source again in the connected state:

ckb/sync/src/types/mod.rs

Lines 1891 to 1903 in 1ce2b34

pub fn try_update_best_known_with_unknown_header_list(&self, pi: PeerIndex) {
// header list is an ordered list, sorted from highest to lowest,
// when header hash unknown, break loop is ok
while let Some(hash) = self.peers().take_unknown_last(pi) {
if let Some(header) = self.header_map.get(&hash) {
self.peers.may_set_best_known_header(pi, header);
} else {
self.peers.insert_unknown_header_hash(pi, hash);
break;
}
}
}
}

Finally, the try_update_best_known_with_unknown_header_list function is removed and get_header_view is used instead of just looking from the headermap.

Because if the block is inserted into the DB and the header map is cleaned up, the unknown list may get stuck in this situation where the hash cannot be found, resulting in reduce the download of the source.

eviction bug

In the ibd process, a node can only request headers data from one peer at a time. For headers, multiple data sources will result in duplicate downloads.

When synchronization starts, the headers_sync_controller of the node will be initialized to the normal state:

if let Some(ref mut controller) = state.headers_sync_controller {
let better_tip_ts = better_tip_header.timestamp();
if let Some(is_timeout) = controller.is_timeout(better_tip_ts, now) {
if is_timeout {
eviction.push(*peer);
continue;
}
} else {
active_chain.send_getheaders_to_peer(nc, *peer, &better_tip_header);
}
}

eviction's remaining judgments will likely result in simultaneous requests for headers to multiple data sources, which is unnecessary in the ibd phase and should be disabled

Check List

Tests

  • Unit test
  • Integration test

Release note

Title Only: Include only the PR title in the release note.

@driftluo driftluo requested a review from a team as a code owner November 14, 2022 11:05
@driftluo driftluo requested review from zhangsoledad and removed request for a team November 14, 2022 11:05
@driftluo driftluo force-pushed the fix-unknown-list-insert branch 3 times, most recently from 266de54 to 6cf3d1b Compare November 14, 2022 11:12
Copy link
Collaborator

@eval-exec eval-exec left a comment

Choose a reason for hiding this comment

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

LGTM.

sync/src/synchronizer/get_headers_process.rs Outdated Show resolved Hide resolved
sync/src/synchronizer/get_headers_process.rs Outdated Show resolved Hide resolved
@driftluo driftluo force-pushed the fix-unknown-list-insert branch from 1a9b952 to d0ed577 Compare November 16, 2022 04:17
@driftluo driftluo force-pushed the fix-unknown-list-insert branch from d0ed577 to 0d1bf47 Compare November 16, 2022 04:18
@driftluo driftluo changed the title fix: unknown list insert should remove genesis hash fix: unknown list should get header from headermap and db Nov 16, 2022
@driftluo driftluo requested a review from quake November 16, 2022 04:22
@driftluo
Copy link
Collaborator Author

bors r+

@bors bors bot merged commit da8f71e into nervosnetwork:develop Nov 16, 2022
@driftluo driftluo deleted the fix-unknown-list-insert branch November 16, 2022 06:05
@doitian doitian mentioned this pull request Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants