Skip to content

Remove the logic related to the score #162

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.

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

Merged
merged 5 commits into from
Feb 21, 2020
Merged

Remove the logic related to the score #162

merged 5 commits into from
Feb 21, 2020

Conversation

somniumism
Copy link
Contributor

@somniumism somniumism commented Feb 12, 2020

I removed the score in headers. This PR may collide with #153 . To prevent a collision, I removed only the logic related to the score, not the field. Structs, which are related to the score, still have the score. This means that the field not be modified. However, logics such as control flow and some functions were removed. To completely remove the score, PR #153 must be merged first. That's on the next issue, and I think it's okay for this PR to merge.

  • Remove the logic related to the score in the verification queue
  • Remove the logic related to the score in the client
  • Remove the logic related to the score in the tendermint
  • Remove the logic related to the score in the blockchain
  • To check: Replace the value of all scores with U256::default() to check the result
  • Remove the score except it of the Header struct and the Block struct

There is no problem when I replaced the value of all scores with a default value.

What remains to be done on another issue.

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

@somniumism somniumism changed the title [WIPRemove the score [WIP] Remove the score Feb 12, 2020
@somniumism somniumism changed the title [WIP] Remove the score Remove the logic related to the score Feb 13, 2020
@somniumism somniumism added the enhancement New feature or request label Feb 13, 2020
@majecty majecty removed the request for review from sgkim126 February 13, 2020 07:03
@somniumism somniumism added do-not-merge Do not merge this PR and removed do-not-merge Do not merge this PR labels Feb 14, 2020
@majecty majecty requested review from HoOngEe and removed request for majecty February 14, 2020 05:11
@HoOngEe
Copy link
Contributor

HoOngEe commented Feb 14, 2020

You can also remove score-related errors. Then please change the log messages related to scores to appropriate ones.

@sgkim126
Copy link
Contributor

You should consider that the score in tendermint is a combination of a block number and a view count. You cannot just replace a score to a block number. When the block number is the same, you should compare the view of the seal.

@somniumism
Copy link
Contributor Author

somniumism commented Feb 17, 2020

@HoOngEe @sgkim126 I added a condition comparing the view, but it causes many test failures. Actually I don't know a good way to compare views, and I'm not sure what I implemented is right. I need your advice on the design of the condition that compares views.

By the way, the score is defined as follows: u256_from_u128(std::u128::MAX) * height - view. And it is used in control flow to determine the best block. And instead of removing the score, we should add conditions that compare numbers and views. So why don't you modify the score instead of adding a new condition?

@HoOngEe
Copy link
Contributor

HoOngEe commented Feb 17, 2020

The failure is caused by solo consensus. It does not contain cur_view field in the right position as in Tendermint, so the general view check is impossible here. We should find another way.

@sgkim126
Copy link
Contributor

As @HoOngEe said, you cannot generally reveal view in the core module because of the solo consensus. How about implementing a compare function for the Seal structure?

@somniumism
Copy link
Contributor Author

somniumism commented Feb 18, 2020

I added a compare function, and I'm trying to solve 2 test failures in Tendermint:
Error: Timeout of 90000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/somniumism/Project/PR/foundry/test/src/e2e.long/tendermint.test.ts)
and
Error: Timeout period is expired while waiting best blocknumber
I'm looking for the cause.

@HoOngEe HoOngEe self-requested a review February 18, 2020 06:35
@somniumism
Copy link
Contributor Author

I checked that the test failures occur in the part of comparing views.

@somniumism
Copy link
Contributor Author

somniumism commented Feb 18, 2020

@HoOngEe I resolved some problems and checked the e2e tests. I'm waiting for your final review.
By the way, I accidentally chose to merge while resolving the conflict.

@HoOngEe HoOngEe requested a review from sgkim126 February 18, 2020 12:28
@somniumism
Copy link
Contributor Author

@HoOngEe @sgkim126 All requested changes were modified. Could you check this PR again? By the way, This PR consists of 4 commits. Is it okay to be merged without having to squash?

@sgkim126
Copy link
Contributor

Is it okay to be merged without having to squash?

I would merge the commits into one, but if it can be compiled for each commit, I don't matter even they are divided.

@sgkim126
Copy link
Contributor

Why does this PR remove the logic related to score, but doesn't remove the score field?

@majecty
Copy link
Contributor

majecty commented Feb 21, 2020

@sgkim126 I requested @somniumism not to remove the score field.

There were PRs that will conflict with this PR. One PR was adding a field in the header struct. The other PR was changing the whole tests.

I thought we can split removing the score into two PRs. One is not using the score. The other one is removing the score field.

IMO, removing the score field after all the big changes in the test directory will be easier.

@somniumism
Copy link
Contributor Author

I would merge the commits into one, but if it can be compiled for each commit, I don't matter even they are divided.

Each commit is normally built. When the review is finished, I will merge the four commits.

@somniumism somniumism requested a review from sgkim126 February 21, 2020 05:50
@somniumism somniumism merged commit 1a9605b into CodeChain-io:master Feb 21, 2020
@somniumism somniumism deleted the score branch February 21, 2020 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants