feat(hardfork): ckb2021 hardfork features (vm related part) #2756

chaoticlonghair · 2021-06-15T07:14:18Z

Changes

New hardfork Features
All changes are followed to the latest RFCs, except:
- Add a "edition" field to all configuration files: CKB chain specification or app configurations.

CKB Chain Edition

If no edition field, the default edition is "2019".
After this PR, the default files created by ckb init will be edition "2021" files.

But the public chains will still use the edition "2019" CKB chain specification as the built-in spec.

At present, there are only to public chains: "mainnet" and "testnet".
The edition "2019" doesn't support hardfork parameters.
How to upgrade from edition "2019" to "2021"?

Modify your spec file (default is specs/dev.toml), ckb.toml and ckb-miner.toml:
- Update all hash_type = "data" to hash_type.kind = "data" and hash_type.vm_version = 0.
- Update all hash_type = "type" to hash_type.kind = "type".
- Insert edition = "2021" into the head of the files.
After upgrade the edition of chain specification, the first time to run the CKB, you have to use --overwrite-spec parameter. The details of this parameter can be found with ckb run --help.

BREAKING CHANGES

The field hash_type in Script is changed from a String to an Object for all JSON RPC methods.

The details can be found in the latest RFCs.
The argument --ba-hash-type for ckb init is split into two arguments: --ba-hash-type-kind and --ba-hash-type-vm-version.

More details can be found with ckb-release init --help.

…and clippy

… mays change the script results

nervos-bot-user · 2021-06-16T10:33:06Z

Benchmark Result

TPS: 472
CKB Version: 6ca81ab
Instance Type: c5.xlarge
Instances Count: 3
Bench Type: In2Out2
CKB Logger Filter: info,ckb=debug

nervos-bot-user · 2021-06-16T13:23:51Z

Sync Test Result

CKB Version: 6ca81ab
Instance Region: ap-southeast-1
Sync Start: 2021-06-16 10:53:41
Sync End: 2021-06-16 13:12:16
Sync Height: 4568343
Sync Speed: 549 blocks/s

…ock_assembler"

…from 0.16.12 to 0.16.20

tx-pool/src/component/pending.rs

tx-pool/src/pool.rs

…bject" BREAKING CHANGE: The field "hash_type" in "Script" is changed from a "String" to an "Object" for all JSON RPC methods.

…poch 0 in default dev spec

…t for this feature

doitian · 2021-06-30T04:44:23Z

script/src/syscalls/exec.rs

