Skip to content

Commit e76bd10

Browse files
janjakubnanistajanjakubnanista
authored
fix: Fix challenger <> supervisor interaction (#230)
**Description** This PR fixes the challenger <> supervisor communication broken by #220. The system has become more complex and constant `SUPERVISOR_ENDPOINT` is no longer sufficient. This constant is still being used in several places - besides `op-geth` though its usages can be removed to (almost) break the circular dependency between `el` <> `supervisor` Relates to ethereum-optimism/optimism#15151
1 parent f9b3519 commit e76bd10

7 files changed

Lines changed: 238 additions & 62 deletions

File tree

‎main.star‎

Lines changed: 81 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -101,63 +101,101 @@ def run(plan, args={}):
101101
name="op_jwt_file",
102102
)
103103

104-
l2s = []
105-
for l2_num, chain in enumerate(optimism_args.chains):
106-
l2s.append(
107-
l2_launcher.launch_l2(
108-
plan,
109-
l2_num,
110-
chain.network_params.name,
111-
chain,
112-
jwt_file,
113-
deployment_output,
114-
l1_config_env_vars,
115-
l1_priv_key,
116-
l1_rpc_url,
117-
global_log_level,
118-
global_node_selectors,
119-
global_tolerations,
120-
persistent,
121-
observability_helper,
122-
interop_params,
123-
)
104+
l2s = [
105+
l2_launcher.launch_l2(
106+
plan,
107+
l2_num,
108+
chain.network_params.name,
109+
chain,
110+
jwt_file,
111+
deployment_output,
112+
l1_config_env_vars,
113+
l1_priv_key,
114+
l1_rpc_url,
115+
global_log_level,
116+
global_node_selectors,
117+
global_tolerations,
118+
persistent,
119+
observability_helper,
120+
interop_params,
124121
)
122+
for l2_num, chain in enumerate(optimism_args.chains)
123+
]
125124

125+
supervisors = []
126126
if interop_params.enabled:
127-
for i, interop_set in enumerate(interop_params.sets):
128-
op_supervisor_launcher.launch(
129-
plan=plan,
130-
interop_set=interop_set,
131-
l1_config_env_vars=l1_config_env_vars,
132-
l2s=l2s,
133-
jwt_file=jwt_file,
134-
observability_helper=observability_helper,
135-
)
127+
# We collect the output of the supervisor launcher since it will be used by the challenger
128+
supervisors = [
129+
supervisor
130+
# Since starlark does not support the walrus operator, we need to use a nested list comprehension
131+
#
132+
# TODO We do this since launcher can return None, maybe it's worth removing all the disabled values from the list beforehand
133+
for supervisor in [
134+
op_supervisor_launcher.launch(
135+
plan=plan,
136+
interop_set=interop_set,
137+
l1_config_env_vars=l1_config_env_vars,
138+
l2s=l2s,
139+
jwt_file=jwt_file,
140+
observability_helper=observability_helper,
141+
)
142+
for interop_set in interop_params.sets
143+
]
144+
if supervisor != None
145+
]
136146

137147
# challenger must launch after supervisor because it depends on it for interop
138148
for l2_num, l2 in enumerate(l2s):
139149
chain = optimism_args.chains[l2_num]
150+
151+
# Nothing happens if the challenger is not enabled
152+
if not chain.challenger_params.enabled:
153+
continue
154+
140155
op_challenger_image = (
141156
chain.challenger_params.image
142157
if chain.challenger_params.image != ""
143158
else input_parser.DEFAULT_CHALLENGER_IMAGES["op-challenger"]
144159
)
145-
if chain.challenger_params.enabled:
146-
op_challenger_launcher.launch(
147-
plan,
148-
l2_num,
149-
"op-challenger-{0}".format(chain.network_params.name),
150-
chain.challenger_params.image,
151-
l2.participants[0].el_context,
152-
l2.participants[0].cl_context,
153-
l1_config_env_vars,
154-
deployment_output,
155-
chain.network_params,
156-
chain.challenger_params,
157-
interop_params,
158-
observability_helper,
160+
161+
# We now need to find the supervisor service for the current L2
162+
#
163+
# Since we allow multiple supervisors to be registered for the same L2 (to allow for intentional misconfiguration),
164+
# we need to find all the supervisors for the current L2, then we pick the first one
165+
l2_supervisors = [
166+
supervisor
167+
for supervisor in supervisors
168+
if chain.network_params.network_id
169+
in [n.network_id for n in supervisor.networks]
170+
]
171+
172+
# If there are multiple supervisors, we just let the user know
173+
num_l2_supervisors = len(l2_supervisors)
174+
if num_l2_supervisors > 1:
175+
plan.print(
176+
"Found more than one ({0} to be precise) supervisor for network {1}. Only the first one will be used for the challenger".format(
177+
num_l2_supervisors, l2.network_id
178+
)
159179
)
160180

181+
# We pick the first supervisor
182+
l2_supervisor = l2_supervisors[0] if num_l2_supervisors > 0 else None
183+
184+
op_challenger_launcher.launch(
185+
plan=plan,
186+
l2_num=l2_num,
187+
service_name="op-challenger-{0}".format(l2.network_id),
188+
image=op_challenger_image,
189+
el_context=l2.participants[0].el_context,
190+
cl_context=l2.participants[0].cl_context,
191+
l1_config_env_vars=l1_config_env_vars,
192+
deployment_output=deployment_output,
193+
network_params=chain.network_params,
194+
challenger_params=chain.challenger_params,
195+
supervisor=l2_supervisor,
196+
observability_helper=observability_helper,
197+
)
198+
161199
observability.launch(
162200
plan, observability_helper, global_node_selectors, observability_params
163201
)

‎network_params.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,10 @@ optimism_package:
5353
global_node_selectors: {}
5454
global_tolerations: []
5555
persistent: false
56+
interop:
57+
sets:
58+
- name: "interop"
59+
participants: "*"
5660
ethereum_package:
5761
participants:
5862
- el_type: geth

‎src/challenger/op-challenger/op_challenger_launcher.star‎

Lines changed: 21 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ ethereum_package_constants = import_module(
99
observability = import_module("../../observability/observability.star")
1010
prometheus = import_module("../../observability/prometheus/prometheus_launcher.star")
1111

12+
constants = import_module("../../package_io/constants.star")
1213
interop_constants = import_module("../../interop/constants.star")
1314
util = import_module("../../util.star")
1415

@@ -34,22 +35,22 @@ def launch(
3435
deployment_output,
3536
network_params,
3637
challenger_params,
37-
interop_params,
38+
supervisor,
3839
observability_helper,
3940
):
4041
config = get_challenger_config(
41-
plan,
42-
l2_num,
43-
service_name,
44-
image,
45-
el_context,
46-
cl_context,
47-
l1_config_env_vars,
48-
deployment_output,
49-
network_params,
50-
challenger_params,
51-
interop_params,
52-
observability_helper,
42+
plan=plan,
43+
l2_num=l2_num,
44+
service_name=service_name,
45+
image=image,
46+
el_context=el_context,
47+
cl_context=cl_context,
48+
l1_config_env_vars=l1_config_env_vars,
49+
deployment_output=deployment_output,
50+
network_params=network_params,
51+
challenger_params=challenger_params,
52+
supervisor=supervisor,
53+
observability_helper=observability_helper,
5354
)
5455

5556
service = plan.add_service(service_name, config)
@@ -72,7 +73,7 @@ def get_challenger_config(
7273
deployment_output,
7374
network_params,
7475
challenger_params,
75-
interop_params,
76+
supervisor,
7677
observability_helper,
7778
):
7879
ports = dict(get_used_ports())
@@ -123,8 +124,12 @@ def get_challenger_config(
123124
if observability_helper.enabled:
124125
observability.configure_op_service_metrics(cmd, ports)
125126

126-
if interop_params.enabled:
127-
cmd.append("--supervisor-rpc=" + interop_constants.SUPERVISOR_ENDPOINT)
127+
if supervisor != None:
128+
cmd.append(
129+
"--supervisor-rpc={0}".format(
130+
supervisor.service.ports[constants.RPC_PORT_ID].url
131+
)
132+
)
128133
# TraceTypeSupper{Cannon|Permissioned} needs --cannon-depset-config to be set
129134
# Added at https://github.com/ethereum-optimism/optimism/pull/14666
130135
# Temporary fix: Add a dummy flag

‎src/interop/constants.star‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ INTEROP_WS_PORT_NUM = 9645
66
SUPERVISOR_SERVICE_NAME = "op-supervisor"
77
SUPERVISOR_RPC_PORT_NUM = 8545
88

9+
# FIXME This endpoint no longer exists and its usages should be dropped
910
SUPERVISOR_ENDPOINT = util.make_http_url(
1011
SUPERVISOR_SERVICE_NAME, SUPERVISOR_RPC_PORT_NUM
1112
)

‎src/interop/op-supervisor/op_supervisor_launcher.star‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,10 @@ def launch(
110110
service,
111111
)
112112

113-
return service
113+
return struct(
114+
service=service,
115+
networks=interop_set_l2s,
116+
)
114117

115118

116119
def get_supervisor_config(

‎test/interop/op-supervisor/op_supervisor_launcher_test.star‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,97 @@ def test_op_supervisor_everything_in_one_set(plan):
340340
)
341341

342342

343+
def test_op_supervisor_with_challenger(plan):
344+
main.run(
345+
plan,
346+
{
347+
"optimism_package": {
348+
"chains": [
349+
{
350+
"network_params": {
351+
"network_id": "1000",
352+
}
353+
},
354+
{
355+
"network_params": {
356+
"network_id": "2000",
357+
}
358+
},
359+
{
360+
"challenger_params": {
361+
"enabled": False,
362+
},
363+
"network_params": {
364+
"network_id": "3000",
365+
},
366+
},
367+
{
368+
"network_params": {
369+
"network_id": "4000",
370+
}
371+
},
372+
],
373+
"interop": {
374+
"sets": [
375+
{
376+
"name": "the-interopest-of-sets",
377+
"participants": ["1000", "2000"],
378+
},
379+
{
380+
"name": "sibling-of-the-interopest-of-sets",
381+
"participants": ["2000", "3000", "4000"],
382+
},
383+
],
384+
},
385+
},
386+
},
387+
)
388+
389+
services = plan.get_services()
390+
challenger_service_configs_by_name = {
391+
s.name: kurtosistest.get_service_config(s.name)
392+
for s in services
393+
if s.name.startswith("op-challenger-")
394+
}
395+
396+
expect.true("op-challenger-1000" in challenger_service_configs_by_name)
397+
expect.true("op-challenger-2000" in challenger_service_configs_by_name)
398+
expect.true("op-challenger-3000" not in challenger_service_configs_by_name)
399+
expect.true("op-challenger-4000" in challenger_service_configs_by_name)
400+
401+
challenger_service_config1000 = challenger_service_configs_by_name[
402+
"op-challenger-1000"
403+
]
404+
challenger_service_config2000 = challenger_service_configs_by_name[
405+
"op-challenger-2000"
406+
]
407+
challenger_service_config4000 = challenger_service_configs_by_name[
408+
"op-challenger-4000"
409+
]
410+
411+
# The first challenger should point to the first supervisor since that's the only one for the 1000 network
412+
expect.true(
413+
"--supervisor-rpc={0}".format(
414+
"http://op-supervisor-the-interopest-of-sets:8545"
415+
)
416+
in challenger_service_config1000.cmd[0]
417+
)
418+
# The second challenger should point to the first supervisor since that's the first one of the two for the 2000 network
419+
expect.true(
420+
"--supervisor-rpc={0}".format(
421+
"http://op-supervisor-the-interopest-of-sets:8545"
422+
)
423+
in challenger_service_config2000.cmd[0]
424+
)
425+
# The third challenger should point to the second supervisor since that's the only one for the 4000 network
426+
expect.true(
427+
"--supervisor-rpc={0}".format(
428+
"http://op-supervisor-sibling-of-the-interopest-of-sets:8545"
429+
)
430+
in challenger_service_config4000.cmd[0]
431+
)
432+
433+
343434
def test_create_dependency_set_filename(plan):
344435
expect.eq(
345436
op_supervisor_launcher.create_dependency_set_filename(

0 commit comments

Comments
 (0)