refactor(verification): move feature activation dependency #896
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glevco
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #896 +/- ##
==========================================
+ Coverage 84.92% 84.96% +0.04%
==========================================
Files 298 298
Lines 22952 22961 +9
Branches 3466 3467 +1
==========================================
+ Hits 19492 19509 +17
+ Misses 2774 2766 -8
Partials 686 686 ☔ View full report in Codecov by Sentry. |
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
5f5d404 to
cbc3dc1
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
cbc3dc1 to
3e70c8c
Compare
1 task
msbrogli
requested changes
Dec 26, 2023
| @@ -58,6 +58,9 @@ def start(self) -> None: | |||
| Log information related to bit signaling. Must be called after the storage is ready and migrations have | |||
| been applied. | |||
| """ | |||
| if not self._feature_service.is_usage_enabled(): | |||
There was a problem hiding this comment.
I feel it should be done somewhere else. It seems error prone to call start() and get no exception because it just skipped initialization.
There was a problem hiding this comment.
This has been removed from this PR.
| @@ -473,24 +473,20 @@ def _get_or_create_bit_signaling_service(self) -> BitSignalingService: | |||
| def _get_or_create_verification_service(self) -> VerificationService: | |||
| if self._verification_service is None: | |||
| verifiers = self._get_or_create_vertex_verifiers() | |||
| self._verification_service = VerificationService(verifiers=verifiers) | |||
| feature_service = self._get_or_create_feature_service() | |||
| self._verification_service = VerificationService(verifiers=verifiers, feature_service=feature_service) | |||
|
|
|||
| return self._verification_service | |||
|
|
|||
| def _get_or_create_vertex_verifiers(self) -> VertexVerifiers: | |||
There was a problem hiding this comment.
Why not simply verify whether it is enabled or not here?
There was a problem hiding this comment.
This has been removed from this PR.
glevco
force-pushed
the
refactor/daa-dependencies
branch
from
289059e to
c4aa2b8
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
3e70c8c to
8f3d009
Compare
msbrogli
requested changes
Dec 27, 2023
glevco
force-pushed
the
refactor/daa-dependencies
branch
from
c4aa2b8 to
e2f823e
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
8f3d009 to
ac02106
Compare
glevco
force-pushed
the
refactor/daa-dependencies
branch
from
e774d52 to
103162b
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
4 times, most recently
from
2527d3c to
1b24b0c
Compare
glevco
changed the title
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
1b24b0c to
0fc7e30
Compare
|
@msbrogli this PR has been simplified, I recommend re-reviewing it. I've updated the Acceptance Criteria. |
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
0fc7e30 to
d0f339c
Compare
glevco
force-pushed
the
refactor/daa-dependencies
branch
from
103162b to
204fa2c
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
d0f339c to
a73d566
Compare
jansegre
previously approved these changes
Jan 26, 2024
glevco
force-pushed
the
refactor/daa-dependencies
branch
from
204fa2c to
b895832
Compare
glevco
force-pushed
the
refactor/daa-dependencies
branch
3 times, most recently
from
7eb1d79 to
ae05d01
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
4 times, most recently
from
4540afe to
d82a5db
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
2 times, most recently
from
a12297b to
eb0f05d
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
eb0f05d to
43d2174
Compare
glevco
force-pushed
the
refactor/feature-activation/enable-usage
branch
from
43d2174 to
3911dbf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #983
Motivation
Move the Feature Activation verification dependency to outside the verification method. This will be useful for Multiprocess Verification.
Acceptance Criteria
FeatureService.get_bits_description()toget_feature_info(), andFeatureDescriptiontoFeatureInfo.BlockVerifier.verify_mandatory_signaling()so it receives a pre-calculated signaling state, instead of calculating it itself with aFeatureService.MergeMinedBlockVerifier.verify_aux_pow()so it receives a pre-calculatedfeature_info, instead of calculating it itself with aFeatureService.verify_aux_pow()verification fromverify_without_storage()toverify().Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged