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

Jump to bottom

Critical Issue: configurable VM parameter could halt the chain. #1418

Closed
piux2 opened this issue Dec 7, 2023 · 4 comments
Closed

Critical Issue: configurable VM parameter could halt the chain. #1418

piux2 opened this issue Dec 7, 2023 · 4 comments

Comments

@piux2
Copy link
Contributor

piux2 commented Dec 7, 2023

Description

a8675d3#r134400942

VM parameter should instead be remain as hard-coded values, as they were originally, or specified as parameters in the genesis file. Using it as a config parameter is not appropriate. It could break consensus and halt the chain.
@piux2 piux2 added this to the 🚀 main.gno.land (required) milestone Dec 7, 2023
@thehowl
Copy link
Member

thehowl commented Dec 7, 2023 •
edited
Loading

2c:

  • the original GnoChess realm required IIRC ~20M gas for publishing, so in general I think we should change the maxgas per TX/block to be either 50M or 100M.
  • regarding your concern, the change by Antonio actually only affects genesis as he stated; the reason for that is that the limit to 10M gas persists in the ConsensusParams

@moul
Copy link
Member

moul commented Dec 7, 2023

Here is the original proposal I made regarding this variable: #828 (review)

@moul
Copy link
Member

moul commented Oct 15, 2024 •
edited
Loading

I believe this issue duplicates another one. I expect all those elements to be configurable as gnoland start options or in the genesis. However, they should also be subject to change through governance votes or hardcoded in other cases.

@moul moul mentioned this issue Oct 15, 2024
Merged
16 tasks
@Kouteki Kouteki removed this from the 🚀 Mainnet launch milestone Oct 16, 2024
@moul
Copy link
Member

moul commented Oct 22, 2024

Closing. There is no longer a maxCycles parameter (#2993). Other configuration parameters may still have an impact, but the goal is to transfer the relevant ones to the new params module (#2920).

@moul moul closed this as completed Oct 22, 2024
moul added a commit that referenced this issue Oct 23, 2024
@moul
feat(tm2): add sdk/params module (#2920)
43dd3f3 
- [x] port x/params -> sdk/params
b930513
- [x] inject in vmkeeper + add std.SetConfig
602245d
- [x] implement in `gnoland` 783a044
	- [x] appchain
    - [x] rpc query
    - [x] txtar
- [x] implement or add comment where we should use it in the existing
codebase
	- [x] namespace's realm target
- [ ] questions
- [x] do we want a `std.GetConfig` from the contract part? -> No, it
allows unsafe, complex, and implicit patterns. If you want to get a
value from another contract, you can either import it or use a registry
pattern. This approach preserves type safety and other GNOVM
protections.
- [ ] do we want to restrict the realms able to call `SetConfig` (only
`r/sys`), or maybe set an expensive gas price?
- [x] after discussion with jae
	- [x] Rename Config -> Param for consistency
- [x] Remove `interface{}` from the setters and use specific types,
including in the tm2 implementation (string, uint64, int64, bool, bytes)
- [x] Remove the `.<type>` suffix addition, but ensure that the type is
explicitly defined by the user; and remove the table.
	- [x] Remove the types table from the tm2 implementation

Related #1418 
Related #1856

---------

Signed-off-by: moul <94029+moul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
🚀 The Launch [DEPRECATED]
Status: No status
🧙‍♂️gno.land core team
Status: Done
🚀 main.gno.land launch 🚀
Status: Done
Milestone
No milestone
Development

No branches or pull requests
5 participants
@moul @thehowl @Kouteki @zivkovicmilos @piux2