Dag sync test case use 0 as fork number #4130

jackzhhuang · 2024-06-05T07:57:44Z

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Other information

nkysg · 2024-06-05T11:36:44Z

chain/src/chain.rs

@@ -832,7 +873,9 @@ impl BlockChain {
                        &tips_hash, blues
                    );
                    let mut blue_blocks = vec![];
-                    let _selected_parent = blues.remove(0);
+                    let selected_parent = blues.remove(0);


https://doc.rust-lang.org/std/vec/struct.Vec.html#method.remove

Removes and returns the element at position index within the vector, shifting all elements after it to the left. Note: Because this shifts over the remaining elements, it has a worst-case performance of O(n). If you don’t need the order of elements to be preserved, use [swap_remove](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.swap_remove) instead. If you’d like to remove elements from the beginning of the Vec, consider using [VecDeque::pop_front](https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.pop_front) instead.

Is this place we need check blue_blocks length greater than 1. I noticed there is another place have the same code.

starcoin/miner/src/create_block_template/mod.rs

Line 365 in 561bf86

let __selected_parent = blues.remove(0);

Let me think about it. Maybe it's better to do it.
.

nkysg · 2024-06-05T11:42:46Z

miner/src/create_block_template/mod.rs

@@ -362,7 +387,9 @@ where
                    );
                    let mut blue_blocks = vec![];

-                    let __selected_parent = blues.remove(0);
+                    let selected_parent = blues.remove(0);


the same issue

simonjiao · 2024-06-05T14:04:04Z

miner/src/create_block_template/mod.rs

-                    let __selected_parent = blues.remove(0);
+                    let selected_parent = blues.remove(0);
+                    previous_header =
+                        self.get_dag_previous_header(previous_header, selected_parent)?;


What's the purpose of updating previous_header here? Do we need to update the miner's block forging process?

I think maybe he wants to solve header not update but blockdag tips update. @jackzhhuang

I have two questions, the second one？ Is there a issue tracking on it?

It is necessary to update the chain object in miner service.
This is the case caused by the 0 dag fork height, see test_block_chain_reset.

simonjiao · 2024-06-05T14:06:54Z

config/src/genesis_config.rs

@@ -718,7 +718,7 @@ static G_EMPTY_BOOT_NODES: Lazy<Vec<MultiaddrWithPeerId>> = Lazy::new(Vec::new);
 const ONE_DAY: u64 = 86400;

 // for test


Please remove this constant and use genesis config to set dag height. See G_DEV_CONFIG.

Historically, this constant is used for transaction timeout and if you make sure that it is should be removed, you remove in your branch.
There are two logic in test case: one for the single chain and other for the dag chain. Both need running in the test net and 0 as fork number. But I saw the fork height in Genesis Config is u64::MAX which is not suitable for those two cases.

And some test case, such as test_multiple_node_sync, the miner service is a must to mint blocks automatically. But in dev net, because the config set, the block will not be minted leading to failure or timeout.

As you said, there's still lots of failed cases which need to a fix if we set dag-fork-height as ZERO in genesis config, right? I don't think this PR has done the work as it's title says, right? Please do the job like what you said.

And some test case, such as test_multiple_node_sync, the miner service is a must to mint blocks automatically. But in dev net, because the config set, the block will not be minted leading to failure or timeout.

So change it to right value, run the tests and fix it. THX.

I really did not understand what you want. But this time, you mean I have to do the job that let the starcoin run in fork height 0 by using the config you defined? If it is, that is fine I am happy to do that. But you can say your requirement directly next time.

Why no checking the dag or not ? This is the branch sync test cases using dag fork height 0 run successfully. Any other big feature or improvement should be in other branches.

Yes. No need to check it, The genesis configuration make sure it always be a DAG, every blocks, headers are constructed for DAG.

And some test case, such as test_multiple_node_sync, the miner service is a must to mint blocks automatically. But in dev net, because the config set, the block will not be minted leading to failure or timeout.

So change it to right value, run the tests and fix it. THX.

I really did not understand what you want. But this time, you mean I have to do the job that let the starcoin run in fork height 0 by using the config you defined? If it is, that is fine I am happy to do that. But you can say your requirement directly next time.

