Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

Aggregate PoRep #1381

Merged
merged 1 commit into from
May 14, 2021
Merged

Aggregate PoRep #1381

merged 1 commit into from
May 14, 2021

Conversation

ZenGround0
Copy link
Contributor

@ZenGround0 ZenGround0 commented Feb 23, 2021

Changes needed for introducing aggregate porep verification into the network.

  • Implement ProveCommitAggregate
  • VM tests of ProveCommit and ProveCommitAggregate to measure gas costs
  • Batch market ComputeDataCommitment call to save gas
  • ComputeDataCommitment testing
  • Check power claim during ProveCommitAggreagate to retain safety properties protecting against programmer error in deadline code
  • Accurate gas table
  • Final min and max aggregate batch parameters and max proof size parameter
  • Extend sector precommit expiry
  • AggregateProofType for upgradeability
  • Unit (and or more extensive VM) tests of ProveCommitAggregate

--edit--
I'm calling final Gas and param estimation out of scope because we'll need to merge this into lotus to start gas rebalancing. We will do the final parameter setting in another PR.

Closes #1257.

@codecov-io
Copy link

codecov-io commented Feb 23, 2021

Codecov Report

Merging #1381 (b3317b8) into master (ddca3ba) will decrease coverage by 0.6%.
The diff coverage is 25.4%.

@@           Coverage Diff            @@
##           master   #1381     +/-   ##
========================================
- Coverage    64.9%   64.3%   -0.7%     
========================================
  Files          81      81             
  Lines        8197    8287     +90     
========================================
+ Hits         5328    5333      +5     
- Misses       1977    2062     +85     
  Partials      892     892             

@codecov-commenter
Copy link

codecov-commenter commented May 2, 2021

Codecov Report

Merging #1381 (68bfbe1) into master (f6a8d2a) will decrease coverage by 0.0%.
The diff coverage is 77.7%.

@@           Coverage Diff            @@
##           master   #1381     +/-   ##
========================================
- Coverage    69.6%   69.6%   -0.1%     
========================================
  Files          72      72             
  Lines        7690    7795    +105     
========================================
+ Hits         5356    5429     +73     
- Misses       1443    1465     +22     
- Partials      891     901     +10     

@ZenGround0 ZenGround0 marked this pull request as ready for review May 5, 2021 19:16
@ZenGround0 ZenGround0 changed the base branch from feat/v5 to master May 5, 2021 19:22
@ZenGround0 ZenGround0 changed the title Spike Aggregate PoRep Aggregate PoRep May 5, 2021
@ZenGround0 ZenGround0 changed the base branch from master to feat/v5 May 5, 2021 20:46
@ZenGround0 ZenGround0 changed the base branch from feat/v5 to master May 5, 2021 20:51
@ZenGround0 ZenGround0 changed the base branch from master to feat/v5 May 5, 2021 20:51
@ZenGround0 ZenGround0 changed the base branch from feat/v5 to master May 5, 2021 20:56
Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review so far.

This PR could have been a number of smaller ones. The data commitment changes are fine, they could land if in a separate PR.

actors/builtin/market/market_actor.go Outdated Show resolved Hide resolved
actors/builtin/market/market_actor.go Show resolved Hide resolved
actors/builtin/market/market_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
actors/test/commit_post_test.go Outdated Show resolved Hide resolved
actors/test/commit_post_test.go Outdated Show resolved Hide resolved
actors/test/commit_post_test.go Outdated Show resolved Hide resolved
}},
{To: builtin.StorageMarketActorAddr, Method: builtin.MethodsMarket.CronTick},
},
}.Matches(t, v.Invocations()[1])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this whole "check nothing in cron" is overkill here.

actors/test/commit_post_test.go Show resolved Hide resolved
actors/builtin/miner/miner_actor.go Outdated Show resolved Hide resolved
@ZenGround0 ZenGround0 removed the request for review from arajasek May 13, 2021 15:11
@BigLep BigLep linked an issue May 13, 2021 that may be closed by this pull request
@@ -74,6 +74,7 @@ func (a Actor) Exports() []interface{} {
22: a.RepayDebt,
23: a.ChangeOwnerAddress,
24: a.DisputeWindowedPoSt,
25: a.ProveCommitAggregate,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZenGround0 you'll need to rebase against the new method in #1407, now landed.

- ProveCommitAggregate
- Update syscalls
- Scenario tests for measuring aggregate vs current porep
- Batch inputs to ComputeDataCommitment
- MaxProveCommitDuration 1 day => 6 days
@ZenGround0 ZenGround0 merged commit af4cd0f into master May 14, 2021
@BigLep BigLep added this to the v6 milestone May 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add b̶a̶t̶c̶h̶e̶d̶ aggregated SectorProveCommit endpoint
7 participants