+        }
+    }
+
+    fn fetch_witness(&self, source: Source, index: usize) -> Option<PackedBytes> {


Do we have any code that will decode the witness as molecule table WitnessArgs. If I put the code in witness, will it cause panic because it is not a valid WitnessArgs?

I saw the discussion in our internal channel.

I understand the result of that discussion that this is not an issue.

So, I ignore this.

script/src/syscalls/mod.rs

driftluo · 2021-06-30T05:51:18Z

tx-pool/src/component/pending.rs

+            .values()
+            .map(|entry| entry.transaction().clone())
+            .collect::<Vec<_>>();
+        self.inner.clear();


::std::mem::take maybe better?

I think this is trivial.

So I decide to ignore it.

After all high priority tasks, I will do this replacement for whole workspace.

keroro520

#benchmark

nervos-bot-user

#benchmark

quake · 2021-07-01T00:22:24Z

tx-pool/src/process.rs

+            let txs_opt = {
+                // This closure is used to limit the lifetime of mutable tx_pool.
+                let mut tx_pool = self.tx_pool.write().await;
+
+                let txs_opt = if hardfork_during_detach || hardfork_during_attach {
+                    // The tx_pool is locked, remove all caches if has any hardfork.
+                    self.txs_verify_cache.write().await.clear();
+                    Some(tx_pool.drain_all_transactions())
+                } else {
+                    None
+                };
+
+                _update_tx_pool_for_reorg(
+                    &mut tx_pool,
+                    &attached,
+                    detached_proposal_id,
+                    snapshot,
+                    &self.callbacks,
+                );
+                self.readd_dettached_tx(&mut tx_pool, retain, fetched_cache);
+
+                txs_opt
+            };

-            self.readd_dettached_tx(&mut tx_pool, retain, fetched_cache)
+            if let Some(txs) = txs_opt {
+                self.try_process_txs(txs).await;
+            }


The combination of the try_process_txs and txs_opt naming made me read the code multiple times before I understood the intent, suggest to remove the Option struct and use if/else directly:

{ let mut tx_pool = self.tx_pool.write().await; if hardfork_during_detach || hardfork_during_attach { // The tx_pool is locked, remove all caches if has any hardfork. self.txs_verify_cache.write().await.clear(); let txs = tx_pool.drain_all_transactions(); _update_tx_pool_for_reorg( &mut tx_pool, &attached, detached_proposal_id, snapshot, &self.callbacks, ); self.readd_dettached_tx(&mut tx_pool, retain, fetched_cache); self.try_process_txs(txs).await; } else { _update_tx_pool_for_reorg( &mut tx_pool, &attached, detached_proposal_id, snapshot, &self.callbacks, ); self.readd_dettached_tx(&mut tx_pool, retain, fetched_cache); }; }

At earlier version, I used empty Vec<_> to present None. May be it's better?

Backup old transactions, reorg, re-process old transactions.

I think this is trivial.

Since this PR is hard to get approvals, and I already got one now.
Please let me fix this in one commit with other suggestions above in another PR after this merged.

chaoticlonghair · 2021-07-01T02:28:48Z

bors r=doitian,quake

bors · 2021-07-01T03:41:49Z

Build succeeded:

ci

tx-pool/src/process.rs

ckb-bin/src/subcommand/init.rs

2818: refactor(hardfork): change field "hash_type" to an enumerated type r=quake,driftluo a=yangby-cryptape This PR doesn't revert the edition feature for spec files and config files. We can do it later, in next PR, after discussions. **BREAKING CHANGES**: Revert breaking changes which were introduced in #2756. Co-authored-by: Boyu Yang <[email protected]>

@doitian

2802: refactor(tests): simplify several construction functions and double ckb script tests r=quake,driftluo a=yangby-cryptape _Fix the issue posted by @doitian in #2756 (comment) _The best way to review this PR is reviewing each commits._ ### Changes - refactor: simplify vm construction functions which based on versions Before this commit, constructing a core vm requires complicated arguments: ```rust #[cfg(has_asm)] let v0_asm_machine = AsmCoreMachine::new(ISA_IMC, VERSION0, max_cycles); #[cfg(not(has_asm))] let v1_not_asm_machine = DefaultCoreMachine::<u64, WXorXMemory<SparseMemory<u64>>>::new( ISA_IMC | ISA_B | ISA_MOP, VERSION1, u64::max_value(), ); ``` After this commit, no longer care about cargo features, CKB VM versions or IMC sets: ```rust let v0_asm_machine = ScriptVersion:V0.init_core_machine(max_cycles); let v1_not_asm_machine = ScriptVersion::V1.init_core_machine_without_limit(); ``` - refactor(tests): simplify tx script verify construction functions Before this commit, verifying requires complicated arguments: ```rust let fork_at = 10; let store = new_store(); let data_loader = DataLoaderWrapper::new(&store); let hardfork_switch = HardForkSwitch::new_without_any_enabled() .as_builder() .rfc_pr_0237(fork_at) .rfc_pr_0234(fork_at) .build() .unwrap(); let consensus = ConsensusBuilder::default() .hardfork_switch(hardfork_switch) .build(); let tx_env0 = { let epoch = EpochNumberWithFraction::new(0, 0, 1); let header = HeaderView::new_advanced_builder() .epoch(epoch.pack()) .build(); TxVerifyEnv::new_commit(&header) }; let tx_env1 = { let epoch = EpochNumberWithFraction::new(fork_at, 0, 1); let header = HeaderView::new_advanced_builder() .epoch(epoch.pack()) .build(); TxVerifyEnv::new_commit(&header) }; let v0_result = TransactionScriptsVerifier::new(&rtx, &consensus, &data_loader, &tx_env0).verify(100_000_000); let v1_result = TransactionScriptsVerifier::new(&rtx, &consensus, &data_loader, &tx_env1).verify(100_000_000); ``` After this commit, no longer care about store, consensus or tx-env: ```rust let verifier = TransactionScriptsVerifierWithEnv::new(); let v0_result = verifier.verify_without_limit(ScriptVersion::V0, &rtx); let v1_result = verifier.verify_without_limit(ScriptVersion::V1, &rtx); ``` For customize verification rules, try the follow method: ```rust fn verify_map<R, F>( &self, version: ScriptVersion, rtx: &ResolvedTransaction, mut verify_func: F, ) -> R where F: FnMut(TransactionScriptsVerifier<'_, DataLoaderWrapper<'_, ChainDB>>) -> R; ``` - test: add more unit tests - test: test both vm version 0 and vm version 1 for all syscalls - test: test both ckb 2019 and ckb 2021 for ckb script features Before this PR, old features are only tested for version 0 and new features are only tested for version 1. Co-authored-by: Boyu Yang <[email protected]>

@doitian

2802: refactor(tests): simplify several construction functions and double ckb script tests r=quake,driftluo,zhangsoledad a=yangby-cryptape _Fix the issue posted by @doitian in #2756 (comment) _The best way to review this PR is reviewing each commits._ ### Changes - refactor: simplify vm construction functions which based on versions Before this commit, constructing a core vm requires complicated arguments: ```rust #[cfg(has_asm)] let v0_asm_machine = AsmCoreMachine::new(ISA_IMC, VERSION0, max_cycles); #[cfg(not(has_asm))] let v1_not_asm_machine = DefaultCoreMachine::<u64, WXorXMemory<SparseMemory<u64>>>::new( ISA_IMC | ISA_B | ISA_MOP, VERSION1, u64::max_value(), ); ``` After this commit, no longer care about cargo features, CKB VM versions or IMC sets: ```rust let v0_asm_machine = ScriptVersion:V0.init_core_machine(max_cycles); let v1_not_asm_machine = ScriptVersion::V1.init_core_machine_without_limit(); ``` - refactor(tests): simplify tx script verify construction functions Before this commit, verifying requires complicated arguments: ```rust let fork_at = 10; let store = new_store(); let data_loader = DataLoaderWrapper::new(&store); let hardfork_switch = HardForkSwitch::new_without_any_enabled() .as_builder() .rfc_pr_0237(fork_at) .rfc_pr_0234(fork_at) .build() .unwrap(); let consensus = ConsensusBuilder::default() .hardfork_switch(hardfork_switch) .build(); let tx_env0 = { let epoch = EpochNumberWithFraction::new(0, 0, 1); let header = HeaderView::new_advanced_builder() .epoch(epoch.pack()) .build(); TxVerifyEnv::new_commit(&header) }; let tx_env1 = { let epoch = EpochNumberWithFraction::new(fork_at, 0, 1); let header = HeaderView::new_advanced_builder() .epoch(epoch.pack()) .build(); TxVerifyEnv::new_commit(&header) }; let v0_result = TransactionScriptsVerifier::new(&rtx, &consensus, &data_loader, &tx_env0).verify(100_000_000); let v1_result = TransactionScriptsVerifier::new(&rtx, &consensus, &data_loader, &tx_env1).verify(100_000_000); ``` After this commit, no longer care about store, consensus or tx-env: ```rust let verifier = TransactionScriptsVerifierWithEnv::new(); let v0_result = verifier.verify_without_limit(ScriptVersion::V0, &rtx); let v1_result = verifier.verify_without_limit(ScriptVersion::V1, &rtx); ``` For customize verification rules, try the follow method: ```rust fn verify_map<R, F>( &self, version: ScriptVersion, rtx: &ResolvedTransaction, mut verify_func: F, ) -> R where F: FnMut(TransactionScriptsVerifier<'_, DataLoaderWrapper<'_, ChainDB>>) -> R; ``` - test: add more unit tests - test: test both vm version 0 and vm version 1 for all syscalls - test: test both ckb 2019 and ckb 2021 for ckb script features Before this PR, old features are only tested for version 0 and new features are only tested for version 1. Co-authored-by: Boyu Yang <[email protected]>

2844: chore: log failed transactions results r=doitian,yangby-cryptape a=keroro520 #2756 (comment) Co-authored-by: keroro <[email protected]>

mohanson and others added 8 commits May 20, 2021 08:49

Bump ckb-vm

01ac46c

feat: new syscalls

ca6d22a

redesign exec

0108a6a

Merge branch mohanson/vm (commit: 0108a6a) into branch ckb2021-develop

323a4b2

chore: new vm and new syscalls: use crates.io; fix compilation, test …

ec59a90

…and clippy

Merge branch 'develop' into ckb2021-develop

784fbb0

feat(hardfork): vm version selection, vm version 1 and syscalls 2

fa40d1c

feat(hardfork): reset scripts caches when a new feature enabled which…

e4dac8a

… mays change the script results

chaoticlonghair marked this pull request as ready for review June 15, 2021 14:46

chaoticlonghair requested review from doitian, keroro520, quake, xxuejie and zhangsoledad as code owners June 15, 2021 14:46

chaoticlonghair requested a review from a team June 15, 2021 14:46

This comment has been minimized.

Sign in to view

chaoticlonghair force-pushed the ckb2021-develop branch from 6ca81ab to 48fe026 Compare June 17, 2021 02:29

Merge branch 'develop' into ckb2021-develop

21a10f9

chaoticlonghair force-pushed the ckb2021-develop branch from 48fe026 to 21a10f9 Compare June 17, 2021 02:59

chaoticlonghair added 2 commits June 17, 2021 12:16

fix(hardfork): ckb init will panic when specifies arguments for "bl…

af792f2

…ock_assembler"

chore(deps): bump ckb-vm from 0.20.0-alpha to 0.20.0-alpha2 and ring …

7fc2e6c

…from 0.16.12 to 0.16.20

chaoticlonghair commented Jun 21, 2021

View reviewed changes

tx-pool/src/component/pending.rs Outdated Show resolved Hide resolved

zhangsoledad reviewed Jun 21, 2021

View reviewed changes

tx-pool/src/pool.rs Outdated Show resolved Hide resolved

chaoticlonghair added the s:hold Status: Put this issue on hold. label Jun 23, 2021

Merge branch 'develop' into ckb2021-develop

aba1e6d

chaoticlonghair force-pushed the ckb2021-develop branch from fdcfe41 to db2caca Compare June 29, 2021 05:16

chaoticlonghair added 3 commits June 29, 2021 14:07

refactor(hardfork): change field "hash_type" from a "String" to an "O…

40b4497

…bject" BREAKING CHANGE: The field "hash_type" in "Script" is changed from a "String" to an "Object" for all JSON RPC methods.

refactor(hardfork): disable hardfork feature if not set; set all to e…

f2b9782

…poch 0 in default dev spec

fix(hardfork): the chain specification name is incorrect

59dc192

chaoticlonghair added s:waiting-on-reviewers Status: Waiting for Review and removed s:hold Status: Put this issue on hold. labels Jun 29, 2021

feat(hardfork): reject unknown vm versions and add an integration tes…

09c7975

…t for this feature

chaoticlonghair force-pushed the ckb2021-develop branch from 2769a49 to 09c7975 Compare June 29, 2021 18:48

doitian reviewed Jun 30, 2021

View reviewed changes

driftluo reviewed Jun 30, 2021

View reviewed changes

doitian approved these changes Jun 30, 2021

View reviewed changes

doitian added the u:ckb2021 Upgrade: Fork ckb2021 label Jun 30, 2021

keroro520 reviewed Jun 30, 2021

View reviewed changes

nervos-bot-user reviewed Jun 30, 2021

View reviewed changes

quake reviewed Jul 1, 2021

View reviewed changes

quake approved these changes Jul 1, 2021

View reviewed changes

bors bot merged commit 79e47fb into develop Jul 1, 2021

bors bot deleted the ckb2021-develop branch July 1, 2021 03:41

chaoticlonghair removed the s:waiting-on-reviewers Status: Waiting for Review label Jul 1, 2021

keroro520 reviewed Jul 1, 2021

View reviewed changes

tx-pool/src/process.rs Show resolved Hide resolved

keroro520 reviewed Jul 1, 2021

View reviewed changes

ckb-bin/src/subcommand/init.rs Show resolved Hide resolved

This was referenced Jul 5, 2021

refactor(tests): simplify several construction functions and double ckb script tests #2802

Merged

refactor(hardfork): change field "hash_type" to an enumerated type #2818

Merged

keroro520 mentioned this pull request Jul 19, 2021

chore: log failed transactions results #2844

Merged

bors bot added a commit that referenced this pull request Jul 22, 2021

Merge #2844

1d2e0f3

2844: chore: log failed transactions results r=doitian,yangby-cryptape a=keroro520 #2756 (comment) Co-authored-by: keroro <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hardfork): ckb2021 hardfork features (vm related part) #2756

feat(hardfork): ckb2021 hardfork features (vm related part) #2756

chaoticlonghair commented Jun 15, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nervos-bot-user commented Jun 16, 2021

This comment has been minimized.

nervos-bot-user commented Jun 16, 2021

doitian Jun 30, 2021

chaoticlonghair Jun 30, 2021 •

edited

Loading

driftluo Jun 30, 2021

chaoticlonghair Jun 30, 2021

keroro520 left a comment

nervos-bot-user left a comment

quake Jul 1, 2021

chaoticlonghair Jul 1, 2021

chaoticlonghair Jul 1, 2021

chaoticlonghair commented Jul 1, 2021

bors bot commented Jul 1, 2021

feat(hardfork): ckb2021 hardfork features (vm related part) #2756

feat(hardfork): ckb2021 hardfork features (vm related part) #2756

Conversation

chaoticlonghair commented Jun 15, 2021 • edited Loading

Changes

CKB Chain Edition

BREAKING CHANGES

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nervos-bot-user commented Jun 16, 2021

Benchmark Result

This comment has been minimized.

nervos-bot-user commented Jun 16, 2021

Sync Test Result

doitian Jun 30, 2021

Choose a reason for hiding this comment

chaoticlonghair Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

driftluo Jun 30, 2021

Choose a reason for hiding this comment

chaoticlonghair Jun 30, 2021

Choose a reason for hiding this comment

keroro520 left a comment

Choose a reason for hiding this comment

nervos-bot-user left a comment

Choose a reason for hiding this comment

quake Jul 1, 2021

Choose a reason for hiding this comment

chaoticlonghair Jul 1, 2021

Choose a reason for hiding this comment

chaoticlonghair Jul 1, 2021

Choose a reason for hiding this comment

chaoticlonghair commented Jul 1, 2021

bors bot commented Jul 1, 2021

chaoticlonghair commented Jun 15, 2021 •

edited

Loading

chaoticlonghair Jun 30, 2021 •

edited

Loading