Rearchitecting the Sharding Node, its Lifecycle, and Services #127

rauljordan · 2018-05-22T11:23:09Z

Hi all,

This is a PR referencing the recent issues #126 #122. The idea is to make our sharding implementation have a single entry point that kicks off an independent node responsible for handling services and protocols related to sharding. Specifically, we consider Notary and Proposer to be protocols that can be registered to a sharding node upon startup, alongside shardp2p, and SMC contract bindings. This design is meant to abstract responsibilities into a more modular fashion and make the node be responsible for the lifecycle of each service, which is something each service should not be concerned with.

Design Rationale

TODO: rauljordan

Requirements for Merge

All tests passing
Notary/Proposer services as they used to before but with the new architecture
Allow for sharding node to manage lifecycle of notaries and proposers
A single place for configuration of this sharding node dependent on cli flags

prestonvanloon · 2018-05-22T15:36:08Z

sharding/proposer/service.go

+// Stop the main loop for proposing collations.
+func (p *Proposer) Stop() error {
+	log.Info("Stopping proposer service")
+	// TODO: Propose collations.


prestonvanloon · 2018-05-22T15:36:22Z

sharding/notary/service.go

+// Stop the main loop for notarizing collations.
+func (n *Notary) Stop() error {
+	log.Info("Stopping notary service")
+	// TODO: Propose collations.


Eliminated this

prestonvanloon · 2018-05-22T15:38:23Z

cmd/geth/shardingcmd.go

+		if protocolFlag == "notary" {
+			return notary.NewNotary(n.Context(), n)
+		}
+		return proposer.NewProposer(n.Context(), n)


if (protocolFlag == "proposer") ?

The default behavior would be just an observer right?

Yeah, added a TODO for default behavior assigned to @terenc3t if the flag is not passed in

That's fine, I can open another PR to handle the default behavior

prestonvanloon · 2018-05-22T15:39:15Z

cmd/utils/flags.go

+	}
+	ProtocolFlag = cli.StringFlag{
+		Name:  "protocol",
+		Usage: "notary | proposer",


Could you add more of a description than this?

+1 on more descriptions

prestonvanloon · 2018-05-22T15:39:46Z

cmd/utils/flags.go

+		Usage: "To become a notary in a sharding node, " + new(big.Int).Div(sharding.NotaryDeposit, new(big.Int).Exp(big.NewInt(10), big.NewInt(18), nil)).String() + " ETH will be deposited into SMC",
+	}
+	ProtocolFlag = cli.StringFlag{
+		Name:  "protocol",


Is protocol the correct flag name? What about actor or role?

Actor sounds good to me

terencechain · 2018-05-22T15:40:03Z

cmd/geth/shardingcmd.go

-func notaryClient(ctx *cli.Context) error {
-	c := notary.NewNotary(ctx)
-	return c.Start()
+func shardingClient(ctx *cli.Context) error {


Can we add an overall comment on what shardingClient is?

terencechain · 2018-05-22T15:46:53Z

cmd/geth/shardingcmd.go

+		if protocolFlag == "notary" {
+			return notary.NewNotary(n.Context(), n)
+		}
+		return proposer.NewProposer(n.Context(), n)


That's fine, I can open another PR to handle the default behavior

terencechain · 2018-05-22T15:48:50Z

cmd/utils/flags.go

+	}
+	ProtocolFlag = cli.StringFlag{
+		Name:  "protocol",
+		Usage: "notary | proposer",


+1 on more descriptions

terencechain · 2018-05-22T15:57:19Z

sharding/README.md

 -   Ability to deposit ETH into the SMC through the command line and to be selected as a notary by the local **SMC** in addition to the ability to withdraw the ETH staked
-   A **proposer client** that listens for pending tx’s, creates collations, and submits them to the SMC
+-   A **proposer protocol** that listens for pending tx’s, creates collations, and submits them to the SMC


protocol sounds funny. Should we change it to node or actor? for the rest of the files too

nisdas · 2018-05-22T15:40:53Z

README.md


-## Running a Collation Proposal Client
+## Running a Collation Proposal Node

 ```


Maybe you can put a short para explaining that if they launch the sharding node without the protocol flag, an observer node is launched instead geth sharding --datadir /path/to/your/datadir --password /path/to/your/password.txt --networkid 12345

nisdas · 2018-05-22T15:42:59Z

cmd/geth/shardingcmd.go

-func notaryClient(ctx *cli.Context) error {
-	c := notary.NewNotary(ctx)
-	return c.Start()
+func shardingClient(ctx *cli.Context) error {


shouldn't it be named shardingNode instead now ?

nisdas · 2018-05-22T15:48:12Z

cmd/geth/shardingcmd.go

+		// TODO(terenc3t): handle case when we just want to start an observer node.
+		if protocolFlag == "notary" {
+			return notary.NewNotary(n.Context(), n)
+		}


make this instead if we will be having observer nodes

else if protocolFlag == "proposer" { return notary.NewProposer(n.Context(), n) } return proposer.NewObserver(n.Context(), n)

nisdas · 2018-05-22T15:51:07Z

sharding/README.md

@@ -183,15 +183,15 @@ Our Ruby Release requires users to start a local geth node running a localized,

    geth sharding-notary --deposit --datadir /path/to/your/datadir --password /path/to/your/password.txt --networkid 12345


change this to geth sharding --protocol "notary" . Same thing below for proposer

nisdas · 2018-05-22T16:55:34Z

LGTM

terencechain · 2018-05-22T18:06:48Z

cmd/geth/shardingcmd.go

+func sharding(ctx *cli.Context) error {
+	// configures a sharding-enabled node using the cli's context.
+	shardingNode := node.NewNode(ctx)
+	registerShardingServices(shardingNode)


registerShardingServices returns an error. Do we want to check for if err != nil?

terencechain · 2018-05-22T21:19:45Z

cmd/geth/shardingcmd.go

+func registerShardingServices(n node.Node) error {
+	actorFlag := n.Context().GlobalString(utils.ActorFlag.Name)
+
+	err := n.Register(func(ctx *cli.Context) (sharding.Service, error) {


ctx is not used here

terencechain

Great work @rauljordan! I'm a fan of this architecture. I see Notary interface and Proposer interface are no longer needed, because the specific actor methods are part service within serviceFuncs

terencechain · 2018-05-22T21:37:51Z

sharding/notary/service.go

+}
+
+// NewNotary creates a new notary instance.
+func NewNotary(ctx *cli.Context, node node.Node) (*Notary, error) {


ctx is unused here, same for NewProposer

terencechain

Looks good. Just one last comment then we can merge

terencechain · 2018-05-22T22:18:12Z

sharding/node/node.go

@@ -103,7 +103,7 @@ func (n *shardingNode) Start() error {
 	// Starts every service attached to the sharding node.
 	for _, serviceFunc := range n.serviceFuncs {
 		// Initializes each service by passing in the node's cli context.


// Initializes each service ~~by passing in the node's cli context~~
We are not passing in the cli context anymore

prestonvanloon

I'm not too sure about those .String()s since they were already there. Consider those comments optional.

Thanks

prestonvanloon · 2018-05-22T22:51:54Z

cmd/utils/flags.go

+	}
+	ActorFlag = cli.StringFlag{
+		Name:  "actor",
+		Usage: `use the --actor "notary" or --actor "proposer" to start a notary or proposer service in the sharding node. If omitted, the sharding node registers an Observer service that simply observes the activity in the sharded network`,


You shouldn't need the quotes for a flag value. It's fine but FYI.

prestonvanloon · 2018-05-22T22:53:33Z

sharding/node/node.go

+
+	scryptN, scryptP, keydir, err := config.AccountConfig()
+	if err != nil {
+		panic(err) // TODO(prestonvanloon): handle this.


This method returns an error, just return the error instead of panic. I had a panic somewhere because I needed to refactor it.

prestonvanloon · 2018-05-22T22:55:55Z

sharding/node/node.go

+	ctx          *cli.Context                  // Command line context.
+	smc          *contracts.SMC                // The deployed sharding management contract.
+	rpcClient    *rpc.Client                   // The RPC client connection to the main geth node.
+	lock         sync.RWMutex                  // Mutex lock for concurrency management.


Why RWMutex vs Mutex if you don't have any RLock / readers?

Do you need this mutex at all if only one method is calling lock? Is it being called multiple times?

Im assuming the register function can be called in various scenarios concurrently, and I'd rather take care of potential concurrency problems now. I'll replace with a simple sync.Mutex.

prestonvanloon · 2018-05-22T22:56:51Z

sharding/node/node.go

+}
+
+// Context returns the cli context.
+func (n *shardingNode) Context() *cli.Context {


Do we need to expose this or would it better to provide a helper method to return the actor flag value? Kind of like DepositFlagSet

I'm just wondering if we can reduce the number of references to cli.Context.

This occurs in various place where we only have access to a Node interface, and I believe we are passing in the context to several other functions that will use this cli context to configure certain options.

Maybe we can explore this as an issue after merge?

prestonvanloon · 2018-05-22T22:59:39Z

sharding/node/utils.go

+
+	contract, err := contracts.NewSMC(sharding.ShardingManagerAddress, n.client)
+	if err != nil {
+		// TODO(terenc3t): handle this


Please return the error. What is there to handle here?

return contracts.NewSMC(...)

prestonvanloon · 2018-05-22T23:02:49Z

sharding/node/utils.go

+	// TODO: Separate contract deployment from the sharding node. It would only need to be deployed
+	// once on the mainnet, so this code would not need to ship with the node.
+	if len(b) == 0 {
+		log.Info(fmt.Sprintf("No sharding manager contract found at %s. Deploying new contract.", sharding.ShardingManagerAddress.String()))


Please do not call String()

prestonvanloon · 2018-05-22T23:02:55Z

sharding/node/utils.go

+
+		txOps, err := n.CreateTXOpts(big.NewInt(0))
+		if err != nil {
+			return nil, fmt.Errorf("unable to intiate the transaction: %v", err)


prestonvanloon · 2018-05-22T23:04:56Z

sharding/node/utils.go

+			return nil, fmt.Errorf("unable to deploy sharding manager contract: %v", err)
+		}
+
+		for pending := true; pending; _, pending, err = n.client.TransactionByHash(context.Background(), tx.Hash()) {


No need for the pending conditional to make a while loop

for { if err = n.client.TransactionByHash(context.Background(), tx.Hash()); err != nil { return nil, fmt.Errorf("unable to get transaction by hash: %v", err) } time.Sleep(1 * time.Second) }

Getting: unreachable code right below it

Ok, I misunderstood the code here.

prestonvanloon · 2018-05-22T23:05:38Z

sharding/node/utils.go

+			time.Sleep(1 * time.Second)
+		}
+
+		log.Info(fmt.Sprintf("New contract deployed at %s", addr.String()))


Do you need to call String()? The formatter should call this.

prestonvanloon

Look good to me. I still disagree with the node interface returning access to a cli.Context object for consumers but I’m happy to explore after merge.

prestonvanloon · 2018-05-23T12:26:06Z

sharding/node/utils.go

+			return nil, fmt.Errorf("unable to deploy sharding manager contract: %v", err)
+		}
+
+		for pending := true; pending; _, pending, err = n.client.TransactionByHash(context.Background(), tx.Hash()) {


Ok, I misunderstood the code here.

Rearchitecting the Sharding Node, its Lifecycle, and Services

Rearchitecting the Sharding Node, its Lifecycle, and Services Former-commit-id: f3cda01

Rearchitecting the Sharding Node, its Lifecycle, and Services Former-commit-id: a19a24106107c24b665c2a76240c7841fb270109 [formerly f3cda01] Former-commit-id: 6932c93

rauljordan added 3 commits May 20, 2018 17:47

sharding: revamp sharding client config to single entrypoint

1153e2c

sharding: simplified newclient func

ba9e6dc

sharding: rename client package to node

184bb5c

rauljordan added in progress labels May 22, 2018

rauljordan added this to the Ruby milestone May 22, 2018

rauljordan self-assigned this May 22, 2018

rauljordan added this to To do in Validator Client via automation May 22, 2018

rauljordan added 8 commits May 22, 2018 06:34

sharding: refactor proposers

4e19d2e

sharding: tests pass

93baa1f

sharding: fixed main entry point, linter errors

b0a990e

sharding: fully functional start func in notary service

d6a365c

sharding: travis passes, all entrypoints work, notary protocol working

710e118

sharding: merge changes from #124 in master

6dc09f5

sharding: update documentation to reflect new entry points

e4c8682

sharding: elim unnecessary comment

f7e67ea

prestonvanloon reviewed May 22, 2018

View reviewed changes

rauljordan mentioned this pull request May 22, 2018

Setup a Simple libp2p Echo Server as a Service Attached to Sharding Node #129

Closed

terencechain reviewed May 22, 2018

View reviewed changes

nisdas reviewed May 22, 2018

View reviewed changes

rauljordan added 2 commits May 22, 2018 12:42

sharding: address review comments, add observer package

8193dce

sharding: edit documentation

6093a61

nisdas approved these changes May 22, 2018

View reviewed changes

prestonvanloon removed the in progress label May 22, 2018

terencechain reviewed May 22, 2018

View reviewed changes

sharding: addressed comments, linters pass

af15543

terencechain approved these changes May 22, 2018

View reviewed changes

rauljordan added 2 commits May 22, 2018 16:20

sharding: address terence comment

4763525

sharding: fetch master changes

6d02a86

prestonvanloon reviewed May 22, 2018

View reviewed changes

rauljordan added 2 commits May 22, 2018 18:35

sharding: address comments, elim string references

98606d5

sharding: return err

90fa357

nisdas mentioned this pull request May 23, 2018

Implementation for the Notary to Vote on Collations #131

Merged

3 tasks

prestonvanloon approved these changes May 23, 2018

View reviewed changes

rauljordan merged commit 9db6161 into prysmaticlabs:master May 23, 2018

Validator Client automation moved this from To do to Done May 23, 2018

rauljordan mentioned this pull request May 23, 2018

Reduce References to cli.Context of the Sharding Node in its Services #132

Closed

prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018

Merge pull request #127 from rauljordan/client-revamp

f3cda01

Rearchitecting the Sharding Node, its Lifecycle, and Services

prestonvanloon pushed a commit that referenced this pull request Jul 22, 2018

Merge pull request #127 from rauljordan/client-revamp

6932c93

Rearchitecting the Sharding Node, its Lifecycle, and Services Former-commit-id: f3cda01

		@@ -183,15 +183,15 @@ Our Ruby Release requires users to start a local geth node running a localized,

		geth sharding-notary --deposit --datadir /path/to/your/datadir --password /path/to/your/password.txt --networkid 12345

Rearchitecting the Sharding Node, its Lifecycle, and Services #127

Rearchitecting the Sharding Node, its Lifecycle, and Services #127

Conversation

rauljordan commented May 22, 2018 • edited Loading

Design Rationale

Requirements for Merge

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

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

nisdas commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencechain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prestonvanloon 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

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

prestonvanloon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauljordan commented May 22, 2018 •

edited

Loading