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

[piersy]

Motivation

Closes #4287

Changes

Sets the ComputeBatchSize option to the default value when starting smeshing.

Test Plan

Tested locally with the following process to verify that the crash no longer occurs after setting this option.
Using config:

{
  "api": {
    "grpc": [
      "debug",
      "node",
      "mesh",
      "globalstate",
      "transaction",
      "smesher"
    ]
  },
  "preset": "testnet",
  "p2p": {
    "bootnodes": [
      "/dns4/testnet-03-bootnode-0.spacemesh.network/tcp/5000/p2p/12D3KooWQMAAL9nkgXJgTM2psJLMbWRgZLjAxJozx7dNkKYczs2V",
      "/dns4/testnet-03-bootnode-1.spacemesh.network/tcp/5000/p2p/12D3KooWF2bhnqsnu2UjxJGZs8KCbzvd91vnud29KaG9rzTNFH78",
      "/dns4/testnet-03-bootnode-2.spacemesh.network/tcp/5000/p2p/12D3KooWQKosE9LZraMfFPg9QRYyKwxT4ipsar37oT6wRy8WqWos",
      "/dns4/testnet-03-bootnode-3.spacemesh.network/tcp/5000/p2p/12D3KooWKMQ5j15x2gzfXfX7uisss7uwkPnqJyoHcfyiYfHgSo3F",
      "/dns4/testnet-03-bootnode-4.spacemesh.network/tcp/5000/p2p/12D3KooWE1YEa3roqamhPt3CNiVRBzS7Lbv8HRbByE7fpSyLAZis",
      "/dns4/testnet-03-bootnode-5.spacemesh.network/tcp/5000/p2p/12D3KooWFCqyEner8hSnBmyQCkcxkuHjwY5BhHTaRYmGuNC1SQET",
      "/dns4/testnet-03-bootnode-6.spacemesh.network/tcp/5000/p2p/12D3KooWA96fvi2pQaQHpS2cpahDJCyQSsBW6A4KwHCPXsDVuiEJ",
      "/dns4/testnet-03-bootnode-7.spacemesh.network/tcp/5000/p2p/12D3KooWRvAVZrK3EURQBh1wdMPRDgjtowU84div3YxWYKuGDaFU",
      "/dns4/testnet-03-bootnode-8.spacemesh.network/tcp/5000/p2p/12D3KooWL3VGmZ4xgT1J5KnVJz7b6wxAUUFAVqNNRURKFfWNXLE4",
      "/dns4/testnet-03-bootnode-9.spacemesh.network/tcp/5000/p2p/12D3KooWCfadA8PUva7eRn1GzdLhnfELRLu81s1na6JbdYivwBgs"
    ]
  },
  "smeshing": {},
  "main": {
    "layer-duration": "300s",
    "layers-per-epoch": 864,
    "tick-size": 9266400,
    "block-gas-limit": 1000000,
    "poet-server": [
      "https://testnet-03-poet-0.spacemesh.network",
      "https://testnet-03-poet-1.spacemesh.network",
      "https://testnet-03-poet-2.spacemesh.network",
      "https://poet-10.spacemesh.network"
    ]
  },
  "genesis": {
    "genesis-time": "2023-04-11T18:00:00.498Z",
    "genesis-extra-data": "testnet-03"
  },
  "poet": {
    "phase-shift": "36h",
    "cycle-gap": "6h",
    "grace-period": "1m"
  },
  "post": {
      "post-labels-per-unit": 536870912,
      "post-max-numunits": 1000,
      "post-min-numunits": 4,
      "post-k1": 279,
      "post-k2": 300,
      "post-k3": 65,
      "post-k2pow-difficulty": 469112881707866048,
      "post-k3pow-difficulty": 29244622850649904
  },
   "hare": {
    "hare-wakeup-delta": "25s",
    "hare-round-duration": "25s",
    "hare-limit-iterations": 4,
    "hare-committee-size": 200
  },
  "tortoise": {
    "tortoise-zdist": 2,
    "tortoise-hdist": 40,
    "tortoise-window-size": 2000,
    "tortoise-delay-layers": 100
  },
  "beacon": {
    "beacon-grace-period-duration": "10m",
    "beacon-proposal-duration": "2m",
    "beacon-first-voting-round-duration": "30m",
    "beacon-rounds-number": 200,
    "beacon-voting-round-duration": "2m",
    "beacon-weak-coin-round-duration": "2m"
  }
}

1. rm -rf testnet_03 ~/post
2. make build && LD_LIBRARY_PATH=./build ./build/go-spacemesh --config config_testnet_03.json --data-folder testnet_03 --json-server
3. In another terminal:

grpc_cli call --json_input localhost:9092 spacemesh.v1.SmesherService.StartSmeshing '  {
    "coinbase": {
        "address": "stest1qqqqqqp8r9d6yzz8kqc3e9t0rgtlu8jwhlhyc4gm2xaut"
    },
    "opts": {
        "data_dir": "~/post",
        "num_units": 4,
        "max_file_size": 2147483648,
        "compute_provider_id": 1,
        "throttle": false
    }
}'

DevOps Notes

• This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
• This PR does not affect public APIs
• This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
• This PR does not make changes to log messages (which monitoring infrastructure may rely on)

[codecov]

Codecov Report

Merging #4293 (128e8ed) into develop (ed85095) will increase coverage by 0.0%.
The diff coverage is 100.0%.

❗ Current head 128e8ed differs from pull request most recent head b698e53. Consider uploading reports for the commit b698e53 to get more accurate results

@@           Coverage Diff           @@
##           develop   #4293   +/-   ##
=======================================
  Coverage     76.1%   76.1%           
=======================================
  Files          240     240           
  Lines        25096   25115   +19     
=======================================
+ Hits         19107   19132   +25     
+ Misses        4764    4761    -3     
+ Partials      1225    1222    -3     

Impacted Files	Coverage Δ
api/grpcserver/smesher_service.go	63.7% <100.0%> (-0.2%)	⬇️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[brusherru]

I've checked that setting smeshing-opts-compute-batch-size in the node config fixes the issue.
So I believe these changes will fix it as well :)

[fasmat]

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.

[pigmej]

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.

[fasmat]

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...

[piersy]

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.

[pigmej]

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

[piersy]

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

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

[piersy]

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?

[fasmat]

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

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

[pigmej]

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

[piersy]

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.

[piersy]

bors merge

[bors]

Build failed (retrying...):

• systest-status

[piersy]

bors merge

[bors]

Already running a review

[bors]

Build failed:

• systest-status

[countvonzero]

bors merge

[bors]

Pull request successfully merged into develop.

Build succeeded:

• ci-status
• systest-status