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

State Manager and initial miner retrieval methods #224

Merged
merged 11 commits into from
Feb 21, 2020

Conversation

dutterbutter
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • Added StateManager and retrieval methods for state objects
  • Added StorageMinerActor, MinerInfo and StorageMinerActorState structures used to retrieve data about miners
  • Included the retrieval of slashed_at and miner_sector_size to block validation method
  • Updated comments

Reference issue to close (if applicable)

It does not close any particular task although it pushes #78 forward.

Other information and links

Block validation will remain incomplete until we implement missing VM components required to retrieve storage power, get a raw miner worker address and can validate miner.

@dutterbutter dutterbutter changed the title Dustin/power actor State Manager and initial miner retrieval methods Feb 12, 2020
blockchain/sync_manager/src/sync.rs Outdated Show resolved Hide resolved
vm/state_manager/src/lib.rs Outdated Show resolved Hide resolved
vm/state_manager/src/lib.rs Outdated Show resolved Hide resolved
vm/state_manager/src/lib.rs Outdated Show resolved Hide resolved
vm/state_manager/src/lib.rs Outdated Show resolved Hide resolved
vm/state_manager/src/errors.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner_actor.rs Show resolved Hide resolved
vm/actor/src/builtin/miner_actor.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/errors.rs Outdated Show resolved Hide resolved
blockchain/sync_manager/src/sync.rs Show resolved Hide resolved
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

few small nits and qs

vm/actor/src/builtin/miner_actor.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner_actor.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner_actor.rs Outdated Show resolved Hide resolved
vm/actor/src/code.rs Outdated Show resolved Hide resolved
vm/actor/src/lib.rs Outdated Show resolved Hide resolved
blockchain/sync_manager/src/sync.rs Show resolved Hide resolved
blockchain/sync_manager/src/errors.rs Show resolved Hide resolved
@@ -233,6 +239,12 @@ impl<'a> Syncer<'a> {
// see https://github.com/filecoin-project/lotus/blob/master/chain/sync.go#L611
header.check_block_signature(header.miner_address())?;

// TODO: incomplete, still need to retrieve power in order to ensure ticket is the winner
let _slash = self.state_manager.miner_slashed(header.miner_address())?;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we check this? Can't correlate with spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/filecoin-project/specs/blob/6ab401c0b92efb6420c6e198ec387cf56dc86057/validation.md

Under Semantical Validation: must be from a valid miner, i.e. has not been slashed

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't appear in the current spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The spec uses the term 'plausible miner' which to me is the equivalent to 'valid miner' the term given in the link I posted above which uses slashing as an example of what makes a 'valid miner', and in lotus, it's being used for their block validation. I am happy to remove but I want to make sure I provide relevant information as to why I included it.

vm/state_manager/src/errors.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/errors.rs Outdated Show resolved Hide resolved
vm/state_manager/src/errors.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
vm/actor/src/code.rs Outdated Show resolved Hide resolved
vm/actor/src/code.rs Outdated Show resolved Hide resolved
vm/actor/src/builtin/miner_actor.rs Show resolved Hide resolved
@dutterbutter dutterbutter merged commit d87704f into master Feb 21, 2020
@dutterbutter dutterbutter deleted the dustin/power-actor branch February 21, 2020 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants