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

RFC30: Ensure that index is less than length in input since field using epoch. #223

Merged
merged 4 commits into from
Apr 1, 2022
Merged

RFC30: Ensure that index is less than length in input since field using epoch. #223

merged 4 commits into from
Apr 1, 2022

Conversation

doitian
Copy link
Member

@doitian doitian commented Feb 4, 2021 •
edited
Loading

This document proposes to prevent a bug in the CKB reference implementation and avoid ambiguity. When a transaction input uses the absolute epoch in the since field, the value is an encoded number e + i / l. This RFC proposes to add a rule to verify that i <= l or i and l are both zero.

@doitian doitian added b:consensus Break consensus soft-fork labels Feb 4, 2021
@doitian doitian requested a review from a team as a code owner February 4, 2021 03:44
@doitian doitian requested review from knwang and xxuejie February 4, 2021 03:44
@doitian doitian mentioned this pull request Apr 25, 2021
Closed
10 tasks
@doitian doitian added the ckb2021 Hard fork scheduled in 2021 label Apr 26, 2021
@doitian doitian changed the title Since absolute epoch using rational number Ensure that index < length in input since field using epoch. May 17, 2021
@yangby-cryptape yangby-cryptape mentioned this pull request May 31, 2021
Merged
bors bot added a commit to nervosnetwork/ckb that referenced this pull request Jun 15, 2021
@bors @zhangsoledad @yangby-cryptape
Merge #2715
b0ab80e 
2715: feat(hardfork): ckb2021 hardfork features r=yangby-cryptape a=yangby-cryptape

### Suggestions for Review

**The best way to review this PR is reviewing each commit independently.**

### Brief introduction for Commits

#### New hardfork Features

**All following features can be found in RFC PRs.**

