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.

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

Fix round timer #26

Merged
merged 10 commits into from
Jan 16, 2020
Merged

Fix round timer #26

merged 10 commits into from
Jan 16, 2020

Conversation

rantan
Copy link
Contributor

@rantan rantan commented Jan 15, 2020

Fix round timer behavior. The timer behavior didn't have consistent policy. So, it worked inconsistent way.

After this PR, Round timer must follow below basically rule.

  • The timer is started on rounds start only.
  • New round is started on only receiving completedblock message or previous round is timeout.

 It is not needed. When node.start() is called, the connection establishment is done in bin/tapyrus-signerd.rs.
It used Block::hash() for checking sighash value.
Round timer must follow below rules
* The timer is started on rounds start only.
* New round is started on only receiving completedblock message or previous round is timeout.
@rantan rantan requested review from azuchi and removed request for azuchi January 15, 2020 05:14
By using schnorr threshold signing, the term of communications for it became longer. So sometimes it needs over 5s.
@azuchi
Copy link
Contributor

azuchi commented Jan 15, 2020

ci looks like failure.

The test node receive nodevss message after it started. This is a effect of
changing which is that 10s sleep code for waiting connect to redis was removed.
To respond to this, the test track only blockvss message.
@rantan
Copy link
Contributor Author

rantan commented Jan 16, 2020

ci looks like failure.

It was fixed!

@rantan rantan requested a review from azuchi January 16, 2020 01:09
@@ -663,15 +680,15 @@ impl<T: TapyrusApi, C: ConnectionManager> SignerNode<T, C> {
new_signatures.len(),
self.params.threshold
);
if candidate_block.hash().unwrap() != blockhash {
if candidate_block.sighash().unwrap() != blockhash {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why Block#hash returns Result? I think it should be return hash::Hash directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Block#sighash can return Hash. I will fix it.

About Block#hash, It is possible to return Error because of no proof block.

Copy link
Contributor

@azuchi azuchi Jan 16, 2020

Choose a reason for hiding this comment

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

About Block#hash, It is possible to return Error because of no proof block.

Computing the hash and validating the block should be considered separately. If proof is empty, Block#hash should return hash value with empty proof and another logic checks block validity.

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 see. I will separate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding validity, current Block implement is roughly. It should use rust-tapyrus create.

I want to change using rust-tapyrus in other PR sometime soon.

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 fixed!

Copy link
Contributor

Choose a reason for hiding this comment

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

Please create new issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here #28

@rantan rantan requested a review from azuchi January 16, 2020 08:12
@azuchi azuchi merged commit c85921d into chaintope:master Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants