Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

removed evaluation.rs #11766

Merged
merged 4 commits into from
Jul 16, 2022
Merged

removed evaluation.rs #11766

merged 4 commits into from
Jul 16, 2022

Conversation

Szegoo
Copy link
Contributor

@Szegoo Szegoo commented Jul 2, 2022 •
edited by davxy
Loading

As mentioned in #11700 the checks in evaluate_initial are not that useful and are there defensively.

Closes #11700

@davxy davxy requested a review from a team July 2, 2022 10:46
@davxy davxy added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 2, 2022
@Szegoo Szegoo requested a review from davxy July 2, 2022 13:00
@davxy
Copy link
Member

davxy commented Jul 4, 2022 •
edited
Loading

LGTM

The only reasons for the evaluate_initial to fail are:

  1. a bug in the codec
  2. a bug in the block builder (i.e. if it sets a bad number or parent hash in the header)

But these things should be detected by the tests and not at run time.

If (for some defensive or conservative reason) the removal of evaluate_initial is not accepted, then we should at least consider to wrap it in a debug_assert!.

@davxy davxy requested a review from a team July 4, 2022 11:01
@bkchr bkchr requested a review from andresilva July 6, 2022 05:07
@bkchr bkchr added the D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit label Jul 6, 2022
@Szegoo
Copy link
Contributor Author

Szegoo commented Jul 8, 2022

@davxy @bkchr I believe that we could merge this.

@bkchr bkchr merged commit 6f65284 into paritytech:master Jul 16, 2022
@Szegoo Szegoo deleted the removed_evaluation.rs branch July 17, 2022 04:34
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@Szegoo @DaviRain-Su
removed evaluation.rs (paritytech#11766)
a16773f 
* removed evaluation.rs

* no need for parent_hash

* fix
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@Szegoo @ark0f
removed evaluation.rs (paritytech#11766)
7ec8a6a 
* removed evaluation.rs

* no need for parent_hash

* fix
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davxy davxy davxy approved these changes

@bkchr bkchr bkchr approved these changes

@andresilva andresilva Awaiting requested review from andresilva

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove consesus/evaluation.rs?
3 participants
@Szegoo @davxy @bkchr