Skip to content

Conversation

@remagpie
Copy link
Contributor

@remagpie remagpie commented Nov 15, 2019

This PR introduces a change in the sync extension message interpretation.
The total_score in Status message is now interpreted as a `nonce.

When the snapshot sync lands, the total_score of a block doesn't mean the total accumulated score from the genesis block anymore.
To handle this, we need to avoid relying on the total_score for synchronization.

There were two parts to change:

  1. Checking if the peer is leading or not
  • This is now handled by checking the best_hash in the local blockchain.
  1. Checking if we need to update the peer status or not
  • This is now handled by nonce in the status message.

There are no changes in the message format itself, so I think it is a backward-compatible patch.

@remagpie remagpie mentioned this pull request Nov 15, 2019
21 tasks
@remagpie remagpie force-pushed the ignore-total-score branch 3 times, most recently from d3a5854 to 0eec7ff Compare November 15, 2019 10:15
@remagpie remagpie force-pushed the ignore-total-score branch 2 times, most recently from 723fc73 to b26b5db Compare November 18, 2019 03:24
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

It's good to use nonce instead of the total score. total_score isn't saved in the state and need the whole history to calculate it. It is hard to manage and verify total_score.

This commit removes last_request and uses nonce instead. Sync extension is using last_requset to give a unique id of requests. The sync extension will work even though we use nonce instead. However, one variable used by two different purposes makes maintenance hard. IMO using two variables for each purpose will make code maintainable.

@remagpie
Copy link
Contributor Author

remagpie commented Nov 18, 2019

It's good to use nonce instead of the total score. total_score isn't saved in the state and need the whole history to calculate it. It is hard to manage and verify total_score.

This commit removes last_request and uses nonce instead. Sync extension is using last_requset to give a unique id of requests. The sync extension will work even though we use nonce instead. However, one variable used by two different purposes makes maintenance hard. IMO using two variables for each purpose will make code maintainable.

Agreed. Let me separate those variables.

@remagpie remagpie force-pushed the ignore-total-score branch 2 times, most recently from fb503c0 to 1b86afa Compare November 18, 2019 09:22
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

Shouldn't we update the self.nonce when a new block is imported?
self.nonce was updated correctly.

When CodeChain is synchronized by snapshot sync, `total_score`
doesn't mean the accumulated score from the genesis block.
To handle this, the sync extension is updated to not rely on
`total_score` for deciding whether a peer is leading or not, and
use the value only as a monotonically increasing nonce.
@remagpie remagpie merged commit f019635 into CodeChain-io:feature/snapshot Nov 19, 2019
@foriequal0 foriequal0 mentioned this pull request Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants