Add NextDoneSet and ProvingSet abstractions to miner actor #2999

ZenGround0 · 2019-06-25T21:45:13Z

This PR adds the NextDoneSet and ProvingSet abstractions to the miner actor. It is best read commit by commit. The first commit introduces a done parameter into submitPoSt. The second adds the NextDoneSet abstraction. The third introduces a SectorSet abstraction to keep the next commit clean. The fourth introduces the ProvingSet.

The edge case of the ProvingSet being updated during the first commitSector message is not handled in this PR. I'm working on this in an upcoming followup.

codecov-io · 2019-06-25T23:02:37Z

Codecov Report

Merging #2999 into master will decrease coverage by <1%.
The diff coverage is 54%.

@@           Coverage Diff           @@
##           master   #2999    +/-   ##
=======================================
- Coverage      45%     44%    -1%     
=======================================
  Files         213     214     +1     
  Lines       12775   12843    +68     
=======================================
- Hits         5775    5742    -33     
- Misses       6117    6227   +110     
+ Partials      883     874     -9

Impacted Files	Coverage Δ
abi/abi.go	`20% <0%> (-2%)`	⬇️
protocol/storage/miner.go	`48% <100%> (ø)`	⬆️
types/intset.go	`68% <14%> (-32%)`	⬇️
actor/builtin/miner/miner.go	`55% <81%> (ø)`	⬆️
actor/builtin/miner/sector_set.go	`92% <92%> (ø)`
proofs/sectorbuilder/testing/builder.go	`0% <0%> (-88%)`	⬇️
proofs/sectorbuilder/testing/harness.go	`0% <0%> (-58%)`	⬇️
protocol/hello/hello.go	`86% <0%> (-3%)`	⬇️
node/node.go	`59% <0%> (-2%)`	⬇️

ZenGround0 · 2019-06-26T13:33:09Z

actor/builtin/miner/miner_test.go

+
+	// Check that the state NextDoneSet is updated
+	miner := state.MustGetActor(st, minerAddr)
+	storage := vms.NewStorage(minerAddr, miner)


Please let me know if there is a better way to look at actor state. This seems sloppy

ZenGround0 · 2019-06-26T13:35:33Z

actor/builtin/miner/miner_test.go

 		require.Equal(t, uint8(0x23), res.Receipt.ExitCode)
 	})
 }

