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.

Sign up for GitHub

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

move BlockTime to plumbing add BlockClock interface #2894

Closed
wants to merge 2 commits into from
Closed

move BlockTime to plumbing add BlockClock interface #2894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78ecec7
move BlockTime to plumbing add BlockClock interface
frrist Jun 5, 2019
bc08932
move DefaultBlockTime to plumbin/clock
frrist Jun 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions commands/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ import (
"github.com/pkg/errors"

"github.com/filecoin-project/go-filecoin/config"
"github.com/filecoin-project/go-filecoin/mining"
"github.com/filecoin-project/go-filecoin/node"
"github.com/filecoin-project/go-filecoin/paths"
"github.com/filecoin-project/go-filecoin/plumbing/clock"
"github.com/filecoin-project/go-filecoin/repo"
)

Expand All @@ -38,7 +38,7 @@ var daemonCmd = &cmds.Command{
cmdkit.BoolOption(OfflineMode, "start the node without networking"),
cmdkit.BoolOption(ELStdout),
cmdkit.BoolOption(IsRelay, "advertise and allow filecoin network traffic to be relayed through this node"),
cmdkit.StringOption(BlockTime, "time a node waits before trying to mine the next block").WithDefault(mining.DefaultBlockTime.String()),
cmdkit.StringOption(BlockTime, "time a node waits before trying to mine the next block").WithDefault(clock.DefaultBlockTime.String()),
},
Run: func(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment) error {
return daemonRun(req, re, env)
Expand Down Expand Up @@ -95,7 +95,7 @@ func daemonRun(req *cmds.Request, re cmds.ResponseEmitter, env cmds.Environment)
if err != nil {
return errors.Wrap(err, "Bad block time passed")
}
opts = append(opts, node.BlockTime(blockTime))
opts = append(opts, node.BlockClock(clock.NewConfiguredBlockClock(blockTime)))

fcn, err := node.New(req.Context, opts...)
if err != nil {
Expand Down
11 changes: 5 additions & 6 deletions consensus/block_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"time"

"github.com/filecoin-project/go-filecoin/plumbing/clock"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the first time plumbing has made its way into the consensus package. I am interested in the long term implications of this change. Does it mean consensus can't be part of plumbing itself? Does it mean consensus will have a stronger pull towards being put on plumbing?

It seems like there is lack of intentional design around the interactions between "core components" and the plumbing injection things. These are just general observations, no action required in this PR.

"github.com/filecoin-project/go-filecoin/types"
)

Expand Down Expand Up @@ -49,16 +50,14 @@ func (ebc *DefaultBlockValidationClock) EpochSeconds() uint64 {

// DefaultBlockValidator implements the BlockValidator interface.
type DefaultBlockValidator struct {
clock BlockValidationClock
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we can replace the BlockValidationClock with the BlockClock interface once it matures a bit more (in the coming block validation PR)

blockTime time.Duration
clock clock.BlockClock
}

// NewDefaultBlockValidator returns a new DefaultBlockValidator. It uses `blkTime`
// to validate blocks and uses the DefaultBlockValidationClock.
func NewDefaultBlockValidator(blkTime time.Duration) *DefaultBlockValidator {
func NewDefaultBlockValidator(c clock.BlockClock) *DefaultBlockValidator {
return &DefaultBlockValidator{
clock: NewDefaultBlockValidationClock(),
blockTime: blkTime,
clock: c,
}
}

Expand All @@ -83,5 +82,5 @@ func (dv *DefaultBlockValidator) ValidateSyntax(ctx context.Context, blk *types.
// BlockTime returns the block time the DefaultBlockValidator uses to validate
/// blocks against.
func (dv *DefaultBlockValidator) BlockTime() time.Duration {
return dv.blockTime
return dv.clock.BlockTime()
}
32 changes: 16 additions & 16 deletions mining/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ import (

var log = logging.Logger("mining")

// DefaultBlockTime is the estimated proving period time.
// We define this so that we can fake mining in the current incomplete system.
const DefaultBlockTime = 30 * time.Second

// Output is the result of a single mining run. It has either a new
// block or an error, mimicing the golang (retVal, error) pattern.
// If a mining run's context is canceled there is no output.
Expand Down Expand Up @@ -72,6 +68,10 @@ type MessageApplier interface {
ApplyMessagesAndPayRewards(ctx context.Context, st state.Tree, vms vm.StorageMap, messages []*types.SignedMessage, minerOwnerAddr address.Address, bh *types.BlockHeight, ancestors []types.TipSet) (consensus.ApplyMessagesResponse, error)
}

type workerPorcelainAPI interface {
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be extended to replace other fields on the DefaultWorker, however that may be outside the scope of the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it could. Furthermore I think the mining worker should eventually be code in the block miner protocol's miner.go and most of the dependencies it shares with the node should be accessed through porcelain.

I also agree all of this is out of scope of your PR.

BlockTime(ctx context.Context) time.Duration
}

// DefaultWorker runs a mining job.
type DefaultWorker struct {
createPoSTFunc DoSomeWorkFunc
Expand All @@ -91,7 +91,7 @@ type DefaultWorker struct {
powerTable consensus.PowerTableView
blockstore blockstore.Blockstore
cstore *hamt.CborIpldStore
blockTime time.Duration
clock workerPorcelainAPI
}

// NewDefaultWorker instantiates a new Worker.
Expand All @@ -107,7 +107,7 @@ func NewDefaultWorker(messageSource MessageSource,
minerOwner address.Address,
minerPubKey []byte,
workerSigner consensus.TicketSigner,
bt time.Duration) *DefaultWorker {
clock workerPorcelainAPI) *DefaultWorker {

w := NewDefaultWorkerWithDeps(messageSource,
getStateTree,
Expand All @@ -121,7 +121,7 @@ func NewDefaultWorker(messageSource MessageSource,
minerOwner,
minerPubKey,
workerSigner,
bt,
clock,
func() {})

// TODO: create real PoST.
Expand All @@ -144,22 +144,22 @@ func NewDefaultWorkerWithDeps(messageSource MessageSource,
minerOwner address.Address,
minerPubKey []byte,
workerSigner consensus.TicketSigner,
bt time.Duration,
clock workerPorcelainAPI,
createPoST DoSomeWorkFunc) *DefaultWorker {
return &DefaultWorker{
blockstore: bs,
clock: clock,
createPoSTFunc: createPoST,
cstore: cst,
getAncestors: getAncestors,
getStateTree: getStateTree,
getWeight: getWeight,
getAncestors: getAncestors,
messageSource: messageSource,
processor: processor,
powerTable: powerTable,
blockstore: bs,
cstore: cst,
createPoSTFunc: createPoST,
minerAddr: miner,
minerOwnerAddr: minerOwner,
minerPubKey: minerPubKey,
blockTime: bt,
powerTable: powerTable,
processor: processor,
workerSigner: workerSigner,
}
}
Expand Down Expand Up @@ -259,5 +259,5 @@ func createProof(challengeSeed types.PoStChallengeSeed, createPoST DoSomeWorkFun
// fakeCreatePoST is the default implementation of DoSomeWorkFunc.
// It simply sleeps for the blockTime.
func (w *DefaultWorker) fakeCreatePoST() {
time.Sleep(w.blockTime)
time.Sleep(w.clock.BlockTime(context.Background()))
}
16 changes: 8 additions & 8 deletions mining/worker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func Test_Mine(t *testing.T) {
outCh := make(chan mining.Output)
worker := mining.NewDefaultWorkerWithDeps(
pool, getStateTree, getWeightTest, getAncestors, th.NewTestProcessor(), mining.NewTestPowerTableView(1),
bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.BlockTimeTest,
bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.NewTestBlockClock(),
CreatePoSTFunc)

go worker.Mine(ctx, tipSet, 0, outCh)
Expand All @@ -70,7 +70,7 @@ func Test_Mine(t *testing.T) {
doSomeWorkCalled = false
ctx, cancel := context.WithCancel(context.Background())
worker := mining.NewDefaultWorkerWithDeps(pool, makeExplodingGetStateTree(st), getWeightTest, getAncestors, th.NewTestProcessor(),
mining.NewTestPowerTableView(1), bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.BlockTimeTest, CreatePoSTFunc)
mining.NewTestPowerTableView(1), bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.NewTestBlockClock(), CreatePoSTFunc)
outCh := make(chan mining.Output)
doSomeWorkCalled = false
go worker.Mine(ctx, tipSet, 0, outCh)
Expand All @@ -85,7 +85,7 @@ func Test_Mine(t *testing.T) {
doSomeWorkCalled = false
ctx, cancel := context.WithCancel(context.Background())
worker := mining.NewDefaultWorkerWithDeps(pool, getStateTree, getWeightTest, getAncestors, th.NewTestProcessor(),
mining.NewTestPowerTableView(1), bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.BlockTimeTest, CreatePoSTFunc)
mining.NewTestPowerTableView(1), bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.NewTestBlockClock(), CreatePoSTFunc)
input := types.TipSet{}
outCh := make(chan mining.Output)
go worker.Mine(ctx, input, 0, outCh)
Expand Down Expand Up @@ -212,7 +212,7 @@ func TestGenerateMultiBlockTipSet(t *testing.T) {
minerOwnerAddr := addrs[3]

worker := mining.NewDefaultWorkerWithDeps(pool, getStateTree, getWeightTest, getAncestors, th.NewTestProcessor(),
&th.TestView{}, bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.BlockTimeTest, CreatePoSTFunc)
&th.TestView{}, bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.NewTestBlockClock(), CreatePoSTFunc)

parents := types.NewSortedCidSet(newCid())
stateRoot := newCid()
Expand Down Expand Up @@ -255,7 +255,7 @@ func TestGeneratePoolBlockResults(t *testing.T) {
return nil, nil
}
worker := mining.NewDefaultWorkerWithDeps(pool, getStateTree, getWeightTest, getAncestors, consensus.NewDefaultProcessor(),
&th.TestView{}, bs, cst, addrs[4], addrs[3], blockSignerAddr, mockSigner, th.BlockTimeTest, CreatePoSTFunc)
&th.TestView{}, bs, cst, addrs[4], addrs[3], blockSignerAddr, mockSigner, th.NewTestBlockClock(), CreatePoSTFunc)

// addr3 doesn't correspond to an extant account, so this will trigger errAccountNotFound -- a temporary failure.
msg1 := types.NewMessage(addrs[2], addrs[0], 0, nil, "", nil)
Expand Down Expand Up @@ -337,7 +337,7 @@ func TestGenerateSetsBasicFields(t *testing.T) {
minerAddr := addrs[4]
minerOwnerAddr := addrs[3]
worker := mining.NewDefaultWorkerWithDeps(pool, getStateTree, getWeightTest, getAncestors, consensus.NewDefaultProcessor(),
&th.TestView{}, bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.BlockTimeTest, CreatePoSTFunc)
&th.TestView{}, bs, cst, minerAddr, minerOwnerAddr, blockSignerAddr, mockSigner, th.NewTestBlockClock(), CreatePoSTFunc)

h := types.Uint64(100)
w := types.Uint64(1000)
Expand Down Expand Up @@ -379,7 +379,7 @@ func TestGenerateWithoutMessages(t *testing.T) {
return nil, nil
}
worker := mining.NewDefaultWorkerWithDeps(pool, getStateTree, getWeightTest, getAncestors, consensus.NewDefaultProcessor(),
&th.TestView{}, bs, cst, addrs[4], addrs[3], blockSignerAddr, mockSigner, th.BlockTimeTest, CreatePoSTFunc)
&th.TestView{}, bs, cst, addrs[4], addrs[3], blockSignerAddr, mockSigner, th.NewTestBlockClock(), CreatePoSTFunc)

assert.Len(t, pool.Pending(), 0)
baseBlock := types.Block{
Expand Down Expand Up @@ -413,7 +413,7 @@ func TestGenerateError(t *testing.T) {
}
worker := mining.NewDefaultWorkerWithDeps(pool, makeExplodingGetStateTree(st), getWeightTest, getAncestors,
consensus.NewDefaultProcessor(),
&th.TestView{}, bs, cst, addrs[4], addrs[3], blockSignerAddr, mockSigner, th.BlockTimeTest, CreatePoSTFunc)
&th.TestView{}, bs, cst, addrs[4], addrs[3], blockSignerAddr, mockSigner, th.NewTestBlockClock(), CreatePoSTFunc)

// This is actually okay and should result in a receipt
msg := types.NewMessage(addrs[0], addrs[1], 0, nil, "", nil)
Expand Down
42 changes: 19 additions & 23 deletions node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import (
"github.com/filecoin-project/go-filecoin/paths"
"github.com/filecoin-project/go-filecoin/plumbing"
"github.com/filecoin-project/go-filecoin/plumbing/cfg"
"github.com/filecoin-project/go-filecoin/plumbing/clock"
"github.com/filecoin-project/go-filecoin/plumbing/cst"
"github.com/filecoin-project/go-filecoin/plumbing/dag"
"github.com/filecoin-project/go-filecoin/plumbing/msg"
Expand Down Expand Up @@ -126,7 +127,6 @@ type Node struct {

// Mining stuff.
AddNewlyMinedBlock newBlockFunc
blockTime time.Duration
cancelMining context.CancelFunc
MiningWorker mining.Worker
MiningScheduler mining.Scheduler
Expand Down Expand Up @@ -185,7 +185,7 @@ type Node struct {

// Config is a helper to aid in the construction of a filecoin node.
type Config struct {
BlockTime time.Duration
Clock clock.BlockClock
Libp2pOpts []libp2p.Option
OfflineMode bool
Verifier proofs.Verifier
Expand Down Expand Up @@ -213,10 +213,10 @@ func IsRelay() ConfigOpt {
}
}

// BlockTime sets the blockTime.
func BlockTime(blockTime time.Duration) ConfigOpt {
// BlockClock sets the clock.
func BlockClock(clk clock.BlockClock) ConfigOpt {
return func(c *Config) error {
c.BlockTime = blockTime
c.Clock = clk
return nil
}
}
Expand Down Expand Up @@ -378,8 +378,14 @@ func (nc *Config) Build(ctx context.Context) (*Node, error) {
// set up pinger
pingService := ping.NewPingService(peerHost)

// DO NOT MERGE this code smells like poo, this smell exists elsewhere,
// should have defaults set on it _before_ we get here?
if nc.Clock == nil {
Copy link
Member Author

@frrist frrist Jun 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not a fan of this pattern, however since --block-time is a command line option I am unsure if there is a way around it. If there are no suggestions during review I will remove this smelly comment and leave this as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about passing an integer parsed from the CLI (or the default) on the node config and passing that to a clock constructor during the porcelain.New call below?

Copy link
Contributor

Choose a reason for hiding this comment

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

You leave the concrete implementation of BlockClock configurable here for testing? Like for a FAST test or something? If not I kind of agree with @ZenGround0.. you can just instantiate the BlockClock given the block-time option.

nc.Clock = clock.NewDefaultBlockClock()
}

// setup block validation
blkValid := consensus.NewDefaultBlockValidator(nc.BlockTime)
blkValid := consensus.NewDefaultBlockValidator(nc.Clock)

// set up bitswap
nwork := bsnet.NewFromIpfsHost(peerHost, router)
Expand Down Expand Up @@ -440,6 +446,7 @@ func (nc *Config) Build(ctx context.Context) (*Node, error) {
PorcelainAPI := porcelain.New(plumbing.New(&plumbing.APIDeps{
Bitswap: bswap,
Chain: chainState,
Clock: nc.Clock,
Config: cfg.NewConfig(nc.Repo),
DAG: dag.NewDAG(merkledag.NewDAGService(bservice)),
Deals: strgdls.New(nc.Repo.DealsDatastore()),
Expand Down Expand Up @@ -470,7 +477,6 @@ func (nc *Config) Build(ctx context.Context) (*Node, error) {
PeerHost: peerHost,
Repo: nc.Repo,
Wallet: fcWallet,
blockTime: nc.BlockTime,
Router: router,
}

Expand Down Expand Up @@ -753,19 +759,9 @@ func (node *Node) miningAddress() (address.Address, error) {
// Note this is mocked behavior, in production this time is determined by how
// long it takes to generate PoSTs.
func (node *Node) MiningTimes() (time.Duration, time.Duration) {
mineDelay := node.GetBlockTime() / mining.MineDelayConversionFactor
return node.GetBlockTime(), mineDelay
}

// GetBlockTime returns the current block time.
// TODO this should be surfaced somewhere in the plumbing API.
func (node *Node) GetBlockTime() time.Duration {
return node.blockTime
}

// SetBlockTime sets the block time.
func (node *Node) SetBlockTime(blockTime time.Duration) {
node.blockTime = blockTime
blockTime := node.PorcelainAPI.BlockTime(context.Background())
mineDelay := blockTime / mining.MineDelayConversionFactor
return blockTime, mineDelay
}

// StartMining causes the node to start feeding blocks to the mining worker and initializes
Expand Down Expand Up @@ -1026,11 +1022,11 @@ func (node *Node) setupProtocols() error {
node.BlockMiningAPI = &blockMiningAPI

// set up retrieval client and api
retapi := retrieval.NewAPI(retrieval.NewClient(node.host, node.blockTime, node.PorcelainAPI))
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: node.host should be accessed on porcelain too in a perfect world. (just observation not relevant to this PR)

retapi := retrieval.NewAPI(retrieval.NewClient(node.host, node.PorcelainAPI))
node.RetrievalAPI = &retapi

// set up storage client and api
smc := storage.NewClient(node.blockTime, node.host, node.PorcelainAPI)
smc := storage.NewClient(node.host, node.PorcelainAPI)
smcAPI := storage.NewAPI(smc)
node.StorageAPI = &smcAPI
return nil
Expand Down Expand Up @@ -1059,7 +1055,7 @@ func (node *Node) CreateMiningWorker(ctx context.Context) (mining.Worker, error)
return mining.NewDefaultWorker(
node.Inbox.Pool(), node.getStateTree, node.getWeight, node.getAncestors, processor, node.PowerTable,
node.Blockstore, node.CborStore(), minerAddr, minerOwnerAddr, minerPubKey,
node.Wallet, node.blockTime), nil
node.Wallet, node.PorcelainAPI), nil
}

// getStateFromKey returns the state tree based on tipset fetched with provided key tsKey
Expand Down
3 changes: 2 additions & 1 deletion node/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/filecoin-project/go-filecoin/consensus"
"github.com/filecoin-project/go-filecoin/mining"
"github.com/filecoin-project/go-filecoin/node"
"github.com/filecoin-project/go-filecoin/plumbing/clock"
"github.com/filecoin-project/go-filecoin/proofs"
"github.com/filecoin-project/go-filecoin/protocol/storage"
"github.com/filecoin-project/go-filecoin/repo"
Expand Down Expand Up @@ -198,7 +199,7 @@ func TestNodeConfig(t *testing.T) {
configOptions := []node.ConfigOpt{
repoConfig(),
node.VerifierConfigOption(verifier),
node.BlockTime(time.Duration(configBlockTime)),
node.BlockClock(clock.NewConfiguredBlockClock(time.Duration(configBlockTime))),
}

initOpts := []node.InitOpt{node.AutoSealIntervalSecondsOpt(120)}
Expand Down
Loading