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.

Sign up for GitHub

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

new mined block should be broadcasted if a node was synced #2324

Closed
wants to merge 4 commits into from
+17 −8
Closed

new mined block should be broadcasted if a node was synced #2324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0bb9398
new mined block should be broadcasted if a node was synced before
garyyu Jan 10, 2019
c3cbfb6
a bit refactor
garyyu Jan 10, 2019
c6094ae
keep mining and keep relaying no matter what
garyyu Jan 10, 2019
e237993
fix the return
garyyu Jan 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions servers/src/common/adapters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,10 +635,6 @@ impl ChainAdapter for ChainToPoolAndNetAdapter {
}
}

if self.sync_state.is_syncing() {
return;
}

// If we mined the block then we want to broadcast the compact block.
// If we received the block from another node then broadcast "header first"
// to minimize network traffic.
Expand All @@ -647,8 +643,10 @@ impl ChainAdapter for ChainToPoolAndNetAdapter {
let cb: CompactBlock = b.clone().into();
self.peers().broadcast_compact_block(&cb);
} else {
// "header first" propagation if we are not the originator of this block
self.peers().broadcast_header(&b.header);
if self.sync_state.is_synced_once() {
Copy link
Member

Choose a reason for hiding this comment

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

Still not convinced the added complexity of is_synced_once is worth it. There is no difference between the 1st and 2nd sync for a node.
Can we not just use the existing is_syncing() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If using existing is_syncing, the fraudulent most work peer will make the gossip block headers propagation pause (because node is in syncing state), until the node leave syncing mode by a fraud detection and banning.

Copy link
Member

Choose a reason for hiding this comment

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

🤦‍♂️ Of course, forgot about that.

We should just always broadcast out "header first" here, regardless of syncing or not, regardless of synced_first or not.
Keep it simple and let peers handle these redundant headers as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I worry there will be too much redundant headers then, and not sure whether the current grin version can handle these flood redundant headers, we don't have much time to test it since only less 4 days before launching.

Also here's a solution to kill that fraudulent work case, I don't understand why simple design is more important than defending from attack.

BTW, does this is_synced_once indeed make design become a very complex one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Look like we couldn't agree each other between us about whether always broadcast out "header first" including initial syncing stage, perhaps it's good to invite more pair of eyes on this review 😄 @ignopeverell what's your opinion on this?

// "header first" propagation if we are not the originator of this block
self.peers().broadcast_header(&b.header);
}
}

// Reconcile the txpool against the new block *after* we have broadcast it too our peers.
Expand Down
15 changes: 13 additions & 2 deletions servers/src/common/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,8 @@ pub enum SyncStatus {
/// Current sync state. Encapsulates the current SyncStatus.
pub struct SyncState {
current: RwLock<SyncStatus>,
/// Flag to indicate whether we already synced once
synced_once: RwLock<bool>,
sync_error: Arc<RwLock<Option<Error>>>,
}

Expand All @@ -290,6 +292,7 @@ impl SyncState {
pub fn new() -> SyncState {
SyncState {
current: RwLock::new(SyncStatus::Initial),
synced_once: RwLock::new(false),
sync_error: Arc::new(RwLock::new(None)),
}
}
Expand All @@ -300,6 +303,11 @@ impl SyncState {
*self.current.read() != SyncStatus::NoSync
}

/// Whether it was synced once after grin running
pub fn is_synced_once(&self) -> bool {
*self.synced_once.read() == true
}

/// Current syncing status
pub fn status(&self) -> SyncStatus {
*self.current.read()
Expand All @@ -312,10 +320,13 @@ impl SyncState {
}

let mut status = self.current.write();
*status = new_status;

debug!("sync_state: sync_status: {:?} -> {:?}", *status, new_status,);
if let SyncStatus::NoSync = new_status {
*self.synced_once.write() = true;
}

*status = new_status;
debug!("sync_state: sync_status: {:?} -> {:?}", *status, new_status,);
}

/// Update txhashset downloading progress
Expand Down