Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Blocks propagation #364

Merged
merged 30 commits into from Feb 8, 2016
Merged

Blocks propagation #364

merged 30 commits into from Feb 8, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Feb 5, 2016

sends new known hashes to peers that probably have not got em

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 5, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 5, 2016

clients that are too far behind should not be tried to be updated

_ => lagging_peers.iter().filter(|_| ::rand::random::<u8>() < 64u8).cloned().collect::<Vec<(usize, H256)>>()
};

match lucky_peers.len() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could be just lucky_peers.resize(min(MIN_PEERS_PROPAGATION, lucky_peers.len())) or lucky_peers.drain(...)

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 6, 2016
}

fn create_latest_block_rlp(chain: &BlockChainClient) -> Bytes {
chain.block(&chain.chain_info().best_block_hash).unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Error should be handled here

@arkpar arkpar added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 6, 2016
@arkpar arkpar removed the A0-pleasereview 🤓 Pull request needs code review. label Feb 7, 2016
trace!(target: "sync", "Sent new hashes to peers: {:?}", peers);
}

pub fn chain_blocks_verified(&mut self, io: &mut SyncIo) {
Copy link
Contributor

Choose a reason for hiding this comment

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

missing documentation.

@gavofyork
Copy link
Contributor

would like to see a little bit of documentation on the private functions

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Feb 7, 2016
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Feb 7, 2016
Conflicts:
	util/src/journaldb.rs
@NikVolf NikVolf added A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 7, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 7, 2016

ah when writing tests in other branch discovered that packet for all propagades should contain difficulty as well

  • resolved

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A4-gotissues 💥 Pull request is reviewed and has significant issues which must be addressed. labels Feb 7, 2016
@@ -38,6 +38,8 @@ pub struct BlockQueueInfo {
pub verified_queue_size: usize,
/// Number of blocks being verified
pub verifying_queue_size: usize,
/// Indicates queue is empty
pub empty: bool
Copy link
Contributor

Choose a reason for hiding this comment

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

is this really a necessary optimisation? (why not fn empty(&self) -> bool { verification.verified.is_empty() && verification.unverified.is_empty() && verification.verifying.is_empty() }?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

function for BlockQueueInfo?
seems reasonable
I guess 'full' should be function as well then, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • resolved

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 8, 2016
gavofyork pushed a commit that referenced this pull request Feb 8, 2016
@gavofyork gavofyork merged commit 666a1c3 into master Feb 8, 2016
@NikVolf NikVolf mentioned this pull request Feb 8, 2016
@NikVolf NikVolf deleted the block-propagation branch February 8, 2016 15:33
@dvdplm dvdplm mentioned this pull request May 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants