Patch/spc work [WIP] #610

sternhenri · 2019-10-31T00:21:43Z

Continuing #602.

Code compiles
Multiple EC winners speccd (EC with multiple leaders #603)
Clock system specc'd
Timestamp Validation specc'd (rest to come later)
BLS linked
wider validation spec (left TODO)
worker key change spec (TODO: interface between a given miner and worker keys is not yet in spec)

Requests for review:

@jbenet

Some of the code snippet inclusions aren't rendering (i.e. serve and look for "something's not right"). I think it's a directory structure issue in the /algorithms section but would love your thoughts
validation of clock_subsystem, in particular for triggering election retries, and rejecting blocks coming in after epoch cutoff
block producer seems to be pretty stale. Could you take a look?
key_store first draft

@ZenGround0

validation of consensus fault rules

@Kubuxu

validation of weight function
validation of leader election as laid out in code and explanation

@whyrusleeping

That keystore is consistent with your model

@ZX

That Power table fits your model

nikkolasg · 2019-10-31T13:09:23Z

Which solution are you speccing out then (from the issue) ?

src/algorithms/expected_consensus.md

sternhenri · 2019-11-05T00:48:15Z

src/systems/filecoin_blockchain/storage_power_consensus/expected_consensus.go

+	return true
+}
+func (self *ExpectedConsensus_I) GetBlockRewards(electionProof ElectionProof, workerAddr addr.Address) UVarint {
+	panic("")


cc @Kubuxu still working on it, but it'll go here

sternhenri · 2019-11-05T00:49:25Z

src/systems/filecoin_nodes/clock/_index.md

-  - Cesium clocks
- Future work:
-  - VDF Clocks
+{{< readfile file="clock_subsystem.id" code="true" lang="go" >}}


cc @ZenGround0 per your question last week. Please let me know what else you need here.

jbenet

LGTM for the most part 👍 -- some changes needed
Note: election post will change this a fair amount so we should plan to merge this first. pls update + plan to land tomorrow

src/algorithms/expected_consensus.md

src/systems/filecoin_mining/storage_mining/storage_miner_actor.go

src/systems/filecoin_nodes/clock/_index.md

src/systems/filecoin_nodes/clock/clock_subsystem.go

jbenet · 2019-11-05T11:36:52Z

src/systems/filecoin_nodes/clock/clock_subsystem.go

+
+func (self *Clock_I) CurrentEpoch() ChainEpoch {
+	return self.currentEpoch_
+}


This should not live here. the Clock does not know the epoch, the Blockchain does, and that's related to finality in chainsync, bootstrap state and more. Clock here should not have (nor report) the epoch -- defer to the blockchain.

there was a previous idea of a Round/EpochClock but that was going to be a smalll abstraction on top of (having a pointer to) the blockchain system, and deferring to it for calculation (including failing to return because system is not in a state able to return an epoch). we ended up not need it so i didnt write it.

Oh maybe you want to calculate what epoch it should be based on a prior epoch-to-time mapping? I think functions like this are appropriate for this package:

func EpochAfterTime(start block.ChainEpoch, t Time) block.ChainEpoch { // returns start + EpochsInSeconds(t) } // genesis being hard coded func EpochAfterGenesis() block.ChainEpoch { // returns EpochAfterTime(genesis, Now() - timeOfGenesis) }

then caller can take responsibility of passing in the right start epoch, etc.

yes exactly. In fact I was thinking there should be a way to register to NewRound() calls from the clock subsystem to trigger retries of leader election given we no longer have VDFs to do this. Just not sure how we've architected this type of thing elsewhere.

epochDuration (or EpochsInSeconds) should be pulled from blockchain?

jbenet · 2019-11-05T11:42:57Z

src/systems/filecoin_nodes/clock/clock_subsystem.go

+func (self *Clock_I) ResetEpoch(genesisTime Time) struct{} {
+	panic("")
+	// self.currentEpoch = (self.UnixNano() - genesisTime) / epochDuration
+}


Was thinking this could be useful when you are syncing with network again and want to remove skew

src/systems/filecoin_nodes/clock/clock_subsystem.go

sternhenri · 2019-11-06T22:09:22Z

src/systems/filecoin_blockchain/storage_power_consensus/storage_power_actor.md

@@ -20,11 +20,26 @@ SPC provides a power table abstraction which tracks miner power (i.e. miner stor

 An invariant of the storage power consensus subsystem is that all storage in the power table must be verified. That is, miners can only derive power from storage they have already proven to the network.

-In order to achieve this, Filecoin delays updating power for new sector commitments until the first valid PoSt in the next proving period corresponding to that sector.
+In order to achieve this, Filecoin delays updating power for new sector commitments until the first valid PoSt in the next proving period corresponding to that sector. (TODO: potential delay this further in order to ensure that any power cut goes undetected at most as long as the shortest power delay on new sector commitments).


@ZX just want to make sure this is what you had understood before.

cc @nikkolasg

If i understand correctly, that means a miner does a sealing, then has to submit a post after that (when he's elected of after the "period" or when he's "surprised").
If so:
We can do this, but I don't think it's useful to do so, since a sealing already contains a "post" somehow, so it's already sure the miner have spend that much amount of storage. The next post proves that the miner kept that storage for that duration. So after a sealing, the power is already there and proven. Not sure if it is needed to require this additional delay. In short, tt definitely doesn't hurt, but i'm not sure if this is needed.

Only keeping storage for a duration should count for power, so has to be in arrears. This is also needed for symmetry with the possible lag in the network detecting that a sector has been dropped.

sternhenri · 2019-11-06T22:10:10Z

src/systems/filecoin_nodes/key_store/key_store.id

@@ -0,0 +1,15 @@
+import filcrypto "github.com/filecoin-project/specs/algorithms/crypto"


see below @whyrusleeping @jbenet

I would argue we may want to disable the ability to have many workers per owner in v1.

We do need this abstraction if we want to enable key changes though (I can spec in forthcoming PR)

I would argue we may want to disable the ability to have many workers per owner in v1.

yeah me too, I remember that the rationals were not clearly expressed last time we discussed this and were leaning towards being opinions rather than fact-based decisions

…/spc_work

anorth · 2019-11-12T05:35:32Z

I know Election PoSt might have interfered with some bits here, but could we converge and land the rest of it soon?

jbenet · 2019-11-12T07:47:37Z

src/algorithms/expected_consensus/_index.md

-    The ECVRF algorithm (must yield a pseudorandom, deterministic output) from Goldberg et al. Section 5, with:
-        Sha256 for our hashing function
-        Secp256k1 for our curve
+`ElectionProof * TotalPower < e * MinerPower * MaxValue`


this is going to change w/ the new election post. let's merge this as is and follow-up pr soon

jbenet · 2019-11-12T07:54:52Z

src/algorithms/expected_consensus/_index.md

+req = e * minerPower/totalPower
+rewCount = ceil(req - draw)
+reward = rewCount * targetRewardPerEpoch/e
+`


FYI -- I'm not reviewing this closely atm -- this -- i believe -- got reviewed by others already. I will give another pass in the future, but i don't have this paged in atm.

jbenet · 2019-11-12T08:10:00Z

src/algorithms/expected_consensus/_index.md

 (TODO: FIP of submitting commitments to block headers to prevent miners censoring slashers in order to gain rewards).

 It is important to note that there exists a third type of consensus fault directly reported by the `CronActor` on `StorageDeal` failures via the `ReportUncommittedPowerFault` method:
- (3) `uncommitted power fault` which occurs when a miner fails to submit their `PostProof` and is thus participating in leader election with undue power (see {{<sref storage_faults>}}).
+- (4) `uncommitted power fault` which occurs when a miner fails to submit their `PostProof` and is thus participating in leader election with undue power (see {{<sref storage_faults>}}).


PoSt (and PoRep) should not be mentioned in EC. no storage specific stuff should be mentioned here (this is very close, it only appears a little). Doing so breaks through / spaghettifies abstractions, which is painful.

Define the things EC needs from other modules (ie the power accounting, the power fault, etc.) and then satisfy them in another module that wires the two together (eg SPC). SPC should be the one mentioning PoSt, and how failure to submit post triggers the uncommitted power fault. etc.

Fair enough. We just mentioned this storage fault as a "consensus fault" of sorts so for the sake of keeping things in the same place decided to include it here but will break out.

jbenet · 2019-11-12T08:12:14Z

src/systems/filecoin_mining/storage_proving/post_generator.md

@@ -1,92 +0,0 @@
---
-title: PoSt Generator


not sure it makes sense to remove this component. this is meant to be a clear explanation of how the StorageProving subsystem creates posts, submits them etc.

The EC work should not talk about PoSt Submission. SPC can mention what's needed. We need a component (part of storage proving) that describes how (in detail) an implementation needs to produce a post.

i would expect most of the Election-PoSt prose to land here

Agreed, the large issue was mostly that this md was a full dup of the storage_mining/mining_cycle.md and content was better suited there. Will recreate a post_generator for the Election-PoSt

jbenet · 2019-11-12T08:14:55Z

Thanks @sternhenri ! 👏

anorth · 2019-11-14T00:06:54Z

src/systems/filecoin_blockchain/storage_power_consensus/storage_power_actor.md

@@ -20,11 +20,26 @@ SPC provides a power table abstraction which tracks miner power (i.e. miner stor

 An invariant of the storage power consensus subsystem is that all storage in the power table must be verified. That is, miners can only derive power from storage they have already proven to the network.

-In order to achieve this, Filecoin delays updating power for new sector commitments until the first valid PoSt in the next proving period corresponding to that sector.
+In order to achieve this, Filecoin delays updating power for new sector commitments until the first valid PoSt in the next proving period corresponding to that sector. (TODO: potential delay this further in order to ensure that any power cut goes undetected at most as long as the shortest power delay on new sector commitments).


Only keeping storage for a duration should count for power, so has to be in arrears. This is also needed for symmetry with the possible lag in the network detecting that a sector has been dropped.

anorth · 2019-11-14T00:14:08Z

src/systems/filecoin_blockchain/struct/block_producer/_index.md


 Now, a set of messages is selected to put into the block. For each message, the miner subtracts `msg.GasPrice * msg.GasLimit` from the sender's account balance, returning a fatal processing error if the sender does not have enough funds (this message should not be included in the chain).

-They then apply the messages state transition, and generate a receipt for it containing the total gas actually used by the execution, the executions exit code, and the return value (see [receipt](data-structures.md#message-receipt) for more details). Then, they refund the sender in the amount of `(msg.GasLimit - GasUsed) * msg.GasPrice`. In the event of a message processing error, the remaining gas is refunded to the user, and all other state changes are reverted. (Note: this is a divergence from the way things are done in Ethereum)
+They then apply the messages state transition, and generate a receipt for it containing the total gas actually used by the execution, the executions exit code, and the return value . Then, they refund the sender in the amount of `(msg.GasLimit - GasUsed) * msg.GasPrice`. In the event of a message processing error, the remaining gas is refunded to the user, and all other state changes are reverted. (Note: this is a divergence from the way things are done in Ethereum)


The above paragraphs are a bit confused wrt the parent state calculation. The only time messages must be evaluated is when computing the parent state. The block reward must be paid for those parent blocks, not the block being proposed. Receipts are first generated when evaluating the parent state.
No evaluation is necessary for the messages being proposed in the new block (tho miners may speculatively do so to optimize what they include).

FYI @icorderi @acruikshank note the divergence from ethereum here. The spec is asking for exact Gas accounting in the case of failures.

Yes. you are completely right. Sorry for the huge lag here, i take it you are updating but let me know if not and I will do so!

Also note that I took a stab at cleaning up what I clearly saw to be stale in this spec. You have a lot of latitude to drive here imho.

sternhenri added 4 commits October 30, 2019 14:14

remove mention of power suspension

743f805

getting code to compile

6a71f15

remove mention of power suspension

fa58c42

getting code to compile

dd0b1b8

sternhenri added 3 commits November 4, 2019 16:41

added clock subsystem, ec breakout + new details, spc updates

2de4405

link to clock code

3f228d0

odd merge

1d02372

sternhenri commented Nov 5, 2019

View reviewed changes

src/algorithms/expected_consensus.md Show resolved Hide resolved

sternhenri commented Nov 5, 2019

View reviewed changes

sternhenri mentioned this pull request Nov 5, 2019

EC with multiple leaders #603

Closed

cleaned up format, got more compilation working, added chain weight code

f0bd66e

jbenet requested changes Nov 5, 2019

View reviewed changes

added diagrams, touched up clock subsys

fe15161

sternhenri requested review from jbenet, Kubuxu and ZenGround0 November 5, 2019 19:25

sternhenri added 2 commits November 6, 2019 20:11

more compilation, added keystore

d339506

explicited and removed confusing PT language

9463977

sternhenri commented Nov 6, 2019

View reviewed changes

sternhenri added 2 commits November 6, 2019 23:10

Merge branch 'master' of github.com:filecoin-project/specs into patch…

c5da8dd

…/spc_work

cleaned code

e0f9e3a

jbenet approved these changes Nov 12, 2019

View reviewed changes

Merge branch 'master' into patch/spc_work

aa07f25

jbenet merged commit 8ba8055 into master Nov 12, 2019

anorth reviewed Nov 14, 2019

View reviewed changes

sternhenri deleted the patch/spc_work branch November 29, 2019 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch/spc work [WIP] #610

Patch/spc work [WIP] #610

sternhenri commented Oct 31, 2019 •

edited

Loading

nikkolasg commented Oct 31, 2019

sternhenri Nov 5, 2019

sternhenri Nov 5, 2019

jbenet left a comment •

edited

Loading

jbenet Nov 5, 2019

jbenet Nov 5, 2019

sternhenri Nov 5, 2019

sternhenri Nov 5, 2019

jbenet Nov 5, 2019

sternhenri Nov 5, 2019

sternhenri Nov 6, 2019

nikkolasg Nov 12, 2019

anorth Nov 14, 2019

sternhenri Nov 6, 2019

nikkolasg Nov 12, 2019

anorth commented Nov 12, 2019

jbenet Nov 12, 2019

jbenet Nov 12, 2019

jbenet Nov 12, 2019

sternhenri Nov 12, 2019

jbenet Nov 12, 2019

sternhenri Nov 12, 2019

jbenet commented Nov 12, 2019

anorth Nov 14, 2019

anorth Nov 14, 2019

sternhenri Nov 18, 2019

		@@ -0,0 +1,15 @@
		import filcrypto "github.com/filecoin-project/specs/algorithms/crypto"

Patch/spc work [WIP] #610

Patch/spc work [WIP] #610

Conversation

sternhenri commented Oct 31, 2019 • edited Loading

nikkolasg commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anorth commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Nov 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sternhenri commented Oct 31, 2019 •

edited

Loading

jbenet left a comment •

edited

Loading