Custom masp epoch tracker #3318
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3318 +/- ##
========================================
Coverage 54.05% 54.05%
========================================
Files 315 315
Lines 106296 106433 +137
========================================
+ Hits 57461 57536 +75
- Misses 48835 48897 +62 ☔ View full report in Codecov by Sentry. |
grarco
force-pushed
the
grarco/masp-custom-epoch
branch
2 times, most recently
from
d778188
to
9eef322
Compare
grarco
requested review from
murisi,
sug0,
batconjurer and
yito88
and removed request for
sug0
| @@ -579,10 +579,13 @@ fn pgf_over_ibc_with_hermes() -> Result<()> { | |||
|
|
|||
| #[test] | |||
| fn proposal_ibc_token_inflation() -> Result<()> { | |||
| const MASP_EPOCH_MULTIPLIER: u64 = 2; | |||
There was a problem hiding this comment.
This param seems to make the test fail (if we set it to 1 instead it works). @yito88 investigated the issue and found out that an update to Hermes solves it and makes this test pass
grarco
force-pushed
the
grarco/masp-custom-epoch
branch
from
1e7a9a2
to
58f5590
Compare
| @@ -46,6 +46,9 @@ pub struct Parameters { | |||
| pub implicit_vp_code_hash: Option<Hash>, | |||
| /// Expected number of epochs per year (read only) | |||
| pub epochs_per_year: u64, | |||
| /// The multiplier for masp epochs (it requires this amount of epochs to | |||
| /// transition to the next masp epoch) | |||
| pub masp_epoch_multiplier: u64, | |||
There was a problem hiding this comment.
| pub masp_epoch_multiplier: u64, | |
| pub masp_epoch_multiplier: NonZeroU64, |
There was a problem hiding this comment.
I like this one but we have also other parameters that could benefit from this. Should this become a wider refactor?
There was a problem hiding this comment.
yeah, replacing primitive integer types with their nonzero equivalents where prudent
There was a problem hiding this comment.
Definitely helpful, because if max_proposal_bytes_key is set to 0, it will cause a consensus failure, crash the nodes and halt the chain.
Also, setting max_block_gas to 0, or at least lower than the minimum required for a transaction, will make the chain non-interactable, but the protocol people told me that is acceptable risk.
* up/grarco/masp-custom-epoch: Const masp epoch functions Fmt Changelog #3318 Changes masp epoch multiplier in ibc proposal test Fixes benchmarks Fixes ibc e2e test Fixes integration tests Adds test for masp epoch progression Helper function for masp epoch parameter. Fixes bug in masp epoch detection Tracks `masp_epoch_multiplier` parameter Fallible masp epoch conversion Refactors `is_masp_new_epoch` Refactors `calculate_masp_rewards` Introduces `MaspEpoch` for type safety and a covnersion method from a normal `Epoch` Changes epochs to masp epochs where needed. Extends some traits to return this value Refactors masp epoch Adds masp custom epoch
* origin/grarco/masp-custom-epoch: Const masp epoch functions Fmt Changelog #3318 Changes masp epoch multiplier in ibc proposal test Fixes benchmarks Fixes ibc e2e test Fixes integration tests Adds test for masp epoch progression Helper function for masp epoch parameter. Fixes bug in masp epoch detection Tracks `masp_epoch_multiplier` parameter Fallible masp epoch conversion Refactors `is_masp_new_epoch` Refactors `calculate_masp_rewards` Introduces `MaspEpoch` for type safety and a covnersion method from a normal `Epoch` Changes epochs to masp epochs where needed. Extends some traits to return this value Refactors masp epoch Adds masp custom epoch
Describe your changes
Closes #2833.
This PR tracks masp epochs differently from the usuale
Epoch. A new protocol parameter defines a multiplicative factor that defines a masp epoch in terms on normal epoch (as in, everyxnormal epochs a new masp epochs starts). This way we can decouple the update logic of the the masp conversion tree from the rest of the protocol.A new
MaspEpochis introduced: this is just a wrapper type aroundEpochto make sure we don't pass the wrong epoch to masp functions that deal with masp epochs.Indicate on which release or other PRs this topic is based on
v0.38.1Checklist before merging to
draft