Skip to content

Conversation

@IdrisHanafi
Copy link

@IdrisHanafi IdrisHanafi commented Aug 15, 2023

What's new? and Motivation

The current approach would allow for empty L1 configs that are required for important aspects. Instead of allowing the code to run and use default type values, we should enforce the values to be set.

For example, let's assume the following l1Config in genesis.json (note: missing polygonZkEVMGlobalExitRootAddress value and incorrectly spelled supernets2Address):

{
  ...
  "l1Config" : {
    "chainId": 420,
    "supernets2Addresssss": "0xc949254d682d8c9ad5682521675b8f43b102aec4",
    "maticTokenAddress": "0xc949254d682d8c9ad5682521675b8f43b102aec4",
    "supernets2DataCommitteeContract": "0x2279B7A0a67DB372996a5FaB50D91eAA73d2eBe6"
  },
  ...
}

The synchronizer will continue to poll for smart contract logs and use default values of 0x0000000000000000000000000000000000000000 for the addresses:

{"jsonrpc":"2.0","id":766,"method":"eth_getLogs","params":[{"address":["0x0000000000000000000000000000000000000000","0x0000000000000000000000000000000000000000"],"fromBlock":"0x2cdb","toBlock":"0x2d3f","topics":null}]}

This will obviously return nothing and cause a lot of issues such as bridging, etc. In other words, filter queries in etherman.go (such as this) will not function properly.

  • Also added formatting to Makefile and ran the formatter

Testing

$ go test
2023-08-15T19:59:22.397Z        INFO    config/config.go:162    config file not found   {"pid": 1185384, "version": "v0.1.0"}
2023-08-15T19:59:22.402Z        INFO    config/config.go:162    config file not found   {"pid": 1185384, "version": "v0.1.0"}
PASS
ok      github.com/0xPolygon/supernets2-node/config     0.261s

and running in a devnet without a valid l1Config:

$ ./dist/supernets2-node run --network custom --custom-network-file /etc/zkevm/genesis.json --cfg /etc/zkevm/config.toml --components "rpc,synchronizer"
panic: failed to load genesis configuration from file. Error: Key: 'L1Config.GlobalExitRootManagerAddr' Error:Field validation for 'GlobalExitRootManagerAddr' failed on the 'required' tag

goroutine 1 [running]:
github.com/0xPolygonHermez/zkevm-node/config.(*Config).loadNetworkConfig(0xc000988e00, 0x13fa740?)
        /opt/zkevm-node/config/network.go:78 +0x1ee
github.com/0xPolygonHermez/zkevm-node/config.Load(0x3932353238343932?, 0x1)
        /opt/zkevm-node/config/config.go:181 +0x41d
main.start(0xc000a20e40)
        /opt/zkevm-node/cmd/run.go:46 +0x45
github.com/urfave/cli/v2.(*Command).Run(0xc0000e06e0, 0xc000a20e40, {0xc0004227e0, 0x9, 0x9})
        /root/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/command.go:274 +0xa42
github.com/urfave/cli/v2.(*Command).Run(0xc0000e1a20, 0xc000a20d00, {0xc000000140, 0xa, 0xa})
        /root/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/command.go:267 +0xc97
github.com/urfave/cli/v2.(*App).RunContext(0xc0008581e0, {0x1a34668?, 0xc000040070}, {0xc000000140, 0xa, 0xa})
        /root/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/app.go:332 +0x616
github.com/urfave/cli/v2.(*App).Run(...)
        /root/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/app.go:309
main.main()
        /opt/zkevm-node/cmd/main.go:191 +0xcdf

Open Question

This is just a small example of where the use case of enforcing the L1 values is required.

  • Are there cases where we do not need to enforce L1 Config values in the zkEVM node?

Future Tasks

If this is a feature to be merged, I suspect the following will have to be done:

  • Merge same logic into zkevm-node repo
  • Update usage docs

