-
Notifications
You must be signed in to change notification settings - Fork 228
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
feat: update last_common_header only in find_blocks_to_fetch #2081
Conversation
Is there any benchmark to show the differences among the three different strategies?
|
@keroro520 CI failed, may need to rebase, please check the log |
No benchmark for this change, as I don't think it is necessary to bench it.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Differences caused by delayed update common:
- There may be a small increase in the consumption of
get_ancester
- Unify all update points, and can unify the common point of each peer relatively quickly. If there is a stuck or a large number of orphan block updates, this is the advantage
c27b0ba
to
e99b150
Compare
benchmark |
Benchmark Result
|
@@ -9,7 +9,7 @@ use ckb_types::{packed, prelude::*}; | |||
pub struct BlockProcess<'a> { | |||
message: packed::SendBlockReader<'a>, | |||
synchronizer: &'a Synchronizer, | |||
peer: PeerIndex, | |||
_peer: PeerIndex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above two code has changed/removed, peer
is unnecessary for BlockProcess
.
However, removing peer
from BlockProcess
would affect many test codes. I just change to the field name to _peer
to disable the compiler noise.
|
||
// NOTE: Never use `LruCache` as container. We have to ensure synchronizing between | ||
// orphan_block_pool and block_status_map, but `LruCache` would prune old items implicitly. | ||
#[derive(Default)] | ||
pub struct OrphanBlockPool { | ||
blocks: RwLock<HashMap<ParentHash, HashMap<packed::Byte32, OrphanBlock>>>, | ||
blocks: RwLock<HashMap<ParentHash, HashMap<packed::Byte32, core::BlockView>>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How this change is related to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is about reverting #1997 .
#1997 records peer-index in OrphanBlockPool
so that we can update the peer-state once accepting an orphan block.
#1997 is required originally, which we update the corresponding peer's peer.last_common_header
every time accepting a new block.
This PR changes the way to update peer.last_common_header
. We don't need to record peer-index inside OrphanBlockPool
, hence I remove it.
bors r=quake,doitian |
Build succeeded:
|
peer.best_known_block
refers to the best-known block we know this peer has announced;peer.last_common_header
refers to the last block we both stored between local and peer. This PR proposes the logic to update the two fields:CompactBlock
orSentHeader
frompeer
, we update thepeer.best_known_block
if the arrival one has greatertotal_difficulty
.peer
, we don't update the correspondinglast_common_header
(remove lines CompactBlock, SendBlock).last_common_header
only infind_blocks_to_fetch
. There are 2 places,fn update_last_common_header
fixeslast_common_header
when the peer has reorganized;last_common_header
when we know a better block has already stored.Fix bug:
status == BLOCK_STORED
=>status.contains(BLOCK_STORED)
Enhence
last_common_ancestor
.