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.

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

services/horizon: Add "DISABLE_TX_SUB" configuration parameter #4914

Closed
urvisavla opened this issue Jun 16, 2023 · 5 comments · Fixed by #4979
Closed

services/horizon: Add "DISABLE_TX_SUB" configuration parameter #4914

urvisavla opened this issue Jun 16, 2023 · 5 comments · Fixed by #4979

Comments

@urvisavla
Copy link
Contributor

urvisavla commented Jun 16, 2023

What problem does your feature solve?

To streamline configuration and enhance flexibility in Horizon, we want to introduce the TX_SUB_DISABLED configuration parameter. This parameter enables or disables transaction submission functionality in Horizon.

What would you like to see?

Add DISABLE_TX_SUB configuration parameter for Transaction Submission in Horizon. This parameter can be set as an environment variable similar to other parameters in Horizon.

Key features:

  1. Enable/disable Transaction Submission: By default DISABLE_TX_SUB should be set to false. Horizon should accept or reject POST requests with a 405 HTTP error code, method not supported fro the /tx endpoint based on this setting.

  2. Enhance startup validation: When DISABLE_TX_SUB is false, Horizon should perform a startup check to make sure data ingestion is happening by checking it's DB and If no ingestion is detected, Horizon should exit with an appropriate error.

  3. If either "INGESTION=true" or "DISABLE_TX_SUB=false" is set in the configuration, Horizon should validate incoming config parameters include network config via either the new parameter "NETWORK" or specify the lower level parameters, if not, a startup error should be emitted stating network config is needed. Otherwise, when DISABLE_TX_SUB=true and INGEST=false, all network oriented config params are no longer needed, such as NETWORK and STELLAR_CORE_URLbecause no core connectivity is required, if the are present, horizon should emit error saying the flags are in conflict.

Confirm that Horizon can use captive core's local HTTP_PORT to submit transactions or does it need to send to a watcher/validator node? We need to mention this expectation accurately in new config docs.

@mollykarcher
Copy link
Contributor

mollykarcher commented Jul 13, 2023

Consider naming these as the inverse (ex. DISABLE_X). We currently have very limited test coverage around command line args, part of this effort should include adding that coverage

@sreuland sreuland changed the title services/horizon: Add "TX_SUB_ENABLED" configuration parameter services/horizon: Add "TX_SUB_DISABLED" configuration parameter Jul 13, 2023
@Shaptic
Copy link
Contributor

Shaptic commented Jul 13, 2023

make sure data ingestion is happening by checking it's DB and If no ingestion is detected, Horizon should exit with an appropriate error.

This is interesting. Do you mean something like watching database state and seeing that e.g. select max(sequence) from history_ledgers; is increasing?

I think just having a Stellar Core to forward txsub and a valid db connection to should be enough, since the admin will see 504s if their db is not ingesting correctly. Wdyt?

@urvisavla
Copy link
Contributor Author

make sure data ingestion is happening by checking it's DB and If no ingestion is detected, Horizon should exit with an appropriate error.

This is interesting. Do you mean something like watching database state and seeing that e.g. select max(sequence) from history_ledgers; is increasing?

Yes, that was my understanding as well.

I think just having a Stellar Core to forward txsub and a valid db connection to should be enough, since the admin will see 504s if their db is not ingesting correctly. Wdyt?

I agree, it seems like that should serve as a sanity check. What do you think @sreuland?

@aditya1702 aditya1702 self-assigned this Jul 17, 2023
@urvisavla
Copy link
Contributor Author

Suggestion to rename the flag TX_SUB_DISABLED to something like DISABLE_TRANSACTION_SUBMISSION or TRANSACTION_SUBMISSION_DISABLED

@aditya1702 aditya1702 changed the title services/horizon: Add "TX_SUB_DISABLED" configuration parameter services/horizon: Add "DISABLE_TX_SUB" configuration parameter Jul 25, 2023
@sreuland
Copy link
Contributor

I think just having a Stellar Core to forward txsub and a valid db connection to should be enough, since the admin will see 504s if their db is not ingesting correctly. Wdyt?

I agree, it seems like that should serve as a sanity check. What do you think @sreuland?

yep, that proposed mechanism to detect ingestion may be all that's needed, it would mean invoking a request against STELLAR_CORE_URL to confirm valid response, etc, that mechanism was intentionally vague in the requirements as was expected to incur some investigation into what might be available for detection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants