apollo_http_server: reload dynamic config upon tx processing

[Itay-Tsabary-Starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[Itay-Tsabary-Starkware]

• apollo_http_server: polling interval in config #11537
• apollo_http_server: add dynamic config poller service #11534
• apollo_http_server: use allow new tx flag #11454
• apollo_http_server: add allow new txs flag to config #11453
• apollo_http_server: reload dynamic config upon tx processing #11452 👈 (View in Graphite)
• apollo_config_manager: add acquiring http server dynamic config #11425
• apollo_http_server: add config manager client #11424
• apollo_node_config: add http server dynamic config to node dynamic config #11423
• apollo_http_server_config: split config to dynamic and static #11422
• deployment: move config value to an appropriate yaml file #11475
• main-v0.14.1-committer

This stack of pull requests is managed by Graphite. Learn more about stacking.

[matanl-starkware]

@matanl-starkware reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware).

---

crates/apollo_http_server/src/errors.rs line 101 at r1 (raw file):

    let (response_code, config_manager_error) = (
        StatusCode::INTERNAL_SERVER_ERROR,
        StarknetError::internal_with_logging("Failed to process client request", err),

We potentially expose inner errors to the outer world. Are you sure we want it?

Code quote:

, err

---

crates/apollo_http_server/src/http_server.rs line 134 at r1 (raw file):

    debug!("ADD_TX_START: Http server received a new transaction.");

    let dynamic_config = app_state.config_manager_client.get_http_server_dynamic_config().await?;

I think we don't want to do this as part of the critical path (add_tx flow).

2 options to consider (both depend on a saved dynamic_config previous value, contained in AppState):

1. (Easy): update it after add_tx_inner finishes.
2. (Harder): update asynchronously in a timely manner, regardless of the add_tx flow.

Code quote:

let dynamic_config = app_state.config_manager_client.get_http_server_dynamic_config().await?;

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @matanl-starkware).

---

crates/apollo_http_server/src/errors.rs line 101 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

We potentially expose inner errors to the outer world. Are you sure we want it?

Agreed. This is based on the pattern of the GW errors though, please see above. What do you suggest? I can avoid logging entirely, this shouldn't happen anyway.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @matanl-starkware).

---

crates/apollo_http_server/src/http_server.rs line 134 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

I think we don't want to do this as part of the critical path (add_tx flow).

2 options to consider (both depend on a saved dynamic_config previous value, contained in AppState):

1. (Easy): update it after add_tx_inner finishes.
2. (Harder): update asynchronously in a timely manner, regardless of the add_tx flow.

Measured locally: this is a sub-millisecond operation (and I was checking in ms granularity :-) )

RE the options:
afaiu option 1 is still on the critical path (happens before the ack is sent)
and also afaiu neither 1 nor 2 won't work as the hyper code clones the app state once, and then uses that same cloned value to serve all requests. Afaiu changing self.app_statewill not affect the value used in the router.

[github-actions]

Artifacts upload workflows:

• Started at 2026-01-07T14:09:38Z

[matanl-starkware]

@matanl-starkware reviewed 11 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware).

---

crates/apollo_http_server/src/errors.rs line 101 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Agreed. This is based on the pattern of the GW errors though, please see above. What do you suggest? I can avoid logging entirely, this shouldn't happen anyway.

Maybe remove the "err" param from the response? Leave it blank or with some generic message (but different than what we return in the GW error flow)?

---

crates/apollo_http_server/src/http_server.rs line 134 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Measured locally: this is a sub-millisecond operation (and I was checking in ms granularity :-) )

RE the options:
afaiu option 1 is still on the critical path (happens before the ack is sent)
and also afaiu neither 1 nor 2 won't work as the hyper code clones the app state once, and then uses that same cloned value to serve all requests. Afaiu changing self.app_statewill not affect the value used in the router.

Latency wise, I agree that <1ms is not a lot. But wouldn't you want to avoid unnecessary operations in the critical path?
Option 1: I meant to update after the ACK is sent, wherever that may be.
Option 2: I'm sure there's a way to share the memory between the threads. You'll figure it out.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matanl-starkware).

---

crates/apollo_http_server/src/http_server.rs line 134 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Latency wise, I agree that <1ms is not a lot. But wouldn't you want to avoid unnecessary operations in the critical path?
Option 1: I meant to update after the ACK is sent, wherever that may be.
Option 2: I'm sure there's a way to share the memory between the threads. You'll figure it out.

I don't see a way to avoid operations on the critical path.

Option 1: once the ACK is sent the tokio task handling it is terminated, this is part of the hyper framework. So, best I can come up with is to spawn a task before that termination that will handle the config update. This is also on the critical path, and anyway adds an undesired behavior, see below.

Option 2: since the shared memory is both readable by some (tx handlers) and written by one (config updater), this calls for a mutex/rwlock that the tx handlers will have to acquire. This will also be on the critical path. I think we should avoid that: a tx handler acquiring the mutex interfers with other tx handlers, and a rwlock will probably end up starving the config updater as there are constant reads happening. Anyway, this is still an operation on the critical path.

IMO the current approach is preferred to the above. Do you think otherwise? Or have a better solution for this?
I guess I could spend some time "reducing" the addition to the critical path, I'm wondering if it's worth the hassle though for a sub ms addition.

[Itay-Tsabary-Starkware]

@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matanl-starkware).

---

crates/apollo_http_server/src/errors.rs line 101 at r1 (raw file):

Previously, matanl-starkware (Matan Lior) wrote…

Maybe remove the "err" param from the response? Leave it blank or with some generic message (but different than what we return in the GW error flow)?

Why is that better? Still returning an inner error string, just less indicative.

[matanl-starkware]

@matanl-starkware reviewed 6 files and all commit messages, made 2 comments, and resolved 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware).

---

crates/apollo_http_server/src/errors.rs line 101 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Why is that better? Still returning an inner error string, just less indicative.

Discussed F2F. Agreed that this is not a real issue.

---

crates/apollo_http_server/src/http_server.rs line 134 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I don't see a way to avoid operations on the critical path.

Option 1: once the ACK is sent the tokio task handling it is terminated, this is part of the hyper framework. So, best I can come up with is to spawn a task before that termination that will handle the config update. This is also on the critical path, and anyway adds an undesired behavior, see below.

Option 2: since the shared memory is both readable by some (tx handlers) and written by one (config updater), this calls for a mutex/rwlock that the tx handlers will have to acquire. This will also be on the critical path. I think we should avoid that: a tx handler acquiring the mutex interfers with other tx handlers, and a rwlock will probably end up starving the config updater as there are constant reads happening. Anyway, this is still an operation on the critical path.

IMO the current approach is preferred to the above. Do you think otherwise? Or have a better solution for this?
I guess I could spend some time "reducing" the addition to the critical path, I'm wondering if it's worth the hassle though for a sub ms addition.

Discussed F2F.
Agreed to leave it as is for now, and optimize with Watch in future PR.