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

Snapshot sync #2047

Merged
merged 7 commits into from
Sep 6, 2016
Merged

Snapshot sync #2047

merged 7 commits into from
Sep 6, 2016

Conversation

arkpar
Copy link
Collaborator

@arkpar arkpar commented Sep 4, 2016

  • Enable snapshot service over IPC
  • Sync to the snapshot from other peers

Still left to do:

@arkpar arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Sep 4, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 84.385% when pulling a98733d on pv64-sync into 59f18ab on master.

@@ -122,11 +122,13 @@ impl SleepState {
/// Call `import_block()` to import a block asynchronously; `flush_queue()` flushes the queue.
pub struct Client {
mode: Mode,
chain: Arc<BlockChain>,
tracedb: Arc<TraceDB<BlockChain>>,
chain: RwLock<Arc<BlockChain>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

RwLock<Arc<T>> doesn't guarantee or provide anything as Arc only has a Deref implementation

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a lock for a shared pointer. It is required to synchronously replace the pointer with another once.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But you can always just clone the Arc instead of accessing the shared pointer. Maybe it's worthing wrapping Arc<BlockChain> into a structure with Deref but without Clone to prevent such situations?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would favor giving Client full ownership of BlockChain, only loaning it out temporarily to the TraceDB.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tomusdrw I need to replace the pointer so that it points to another object. This is not the same as cloning Arc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rphmeier I'll leave this to another PR. This one is too big already

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I understand, but the point is that even if you replace the pointer someone might be using the old one anyway (because after getting a lock you can just clone the Arc and release the lock).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, ok. I'm sure that's not happening cause this pointer is not exposed out of the client. It is only used in TraceDB and we should really just pass a reference there as @rphmeier suggested.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 84.364% when pulling 2b3df3b on pv64-sync into 59f18ab on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 84.572% when pulling da2940b on pv64-sync into 4e466f0 on master.

@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 6, 2016
@arkpar arkpar merged commit 5c5d9c8 into master Sep 6, 2016
@arkpar arkpar deleted the pv64-sync branch October 3, 2016 15:34
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.

4 participants