ToniRamirezM and others added 30 commits June 30, 2023 16:40
* Hotfix v0.1.4 to main (0xPolygon#2250)

* fix concurrent web socket writes

* fix eth_syncing

* fix custom trace internal tx call error handling and update prover

* add test to custom tracer depth issue; fix internal call error and gas used

* fix custom tracer for internal tx with error and no more steps after it

* remove debug code

* Make max grpc message size configurable  (0xPolygon#2179)

* make max grpc message size configurable

* fix state tests

* fix tests

* fix tests

* get SequencerNodeURI from SC if empty and not IsTrustedSequencer

* Optimize trace (0xPolygon#2183)

* optimize trace

* fix memory reading

* update docker image

* update prover image

* fix converter

* fix memory

* fix step memory

* fix structlogs

* fix structlogs

* fix structlogs

* fix structlogs

* fix structlogs

* fix structlogs

* fix structlogs

* fix structlogs

* update prover image

* fix struclogs

* fix memory size

* fix memory size

* fix memory size

* refactor memory resize

* refactor memory resize

* move log for the best fitting tx (0xPolygon#2192)

* fix load zkCounters from pool

* remove unnecessary log.info

* add custom tracer support to CREATES opcode without depth increase (0xPolygon#2213)

* logs

* fix getting stateroot from previous batch (GetWIPBatch)

* logs

* Fix GetWipBatch when previous last batch is a forced batch

* fix forcedBatch trusted state

* Revert "fix getting stateroot from previous batch (GetWIPBatch)"

This reverts commit 860f0e7.

* force GHA

* add pool limits (0xPolygon#2189)

* Hotfix/batch l2 data (0xPolygon#2223)

* Fix BatchL2Data

* Force GHA

* remove failed txs from the pool limit check (0xPolygon#2233)

* debug trace by batch number via external rpc requests (0xPolygon#2235)

* fix trace batch remote requests in parallel limitation (0xPolygon#2244)

* Added RPC.TraceBatchUseHTTPS config parameter

* fix executor version

---------

Co-authored-by: tclemos <thiago@polygon.technology>
Co-authored-by: tclemos <thiago@iden3.com>
Co-authored-by: Toni Ramírez <58293609+ToniRamirezM@users.noreply.github.com>
Co-authored-by: agnusmor <agnusmor@gmail.com>
Co-authored-by: agnusmor <100322135+agnusmor@users.noreply.github.com>
Co-authored-by: Thiago Coimbra Lemos <tclemos@users.noreply.github.com>

* fix test

* fix test

---------

Co-authored-by: tclemos <thiago@polygon.technology>
Co-authored-by: tclemos <thiago@iden3.com>
Co-authored-by: Toni Ramírez <58293609+ToniRamirezM@users.noreply.github.com>
Co-authored-by: agnusmor <agnusmor@gmail.com>
Co-authored-by: agnusmor <100322135+agnusmor@users.noreply.github.com>
Co-authored-by: Thiago Coimbra Lemos <tclemos@users.noreply.github.com>
* effective GasPrice refactor

* bugs fixes and finalizer tests fixes

* fix typo

* fix calculate effective gasprice percentage

* fix test gas price
* effective gas price returned by the rpc in the receipt

* linter
* bugfix: fixing l2blocks timestamp for the fist batch

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>

* fix finalizer unit test

---------

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>
…ssword from etherman.Config that are not in use
* fix fea2scalar and gas used

* suggestion

* fix fea2scalar

* suggestion
* fix pending tx when duplicate nonce

* set pool.transaction.failed_reason to NULL when updating an existing tx

* add more log details when adding tx to AddrQueue

* fix query to add tx to the pool. Fix lint errors

* change failed_reason for tx discarded due duplicate nonce
…2273)

* Return a tx from the pool only if it is

* fix TestGetTransactionByHash

---------

Co-authored-by: agnusmor <agnusmor@gmail.com>
…gon#2200-add-documentation-for-node-config-file-2

Feature/0xPolygon#2200 generate json-schema + docs for node config file and network_custom
…t forkId is under 5.

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>
…eck-for-fork-id-to-skip-effectivePercentage

improve: adding check to skip appending effectivePercentage if current forkId is under 5.
…om config param.

Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>
…equencer-on-batch-num

feat: adding functionality to stop sequencer on specific batch num from config param.
Signed-off-by: Nikolay Nedkov <nikolai_nedkov@yahoo.com>
…t-for-X-Real-IP

patch: adding print for X-Real-IP in JSON-RPC
@arnaubennassar
Copy link
Collaborator

arnaubennassar commented Aug 31, 2023

@IdrisHanafi sorry for taking long time to check this PR. I think it makes sense, but since it's not related to the CDK / Validium, I'd suggest to open this PR on the upstream repo so we can all benefit from it

Maybe the argument is that there are modes of running where L1 config is not required, and this prevents config from being parsed in those situations?

Actually this is a valid point, not sure if there is such a situation where the network config file is required but not the L1 config in particular. TBH, if such a case exists, it's most likely a poor implementation.

But anyway, I do agree that if the config is wrong the app would crash quite soon actually

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants