cspr-rad / kairos

Apache License 2.0
2 stars 0 forks source link

nixos/cctl: use exponential backoff, remove unused code #125

Closed marijanp closed 2 months ago

marijanp commented 2 months ago
Avi-D-coder commented 2 months ago

I don't think this is working. I am setting the chainspec latest commits of the contract branch but the nixos vm test still fails with the same deploy size error. see https://github.com/cspr-rad/kairos/pull/120

The error still states the default max_deploy size not the changed one.

koxu1996 commented 2 months ago

There are 2 different configs file used to configure Casper node:

This PR pulls config and incorrectly use it as a chainspec. To increase max deploy size you need to customize both - see https://github.com/cspr-rad/kairos/pull/120#issuecomment-2167939187.

Instead of pulling those files from custom node fork, I would rather use official protocol release with 2 sed commands, or patch file.

Avi-D-coder commented 2 months ago

There are 2 different configs file used to configure Casper node:

* config.toml

* chainspec.toml

This PR pulls config and incorrectly use it as a chainspec. To increase max deploy size you need to customize both - see #120 (comment).

Instead of pulling those files from custom node fork, I would rather use official protocol release with 2 sed commands, or patch file.

I switched it to chainspec.toml in https://github.com/cspr-rad/kairos/pull/120/commits/3f4b56f33dff42b8292fa54a89358f61af48e55b, I did not include the changed config. Maybe that's why the error is still happening, but the error still states kairos: output: deploy size of 1465741 bytes exceeds limit of 1048576, 1048576 is the unmodified chainspec.toml value, so don't think the chainspec flag is doing anything.

marijanp commented 2 months ago

@Avi-D-coder the problem was also due to an incorrect argument format for the cctl command. It expects the arguments to be passed as config=<path> chainspec=<path> it should be fixed now I've verified the generated files manually, please try again.

marijanp commented 2 months ago

@koxu1996 for now the goal was to get a working as simple as possible. I would also prefer to introduce nixos options where a user can say

services.cctl.maxBodyBytes = <num-bytes>;
services.cctl.maxBlockSize = <size>;
Avi-D-coder commented 2 months ago

I prefer the whole file style, I would rather not have to match the seds to the changes made in the upstream files we use in the rust contract tests.

https://github.com/cspr-rad/kairos/blob/2dd35183d7ad0112cc55500d429a67c12a11f5ef/nixos/tests/end-to-end.nix#L29C1-L39C9

One option would be to set the default to the upstream branch https://github.com/cspr-rad/casper-node/tree/kairos-testing-chainspec and then allow seds.

koxu1996 commented 2 months ago

@Avi-D-coder What about using official config files instead of those from cspr-rad fork?

marijanp commented 2 months ago

@koxu1996 this is currently using the static resorces from the casper-node sources. do they differ from the ones you've linked?

I personally don't have any preferences to which approach we are going to pursue. The sed calls can be abstracted away such that we have module options as mentioned earlier. I would only prefer to keep it simple for our current usecase.

koxu1996 commented 2 months ago

@marijanp They are almost the same. I approved PR, but using official protocol releases would be better.