Remove score in headers

[sgkim126]

#1 (comment)

[sgkim126]

I read the code, but it seems not easy.
Currently, many parts use the score to decide the best block.
I think we should remove these parts first.

[somniumism]

@sgkim126 @majecty @HoOngEe Parts using score can be subdivided as follows: core/blockchain, core/consensus/tendermint, core/client, core/verification/queue, core/scheme, and json/src/scheme. I will explain how each part uses score.

---

core/blockchain

The usage of "score"

The score is defined in BlockDetails:

// blockchain/extras.rs
pub struct BlockDetails {
    pub number: BlockNumber,
    /// Total score of the block and all its parents
    pub total_score: U256,
    pub parent: BlockHash,
}

I think the most important parts where score is used is best_block_changed() function and best_header_changed() function:

// best_block_changed() in blockchain/blockchain.rs
if parent_details_of_new_block.total_score + new_header.score() > self.best_proposal_block_detail().total_score
            && engine.can_change_canon_chain(
                new_header.hash(),
                parent_hash_of_new_block,
                grandparent_hash_of_new_block,
                prev_best_hash,
            )



        {

and,

// best_header_changed() in blockchain/headerchain.rs
let is_new_best = parent_details_of_new_header.total_score + new_header.score()
            > self.best_proposal_header_detail().total_score
            && engine.can_change_canon_chain(
                new_header.hash(),
                parent_hash_of_new_header,
                grandparent_hash_of_new_header,
                prev_best_hash,
            );

The condition flow in blockchain.rs seems to change the new block to the best block if the sum of the new block's score and the parent block's score is higher than the total_score of now best block. If the condition flow is not satisfied, it returns BestBlockChanged::None. Likewise, the condition flow in headerchain.rs seems to change the new block to the best block in the similar condition.

Proposal

Here, to remove score without changing the logic, it would be better to replace it with the height of the block. As we know, ‘score’ is related to the height of the block, and it doesn’t look like there will be any problem if the height is used without it. The other part of using score is updating the value of total_score. Therefore, I don't think it matters.

---

core/consensus/tendermint

The usage of "score"

I think the most important part where score is used is verify_header_basic() function:

// verify_header_basic() in tendermint/worker.rs
        let score = calculate_score(height, author_view);

        if *header.score() != score {
            return Err(BlockError::InvalidScore(Mismatch {
                expected: score,
                found: *header.score(),
            })
            .into())
        }

Proposal

In this function, I think the score is being used to verify the headers. If the calculated score value differs from the current header's score, it returns an error. So, I think it is better to replace score with a means that can be verified, such as checksum or height. Like core/blockchain, the other part of using score is updating the value of total_score. Therefore, I don't think it matters.

---

core/client

The usage of "score"

score and its friends are defined in BlockchainInfo struct in core/src:

// BlockChainInfo in core/src/block.rs
pub struct BlockChainInfo {
    /// Score of the canonical chain.
    pub best_score: U256,
    /// The best score from proposal blocks.
    pub best_proposal_score: U256,
    /// Block queue score.
    pub pending_total_score: U256,

Proposal

I think they are not used for verification and condition flow, but just for recording. When I removed them and modified some of the codes to check results caused by removing the score, I confirmed that the test results are normal. There are no condition flow and no verification that use the score. I think I can easily remove the score here.

---

core/verification/queue

The usage of "score"

I think the most important part where score is used is import() in impl VerificationQueue:

// import() in queue/mod.rs
match K::create(input, &*self.engine) {
            Ok(item) => {
                self.verification.sizes.unverified.fetch_add(item.mem_usage(), AtomicOrdering::SeqCst);

                self.processing.write().insert(h, item.score());
                {
                    let mut ts = self.total_score.write();
                    *ts += item.score();
                }

                self.verification.unverified.lock().push_back(item);
                self.more_to_verify.notify_all();
                Ok(h)
            }
            Err(err) => {
                match err {

Proposal

The import() function is to add a block to the queue. In the process of verifying queues, I checked that the score is used. When I removed codes related to verification and to check results caused by removing the score, I checked that some test codes fail. I couldn't find the exact cause of the failure. The test code is return_error_for_duplicates() and I founded that it returns an error called "must process error". I think I have some difficulty working on this part.

The other part of using score is updating its value, so I don't think the other part matters.

---

core/scheme

The usage of "score"

/// Genesis components.
pub struct Genesis {
    /// Seal.
    pub seal: Seal,
    /// Score.
    pub score: U256,
    /// Author.
    pub author: Address,
    /// Timestamp.
    pub timestamp: u64,

pub struct Scheme {
    pub name: String,
    pub engine: Arc<dyn CodeChainEngine>,
    pub data_dir: String,
    pub nodes: Vec<String>,
    pub parent_hash: BlockHash,
    pub author: Address,
    pub score: U256,
    ....

Proposal

In this part, since the core is declared only and is not used for specific purposes, it will be easy to remove.

---

json/src/scheme

The usage of "score"

Here, there are test codes in JSON format as follows:

// scheme/cuckoo.rs
    fn cuckoo_deserialization() {
        let s = r#"{
            "params": {
                "blockReward": "0x0d",
                "blockInterval" : "120",
                "minScore" : "0x020000",
                "maxVertex" : "16",
                "maxEdge" : "8",
                "cycleLength" : "6",
                "recommendedConfirmation": 6
            }
        }"#;

and,

// scheme/genesis.rs
    fn genesis_deserialization() {
        let s = r#"{
            "score": "0x400000000",
            "seal": {
                "tendermint": {
                    "prev_view": "0x0",
                    "cur_view": "0x0",
                    "precommits": [ ... 

Proposal

I think I can revise other things first and then modify this. Nothing matters except this.

---

Summary

In summary, I subdivided the parts that are using the score and explained how they are being used. If I remove the score in all parts at once, the process of removing it can be time-consuming and difficult. Therefore, after discussing with @majecty , I will remove score in the following order: 1) core/verification/queue, 2) core/client, 3) core/consensus/tendermint, 4) core/blockchain, and 5) others. This plan should be discussed further with @HoOngEe . Do you think it is fine that I start working?

[majecty]

In this function, I think the score is being used to verify the headers. If the calculated score value differs from the current header's score, it returns an error. So, I think it is better to replace score with a means that can be verified, such as checksum or height.

Could you check whether height and view is verified or not? If height and view is verified in other code, we don't need to check it again. We can just remove the score verification logic.

[majecty]

@HoOngEe

Here, to remove score without changing the logic, it would be better to replace it with the height of the block. As we know, ‘score’ is related to the height of the block, and it doesn’t look like there will be any problem if the height is used without it. The other part of using score is updating the value of total_score. Therefore, I don't think it matters.

IMO, we may use the block number. However, we should check how the header sync works again. @HoOngEe, Could you check it? I remembered that the score was used to select the best proposal block. Previous header sync code was requesting a peer's headers if the peer has a higher scored header. I heard that snapshot sync changed some of the behavior. So I want to know the relationship between header sync and best proposal header.

[sgkim126]

How about moving the prev_view and cur_view from a seal to a header?

[majecty]

@sgkim126 I agree with you. We don't need a fat seal field anymore.

[majecty]

@sgkim126 IMO, let's move the "prev_view" and "cur_view" fields in a separate issue. To change the header struct, we need to change some tests, and it will conflict with Junha's PR. #153

[somniumism]

Could you check whether height and view is verified or not? If height and view is verified in other code, we don't need to check it again. We can just remove the score verification logic.

@majecty I found these codes related to verification for the height and the view.

1. The function vote_on_header_for_proposal() has assert_eq!(header.number(), self.height);
2. The functiongenerate_seal() has assert_eq!(height, self.height);
3. The function proposal_generated() has proposal_height == self.height && proposal_author_view != self.view

[majecty]

@somniumism Then we don't need to verify them again. Thanks.

[HoOngEe]

@somniumism I think you can start working on it.

[somniumism]

We split removing the score into two PRs. One is not using the score. The other one is removing the score field. Changing the Header and Block field, or Removing the logic and related to the Header and Block can cause a collision, so we don't have to do this right now. So I have worked on the former PR #162 (not using the score), and it was merged. The next work still remaining is as follows:

• Remove the score in all the field
• To check: Check e2e tests
• Modify e2e tests for the Foundry without the score

I will close this issue for the time being and create new issue #208 .