If you donot want it, don't do it. This is a small team, everyone should do his best.

As the pr title shows and at the meeting I was assigned to let the test cases ci run successfullly at fork height 0 and the rest will be done in your branch, now you ask me to do the rest? Ensure this or I am afraid of the code conflict.

As the pr title shows and at the meeting I was assigned to let the test cases ci run successfullly at fork height 0 and the rest will be done in your branch, now you ask me to do the rest? Ensure this or I am afraid of the code conflict.

Suit yourself.

simonjiao · 2024-06-07T02:18:00Z

sync/src/sync.rs

@@ -233,7 +233,7 @@ impl SyncService {
                storage.get_block_info(current_block_id)?.ok_or_else(|| {
                    format_err!("Can not find block info by id: {}", current_block_id)
                })?;
-            let dag_fork_height = chain_service.dag_fork_height().await?;
+            let dag_fork_height = Some(config.net().genesis_config().dag_effective_height);


For safety, we need get to it from statedb, technically, the pre-set value is only used when executing genesis block.

it means this value can be changed through somewhere, such as framework?

it means this value can be changed through somewhere, such as framework?

Yes. DAO is one way to reset it.

Why do we still need dag_fork_height ?

Because the code maintains synchronization logic of the sing chain and dag and it is flexible.

I remember that sanlee42 suggests that we use some cfg macros @sanlee42 .

sanlee42 · 2024-06-07T02:37:23Z

chain/tests/test_txn_info_and_proof.rs

@@ -48,18 +48,15 @@ fn test_transaction_info_and_proof_1() -> Result<()> {
    let config = Arc::new(NodeConfig::random_for_test());
    let mut block_chain = test_helper::gen_blockchain_for_test(config.net())?;

-    // set the flexidag fork height to G_TEST_DAG_FORK_HEIGHT
    block_chain
        .dag()
        .save_dag_state(*G_TEST_DAG_FORK_STATE_KEY, DagState { tips: vec![] })?;


why we still need G_TEST_DAG_FORK_STATE_KEY?

sanlee42 · 2024-06-07T02:58:55Z

sync/src/block_connector/write_block_chain.rs

+        use starcoin_config::genesis_config::G_TEST_DAG_FORK_STATE_KEY;
+        use starcoin_dag::consensusdb::consenses_state::DagState;
+
+        dag.save_dag_state(*G_TEST_DAG_FORK_STATE_KEY, DagState { tips: vec![] })?;


consider remove it?

sanlee42 · 2024-06-07T03:04:08Z

sync/src/sync.rs

@@ -233,7 +233,7 @@ impl SyncService {
                storage.get_block_info(current_block_id)?.ok_or_else(|| {
                    format_err!("Can not find block info by id: {}", current_block_id)
                })?;
-            let dag_fork_height = chain_service.dag_fork_height().await?;
+            let dag_fork_height = Some(config.net().genesis_config().dag_effective_height);


it means this value can be changed through somewhere, such as framework?

jackzhhuang · 2024-06-07T03:31:10Z

I am still working on the code. I will periodically save and stage the changes.

sanlee42 · 2024-06-07T06:50:54Z

I am still working on the code. I will periodically save and stage the changes.

You can draft it until it finish.

sync/src/tasks/tests_dag.rs

simonjiao · 2024-06-07T11:22:04Z

sync/src/tasks/tests.rs

@@ -105,6 +105,9 @@ pub async fn test_sync_invalid_target() -> Result<()> {
    Ok(())
 }

+#[ignore = "This test is for the scenario that a block failed to connect to the main will be stored in the \
+failure storage and the sync will return failure instantly the next time the block shows up again, \
+which is no longer suitable for the dag"]


Change the networkid to network with u64::max dag fork height, such as Test. This test can be fixed.

It cannot be fixed as you said. You try and will know.

emm... It did fail. This is a case for the single chain, and should be fixed by this walkaround solution. Could some updates break the procedures? But anyway, it's your call.

sync/src/tasks/mock.rs

sync/src/tasks/tests_dag.rs

2, return test message in apply failed function

2, remove dag genesisi enum in chain type

…arcoin into dag-master-sync-testcase0

* use 0 as fork number * fix some test case that uses 0 as fork number * check the blues returned by the ghostdata whether it is empty * use genesis config to get the dag fork number * fix clippy * basic sync dag test case pass * fix all sync test case running in the dag using the genesis config * 1, use match to determine the way of the execution and verification 2, return test message in apply failed function * fix test_flexidag_config_get_for_halley * add dag test genesis * add is dag test in test_example_config_compact * remove the used files and codes * fix test_custom_genesis * add is dag test beside the is test * fix test_generated_schema_are_up_to_date_in_git * fix fmt * add dag test in node.json * add check genesis identical between two nodes in test_sync_dag_blocks * return error if no blue uncle found, no expect no panic * add test case test_dag_uncles * fix genesis generating problem * renew halley genesis * renew halley genesis * renew halley genesis * remove the dag init in dag block execution process * 1, change dag type to chain type 2, remove dag genesisi enum in chain type * use 0 as fork number * fix some test case that uses 0 as fork number * check the blues returned by the ghostdata whether it is empty * use genesis config to get the dag fork number * fix clippy * basic sync dag test case pass * fix all sync test case running in the dag using the genesis config * 1, use match to determine the way of the execution and verification 2, return test message in apply failed function * fix test_flexidag_config_get_for_halley * add dag test genesis * add is dag test in test_example_config_compact * remove the used files and codes * fix test_custom_genesis * add is dag test beside the is test * fix test_generated_schema_are_up_to_date_in_git * fix fmt * add dag test in node.json * add check genesis identical between two nodes in test_sync_dag_blocks * return error if no blue uncle found, no expect no panic * add test case test_dag_uncles * fix genesis generating problem * renew halley genesis * renew halley genesis * renew halley genesis * remove the dag init in dag block execution process * 1, change dag type to chain type 2, remove dag genesisi enum in chain type * Fix clippy * rebase dag master * add new dag block subscribe --------- Co-authored-by: 0xa <hit.cs.lijun@gmail.com>

jackzhhuang requested review from welbon and simonjiao June 5, 2024 07:57

jackzhhuang requested review from sanlee42 and nkysg as code owners June 5, 2024 07:57

nkysg reviewed Jun 5, 2024

View reviewed changes

nkysg self-requested a review June 5, 2024 12:15

simonjiao requested changes Jun 5, 2024

View reviewed changes

nkysg mentioned this pull request Jun 6, 2024

Use 0 as dag fork number in test case #4129

Closed

7 tasks

jackzhhuang requested a review from jolestar as a code owner June 7, 2024 02:00

simonjiao reviewed Jun 7, 2024

View reviewed changes

sanlee42 requested changes Jun 7, 2024

View reviewed changes

sanlee42 removed the request for review from jolestar June 7, 2024 03:05

jackzhhuang marked this pull request as draft June 7, 2024 07:29

jackzhhuang marked this pull request as ready for review June 7, 2024 10:49

simonjiao requested changes Jun 7, 2024

View reviewed changes

jackzhhuang force-pushed the dag-master-sync-testcase0 branch from f4c705a to 903bf14 Compare June 11, 2024 10:31

simonjiao requested changes Jun 12, 2024

View reviewed changes

sync/src/tasks/mock.rs Show resolved Hide resolved

sync/src/tasks/tests_dag.rs Show resolved Hide resolved

sync/src/tasks/tests_dag.rs Outdated Show resolved Hide resolved

jackzhhuang added 10 commits June 14, 2024 09:37

use 0 as fork number

8782946

fix some test case that uses 0 as fork number

4466759

check the blues returned by the ghostdata whether it is empty

c5de764

use genesis config to get the dag fork number

be0578c

fix clippy

d2d7e9a

basic sync dag test case pass

e337c81

fix all sync test case running in the dag using the genesis config

0a291b8

1, use match to determine the way of the execution and verification

70293af

2, return test message in apply failed function

fix test_flexidag_config_get_for_halley

bde0781

add dag test genesis

e8a3466

jackzhhuang and others added 21 commits June 18, 2024 21:07

1, use match to determine the way of the execution and verification

1dbf862

2, return test message in apply failed function

fix test_flexidag_config_get_for_halley

5393132

add dag test genesis

c8b2f25

add is dag test in test_example_config_compact

5f3173a

remove the used files and codes

f95e4a9

fix test_custom_genesis

0eb9da8

add is dag test beside the is test

83997b6

fix test_generated_schema_are_up_to_date_in_git

c6aee22

fix fmt

8ad6573

add dag test in node.json

2acb5db

add check genesis identical between two nodes in test_sync_dag_blocks

e9a6f3d

return error if no blue uncle found, no expect no panic

b8d4a2b

add test case test_dag_uncles

9124240

fix genesis generating problem

cb0eba4

renew halley genesis

0ac93b2

renew halley genesis

42c5219

renew halley genesis

f400a14

remove the dag init in dag block execution process

7e5b581

1, change dag type to chain type

604004d

2, remove dag genesisi enum in chain type

Merge branch 'dag-master' into dag-master-sync-testcase0

d80d068

Fix clippy

2a7bf8c

sanlee42 force-pushed the dag-master-sync-testcase0 branch from f7c4571 to 2a7bf8c Compare June 18, 2024 15:59

jackzhhuang added 3 commits June 19, 2024 00:10

rebase dag master

f4211bb

Merge branch 'dag-master-sync-testcase0' of github.com:jackzhhuang/st…

b8b0431

…arcoin into dag-master-sync-testcase0

add new dag block subscribe

6e35a64

jackzhhuang enabled auto-merge (squash) June 18, 2024 17:26

jackzhhuang merged commit 6069524 into starcoinorg:dag-master Jun 19, 2024
2 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dag sync test case use 0 as fork number #4130

Dag sync test case use 0 as fork number #4130

jackzhhuang commented Jun 5, 2024

nkysg Jun 5, 2024 •

edited

Loading

jackzhhuang Jun 5, 2024

nkysg Jun 5, 2024

simonjiao Jun 5, 2024 •

edited

Loading

nkysg Jun 5, 2024

jackzhhuang Jun 6, 2024

simonjiao Jun 6, 2024

jackzhhuang Jun 6, 2024

simonjiao Jun 5, 2024

jackzhhuang Jun 6, 2024

jackzhhuang Jun 6, 2024 •

edited

Loading

jackzhhuang Jun 6, 2024

simonjiao Jun 6, 2024 •

edited

Loading

jackzhhuang Jun 6, 2024

simonjiao Jun 6, 2024

simonjiao Jun 6, 2024 •

edited

Loading

jackzhhuang Jun 6, 2024

simonjiao Jun 6, 2024

simonjiao Jun 7, 2024 •

edited

Loading

sanlee42 Jun 7, 2024

simonjiao Jun 7, 2024

nkysg Jun 7, 2024

jackzhhuang Jun 11, 2024

nkysg Jun 11, 2024

sanlee42 Jun 7, 2024

sanlee42 Jun 7, 2024

sanlee42 Jun 7, 2024

jackzhhuang commented Jun 7, 2024

sanlee42 commented Jun 7, 2024

simonjiao Jun 7, 2024

jackzhhuang Jun 11, 2024

simonjiao Jun 11, 2024 •

edited

Loading

		@@ -718,7 +718,7 @@ static G_EMPTY_BOOT_NODES: Lazy<Vec<MultiaddrWithPeerId>> = Lazy::new(Vec::new);
		const ONE_DAY: u64 = 86400;

		// for test

Dag sync test case use 0 as fork number #4130

Dag sync test case use 0 as fork number #4130

Conversation

jackzhhuang commented Jun 5, 2024

Pull request type

What is the current behavior?

What is the new behavior?

Other information

nkysg Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjiao Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzhhuang Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjiao Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjiao Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjiao Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzhhuang commented Jun 7, 2024

sanlee42 commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonjiao Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

nkysg Jun 5, 2024 •

edited

Loading

simonjiao Jun 5, 2024 •

edited

Loading

jackzhhuang Jun 6, 2024 •

edited

Loading

simonjiao Jun 6, 2024 •

edited

Loading

simonjiao Jun 6, 2024 •

edited

Loading

simonjiao Jun 7, 2024 •

edited

Loading

simonjiao Jun 11, 2024 •

edited

Loading