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

Major sync <-> client interactions refactoring #1572

Merged
merged 32 commits into from
Jul 11, 2016
Merged

Major sync <-> client interactions refactoring #1572

merged 32 commits into from
Jul 11, 2016

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Jul 8, 2016 •
edited
Loading

  • All interactions are done through a number of traits now

ChainNotify
ManageNetwork
SyncProvider

  • ethsync now maintains network
  • io-messaging removed from the network service
  • it is possible to have a client without any network now

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 8, 2016
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jul 8, 2016
arkpar
arkpar reviewed Jul 9, 2016
View reviewed changes
ethcore/src/block_queue.rs
@@ -104,7 +104,7 @@ struct VerifyingBlock {
struct QueueSignal {
deleting: Arc<AtomicBool>,
signalled: AtomicBool,
message_channel: IoChannel<NetSyncMessage>,
message_channel: IoChannel<SyncMessage>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

SyncMessage should be renamed to IoMessage or ClientIoMessage

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 10, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 10, 2016

@gavofyork resolved!

gavofyork
gavofyork reviewed Jul 10, 2016
View reviewed changes
sync/src/lib.rs
fn stop_network(&self) {
self.network.with_context(ETH_PROTOCOL, |context| {
let mut sync_io = NetSyncIo::new(context, self.handler.chain.deref());
self.handler.sync.write().unwrap().abort(&mut sync_io);
Copy link
Contributor

Choose a reason for hiding this comment

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

unwrapped_write()

@gavofyork
Copy link
Contributor

many more .lock/read/write().unwrap()s have been re-introduced. more careful merging needed.

@gavofyork gavofyork 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 Jul 10, 2016
@NikVolf
Copy link
Contributor Author

NikVolf commented Jul 10, 2016

@gavofyork 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 Jul 10, 2016
gavofyork
gavofyork reviewed Jul 11, 2016
View reviewed changes
sync/src/lib.rs
/// Creates and register protocol with the network service
pub fn new(config: SyncConfig, chain: Arc<Client>, network_config: NetworkConfiguration) -> Arc<EthSync> {
let chain_sync = ChainSync::new(config, chain.deref());
let service = NetworkService::new(network_config).unwrap();
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 new? if so, prove or remove

@gavofyork gavofyork added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Jul 11, 2016
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 11, 2016
@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 11, 2016
@arkpar arkpar merged commit d3695d0 into master Jul 11, 2016
@arkpar arkpar deleted the sync-net-refact branch July 11, 2016 15:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NikVolf @gavofyork @arkpar