Startpost rpc #1312

antonlerner · 2019-08-12T15:24:15Z

Closing #760.

…tx id alongside ok message closing #1247

moshababo · 2019-08-13T10:35:17Z

activation/activation.go

@@ -60,6 +61,12 @@ type BytesStore interface {
 	Get(key []byte) ([]byte, error)
 }

+const (
+	Idle       = 1


activation/activation.go

moshababo · 2019-08-13T10:47:56Z

activation/activation.go

@@ -222,6 +232,44 @@ func (b *Builder) buildNipstChallenge(epoch types.EpochId) error {
 	return nil
 }

+func (b *Builder) StartPost(rewardAddress address.Address, logicalDrive string, commitmentSize uint64) error {


logicalDrive is a bit misleading, because it's not merely the drive. What about datadir?

moshababo · 2019-08-13T10:48:59Z

activation/activation.go

@@ -222,6 +232,44 @@ func (b *Builder) buildNipstChallenge(epoch types.EpochId) error {
 	return nil
 }

+func (b *Builder) StartPost(rewardAddress address.Address, logicalDrive string, commitmentSize uint64) error {


Instead of commitmentSize, perhaps space, since it's already what's used.

moshababo · 2019-08-13T12:05:20Z

cmd/node/node.go

@@ -436,6 +434,15 @@ func (app *SpacemeshApp) startServices() {
 		log.Panic("cannot start block producer")
 	}
 	app.poetListener.Start()
+
+	if app.Config.StartMining {


Line 400:

if app.Config.CoinbaseAccount == "" { app.log.Panic("cannot start mining without Coinbase account") }

If StartMining is false then you should not panic.

This wasn't fixed yet.
And after what you did here:

if coinBase.Big().Uint64() == 0 && app.Config.StartMining { app.log.Panic("invalid Coinbase account") }

It should probably get removed entirely.

moshababo · 2019-08-13T12:09:16Z

nipst/nipst.go

@@ -28,6 +28,13 @@ type PostProverClient interface {
 	// the amortized computational complexity can be made arbitrarily small.
 	execute(id []byte, challenge []byte, timeout time.Duration) (proof *types.PostProof, err error)

+	// set the needed params for setting up post commitment in the specified logical drive and with
+	// requested commitment size
+	SetPostParams(logicalDrive string, commitmentSize uint64)


Method doc should start with SetPostParams. Also, you can just call it SetParams.

moshababo · 2019-08-13T12:10:34Z

nipst/nipst.go

+	// requested commitment size
+	SetPostParams(logicalDrive string, commitmentSize uint64)
+
+	//Reset deletes


Minor: space between "//" and "Reset". Dot in the end. More description.

moshababo · 2019-08-13T12:11:55Z

nipst/nipst_test.go

+	drive := "/tmp/anton"
+	unitSize := 2048
+	_, err := nb.InitializePost(drive, uint64(unitSize))
+	defer nb.postProver.Reset()


You should also assert Reset response.

moshababo · 2019-08-13T12:20:28Z

nipst/nipst.go

 	defTimeout := 5 * time.Second // TODO: replace temporary solution

+	nb.postCfg.DataDir = logicalDrive
+	nb.postCfg.SpacePerUnit = commitmentSize
+	nb.postProver.SetPostParams(logicalDrive, commitmentSize)


Having post cfg on both the nipst builder and the post prover looks confusing. See if you can have in one place only.

moshababo · 2019-08-15T09:18:05Z

activation/activation.go

@@ -222,6 +235,53 @@ func (b *Builder) buildNipstChallenge(epoch types.EpochId) error {
 	return nil
 }

+func (b *Builder) StartPost(rewardAddress address.Address, dataDir string, space uint64) error {


You changed logicalDrive to dataDir and commitmentSize to space here but not in many other places. It should all be the same, and if the context is lost, you can do postDatadir & postSpace, or something similar.

There are many more leftovers.

activation/activation.go

moshababo · 2019-08-15T09:39:06Z

activation/activation.go

+	b.log.Info("Starting post, reward address: %x", rewardAddress)
+	b.SetCoinbaseAccount(rewardAddress)
+	b.postInitLock.Lock()
+	if b.initStatus != Idle {


Last time I wrote:

It might be worth to check for InProgress status directly, instead of assuming that's the case if we're not on Idle && IsPostInitialized() returns false. Whatever you think.

Now after IsPostInitialized() removal, you're not covering the Done case.
So this need to be fixed, and also you should set the Done statue value if you're getting "already initialized" err from the initializer. Also, IMO "already initialized" should not be appended to "PoST initialization failed: " string.

Why not to do this now as if b.initStatus == InProgress?

moshababo · 2019-08-15T09:40:53Z

api/grpc_server.go

+	}
+	return &pb.SimpleMessage{Value: "ok"}, nil
+}
+
 func (s SpacemeshGrpcService) SetCommitmentSize(ctx context.Context, message *pb.CommitmentSizeMessage) (*pb.SimpleMessage, error) {


If you don't want to do it now, please leave a TODO.

moshababo · 2019-08-15T09:42:06Z

api/grpc_server.go

@@ -167,6 +179,15 @@ func (s SpacemeshGrpcService) startServiceInternal(status chan bool) {

 }

+func (s SpacemeshGrpcService) StartMining(ctx context.Context, message *pb.InitPost) (*pb.SimpleMessage, error) {
+	addr, err := address.StringToAddress(message.Coinbase)
+	err = s.Mining.StartPost(addr, message.LogicalDrive, message.CommitmentSize)


If you don't want to do it now, please leave a TODO.

moshababo · 2019-08-15T09:48:36Z

cmd/node/node.go

@@ -436,6 +434,15 @@ func (app *SpacemeshApp) startServices() {
 		log.Panic("cannot start block producer")
 	}
 	app.poetListener.Start()
+
+	if app.Config.StartMining {


This wasn't fixed yet.
And after what you did here:

if coinBase.Big().Uint64() == 0 && app.Config.StartMining { app.log.Panic("invalid Coinbase account") }

It should probably get removed entirely.

moshababo · 2019-08-15T10:11:46Z

activation/activation.go

+	b.log.Info("Starting post, reward address: %x", rewardAddress)
+	b.SetCoinbaseAccount(rewardAddress)
+	b.postInitLock.Lock()
+	if b.initStatus != Idle {


Why not to do this now as if b.initStatus == InProgress?

moshababo · 2019-08-15T10:12:48Z

activation/activation.go

@@ -222,6 +235,53 @@ func (b *Builder) buildNipstChallenge(epoch types.EpochId) error {
 	return nil
 }

+func (b *Builder) StartPost(rewardAddress address.Address, dataDir string, space uint64) error {


There are many more leftovers.

moshababo · 2019-08-15T10:15:02Z

activation/activation.go

+		_, err := b.nipstBuilder.InitializePost(dataDir, space) // TODO: add proof to first ATX
+		b.postInitLock.Lock()
+		if err != nil {
+			b.initStatus = Idle


You're not inspecting "already initialized" to set a Done status.
If you don't want to do it now, leave a TODO.

there is no "already initialized error, we use the initStatus flag to know this

I was referring to an error which comes from go-spacemesh/post initializer. And my mistake - it will return "already completed" and not "already initialized" (that was the old version).

then I think we should not return an error in such a case, and simply return the proof, I think It's not the best idea to keep track for specific error messages in between repos, since this can easily break

antonlerner added 5 commits August 11, 2019 17:18

Added GRPC for initialization of post, submittransaction now returns …

b2e72dc

…tx id alongside ok message closing #1247

added support for next layer rewards RPC

67f1716

added reset to nipst in UT

d0806a1

added flag for init post on node init, added this flag to automation

25b9364

after fmt

0804d61

moshababo reviewed Aug 13, 2019

View reviewed changes

antonlerner added 5 commits August 14, 2019 12:44

added prints and flag for init post on node init

77860cf

after CR

4408fba

after fmt

6debcd2

Merge branch 'develop' into startpost_rpc

d0274f1

fixed merge error

3717cde

moshababo reviewed Aug 15, 2019

View reviewed changes

antonlerner added 3 commits August 15, 2019 13:04

more changes after cr

ca14381

added todos in grpc server api calls

6f50b1e

renaming commitment size to space and logical drive to dataDir

359864d

moshababo reviewed Aug 15, 2019

View reviewed changes

moshababo approved these changes Aug 15, 2019

View reviewed changes

antonlerner and others added 3 commits August 15, 2019 13:26

after more fixes

e2c2db5

renaming UT as well

a5a8b77

go mod tidy

152f8da

antonlerner merged commit 547510f into develop Aug 15, 2019

moshababo mentioned this pull request Aug 20, 2019

SetRewardAddress api #1244

Closed

noamnelke deleted the startpost_rpc branch December 25, 2019 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Startpost rpc #1312

Startpost rpc #1312

antonlerner commented Aug 12, 2019 •

edited by moshababo

moshababo Aug 13, 2019

moshababo Aug 13, 2019

moshababo Aug 13, 2019

moshababo Aug 13, 2019

moshababo Aug 15, 2019

moshababo Aug 13, 2019

moshababo Aug 13, 2019

moshababo Aug 13, 2019

moshababo Aug 13, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

moshababo Aug 15, 2019

antonlerner Aug 15, 2019

moshababo Aug 15, 2019

antonlerner Aug 15, 2019 •

edited

Startpost rpc #1312

Startpost rpc #1312

Conversation

antonlerner commented Aug 12, 2019 • edited by moshababo

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonlerner Aug 15, 2019 • edited

Choose a reason for hiding this comment

antonlerner commented Aug 12, 2019 •

edited by moshababo

antonlerner Aug 15, 2019 •

edited