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

[Merged by Bors] - Ensure ComputeBatchSize set when starting smeshing #4293

Closed
wants to merge 4 commits into from
+16 −14
Closed

[Merged by Bors] - Ensure ComputeBatchSize set when starting smeshing #4293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eeea741
Ensure smeshing api uses initial smeshing config
piersy Apr 13, 2023
d97a163
Rename optsCopy -> opts
piersy Apr 17, 2023
128e8ed
Merge remote-tracking branch 'origin/develop' into piersy/fix-crash-o…
piersy Apr 17, 2023
b698e53
Merge branch 'develop' into piersy/fix-crash-on-start-smeshing
countvonzero Apr 17, 2023
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
2 changes: 1 addition & 1 deletion api/grpcserver/grpcserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,7 @@ func TestSmesherService(t *testing.T) {
postProvider.EXPECT().Status().Return(&activation.PostSetupStatus{}).AnyTimes()
postProvider.EXPECT().ComputeProviders().Return(nil).AnyTimes()
smeshingAPI := &SmeshingAPIMock{}
svc := NewSmesherService(postProvider, smeshingAPI, 10*time.Millisecond)
svc := NewSmesherService(postProvider, smeshingAPI, 10*time.Millisecond, activation.DefaultPostSetupOpts())
t.Cleanup(launchServer(t, cfg, svc))

ctx, cancel := context.WithTimeout(context.Background(), time.Second)
Expand Down
21 changes: 11 additions & 10 deletions api/grpcserver/smesher_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type SmesherService struct {
smeshingProvider activation.SmeshingProvider

streamInterval time.Duration
postOpts activation.PostSetupOpts
Copy link
Member

Choose a reason for hiding this comment

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

Instead of passing in the default options here, I think we should give smapp the option to set ComputeBatchSize. This parameter can make a big difference in initialization time on different machines and only fall back to config.DefaultComputeBatchSize if not provided by smapp.

Copy link
Member

Choose a reason for hiding this comment

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

Yes and no. Smapp IMHO can already set the value as they need. The problem is that we would rather need spacemeshos/post#125 than ask the user for the BatchSize. POST is already waaaay too complex with waaaay too many variables. I'd honestly rather leave an open possibility for power-users and hide it from normal users.

Copy link
Member

Choose a reason for hiding this comment

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

We don't have to do this now. We can also just set ComputeBatchSize to config.DefaultComputeBatchSize (as commented below) and maybe do this in the future, similar how Scrypt options are handled...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not just the default options here though, they are the options provided at startup, which default to the defaults if not set.

So I think there are 2 reasonable approaches here:

  • Do what this PR does, so take the options provided at startup and then allow them to be further adjusted by the api call.
  • Force API users to specify a full set of options, and don't use any defaults.

I don't have a clear view of what would be the better approach.

What I think we should avoid is allowing users to provide smeshing options at startup, and having an API which accepts partial smeshing options, but then does not use the options initially provided by the user for the remaining fields. I think this will confuse users.

Copy link
Member

Choose a reason for hiding this comment

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

@piersy important note, you somewhat cannot easily modify the batchSize after the process has started.

It's more like all-or-nothing approach here.

The best would be imho "defaults unless specified" in single call.

@fasmat scrypt options are "hardcoded" no one should modify them in easy way, I would not put them to the same bucket.

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 best would be imho "defaults unless specified" in single call

@pigmej Is that one of the 2 reasonable approaches I listed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know it doesn't make sense to allow them to be able to be changed, but we are not treating them differently from any other post options besides not being able to change them via grpc.

So as I understand it both compute batch size and scrypt options are currently user configurable, but only with the config provided at startup, if they shouldn't be then this becomes pretty simple since all options would be specified by the API call.

But why are they user configurable if they shouldn't be?

Copy link
Member

Choose a reason for hiding this comment

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

But why are they user configurable if they shouldn't be?

Because of how we handle our config packages 😅: #4297 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

@piersy it's even worse... you can technically have a different batchSize than other nodes as that's local only. But script N is a value for the whole network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it sounds like we need a construct similar to ethereum's chain config that allows setting network wide parameters, do we have such a thing?

But for the purposes of this PR I think the current approach is fine. Then we'll wan't a followup PR to remove the ability to set scrypt params per node. And it seems like we need a discussion around batch size.

}

// RegisterService registers this service with a grpc server instance.
Expand All @@ -33,8 +34,8 @@ func (s SmesherService) RegisterService(server *Server) {
}

// NewSmesherService creates a new grpc service using config data.
func NewSmesherService(post postSetupProvider, smeshing activation.SmeshingProvider, streamInterval time.Duration) *SmesherService {
return &SmesherService{post, smeshing, streamInterval}
func NewSmesherService(post postSetupProvider, smeshing activation.SmeshingProvider, streamInterval time.Duration, postOpts activation.PostSetupOpts) *SmesherService {
return &SmesherService{post, smeshing, streamInterval, postOpts}
}

// IsSmeshing reports whether the node is smeshing.
Expand Down Expand Up @@ -67,14 +68,14 @@ func (s SmesherService) StartSmeshing(ctx context.Context, in *pb.StartSmeshingR
return nil, status.Error(codes.InvalidArgument, "`Opts.MaxFileSize` must be provided")
}

opts := activation.PostSetupOpts{
DataDir: in.Opts.DataDir,
NumUnits: in.Opts.NumUnits,
MaxFileSize: in.Opts.MaxFileSize,
ComputeProviderID: int(in.Opts.ComputeProviderId),
Throttle: in.Opts.Throttle,
Scrypt: config.DefaultLabelParams(),
fasmat marked this conversation as resolved.
Show resolved Hide resolved
}
// Copy provided post opts
opts := s.postOpts
// Overlay api provided opts
opts.DataDir = in.Opts.DataDir
opts.NumUnits = in.Opts.NumUnits
opts.MaxFileSize = in.Opts.MaxFileSize
opts.ComputeProviderID = int(in.Opts.ComputeProviderId)
opts.Throttle = in.Opts.Throttle

coinbaseAddr, err := types.StringToAddress(in.Coinbase.Address)
if err != nil {
Expand Down
5 changes: 3 additions & 2 deletions api/grpcserver/smesher_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ func TestPostConfig(t *testing.T) {
postSetupProvider := activation.NewMockpostSetupProvider(ctrl)
smeshingProvider := activation.NewMockSmeshingProvider(ctrl)

svc := grpcserver.NewSmesherService(postSetupProvider, smeshingProvider, time.Second)
svc := grpcserver.NewSmesherService(postSetupProvider, smeshingProvider, time.Second, activation.DefaultPostSetupOpts())

postConfig := activation.PostConfig{
MinNumUnits: rand.Uint32(),
Expand All @@ -47,7 +47,7 @@ func TestStartSmeshingPassesCorrectSmeshingOpts(t *testing.T) {
ctrl := gomock.NewController(t)
postSetupProvider := activation.NewMockpostSetupProvider(ctrl)
smeshingProvider := activation.NewMockSmeshingProvider(ctrl)
svc := grpcserver.NewSmesherService(postSetupProvider, smeshingProvider, time.Second)
svc := grpcserver.NewSmesherService(postSetupProvider, smeshingProvider, time.Second, activation.DefaultPostSetupOpts())

types.DefaultTestAddressConfig()
addr, err := types.StringToAddress("stest1qqqqqqrs60l66w5uksxzmaznwq6xnhqfv56c28qlkm4a5")
Expand All @@ -59,6 +59,7 @@ func TestStartSmeshingPassesCorrectSmeshingOpts(t *testing.T) {
ComputeProviderID: 7,
Throttle: true,
Scrypt: config.DefaultLabelParams(),
ComputeBatchSize: config.DefaultComputeBatchSize,
}).Return(nil)

_, err = svc.StartSmeshing(context.Background(), &pb.StartSmeshingRequest{
Expand Down
2 changes: 1 addition & 1 deletion cmd/node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,7 @@ func (app *App) initService(ctx context.Context, svc grpcserver.Service) (grpcse
case grpcserver.Node:
return grpcserver.NewNodeService(ctx, app.host, app.mesh, app.clock, app.syncer, cmd.Version, cmd.Commit), nil
case grpcserver.Smesher:
return grpcserver.NewSmesherService(app.postSetupMgr, app.atxBuilder, app.Config.API.SmesherStreamInterval), nil
return grpcserver.NewSmesherService(app.postSetupMgr, app.atxBuilder, app.Config.API.SmesherStreamInterval, app.Config.SMESHING.Opts), nil
case grpcserver.Transaction:
return grpcserver.NewTransactionService(app.db, app.host, app.mesh, app.conState, app.syncer, app.txHandler), nil
case grpcserver.Activation:
Expand Down