Skip to content

Add reseal timer to miner #944

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

Jump to bottom
Merged
merged 6 commits into from
Dec 13, 2018
Merged

Add reseal timer to miner #944

merged 6 commits into from
Dec 13, 2018

Conversation

@hyunsikjeong
Copy link
Contributor

@hyunsikjeong hyunsikjeong commented Dec 6, 2018 •
edited
Loading

  1. Remove global codechain-timer types in codechain-network
    Now, codechain-timer is not an external package.
  2. Add timer to the client
    To resolve Miner does not mine transactions after reseal_min_period elapsed #239, resolve Make PoW mine blocks even when there is no pending parcel in the mempool #613

sgkim126
sgkim126 reviewed Dec 6, 2018
View reviewed changes
sgkim126
sgkim126 reviewed Dec 6, 2018
View reviewed changes
@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch 2 times, most recently from 6d7bc23 to 53b3455 Compare December 7, 2018 00:01
@sgkim126
Copy link
Contributor

sgkim126 commented Dec 7, 2018

LGTM
@foriequal0 How about you?

@hyunsikjeong
Copy link
Contributor Author

hyunsikjeong commented Dec 7, 2018

I'm going to change the code to add miner to the client, not the miner.

@hyunsikjeong hyunsikjeong changed the title Add timer to miner [WIP] Add timer to client Dec 7, 2018
@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch from 53b3455 to 1f14a1b Compare December 7, 2018 00:55
@hyunsikjeong hyunsikjeong changed the title [WIP] Add timer to client [WIP] Add reseal timer to miner Dec 7, 2018
@hyunsikjeong hyunsikjeong changed the title [WIP] Add reseal timer to miner Add reseal timer to miner Dec 7, 2018
@hyunsikjeong hyunsikjeong changed the title Add reseal timer to miner [WIP] Add reseal timer to miner Dec 7, 2018
@hyunsikjeong
Copy link
Contributor Author

hyunsikjeong commented Dec 7, 2018

I'll finish the code to fix #239 in this PR.

@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch 2 times, most recently from d3ab2c0 to 4073078 Compare December 7, 2018 02:29
sgkim126
sgkim126 reviewed Dec 7, 2018
View reviewed changes
@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch from 4073078 to 535b992 Compare December 7, 2018 03:07
sgkim126
sgkim126 reviewed Dec 7, 2018
View reviewed changes
foriequal0
foriequal0 requested changes Dec 7, 2018
View reviewed changes
@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch 3 times, most recently from 69b1df8 to bfd6a63 Compare December 7, 2018 06:10
@hyunsikjeong
Copy link
Contributor Author

hyunsikjeong commented Dec 7, 2018

There's a problem with the Timelock test on Travis, but the integration test works well in my local env. I'm figuring out now... 😢

@foriequal0 foriequal0 mentioned this pull request Dec 7, 2018
Closed
@sgkim126
Copy link
Contributor

sgkim126 commented Dec 7, 2018

@jhs7jhs Would you make another PR for 13f1f3c

@hyunsikjeong
Copy link
Contributor Author

hyunsikjeong commented Dec 7, 2018

@sgkim126 Here it is: #946

@hyunsikjeong hyunsikjeong mentioned this pull request Dec 7, 2018
Merged
foriequal0
foriequal0 previously approved these changes Dec 7, 2018
View reviewed changes
@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch 2 times, most recently from 7e7f7d3 to 48029d4 Compare December 10, 2018 01:18
@hyunsikjeong hyunsikjeong changed the title [WIP] Add reseal timer to miner Add reseal timer to miner Dec 10, 2018
@hyunsikjeong
Copy link
Contributor Author

hyunsikjeong commented Dec 10, 2018

@sgkim126 @foriequal0 Can you review this PR? I think it's finished.

@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch 3 times, most recently from c1ee749 to efd4b8c Compare December 10, 2018 09:26
sgkim126
sgkim126 reviewed Dec 10, 2018
View reviewed changes
@hyunsikjeong hyunsikjeong requested a review from majecty December 11, 2018 03:37
@majecty
Copy link
Contributor

majecty commented Dec 11, 2018

LGTM

majecty
majecty previously approved these changes Dec 11, 2018
View reviewed changes
@hyunsikjeong hyunsikjeong force-pushed the fix-reseal-min-period branch 2 times, most recently from 71c10a2 to 6b3264e Compare December 11, 2018 09:45
@hyunsikjeong
Copy link
Contributor Author

hyunsikjeong commented Dec 11, 2018 •
edited
Loading

@foriequal0 @sgkim126 Sorry, code is changed. Could you check the code again?
@majecty Also, update_sealing takes an argument allow_empty_block. Currently Tendermint uses update_sealing with allow_empty_block = true. Is this correct behavior?
@joojis From this patch, a node does not create an empty block if you send a timelock parcel currently not available. Therefore some timelock test cases are changed. Could you check those?

@hyunsikjeong hyunsikjeong requested a review from joojis December 11, 2018 09:47
joojis
joojis reviewed Dec 12, 2018
View reviewed changes
Copy link
Contributor

@joojis joojis left a comment

Choose a reason for hiding this comment

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

I checked the changed test scenarios

Hyunsik Jeong added 4 commits December 12, 2018 10:42
Load timer types from codechain-timer
c78bc0e
Add reseal timer to client
d4de4ee
Add flag to disable reseal timer
7fc9306
Fix timelock test as the current behavior
acbca6a 
In the original code, sending parcels with Timelock which are currently
not possible make an empty block.
But now, it will not create any blocks.
@hyunsikjeong hyunsikjeong merged commit 32fbe07 into CodeChain-io:master Dec 13, 2018
@hyunsikjeong hyunsikjeong deleted the fix-reseal-min-period branch January 4, 2019 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joojis joojis joojis left review comments

@sgkim126 sgkim126 sgkim126 approved these changes

@majecty majecty majecty left review comments

@foriequal0 foriequal0 foriequal0 left review comments

Assignees

No one assigned

Labels

miner

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make PoW mine blocks even when there is no pending parcel in the mempool Miner does not mine transactions after reseal_min_period elapsed

5 participants

@hyunsikjeong @sgkim126 @majecty @foriequal0 @joojis