feat(feature-activation): implement closest_block metadata and Feature Activation for Transactions #933
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
1 task
…e Activation for Transactions
glevco
force-pushed
the
feat/feature-activation/txs-closest-block
branch
from
cfe1640
to
d3d069c
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## fix/feature-activation/add-missing-migration #933 +/- ##
================================================================================
+ Coverage 85.30% 85.32% +0.02%
================================================================================
Files 289 290 +1
Lines 22411 22465 +54
Branches 3373 3381 +8
================================================================================
+ Hits 19117 19168 +51
- Misses 2620 2623 +3
Partials 674 674 ☔ View full report in Codecov by Sentry. |
This was referenced Jan 23, 2024
msbrogli
requested changes
Jan 25, 2024
| """Returns whether a Feature is active at a certain block.""" | ||
| def is_feature_active_for_transaction(self, *, tx: 'Transaction', feature: Feature) -> bool: | ||
| """Return whether a Feature is active for a certain Transaction.""" | ||
| metadata = tx.get_metadata() |
There was a problem hiding this comment.
Assert that isinstance(tx, Transaction) to prevent bugs if mypy fails to detect wrong call types.
| this_closest_block = vertex | ||
| elif isinstance(vertex, Transaction): | ||
| this_closest_block_id = ( | ||
| self._settings.GENESIS_BLOCK_HASH if vertex.is_genesis else not_none(vertex_meta.closest_block) |
There was a problem hiding this comment.
I guess you can safely do not_none(vertex_meta.closest_block).
| return | ||
| assert isinstance(self, Transaction) | ||
| assert self.storage is not None | ||
| metadata = self.get_metadata() |
There was a problem hiding this comment.
Check if metadata.closest_block is not None and returns if true.
|
|
||
| if isinstance(vertex, Block): | ||
| assert vertex_meta.closest_block is None | ||
| this_closest_block = vertex |
| # For Blocks, this is None. For Transactions, this is the Block with the greatest height that is a direct or | ||
| # indirect dependency (ancestor) of the transaction, including both funds and confirmation DAGs. | ||
| # It's used by Feature Activation for Transactions. | ||
| closest_block: VertexId | None |
There was a problem hiding this comment.
I'd name it closest_ancestor_block.
glevco
force-pushed
the
fix/feature-activation/add-missing-migration
branch
2 times, most recently
from
d5306f4
to
bbcd88a
Compare
Base automatically changed from
fix/feature-activation/add-missing-migration
to
master
January 29, 2024 03:20
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 #927
Motivation
Implement the new
closest_blockmetadata that will enable Feature Activation for Transactions.Acceptance Criteria
FeatureService.is_feature_active()tois_feature_active_for_block().FeatureService.is_feature_active_for_transaction().closest_blockmetadata, with a migration for it.BaseTransaction._update_closest_block_metadata().Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged