Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplication of the storage market actor interface in storage-market.md #324

Closed
wants to merge 1 commit into from

Conversation

anorth
Copy link
Member

@anorth anorth commented Jun 3, 2019

This exposes that SlashStorageFault is currently missing pseudocode.

Re: #92, #292

actors.md Outdated
@@ -186,16 +189,16 @@ Parameters:
Return: Address

```go
func CreateStorageMiner(worker Address, sectorSize BytesAmount, pid PeerID) Address {
Copy link
Member

Choose a reason for hiding this comment

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

this change is incorrect. please keep it as an address.

Copy link
Member Author

Choose a reason for hiding this comment

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

Roger that. I inferred pubkey from its use at 198, and storage-market.md. Address is consistent with the constructor.

actors.md Outdated
miner.SelfDestruct()
}
```

### SlashStorageFault

> SlashStorageFault penalizes a storage miner for not submitting a PoSt within the required [time window](#TODO-link-to-faulty-submission). This may be called by anyone who detects the faulty behavior. The slashed miner loses all of their staked collateral and all of their power, and as a result, is no longer a candidate leader for extending the chain. See [faults](faults.md).
Copy link
Member

Choose a reason for hiding this comment

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

its worth linking directly to the unreported storage fault slashing section that this is for.

Also, what is 'TODO-link-to-faulty-submission' supposed to link to?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Linkified.
  2. I don't know, I copied this from storage-market.md. I'll remove the TODO if you don't know what it's standing for.

actors.md Outdated

```go
func SlashStorageFault(miner Address) {
// TODO
Copy link
Member

Choose a reason for hiding this comment

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

+1, will file an issue, thanks

Copy link
Member

Choose a reason for hiding this comment

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

ah, right. I thought I had written this already, its a method on the miner itself.

Copy link
Member

Choose a reason for hiding this comment

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

It was initially on the storage market, but all it did was call through to a method on the miner, and didnt actually do anything on the storage market, so it was moved to the miner.

It definitely feels weird to have the consensus slashing here, and the storage slashing there though, so I could be convinced that it should be moved back. But we would then be adding network overhead for a small UX improvement.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, of course, thanks for spotting. I'll remove this.

@anorth anorth force-pushed the anorth/actors branch 2 times, most recently from c6caf91 to 56a8d3a Compare June 4, 2019 04:21

TODO: rephrase the above to make it clear that power is updated only on PoSt submission and when slashed.
Each individual miner reports its power through its on-chain actor. A miner's power (and hence the total power) is updated only upon PoSt submission (when the set of sectors being proved may change) or when a miner is slashed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to note that this submission is dependent on the individual miner, rather than a global operation

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is already clear, as this is talking about a specific miner, nvm

faults.md Outdated Show resolved Hide resolved
@anorth
Copy link
Member Author

anorth commented Jun 12, 2019

@whyrusleeping ready for merge.

@pooja pooja mentioned this pull request Jul 30, 2019
18 tasks
@anorth
Copy link
Member Author

anorth commented Aug 1, 2019

It looks like this has rotted and will need re-doing against the changed actors spec.

@dignifiedquire
Copy link
Contributor

It looks like this has rotted and will need re-doing against the changed actors spec.

sorry about that, please ping me when you have rebased onto master and I'll make sure we get it in

@pooja
Copy link
Contributor

pooja commented Aug 27, 2019

@anorth Could you please rebase onto master? We can merge it in afterwards!

@pooja pooja mentioned this pull request Aug 27, 2019
22 tasks
@anorth
Copy link
Member Author

anorth commented Aug 28, 2019 via email

@pooja pooja added this to structure/refactors in Issue Groupings Sep 10, 2019
@anorth anorth closed this Oct 13, 2019
@nicola nicola deleted the anorth/actors branch November 29, 2019 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Issue Groupings
structure/refactors
Development

Successfully merging this pull request may close these issues.

None yet

4 participants