Expiration Extension for fip45 #653
Conversation
ZenGround0
commented
Sep 14, 2022
•
ZenGround0
commented
- miner logic worked out
- verifreg claim reading call implemented and called from miner (Verifreg GetClaims method #655)
- fix old tests broken by change
- simple unit tests of new expiration
- actually refactor
ZenGround0
force-pushed
the
feat/expiration-fip45
branch
from
0f18255 to
89586ce
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## decouple-fil+ #653 +/- ##
================================================
Coverage ? 84.74%
================================================
Files ? 95
Lines ? 19299
Branches ? 0
================================================
Hits ? 16354
Misses ? 2945
Partials ? 0 |
| // ExtendSectorExpiration param | ||
| struct ExtendExpirationsInner { | ||
| extensions: Vec<ValidatedExpirationExtension>, | ||
| claims: Option<BTreeMap<SectorNumber, u64>>, |
There was a problem hiding this comment.
Aside: You could just use two arguments rather than this struct, since it's not even serializable
There was a problem hiding this comment.
Understood, the idea was to perpetuate the pattern @Kubuxu has been using.
|
can we have a better PR title please? and this might close #596 , right? |
| new_sector.verified_deal_weight = | ||
| verified_deal_space * (new_sector.expiration - new_sector.activation); | ||
| } else { | ||
| new_sector.expiration = new_expiration |
There was a problem hiding this comment.
We weren't extending non QAP sectors before, refactoring helped me catch this bug 🙏
There was a problem hiding this comment.
Consider pulling this line (and 3761) out after the if block
There was a problem hiding this comment.
I thought about this but then we don't have the nice property that duration is specified in terms of fields on the sector info (new_sector.expiration - new_sector.activation) which you asked for in the past and I agree makes this tricky detail very explicit.
actors/miner/src/lib.rs
Outdated
|
|
||
| for sc in &decl.sectors_with_claims { | ||
| let claims = get_claims(rt, &sc.maintain_claims) | ||
| .context(format!("failed to get claims for sector {}", sc.sector_number))?; |
There was a problem hiding this comment.
Use with_context whenever format!ing a message.
There was a problem hiding this comment.
Is this to address @Kubuxu's concern about the expense of format being occurred eagerly in the case where there is no error?
| new_sector.verified_deal_weight = | ||
| verified_deal_space * (new_sector.expiration - new_sector.activation); | ||
| } else { | ||
| new_sector.expiration = new_expiration |
There was a problem hiding this comment.
Consider pulling this line (and 3761) out after the if block
actors/miner/src/types.rs
Outdated
| @@ -145,6 +148,53 @@ pub struct ExpirationExtension { | |||
| pub sectors: UnvalidatedBitField, | |||
| pub new_expiration: ChainEpoch, | |||
| } | |||
| #[allow(clippy::too_many_arguments)] // validate mut prevents implementing From | |||
| impl Into<ValidatedExpirationExtension> for ExpirationExtension2 { | |||
* Expiration Extension 2 handling simple qap extensions * No legacy extension for new sectors * mutate_state in test_vm to create legacy sector to maintain deal weight fraction test * extend_expiration2 miner harness for unit tests * Refactored expiration extension methods to use shared code Co-authored-by: zenground0 <[email protected]>