+func TestMinerSubmitPoStNextDoneChanges(t *testing.T) {


I made a new test instead of a new t.Run subfunction to try to prevent further complicating the TestMinerSubmitPoSt setup. This seemed worth it to me but I'd appreciate feedback on best practices for actor tests if anything about this seems ugly.

ZenGround0 · 2019-06-26T13:37:55Z

types/intset.go

+var intSetAtlasEntry = atlas.BuildEntry(IntSet{}).Transform().
+	TransformMarshal(atlas.MakeMarshalTransformFunc(
+		func(is IntSet) ([]byte, error) {
+			// TODO #2889: we should be using RLE+ to serialize intsets when it lands,


cc @ingar when taking #2889 these calls should be updated with is.ToRLE() and IntSetFromRLE() (or whatever interface you go with)

ZenGround0 · 2019-06-26T16:11:49Z

actor/builtin/miner/sector_set.go

+// SectorSet is a collection of sector commitments indexed by their integer
+// sectorID.  Due to a bug in refmt, the sector id-keys need to be stringified.
+// See also: https://github.com/polydawn/refmt/issues/35.
+type SectorSet map[string]types.Commitments


This type decouples the SectorSet's implementation from its interface a bit better than before which helps with updating the miner actor's commitments and proving sets. This type doesn't capture the interface we can expect in the near future which will need to pass around the miner's storage and return cids.

laser

Approved, with a question about BitField versus IntSet.

laser · 2019-06-28T11:16:18Z

actor/builtin/miner/miner.go

@@ -719,7 +726,7 @@ func (ma *Actor) GetPower(ctx exec.VMContext) (*types.BytesAmount, uint8, error)

 // SubmitPoSt is used to submit a coalesced PoST to the chain to convince the chain
 // that you have been actually storing the files you claim to be.
-func (ma *Actor) SubmitPoSt(ctx exec.VMContext, poStProofs []types.PoStProof) (uint8, error) {
+func (ma *Actor) SubmitPoSt(ctx exec.VMContext, poStProofs []types.PoStProof, done types.IntSet) (uint8, error) {


QUESTION: I observe that the spec uses the BitField type (an RLE+ encoded bit set) where you've used an IntSet (a slice of u64). Is this a temporary divergence from the spec (perhaps because we don't yet have a BitField type with corresponding set operations)? If not, would you upstream this change to the spec?

This is not temporary and I have considered upstreaming changes to the spec. The issue is that the spec doesn't really distinguish between encodings of VM data and internal VM datastructures and the Bitfield is just the encoding of an IntSet. I've been told that trying to put specifics of VM internals (like how the IntSet is implemented) into the spec is out of scope because that gets into implementation choices. So there is a fine line I don't have a good feel for. I don't think I am the right person to start introducing this subtle distinction into the spec or that it would even be a welcome addition.

I've been told that trying to put specifics of VM internals (like how the IntSet is implemented) into the spec is out of scope because that gets into implementation choices.

I guess all that really matters is that go-filecoin can send and receive submitPoSt messages whose bytes are conformant with the specification (i.e. go-filecoin knows how to send/receive RLE+ bit sets, in this context). If we choose to transform the decoded bit set into a slice of uint64s (in Go), then that's an implementation detail of go-filecoin that doesn't need upstreaming. Does this overlap with what you're thinking, @ZenGround0?

Yes this lines up. And once the RLE+ PR lands all IntSet parameters will be RLE+ encoded.

anorth · 2019-06-29T08:59:43Z

actor/builtin/miner/miner.go

+
+	// ProvingSet is the set of sectorIDs this miner is currently proving.
+	// It is only updated when a PoSt is submitted.
+	ProvingSet types.IntSet


It would be logical to declare ProvingSet before NextDoneSet.

I think we can clarify the comment:
ProvingSet is the set of sector IDs which the miner has committed and for which it has submitted at least one PoSt and not yet declared done or failed.

We have different ideas about what the proving set entails. The way I read the spec the ProvingSet is the set of sectorIDs the miner has committed and must prove to the network during this proving period. sectorIDs of sectors without any PoSts enter into the proving set all the time after a miner first commits sectors. This is the way this code is written too. I don't know where the "at least one PoSt" requirement is coming from.

sectorIDs of sectors without any PoSts enter into the proving set all the time after a miner first commits sectors.

My reading of the spec is that a sector enters the proving set:

When the sector is committed if the ProvingSet is empty. In this case the proving period is also reset.

At the next SubmitPoSt if proving is already underway (the ProvingSet is non-empty).

This logic means a sector will never be added to a proving set in the middle of a proving period and that proving periods will start as soon as possible. I think it's a bit too much detail to include when commenting the declaration of of the variable. My preference would be to simply remove the "It is only updated when a PoSt is submitted" and leave "ProvingSet is the set of scctorIDs this miner is currently proving", which is at least accurate.

anorth · 2019-06-29T09:01:09Z

actor/builtin/miner/miner.go

-		sectorIDstr := strconv.FormatUint(sectorID, 10)
-
-		_, ok := state.SectorCommitments[sectorIDstr]
+		ok := state.SectorCommitments.Has(sectorID)


"ok" is a bad name for this. How about "has" or "committed"?

anorth · 2019-06-29T09:15:41Z

actor/builtin/miner/miner_test.go

+	res, err = th.CreateAndApplyTestMessage(t, st, vms, minerAddr, 0, firstCommitBlockHeight+1, "commitSector", ancestors, uint64(2), th.MakeCommitment(), th.MakeCommitment(), th.MakeCommitment(), th.MakeRandomBytes(types.TwoPoRepProofPartitions.ProofLen()))
+	require.NoError(t, err)
+	require.NoError(t, res.ExecutionError)
+	require.Equal(t, uint8(0), res.Receipt.ExitCode)


Check ProvingSet is empty

anorth · 2019-06-29T09:20:53Z

actor/builtin/miner/sector_set.go

+	for idStr := range ss {
+		id, err := str2ID(idStr)
+		if err != nil {
+			return nil, errors.RevertErrorWrap(err, "corrupt sectorset id")


I would expect this wrapping to be done at the VM boundary: code inside the actor can either succeed or fail, and any failure will revert the transaction but not be a "fault" error. The actor code in general should not depend on these error abstractions.

Am I correctly reading this as a suggestion that we not call errors.NewRevertError or errors.NewFaultError from within actor code? If so this is a big technical debt concern because we do this very commonly in actor code. It is an entrenched pattern and redesigning an alternative to use here is out of scope.

Am I misreading this? If so could you please clarify the concrete change you'd like to see? If not we should file an issue to correct this.

The way the VM is set up, it requires actors to declare errors to be reverts or faults. This makes some sense because this determination can't be made externally, but it would be nice if faults had to be wrapped and any non-fault error was treated as a revert. I think this system was set up for safety (forcing the developer to choose).

!! @ZenGround0 you interpreted correctly but I misunderstood the current convention and am now very alarmed by it.

set up for safety

But it's not! Nothing forces the developer to choose because the methods are declared to return simply error. A quick grep shows at least five calls to methods like errors.{New|Wrap|Errorf} in actor code. I would bet large amounts of money that there are errors returned from code called by actors that are not wrapped.

Also, this is supposed to be a VM, how can anything from inside here be intentionally causing a node to shutdown? If such a case is part of message execution, a node could only crash-loop until the message expired from all pools. That's a horrible DOS vector.

It looks like "faults" are not handled as harshly as they're documented to be, so a node won't shutdown. I've filed #3009 to clear this all up (later).

anorth · 2019-06-29T09:21:20Z

actor/builtin/miner/sector_set.go

+}
+
+// TODO: use uint64 as map keys instead of this abomination, once refmt is fixed.
+// https://github.com/polydawn/refmt/issues/35


Please file a tracking issue in go-filecoin too

anorth · 2019-06-29T09:23:17Z

Sorry for the delay

acruikshank

This looks solid. I like the choices you made while structuring the code.

- Add done parameter to submitPoSt - Add NextDoneSet to miner actor State - Update NextDoneSet during submitPoSt - Add SectorSet abstraction to miner actor - Write Drop and IDs methods for SectorSet - Update ProvingSet during submitPoSt

ZenGround0 force-pushed the feat/2948 branch from 9461837 to 2621f26 Compare June 26, 2019 13:32

ZenGround0 commented Jun 26, 2019

View reviewed changes

ZenGround0 force-pushed the feat/2948 branch from 606b1b0 to 87e57d9 Compare June 26, 2019 16:04

ZenGround0 commented Jun 26, 2019

View reviewed changes

ZenGround0 force-pushed the feat/2948 branch 2 times, most recently from 1c0c849 to de9c777 Compare June 26, 2019 18:04

ZenGround0 marked this pull request as ready for review June 26, 2019 19:05

ZenGround0 changed the title ~~Adding done parameter to submitPoSt miner actor method~~ Add NextDoneSet and ProvingSet abstractions to miner actor Jun 26, 2019

ZenGround0 requested review from laser and acruikshank June 26, 2019 19:06

This was referenced Jun 26, 2019

Add fault and recovered sets to SubmitPost #2889

Closed

Update power based on previous proving set #3002

Merged

Only use commitments from current proving set as arguments to PoSt verify #3003

Closed

laser approved these changes Jun 28, 2019

View reviewed changes

anorth approved these changes Jun 29, 2019

View reviewed changes

acruikshank approved these changes Jun 29, 2019

View reviewed changes

ZenGround0 force-pushed the feat/2948 branch from 54a66a5 to c3a3046 Compare July 1, 2019 15:35

Add NextDoneSet and ProvingSet to miner actor

0a8021c

- Add done parameter to submitPoSt - Add NextDoneSet to miner actor State - Update NextDoneSet during submitPoSt - Add SectorSet abstraction to miner actor - Write Drop and IDs methods for SectorSet - Update ProvingSet during submitPoSt

ZenGround0 force-pushed the feat/2948 branch from c3a3046 to 0a8021c Compare July 1, 2019 15:51

ZenGround0 merged commit 451562c into master Jul 1, 2019

anorth mentioned this pull request Jul 2, 2019

Revise actor error handling for safety and robustness #3009

Closed

ZenGround0 deleted the feat/2948 branch October 29, 2019 14:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NextDoneSet and ProvingSet abstractions to miner actor #2999

Add NextDoneSet and ProvingSet abstractions to miner actor #2999

ZenGround0 commented Jun 25, 2019 •

edited

Loading

codecov-io commented Jun 25, 2019 •

edited

Loading

ZenGround0 Jun 26, 2019

ZenGround0 Jun 26, 2019

ZenGround0 Jun 26, 2019

ZenGround0 Jun 26, 2019

laser left a comment

laser Jun 28, 2019

ZenGround0 Jun 28, 2019

laser Jun 28, 2019

ZenGround0 Jun 28, 2019

anorth Jun 29, 2019

ZenGround0 Jul 1, 2019

acruikshank Jul 1, 2019

anorth Jun 29, 2019

anorth Jun 29, 2019

anorth Jun 29, 2019

ZenGround0 Jul 1, 2019 •

edited

Loading

acruikshank Jul 1, 2019

anorth Jul 2, 2019

anorth Jul 2, 2019

anorth Jun 29, 2019

ZenGround0 Jul 1, 2019

anorth commented Jun 29, 2019

acruikshank left a comment

Add NextDoneSet and ProvingSet abstractions to miner actor #2999

Add NextDoneSet and ProvingSet abstractions to miner actor #2999

Conversation

ZenGround0 commented Jun 25, 2019 • edited Loading

codecov-io commented Jun 25, 2019 • edited Loading

Codecov Report

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

laser left a 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

ZenGround0 Jul 1, 2019 • 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

anorth commented Jun 29, 2019

acruikshank left a comment

Choose a reason for hiding this comment

ZenGround0 commented Jun 25, 2019 •

edited

Loading

codecov-io commented Jun 25, 2019 •

edited

Loading

ZenGround0 Jul 1, 2019 •

edited

Loading