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

Jump to bottom

compress header #1643

Merged
merged 5 commits into from
Sep 27, 2019
Merged

compress header #1643

merged 5 commits into from
Sep 27, 2019

Conversation

zhangsoledad
Copy link
Member

@zhangsoledad zhangsoledad commented Sep 25, 2019 •
edited
Loading

  • remove uncles_count
  • merge witnesses_root & transaction_root
  • replace difficulty with compact_target

@zhangsoledad zhangsoledad requested a review from a team September 25, 2019 07:48
nervos-bot[bot]
nervos-bot bot previously requested changes Sep 25, 2019
View reviewed changes
Copy link

@nervos-bot nervos-bot bot left a comment

Choose a reason for hiding this comment

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

Hold as requested by @zhangsoledad.

@doitian doitian added this to 👀 Awaiting review in CKB - Pull Requests Sep 26, 2019
@doitian doitian moved this from 👀 Awaiting review to 🔴 High priority in CKB - Pull Requests Sep 26, 2019
doitian
doitian previously requested changes Sep 26, 2019
View reviewed changes
util/types/src/utilities/difficulty.rs Outdated Show resolved Hide resolved
util/types/src/utilities/difficulty.rs Outdated Show resolved Hide resolved
CKB - Pull Requests automation moved this from 🔴 High priority to 👀 Awaiting review Sep 26, 2019
@zhangsoledad zhangsoledad force-pushed the zhangsoledad/compress_header branch 2 times, most recently from 1134a01 to 5753e83 Compare September 26, 2019 14:42
@zhangsoledad zhangsoledad changed the title [WIP] compress header compress header Sep 26, 2019
@nervos-bot nervos-bot bot dismissed their stale review September 26, 2019 14:42

Unhold as requested by @zhangsoledad.

@zhangsoledad zhangsoledad added the s:waiting-on-reviewers Status: Waiting for Review label Sep 26, 2019
@doitian doitian dismissed their stale review September 27, 2019 01:40

Requests fulfilled

@doitian doitian moved this from 👀 Awaiting review to 🔴 High priority in CKB - Pull Requests Sep 27, 2019
CKB - Pull Requests automation moved this from 🔴 High priority to ✅ Reviewer approved Sep 27, 2019
u2
u2 previously requested changes Sep 27, 2019
View reviewed changes
Copy link
Contributor

@u2 u2 left a comment

Choose a reason for hiding this comment

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

There may be some problem in compact_to_target overflow checking.

util/types/src/utilities/difficulty.rs Outdated Show resolved Hide resolved
CKB - Pull Requests automation moved this from ✅ Reviewer approved to 👀 Awaiting review Sep 27, 2019
@zhangsoledad zhangsoledad dismissed u2’s stale review September 27, 2019 10:21

whether target is valid is different concept with overflow check.

CKB - Pull Requests automation moved this from 🔴 High priority to 👀 Awaiting review Sep 27, 2019
CKB - Pull Requests automation moved this from 👀 Awaiting review to ✅ Reviewer approved Sep 27, 2019
@doitian
Copy link
Member

doitian commented Sep 27, 2019

bors r+

@nervos-bot nervos-bot bot added the s:ready-to-merge Status: Waiting to be merged. label Sep 27, 2019
bors bot added a commit that referenced this pull request Sep 27, 2019
@bors @zhangsoledad
Merge #1643
54ac808 
1643: compress header r=doitian a=zhangsoledad

* remove uncles_count
* merge witnesses_root & transaction_root
* replace difficulty with compact_target

Co-authored-by: zhangsoledad <787953403@qq.com>
@bors
Copy link
Contributor

bors bot commented Sep 27, 2019

Build failed

  • continuous-integration/travis-ci/push

@zhangsoledad
Copy link
Member Author

@u2 your opinion is more reasonable.
compact_to_target, not compact_encode, it's not a general encode.
I apply it after thinking

@zhangsoledad
Copy link
Member Author

bors r=u2,doitian

bors bot added a commit that referenced this pull request Sep 27, 2019
@bors @zhangsoledad
Merge #1643
7741b2e 
1643: compress header r=u2,doitian a=zhangsoledad

* remove uncles_count
* merge witnesses_root & transaction_root
* replace difficulty with compact_target

Co-authored-by: zhangsoledad <787953403@qq.com>
@bors
Copy link
Contributor

bors bot commented Sep 27, 2019

Build failed

  • continuous-integration/travis-ci/push

@zhangsoledad
Copy link
Member Author

nervosnetwork/ckb-system-scripts#50

@zhangsoledad
Copy link
Member Author

bors retry

bors bot added a commit that referenced this pull request Sep 27, 2019
@bors @zhangsoledad
Merge #1643
39c33cb 
1643: compress header r=u2,doitian a=zhangsoledad

* remove uncles_count
* merge witnesses_root & transaction_root
* replace difficulty with compact_target

Co-authored-by: zhangsoledad <787953403@qq.com>
@bors
Copy link
Contributor

bors bot commented Sep 27, 2019

Canceled

@zhangsoledad
Copy link
Member Author

bors r=u2,doitian

bors bot added a commit that referenced this pull request Sep 27, 2019
@bors @zhangsoledad
Merge #1643
d2708b4 
1643: compress header r=u2,doitian a=zhangsoledad

* remove uncles_count
* merge witnesses_root & transaction_root
* replace difficulty with compact_target

Co-authored-by: zhangsoledad <787953403@qq.com>
@bors
Copy link
Contributor

bors bot commented Sep 27, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 9d812af into nervosnetwork:develop Sep 27, 2019
CKB - Pull Requests automation moved this from ✅ Reviewer approved to Done Sep 27, 2019
@zhangsoledad zhangsoledad deleted the zhangsoledad/compress_header branch September 27, 2019 15:34
@ashchan ashchan mentioned this pull request Sep 28, 2019
Merged
@Keith-CY Keith-CY mentioned this pull request Sep 29, 2019
Merged
@quake quake mentioned this pull request Oct 9, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjyr jjyr jjyr left review comments

@doitian doitian doitian approved these changes

@u2 u2 u2 approved these changes

@nervos-bot nervos-bot[bot] nervos-bot left review comments

@keroro520 keroro520 Awaiting requested review from keroro520

Assignees
No one assigned
Labels
s:ready-to-merge Status: Waiting to be merged. s:waiting-on-reviewers Status: Waiting for Review
Projects
CKB - Pull Requests
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zhangsoledad @doitian @u2 @jjyr