ckb2021: fix load_cell_data_hash syscall #228
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
doitian
added
b:p2p
quake
requested changes
Jun 2, 2021
|
|
||
| ## Abstract | ||
|
|
||
| When the syscalls `load_cell_data` and `load_cell_data_hash` load cell from a transaction still in the pool, they should load correct data and hash as the transaction is already in the chain. |
There was a problem hiding this comment.
There are no load_cell_data_hash syscall in current version, it should be load_cell_by_field with the field value 1:
https://github.com/nervosnetwork/rfcs/blob/f0bf9fd6c627f5669e471543d3ba5d62667b592e/rfcs/0009-vm-syscalls/0009-vm-syscalls.md#load-cell-by-field
|
|
||
| The syscall `load_cell_data` has bee fixed in [this security advisory](https://github.com/nervosnetwork/ckb/security/advisories/GHSA-29c2-65rj-h343). It now can load data of the cell that is in the pool. | ||
|
|
||
| But the syscall `load_cell_data_hash` will fail if it tries to load the hash of a cell not in the chain. The fixing is easy but it will lead to nodes running new version blocked by nodes running the old version. |
There was a problem hiding this comment.
accroding to unit test, the syscall wil fail even the cell in the chain, can we confirm this behavior:
https://github.com/nervosnetwork/ckb/blob/develop/chain/src/tests/load_input_data_hash_cell.rs#L90
zhangsoledad
reviewed
Jun 2, 2021
Comment on lines
+20
to
+21
| Because of a bug, the syscalls `load_cell_data` and `load_cell_data_hash` have wrong behaviors when the referenced cell is from a transaction still in the transaction pool. | ||
|
|
There was a problem hiding this comment.
Suggested change
| Because of a bug, the syscalls `load_cell_data` and `load_cell_data_hash` have wrong behaviors when the referenced cell is from a transaction still in the transaction pool. | |
| Because of a bug, the syscalls `load_cell_data_hash` has wrong behaviors. It will fail except in the following situations, the referenced transaction still in the transaction pool for an unconfirmed transaction, or the referenced transaction packaged in the same block. It's non-deterministic. |
There was a problem hiding this comment.
Actually, load_cell_data has no problem. it is forbidden by mistake in GHSA-29c2-65rj-h343, recovered in nervosnetwork/ckb#2382
zhangsoledad
reviewed
Jun 2, 2021
|
|
||
| Because of a bug, the syscalls `load_cell_data` and `load_cell_data_hash` have wrong behaviors when the referenced cell is from a transaction still in the transaction pool. | ||
|
|
||
| The syscall `load_cell_data` has bee fixed in [this security advisory](https://github.com/nervosnetwork/ckb/security/advisories/GHSA-29c2-65rj-h343). It now can load data of the cell that is in the pool. |
There was a problem hiding this comment.
Suggested change
| The syscall `load_cell_data` has bee fixed in [this security advisory](https://github.com/nervosnetwork/ckb/security/advisories/GHSA-29c2-65rj-h343). It now can load data of the cell that is in the pool. | |
| Trasaction pool temporary forbidden syscall `load_cell_data_hash` in [this security advisory](https://github.com/nervosnetwork/ckb/security/advisories/GHSA-q73f-w3h7-7wcc). `load_cell_data_hash` transaction will be rejected by `rpc` or `network`, but on-chain verification is not affected. |
zhangsoledad
reviewed
Jun 2, 2021
|
|
||
| The syscall `load_cell_data` has bee fixed in [this security advisory](https://github.com/nervosnetwork/ckb/security/advisories/GHSA-29c2-65rj-h343). It now can load data of the cell that is in the pool. | ||
|
|
||
| But the syscall `load_cell_data_hash` will fail if it tries to load the hash of a cell not in the chain. The fixing is easy but it will lead to nodes running new version blocked by nodes running the old version. |
There was a problem hiding this comment.
Suggested change
| But the syscall `load_cell_data_hash` will fail if it tries to load the hash of a cell not in the chain. The fixing is easy but it will lead to nodes running new version blocked by nodes running the old version. |
bors bot
added a commit
to nervosnetwork/ckb
that referenced
this pull request
Jun 15, 2021
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
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.