- feat(hardfork): in the "since epoch", the index should be less than length

  Ref: [CKB-RFCs PR 223: Ensure that index < length in input since field using epoch](nervosnetwork/rfcs#223)

- feat(hardfork): use block timestamp of input cells as relative since start timestamp

  Ref: [CKB-RFCs PR 221: Use Block Timestamp as Start Timestamp in Since](nervosnetwork/rfcs#221)

- **[Reverted]** ~~feat(hardfork): allow unknown block versions and transactions versions~~

  ~~Ref: [CKB-RFCs PR 230: Allow unknown tx & block version](nervosnetwork/rfcs#230

- feat(hardfork): allow script multiple matches on identical data for type hash-type scripts

  Ref: [CKB-RFCs PR 222: Allow script multiple matches on identical code](nervosnetwork/rfcs#222)

- feat(hardfork): reuse the uncles hash in the header as the extra hash

  Ref: [CKB-RFCs PR 224: Add a variable length field in the block header](nervosnetwork/rfcs#224)

- **[Reverted]** ~~feat(hardfork): allow loading uncommitted cell data hashes from tx pool~~

  ~~Ref: [CKB-RFCs PR 228: ckb2021: fix load_cell_data_hash syscall](nervosnetwork/rfcs#228

#### Other Important Commits

- feat(hardfork): setup the components for hard fork features

- refactor: let verifiers know the real environment that the transaction is in

  Almost all features require this refactor commit.

- refactor: remove useless parameter "with_data" because it always be true (tricky)

  So I can change less APIs and less code to apply the feature: allow loading uncommitted cell data hashes from tx pool.

### About Tests

Almost all features have detailed integration tests (or unit tests):
- Many blocks before hardfork;
- Only one block before hardfork;
- The block at hardfork;
- Many blocks after hardfork.

All commits can passed all integration tests and unit tests.

Co-authored-by: zhangsoledad <787953403@qq.com>
Co-authored-by: Boyu Yang <yangby@cryptape.com>
bors bot added a commit to nervosnetwork/ckb that referenced this pull request Jun 15, 2021
@bors @zhangsoledad @yangby-cryptape
Merge #2715
85d04c3 
2715: feat(hardfork): ckb2021 hardfork features r=quake,doitian,zhangsoledad a=yangby-cryptape

### Suggestions for Review

**The best way to review this PR is reviewing each commit independently.**

### Brief introduction for Commits

#### New hardfork Features

**All following features can be found in RFC PRs.**

- feat(hardfork): in the "since epoch", the index should be less than length

  Ref: [CKB-RFCs PR 223: Ensure that index < length in input since field using epoch](nervosnetwork/rfcs#223)

- feat(hardfork): use block timestamp of input cells as relative since start timestamp

  Ref: [CKB-RFCs PR 221: Use Block Timestamp as Start Timestamp in Since](nervosnetwork/rfcs#221)

- **[Reverted]** ~~feat(hardfork): allow unknown block versions and transactions versions~~

  ~~Ref: [CKB-RFCs PR 230: Allow unknown tx & block version](nervosnetwork/rfcs#230

- feat(hardfork): allow script multiple matches on identical data for type hash-type scripts

  Ref: [CKB-RFCs PR 222: Allow script multiple matches on identical code](nervosnetwork/rfcs#222)

- feat(hardfork): reuse the uncles hash in the header as the extra hash

  Ref: [CKB-RFCs PR 224: Add a variable length field in the block header](nervosnetwork/rfcs#224)

- **[Reverted]** ~~feat(hardfork): allow loading uncommitted cell data hashes from tx pool~~

  ~~Ref: [CKB-RFCs PR 228: ckb2021: fix load_cell_data_hash syscall](nervosnetwork/rfcs#228

#### Other Important Commits

- feat(hardfork): setup the components for hard fork features

- refactor: let verifiers know the real environment that the transaction is in

  Almost all features require this refactor commit.

- refactor: remove useless parameter "with_data" because it always be true (tricky)

  So I can change less APIs and less code to apply the feature: allow loading uncommitted cell data hashes from tx pool.

### About Tests

Almost all features have detailed integration tests (or unit tests):
- Many blocks before hardfork;
- Only one block before hardfork;
- The block at hardfork;
- Many blocks after hardfork.

All commits can passed all integration tests and unit tests.

Co-authored-by: zhangsoledad <787953403@qq.com>
Co-authored-by: Boyu Yang <yangby@cryptape.com>
@yangby-cryptape yangby-cryptape mentioned this pull request Jun 28, 2021
Merged
yangby-cryptape
yangby-cryptape reviewed Jul 13, 2021
View reviewed changes
Copy link
Contributor

@yangby-cryptape yangby-cryptape left a comment

Choose a reason for hiding this comment

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

It seems that > doesn't require any escape character.

...0000-since-absolute-epoch-as-rational-number/0000-since-absolute-epoch-as-rational-number.md Outdated Show resolved Hide resolved
...0000-since-absolute-epoch-as-rational-number/0000-since-absolute-epoch-as-rational-number.md Outdated Show resolved Hide resolved
...0000-since-absolute-epoch-as-rational-number/0000-since-absolute-epoch-as-rational-number.md Outdated Show resolved Hide resolved
@doitian doitian changed the title Ensure that index < length in input since field using epoch. RFC223: Ensure that index < length in input since field using epoch. Jul 22, 2021
@doitian doitian force-pushed the since-absolute-epoch-as-rational-number branch from 8578404 to 552c2a1 Compare July 22, 2021 14:51
@doitian doitian mentioned this pull request Jul 23, 2021
Open
14 tasks
@doitian doitian changed the title RFC223: Ensure that index < length in input since field using epoch. RFC30: Ensure that index < length in input since field using epoch. Jul 23, 2021
@doitian doitian force-pushed the since-absolute-epoch-as-rational-number branch from 552c2a1 to 77fa9d3 Compare July 23, 2021 07:29
@doitian doitian force-pushed the since-absolute-epoch-as-rational-number branch from 77fa9d3 to c8fac5f Compare July 23, 2021 07:34
@doitian doitian changed the title RFC30: Ensure that index < length in input since field using epoch. RFC30: Ensure that index is less than length in input since field using epoch. Jul 24, 2021
@jjyr jjyr mentioned this pull request Aug 2, 2021
Merged
3 tasks
@keroro520 keroro520 mentioned this pull request Aug 11, 2021
Merged
xxuejie
xxuejie reviewed Mar 3, 2022
View reviewed changes
rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-index-less-than-length-in-since.md Outdated Show resolved Hide resolved
rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-index-less-than-length-in-since.md Outdated Show resolved Hide resolved
rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-index-less-than-length-in-since.md Outdated Show resolved Hide resolved
rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-index-less-than-length-in-since.md Outdated Show resolved Hide resolved
rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-index-less-than-length-in-since.md Outdated Show resolved Hide resolved
rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-index-less-than-length-in-since.md Outdated Show resolved Hide resolved
rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-index-less-than-length-in-since.md Outdated Show resolved Hide resolved
@doitian doitian requested review from janx and xxuejie and removed request for knwang March 4, 2022 02:11
@doitian
Copy link
Member Author

doitian commented Mar 4, 2022

Assigned reviewers: @xxuejie @janx

@doitian @janx
Update rfcs/0030-ensure-index-less-than-length-in-since/0030-ensure-i…
8aca73f 
…ndex-less-than-length-in-since.md

Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
@doitian doitian requested a review from janx March 17, 2022 02:25
@doitian doitian added the s:fcp Final Comment Period. PR is merged when there's no comments in 14 days. label Mar 18, 2022
@doitian
Copy link
Member Author

doitian commented Mar 18, 2022

FCP will end on 2022-04-01

@doitian doitian merged commit 1cbeeb7 into nervosnetwork:master Apr 1, 2022
@doitian doitian deleted the since-absolute-epoch-as-rational-number branch April 1, 2022 03:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@xxuejie xxuejie xxuejie approved these changes

@janx janx janx approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@doitian @janx @xxuejie @yangby-cryptape