abstract espresso configs as type #916
Conversation
arbnode/batch_poster.go
Outdated
| MinimumHotshotBlockNum uint64 `koanf:"minimum-hotshot-block-num"` | ||
| } | ||
|
|
||
| type EspressoBatchPosterConfig struct { |
There was a problem hiding this comment.
can we move this to a separate file? Maybe under the espresso folder and then just import here?
There was a problem hiding this comment.
Yeah I think moving this will help minimize merge conflicts in the batch_poster.go file, Additionally, it'll make git diffs cleaner for future version updates.
arbnode/batch_poster.go
Outdated
| DangerousBatchPosterConfigAddOptions(prefix+".dangerous", f) | ||
| } | ||
|
|
||
| func EspressoBatchPosterConfigAddOptions(prefix string, f *pflag.FlagSet) { |
varun-doshi
requested review from
ImJeremyHe,
jbearer,
jjeangal and
philippecamacho
as code owners
varun-doshi
force-pushed
the
varun/esp-config
branch
from
a95b989 to
302d81d
Compare
❌ 4 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
|
|
arbnode/node.go
Outdated
| EspressoCaffNode EspressoCaffNodeConfig `koanf:"espresso-caff-node"` | ||
| EspressoBatchPoster espressobatchposter.EspressoBatchPosterConfig `koanf:"espresso-batch-poster"` |
There was a problem hiding this comment.
We can make it shorter here. Since the struct name has been EspressoConfig, I think we can omit espresso in its fields.
And both Caff node and poster are using espresso streamer, there are some same configs. I would like to add a Streamer field to config the espresso streamer and both bathcer and caff node read it
There was a problem hiding this comment.
HotShotBlock and Dangerous.HotShotBlock have been added to the StreamerConfig. Batcher and Caff node will read values from there.
The retryTime could possibly also be shared but I noticed the Default values for retryTime is different in Batcher vs Caff
In Batcher:
EspressoTxnsPollingInterval: time.Second,
In Caff:
RetryTime: time.Second * 2,
There was a problem hiding this comment.
Can you re-organize them? Otherwise it woule be confusing to use
There was a problem hiding this comment.
to clarify; I'll add EspressoTxnsPollingInterval in the streamerConfig?
If so, what should we keep the default value?
time.Second or time.Second*2
There was a problem hiding this comment.
I think time.Second makes more sense here for now. We want to be polling slightly faster than hotshot will produce blocks, which right now can be down to around 1 block every 2 seconds.
There is some more work being done to increase the speed of hotshot, so this might change again in the future, but for now, 1 second should be fine.
arbnode/node.go
Outdated
| "github.com/offchainlabs/nitro/daprovider/das" | ||
| "github.com/offchainlabs/nitro/daprovider/data_streaming" | ||
| "github.com/offchainlabs/nitro/daprovider/factory" | ||
| espressobatchposter "github.com/offchainlabs/nitro/espresso/batch_poster" |
There was a problem hiding this comment.
Do we have to use a new package? Maybe I am wrong but I think we just need a file under the arbnode folder
|
|
||
| // Espresso Specific config // | ||
|
|
||
| // Hotshot currently produces blocks at average of 2 seconds | ||
| // We set it to 1 second to get updates more often than blocks are produced | ||
| EspressoTxnsPollingInterval: time.Second, | ||
| // We should send to espresso at a speed faster than the speed nitro is producing messages | ||
| EspressoTxnsSendingInterval: 125 * time.Millisecond, | ||
| EspressoTxnsResubmissionInterval: 2 * time.Second, | ||
| ResubmitEspressoTxDeadline: 10 * time.Minute, | ||
| HotShotUrl: "", | ||
| EspressoTeeType: "NITRO", | ||
| EspressoRegisterServiceConfig: espressotee.DefaultEspressoRegisterServiceConfig, | ||
| // EspressoTxSizeLimit is 1 MB, to have some buffer we set it to 900 KB | ||
| EspressoTxSizeLimit: 900 * 1024, | ||
| UserDataAttestationFile: "", | ||
| QuoteFile: "", | ||
| AttestationServiceURL: "", | ||
| HotShotBlock: 1, | ||
| HotShotFirstPostingBlock: 1, | ||
| InitBatcherAddresses: []string{}, | ||
| EspressoEventPollingStep: 100, | ||
| AddressMonitorStep: 100, | ||
| AddressMonitorStartL1: 1, |
system_tests/common_test.go
Outdated
| func TestEspressoConfigParsing(t *testing.T) { | ||
| inputSource := map[string]interface{}{ | ||
| "sequencer": true, | ||
| "espresso": map[string]interface{}{ | ||
| "espresso-caff-node": map[string]interface{}{ | ||
| "enable": true, | ||
| }, | ||
| "espresso-batch-poster": map[string]interface{}{ | ||
| "espresso-tee-type": "NITRO", | ||
| "hotshot-url": "http://localhost:8080", | ||
| "espresso-tx-size-limit": int64(900000), | ||
| "hotshot-block": uint64(100), | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| k := koanf.New(".") | ||
| err := k.Load(confmap.Provider(inputSource, "."), nil) | ||
| require.NoError(t, err) | ||
|
|
||
| var parsedConfig arbnode.Config | ||
| err = k.UnmarshalWithConf("", &parsedConfig, koanf.UnmarshalConf{Tag: "koanf"}) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, true, parsedConfig.Sequencer) | ||
| assert.Equal(t, true, parsedConfig.Espresso.EspressoCaffNode.Enable) | ||
| assert.Equal(t, "NITRO", parsedConfig.Espresso.EspressoBatchPoster.EspressoTeeType) | ||
| assert.Equal(t, "http://localhost:8080", parsedConfig.Espresso.EspressoBatchPoster.HotShotUrl) | ||
| assert.Equal(t, int64(900000), parsedConfig.Espresso.EspressoBatchPoster.EspressoTxSizeLimit) | ||
| assert.Equal(t, uint64(100), parsedConfig.Espresso.EspressoBatchPoster.HotShotBlock) | ||
| } |
There was a problem hiding this comment.
common_test.go is an upstream file we should avoid unnecessary changes. Can you move this unit test to another file that starts with espresso? If no appropriate file exists, you can create a new one.
arbnode/batch_poster.go
Outdated
| espressoConfig EspressoBatchConfigFetcher | ||
| bytesType abi.Type | ||
| bytes32ArrayType abi.Type | ||
| blobsAttestationArguments abi.Arguments | ||
| espressoStreamer *espressostreamer.EspressoStreamer | ||
| espressoBatcherAddrMonitor BatcherAddrMonitorInterface | ||
| espressoRestarting bool | ||
| signerAddr common.Address | ||
|
|
||
| espressoStreamerConfig EspressoStreamerConfigFetcher |
There was a problem hiding this comment.
I think we can pass the Espresso config directly here. I know it feels a bit awkward since the Espresso config contains caff-node that the batcher doesn't need, but this approach keeps the change minimally invasive.
| type EspressoStreamerConfig struct { | ||
| HotShotBlock uint64 `koanf:"hotshot-block"` | ||
| Dangerous DangerousEspressoStreamerConfig `koanf:"dangerous"` | ||
| } |
There was a problem hiding this comment.
Should not this contain some fields indicating the polling interval?
There was a problem hiding this comment.
yes here is the reasoning
do you mean a py script to convert the old json config to the new one? |
Yes!. I think Python should be the most appropriate since there is already a py script under this folder. |
|
Script to migrate old config to new config added: python3 ./scripts/migrate-config-file.py config.json new_node_config.jsonI've tested the script with the following config file: { "parent-chain": {
"connection": {
"url": "${parent_chain_url_secret_arn}"
}
},
"chain": {
"info-files": ["/config/l2_chain_info.json"]
},
"node": {
"parent-chain-reader": {
"poll-interval": "60s",
"poll-only": true
},
"staker": {
"parent-chain-wallet": {
"private-key": "${staker_secret_arn}"
},
"disable-challenge": false,
"enable": true,
"staker-interval": "120s",
"make-assertion-interval": "120s",
"strategy": "MakeNodes"
},
"sequencer": true,
"dangerous": {
"no-sequencer-coordinator": true,
"disable-blob-reader": true
},
"delayed-sequencer": {
"enable": true,
"finalize-distance": 6,
"use-merge-finality": false
},
"seq-coordinator": {
"enable": false,
"redis-url": "redis://redis:6379",
"lockout-duration": "30s",
"lockout-spare": "1s",
"my-url": "",
"retry-interval": "0.5s",
"seq-num-duration": "24h0m0s",
"update-interval": "3s"
},
"espresso-caff-node": {
"enable": false,
"namespace": 23,
"espresso-tee-type": "${espresso_tee_type}",
"dangerous":{
"minimum-hotshot-block-num":56,
"ignore-database-hotshot-block":true,
"ignore-database-from-block":false
}
},
"batch-poster": {
"enable": true,
"data-poster": {
"max-base-fee": 10
},
"espresso-register-service-config": {
"max-retries": 5,
"max-base-fee": 10
},
"poll-interval": "10s",
"redis-url": "",
"l1-block-bound": "ignore",
"wait-for-max-delay": true,
"resubmit-espresso-tx-deadline": "2m",
"parent-chain-wallet": {
"private-key": "${batch_poster_secret_arn}"
},
"max-delay": "1h0m0s",
"max-empty-batch-delay": "1h0m0s",
"post-4844-blobs": false,
"hotshot-url": "https://localhost:9090",
"light-client-address": "${light_client_address}",
"espresso-tee-type": "${espresso_tee_type}",
"hotshot-first-posting-block": 1,
"address-monitor-start-l1": 1,
"hotshot-block": 10,
"address-valid-ranges":[
{
"address":"${address1}",
"from":1,
"to":99
},
{
"address":"${address2}",
"from":99,
"to":199
}
]
},
"block-validator": {
"enable": true,
"validation-server": {
"url": "${validation_server_url}",
"jwtsecret": "/config/val_jwt.hex"
}
},
"feed": {
"input": {
"url": ""
},
"output": {
"enable": true,
"signed": false
}
},
"data-availability": {
"enable": false
}
},
"execution": {
"sequencer": {
"enable": true
},
"forwarding-target": "null"
},
"persistent": {
"chain": "local",
"db-engine": "leveldb"
},
"ws": {
"addr": "0.0.0.0",
"api": [
"eth",
"net",
"web3",
"arb",
"debug",
"txpool"
]
},
"http": {
"addr": "0.0.0.0",
"api": [
"eth",
"net",
"web3",
"arb",
"debug",
"txpool"
],
"vhosts": "*",
"corsdomain": "*"
}
}
Which gives the following updated config(this is also what is being used in the Go test: {
"chain": {
"info-files": [
"/config/l2_chain_info.json"
]
},
"espresso": {
"batch-poster": {
"address-monitor-start-l1": 1,
"address-valid-ranges": [
{
"address": "${address1}",
"from": 1,
"to": 99
},
{
"address": "${address2}",
"from": 99,
"to": 199
}
],
"espresso-register-service-config": {
"max-base-fee": 10,
"max-retries": 5
},
"espresso-tee-type": "${espresso_tee_type}",
"hotshot-first-posting-block": 1,
"hotshot-url": "https://localhost:9090",
"resubmit-espresso-tx-deadline": "2m"
},
"caff-node": {
"dangerous": {
"ignore-database-from-block": false,
"ignore-database-hotshot-block": true
},
"enable": false,
"espresso-tee-type": "${espresso_tee_type}",
"namespace": 23
},
"streamer": {
"dangerous": {
"minimum-hotshot-block-num": 56
},
"hotshot-block": 10
}
},
"execution": {
"forwarding-target": "null",
"sequencer": {
"enable": true
}
},
"http": {
"addr": "0.0.0.0",
"api": [
"eth",
"net",
"web3",
"arb",
"debug",
"txpool"
],
"corsdomain": "*",
"vhosts": "*"
},
"node": {
"batch-poster": {
"data-poster": {
"max-base-fee": 10
},
"enable": true,
"l1-block-bound": "ignore",
"light-client-address": "${light_client_address}",
"max-delay": "1h0m0s",
"max-empty-batch-delay": "1h0m0s",
"parent-chain-wallet": {
"private-key": "${batch_poster_secret_arn}"
},
"poll-interval": "10s",
"post-4844-blobs": false,
"redis-url": "",
"wait-for-max-delay": true
},
"block-validator": {
"enable": true,
"validation-server": {
"jwtsecret": "/config/val_jwt.hex",
"url": "${validation_server_url}"
}
},
"dangerous": {
"disable-blob-reader": true,
"no-sequencer-coordinator": true
},
"data-availability": {
"enable": false
},
"delayed-sequencer": {
"enable": true,
"finalize-distance": 6,
"use-merge-finality": false
},
"feed": {
"input": {
"url": ""
},
"output": {
"enable": true,
"signed": false
}
},
"parent-chain-reader": {
"poll-interval": "60s",
"poll-only": true
},
"seq-coordinator": {
"enable": false,
"lockout-duration": "30s",
"lockout-spare": "1s",
"my-url": "",
"redis-url": "redis://redis:6379",
"retry-interval": "0.5s",
"seq-num-duration": "24h0m0s",
"update-interval": "3s"
},
"sequencer": true,
"staker": {
"disable-challenge": false,
"enable": true,
"make-assertion-interval": "120s",
"parent-chain-wallet": {
"private-key": "${staker_secret_arn}"
},
"staker-interval": "120s",
"strategy": "MakeNodes"
}
},
"parent-chain": {
"connection": {
"url": "${parent_chain_url_secret_arn}"
}
},
"persistent": {
"chain": "local",
"db-engine": "leveldb"
},
"ws": {
"addr": "0.0.0.0",
"api": [
"eth",
"net",
"web3",
"arb",
"debug",
"txpool"
]
}
} |
| espressoRestarting bool | ||
| signerAddr common.Address | ||
|
|
||
| // espressoStreamerConfig EspressoStreamerConfigFetcher |
There was a problem hiding this comment.
can you remove this comment? if not needed
| } | ||
|
|
||
| func TestEspressoConfigMigration(t *testing.T) { | ||
| const migratedJSON = `{ |
There was a problem hiding this comment.
can this not be a file? That will be cleaner
| @@ -0,0 +1,114 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
I think this script should live in https://github.com/EspressoSystems/integration-internal-tools instead of this repo
There was a problem hiding this comment.
If this repository is used externally, it would be nice to have it here. The script isn’t sensitive, so I think it’s fine to include it as well.
| "espresso-tee-type": ("batch-poster", "espresso-tee-type"), | ||
| "espresso-register-service-config": ("batch-poster", "espresso-register-service-config"), | ||
| "hotshot-url": ("batch-poster", "hotshot-url"), | ||
| "espresso-txns-polling-interval": ("batch-poster", "espresso-txns-polling-interval"), | ||
| "espresso-txns-sending-interval": ("batch-poster", "espresso-txns-sending-interval"), | ||
| "espresso-txns-resubmission-interval": ("batch-poster", "espresso-txns-resubmission-interval"), | ||
| "resubmit-espresso-tx-deadline": ("batch-poster", "resubmit-espresso-tx-deadline"), |
There was a problem hiding this comment.
Can we also rename the espresso* configs? They’re already under the espresso config block, so the prefix feels a bit redundant.
Closes: Asana Task
This PR:
EspressoBatchPosterConfigwhich hold Espresso fields for the Batch Poster. This separates the general configs from espresso specific ones.BatchPosterhas a new fieldEspressoBatchConfigFetcherwhich is an interface similar toBatchConfigFetcherEspressoConfigwhich will hold Espresso specific configs and fields. Currently, it holdsEspressoCaffNodeConfigandEspressoBatchPosterConfigConfignow has a structure like this:TestEspressoConfigParsingEnd result of this PR should be that we can have something similar to this as the config file:
Key places to review:
arbnode/batch_poster.goarbnode/node.goTesting
nitro-nodee2e test. I believe that one is failing because I haven't changed theconfig.tsfile insidenitro-node(please correct me if I'm wrong on this)What is left:
EspressoConfig