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

RFC36: remove header deps immature rule #240

Merged
merged 4 commits into from
Mar 18, 2022
Merged

RFC36: remove header deps immature rule #240

merged 4 commits into from
Mar 18, 2022

Conversation

doitian
Copy link
Member

@doitian doitian commented Jul 1, 2021 •
edited

This document proposes removeing the loading header immature rule.

In the consensus ckb2019, the header dep must reference the block which is 4 epochs ago. After this RFC is activated, the transaction can use any existing blocks in the chain as the header dep.

@doitian doitian requested a review from a team as a code owner July 1, 2021 08:11
@doitian doitian marked this pull request as draft July 1, 2021 08:12
@doitian doitian added ckb2021 Hard fork scheduled in 2021 b:consensus Break consensus hard-fork labels Jul 1, 2021
@yangby-cryptape yangby-cryptape mentioned this pull request Jul 1, 2021
Merged
bors bot added a commit to nervosnetwork/ckb that referenced this pull request Jul 5, 2021
@bors @yangby-cryptape
Merge #2797 #2798
05ff930 
2797: feat(hardfork): reject vm1 lock script before hardfork started to keep compatible with old clients r=doitian,quake a=yangby-cryptape

### Changes

- refactor: merge time relative verifiers in ContextualTransactionVerifier

  Before this commit:
  - `TimeRelativeTransactionVerifier = MaturityVerifier + SinceVerifier`
  - `ContextualTransactionVerifier = MaturityVerifier + SinceVerifier + OtherVerifiers`

  After this commit:
  - `TimeRelativeTransactionVerifier = MaturityVerifier + SinceVerifier`
  - `ContextualTransactionVerifier = TimeRelativeTransactionVerifier + OtherVerifiers`

- feat(hardfork): reject vm1 lock script before hardfork started to keep compatible with old clients

  Add `CompatibleVerifier` to `ContextualTransactionVerifier` to check compatible between different versions CKB clients.

  When a new client with hardfork features released, before the hardfork started, the old CKB clients will still be able to work.
  So, the new CKB client have to add several necessary checks to avoid fork attacks.

  After hardfork, the old clients will be no longer available. Then we can delete all code in that verifier until next hardfork.

  In current version, `CompatibleVerifier` only has one feature:
  - Reject vm1 lock script before hardfork started to keep compatible with old clients.

2798: feat(hardfork): remove the header deps immature rule r=quake,driftluo a=yangby-cryptape

### Changes

- New hardfork Features

  -  [CKB-RFCs PR 240: RFC: Remove header deps immature rule](nervosnetwork/rfcs#240)

- refactor: resolve transaction with bitflags

Co-authored-by: Boyu Yang <yangby@cryptape.com>
@doitian doitian added this to Draft in ckb2021 Jul 5, 2021
@doitian doitian force-pushed the remove-header-dep-immature-rule branch from 9262097 to 2f8b038 Compare July 14, 2021 02:04
This was referenced Jul 23, 2021
Closed
Open
@doitian doitian changed the title Initialize RFC: remove header deps immature rule RFC36: remove header deps immature rule Jul 23, 2021
@doitian doitian force-pushed the remove-header-dep-immature-rule branch 2 times, most recently from e66a144 to f31e93e Compare July 23, 2021 07:53
@doitian doitian marked this pull request as ready for review July 23, 2021 07:53
@doitian doitian requested a review from xxuejie as a code owner July 23, 2021 07:53
@doitian doitian force-pushed the remove-header-dep-immature-rule branch from f31e93e to 48a1d78 Compare July 23, 2021 08:04
@doitian doitian moved this from Draft to In review in ckb2021 Jul 23, 2021
malinoisls01
malinoisls01 previously approved these changes Nov 26, 2021
View reviewed changes
yangby-cryptape
yangby-cryptape previously approved these changes Jan 4, 2022
View reviewed changes
zhangsoledad
zhangsoledad previously approved these changes Mar 1, 2022
View reviewed changes
@doitian @zhangsoledad @janx
Apply suggestions from code review
1348ebd 
Co-authored-by: zhangsoledad <787953403@qq.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
@doitian doitian requested review from janx and zhangsoledad and removed request for quake March 2, 2022 03:48
@doitian
Copy link
Member Author

doitian commented Mar 4, 2022

Assigned reviewers: @janx @zhangsoledad

janx
janx approved these changes Mar 4, 2022
View reviewed changes
Copy link
Member

@janx janx left a comment

Choose a reason for hiding this comment

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

The PR looks good. Don't forget to update repo README - can use a different PR to avoid merge conflicts.

@doitian doitian added the s:fcp Final Comment Period. PR is merged when there's no comments in 14 days. label Mar 14, 2022
@doitian doitian moved this from In review to Final Comment Period in ckb2021 Mar 14, 2022
@doitian doitian merged commit 3003329 into nervosnetwork:master Mar 18, 2022
ckb2021 automation moved this from Final Comment Period to Done Mar 18, 2022
@doitian doitian deleted the remove-header-dep-immature-rule branch March 18, 2022 02:28
@jordanmack jordanmack mentioned this pull request May 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janx janx janx approved these changes

@zhangsoledad zhangsoledad zhangsoledad approved these changes

@yangby-cryptape yangby-cryptape yangby-cryptape left review comments

@malinoisls01 malinoisls01 malinoisls01 left review comments

@xxuejie xxuejie Awaiting requested review from xxuejie xxuejie is a code owner

Assignees
No one assigned
Labels
b:consensus Break consensus ckb2021 Hard fork scheduled in 2021 hard-fork s:fcp Final Comment Period. PR is merged when there's no comments in 14 days.
Projects
ckb2021
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@doitian @janx @zhangsoledad @yangby-cryptape @malinoisls01