move BlockTime to plumbing add BlockClock interface #2894

frrist · 2019-06-05T17:40:34Z

What

This PR move BlockTime to a method on the plumbing api and adds an interface called BlockClock. In coming PR's this interface can be extended to return the current time (time.Now()) so that mining may timestamp blocks. This makes progress towards implementing a soft-block dealy.

frrist · 2019-06-05T17:44:10Z

consensus/block_validation.go

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

 // DefaultBlockValidator implements the BlockValidator interface.
 type DefaultBlockValidator struct {
-	clock     BlockValidationClock


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

frrist · 2019-06-05T17:45:26Z

mining/worker.go

@@ -72,6 +72,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 {


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

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.

frrist · 2019-06-05T17:47:16Z

node/node.go

@@ -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 {


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.

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?

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.

frrist · 2019-06-05T17:48:27Z

plumbing/clock/clock.go

+)
+
+// BlockClock defines an interface for fetching the block time.
+type BlockClock interface {


This is the interface we can extend, adding a method like EpochSeconds. Miners will then call this method when adding a timestamp to the block.

codecov-io · 2019-06-05T17:56:35Z

Codecov Report

Merging #2894 into master will decrease coverage by <1%.
The diff coverage is 75%.

@@          Coverage Diff           @@
##           master   #2894   +/-   ##
======================================
- Coverage      43%     43%   -1%     
======================================
  Files         208     208           
  Lines       12409   12404    -5     
======================================
- Hits         5453    5380   -73     
- Misses       6135    6216   +81     
+ Partials      821     808   -13

Impacted Files	Coverage Δ
protocol/storage/miner.go	`23% <ø> (ø)`	⬆️
commands/daemon.go	`0% <0%> (ø)`	⬆️
consensus/block_validation.go	`0% <0%> (ø)`	⬆️
node/node.go	`58% <100%> (-3%)`	⬇️
protocol/storage/client.go	`48% <100%> (-1%)`	⬇️
protocol/retrieval/client.go	`23% <100%> (ø)`	⬆️
mining/worker.go	`54% <70%> (ø)`	⬆️
proofs/sectorbuilder/testing/builder.go	`0% <0%> (-88%)`	⬇️
proofs/sectorbuilder/testing/harness.go	`0% <0%> (-58%)`	⬇️

ZenGround0 · 2019-06-06T19:29:28Z

consensus/block_validation.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"time"

+	"github.com/filecoin-project/go-filecoin/plumbing/clock"


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.

ZenGround0 · 2019-06-06T19:39:02Z

mining/worker.go

@@ -72,6 +72,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 {


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.

ZenGround0 · 2019-06-06T19:47:30Z

node/node.go

@@ -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 {


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?

ZenGround0 · 2019-06-06T19:48:07Z

node/node.go

@@ -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))


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

anorth

Sorry to do this, but this is not an appropriate use of "clock" as a metaphor.

This PR puts an accessor function for the block period on the plumbing API, and that's great. I don't immediately see how a function that always returns the same value is better than a constant here, but no real objection. On the surface it seems like a decent pattern for the plumbing to expose a bunch of configuration values, of which this is the first of many.

I object to calling this a clock. A method that returns the same value every time you call it is just what a clock isn't. This returns a Duration while a clock will return something like a Time. This is just a configuration value getter. We should not add actual time-related functions to the interface, thereby coupling the static configuration with an abstraction over time.

I think nomenclature confuses things a bit. We should more precisely talk about the block interval or duration, not block time.

I'm fine with the shape of this change, but the words "time" and "clock" should be struck from it.

frrist · 2019-06-11T18:11:45Z

I don't immediately see how a function that always returns the same value is better than a constant here, but no real objection.

Initially BlockTime had a setter and a getter. Since the setter was never referenced anywhere I opted to remove it rather than add it to plumbing. In the event that we want a setter for this value, the code change to add it will be smaller if this is left as a getter.

Moving this to plumbing also allows the BlockTime value to be referenced in the consensus/ package. Previously there existed a circular dep between mining/ and consensus/ that prevented this.

I object to calling this a clock. A method that returns the same value every time you call it is just what a clock isn't. This returns a Duration while a clock will return something like a Time. This is just a configuration value getter.

I agree that naming this thing *Clock is a bit confusing at first. In a follow on PR I add a method EpochSeconds to this interface, it returns time.Now() and also has a mock for tests. Together, both of these methods are used by the BlockValidator to validate blocks.

I can't say I am onboard with striking the term "time" from this completely as "BlockTime" is a fairly well defined term in the crypto world. I would be willing to make the argument that we can think about BlockTime like we think about time.Second. An in this sense, I believe "Clock" is an acceptable metaphor here. I also believe that on its own this PR doesn't make a ton of sense, but when viewed in conjunction with #2897 and #2899 the metaphore becomes clearer.

anorth · 2019-06-11T20:51:17Z

Depending on existing structure, it may be better to have the block validator take the duration configuration at construction, but the current time as a parameter to the Validate() method (if the caller needs a clock anyway, or it's easier to plumb there). Or, take both the configuration and clock at construction. Either is fine.

frrist · 2019-06-12T16:50:21Z

Closing in favor of #2914

frrist self-assigned this Jun 5, 2019

frrist requested review from ZenGround0 and anorth June 5, 2019 17:43

frrist commented Jun 5, 2019

View reviewed changes

This was referenced Jun 5, 2019

add timestamp field to block struct #2895

Closed

Feat/block delay #2890

Closed

frrist force-pushed the feat/miner-worker-clock branch from ac3fc37 to 583f6be Compare June 6, 2019 14:28

move BlockTime to plumbing add BlockClock interface

78ecec7

frrist force-pushed the feat/miner-worker-clock branch from 583f6be to 29993c1 Compare June 6, 2019 15:37

move DefaultBlockTime to plumbin/clock

bc08932

frrist force-pushed the feat/miner-worker-clock branch from 29993c1 to bc08932 Compare June 6, 2019 15:47

frrist mentioned this pull request Jun 6, 2019

Feat/timestamp block #2897

Merged

ZenGround0 approved these changes Jun 6, 2019

View reviewed changes

anorth requested changes Jun 11, 2019

View reviewed changes

frrist closed this Jun 12, 2019

zl03jsj deleted the feat/miner-worker-clock branch July 14, 2022 09:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move BlockTime to plumbing add BlockClock interface #2894

move BlockTime to plumbing add BlockClock interface #2894

frrist commented Jun 5, 2019 •

edited

Loading

frrist Jun 5, 2019

frrist Jun 5, 2019

ZenGround0 Jun 6, 2019

frrist Jun 5, 2019 •

edited

Loading

ZenGround0 Jun 6, 2019

ingar Jun 11, 2019

frrist Jun 5, 2019

codecov-io commented Jun 5, 2019 •

edited

Loading

ZenGround0 Jun 6, 2019

ZenGround0 Jun 6, 2019

ZenGround0 Jun 6, 2019

ZenGround0 Jun 6, 2019

ZenGround0 Jun 6, 2019

anorth left a comment

frrist commented Jun 11, 2019 •

edited

Loading

anorth commented Jun 11, 2019

frrist commented Jun 12, 2019

move BlockTime to plumbing add BlockClock interface #2894

move BlockTime to plumbing add BlockClock interface #2894

Conversation

frrist commented Jun 5, 2019 • edited Loading

What

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frrist Jun 5, 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

codecov-io commented Jun 5, 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

Choose a reason for hiding this comment

anorth left a comment

Choose a reason for hiding this comment

frrist commented Jun 11, 2019 • edited Loading

anorth commented Jun 11, 2019

frrist commented Jun 12, 2019

frrist commented Jun 5, 2019 •

edited

Loading

frrist Jun 5, 2019 •

edited

Loading

codecov-io commented Jun 5, 2019 •

edited

Loading

frrist commented Jun 11, 2019 •

edited

Loading