Skip to content
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

Block: low-level validation methods refactor #1954

Closed
wants to merge 10 commits into from

Conversation

emersonmacro
Copy link
Contributor

Closes #1879 and #1889

Previous branch in #1925 got stale, so opening a new branch/PR

@codecov
Copy link

codecov bot commented Jun 12, 2022

Codecov Report

Merging #1954 (770b84e) into develop (97554f2) will increase coverage by 3.43%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block ?
blockchain ?
client ?
common 95.01% <ø> (ø)
devp2p 82.64% <ø> (ø)
ethash ?
statemanager 84.55% <ø> (ø)
trie 80.19% <ø> (-1.11%) ⬇️
tx 92.18% <ø> (ø)
util 87.32% <ø> (ø)
vm ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member

Have taken a deeper look here, there are still some basic problems:

  1. This should have been targeted towards master and not develop, not sure if this was some miscommunication? 🤔

  2. The separation lines on the commits here are still not optimal, the base line on this would have been that things (tests) should work after every single commit. Theoretically this should be very much possible since all outlined tasks from the initial issue description from Block: Low-Level Validation Method Refactor #1879 are very much atomic "we move this from a to b" tasks. With commits like 9f1dad0 - removing methods on the one side but not adding to the other - we are again back in the situation that things get into an inconsistent and hard-to-debug in-between state.

I will move this out of the Beta-1 releases, this is getting too short termed.

Emmett likely won't have the time to finish. If someone wants to pick this up we might still be able to integrate in Beta-2 and still integrate in the breaking releases, otherwise it is also no total drama / irretrievable loss if we skip. Would still be nice to integrate though.

@holgerd77
Copy link
Member

Superseeded by #1959, will close.

@holgerd77 holgerd77 closed this Jun 21, 2022
@holgerd77 holgerd77 deleted the block-validation-refactor-2 branch March 2, 2023 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants