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.

Sign up for GitHub

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

Jump to bottom

services/horizon: Refactor Captive Core configuration flags #3262

Merged
merged 15 commits into from
Dec 4, 2020
Merged

services/horizon: Refactor Captive Core configuration flags #3262

merged 15 commits into from
Dec 4, 2020

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Dec 1, 2020

  • Rename --stellar-core-config-path to --stellar-captive-core-quorum-path.
    The core config path is automatically generated in captive-core online mode.
    --stellar-captive-core-quorum-path should point to a toml config file containing
    [QUORUM_SET] entries, with the same semantics as in the stellar core configuration.

  • Add --stellar-captive-core-http-port to indicate what port (if any) captive core
    shoulf listen on. If --stellar-core-url is unset and the local Captive core is enabled,
    --stellar-core-url will be implicitly use http://localhost:<stellar_captive_core_http_port>

  • Remove unused entries in CaptiveStellarCore

Closes #3211

@cla-bot cla-bot bot added the cla: yes label Dec 1, 2020
@2opremio 2opremio force-pushed the replace-core-cfg-by-quorum-cfg branch 4 times, most recently from 091a6c7 to 858c1f5 Compare December 1, 2020 17:58
@2opremio 2opremio requested a review from a team December 1, 2020 18:22
bartekn
bartekn reviewed Dec 2, 2020
View reviewed changes
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I remember you wanted to add some improvements so will approve later.

ingest/ledgerbackend/stellar_core_runner.go Outdated Show resolved Hide resolved
@2opremio 2opremio marked this pull request as ready for review December 2, 2020 21:00
@2opremio
Refactor Captive Core configuration flags
6d8b477 
* Rename `--stellar-core-config-path` to `--stellar-captive-core-quorum-path`.
  The core config path is automatically generated in captive-core online mode.
  `--stellar-captive-core-quorum-path` should point to a toml config file containing
  `[QUORUM_SET]` entries, with the same semantics as in the stellar core configuration.

* Add `--stellar-captive-core-http-port` to indicate what port (if any) captive core
  shoulf listen on. If `--stellar-core-url` is unset and the local Captive core is enabled,
  `--stellar-core-url` will be implicitly use `http://localhost:<stellar_captive_core_http_port>`

* Remove unused entries in `CaptiveStellarCore`
@2opremio
Check quorum config file
74740ab
@2opremio
Update the documentation
6cc106f
@2opremio
Replace quorum file by config addendum file
5968553
@2opremio
Fix addendum config path check
b152f41
@2opremio
Be consistent and use uint for the captive core port
640f690
@2opremio
Fix bug in http port formatting
20373d9
@2opremio
Improve-documentation
6d09808
@2opremio
Fix captivecore commandline flags
5539f81
@2opremio 2opremio force-pushed the replace-core-cfg-by-quorum-cfg branch from 34edf52 to 5539f81 Compare December 2, 2020 21:01
@2opremio
Copy link
Contributor Author

2opremio commented Dec 2, 2020

UPDATE

It turns out that [QUORUM_SET] entries were not enough to specify how to connect to a remote network (they don't include host names). Additionally [QUORUM_SET] is deprecated by Stellar Core

In reality we need a combination of [[HOME_DOMAINS]] and [[VALIDATORS]].

Thus, I have renamed --stellar-captive-core-quorum-path to captive-core-addendum-path, and I have documented it at https://github.com/stellar/go/pull/3262/files#diff-dcbfcf6a4881548aa52f3762303b0f1cf6ad6441a011cfee5273961012619e5e

PTAL

bartekn
bartekn reviewed Dec 3, 2020
View reviewed changes
ingest/ledgerbackend/captive_core_backend.go Outdated Show resolved Hide resolved
ingest/ledgerbackend/captive_core_backend.go Outdated Show resolved Hide resolved
ingest/ledgerbackend/stellar_core_runner_test.go Show resolved Hide resolved
services/horizon/internal/docs/captive_core.md Outdated Show resolved Hide resolved
services/horizon/internal/ingest/main.go Outdated Show resolved Hide resolved
services/horizon/internal/config.go Show resolved Hide resolved
services/horizon/internal/docs/captive_core.md Outdated Show resolved Hide resolved
ingest/ledgerbackend/stellar_core_runner.go Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the replace-core-cfg-by-quorum-cfg branch from 42b7c27 to c83bce9 Compare December 3, 2020 14:18
@2opremio 2opremio force-pushed the replace-core-cfg-by-quorum-cfg branch from c83bce9 to 4035167 Compare December 3, 2020 15:01
@2opremio 2opremio changed the title Refactor Captive Core configuration flags servoces/horizon: Refactor Captive Core configuration flags Dec 3, 2020
tamirms
tamirms reviewed Dec 4, 2020
View reviewed changes
ingest/ledgerbackend/stellar_core_runner.go Outdated
func (r *stellarCoreRunner) generateConfig() (string, error) {
if r.mode == stellarCoreRunnerModeOnline && r.coreConfigAddendumPath == "" {
return "", errors.New("stellar-core addendum config file path cannot be empty in online mode")
}
lines := []string{
"# Generated file -- do not edit",
"RUN_STANDALONE=true",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I missed this during my earlier review but, I think "RUN_STANDALONE=true" should only be enabled in offline mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it it ... but this enables peer connections, I think. Are we sure about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the stellar-core doc when you set it to true it prevents core from connecting to other peers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in online mode we need to be able to connect to peers to sync with consensus

@bartekn bartekn changed the title servoces/horizon: Refactor Captive Core configuration flags services/horizon: Refactor Captive Core configuration flags Dec 4, 2020
@2opremio 2opremio merged commit ea380a8 into stellar:master Dec 4, 2020
@2opremio 2opremio deleted the replace-core-cfg-by-quorum-cfg branch December 4, 2020 18:19
ire-and-curses
ire-and-curses reviewed Dec 7, 2020
View reviewed changes
services/horizon/internal/docs/captive_core.md
ADDRESS="core-testnet1.stellar.org"
```

The full configuration to be used will be printed out by Horizon when runnign horizon with `--log-level debug`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The full configuration to be used will be printed out by Horizon when runnign horizon with `--log-level debug`
The full configuration to be used will be printed out by Horizon when running horizon with `--log-level debug`,

Shaptic
Shaptic reviewed Dec 17, 2020
View reviewed changes
ingest/ledgerbackend/stellar_core_runner_test.go
NAME="sdf_testnet_1"
HOME_DOMAIN="testnet.stellar.org"
PUBLIC_KEY="GDKXE2OZMJIPOSLNA6N6F2BVCI3O777I2OOC4BV7VOYUEHYX7RTRYA7Y"
ADDRESS="core-testnet1.stellar.org"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that this is not a valid configuration stub. From the Horizon Captive Core logs:

Got an exception: Failed to parse '/tmp/captive-stellar-core-3bdb97927775752a/stellar-core.conf' :Critical and High quality validators sdf_testnet_1 must have redundancy of at least 3

Copy link
Contributor Author

@2opremio 2opremio Dec 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it has to be "MEDIUM", I fixed it in the docs, but not here (since it's not really relevant for the test).

@bartekn bartekn mentioned this pull request Dec 18, 2020
Merged
bartekn added a commit to stellar/packages that referenced this pull request Dec 21, 2020
@bartekn
Update stellar-captive-core config files (#129)
a051ea0 
This commit updates config files in `stellar-captive-core` package.
Starting from stellar/go#3262 config file for stellar-core is generated
by Horizon (except quorum slices that need to be explicitly set by the
admin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ire-and-curses ire-and-curses ire-and-curses left review comments

@Shaptic Shaptic Shaptic left review comments

@bartekn bartekn bartekn left review comments

@tamirms tamirms tamirms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Research generating stellar-core.cfg in Captive-Core online mode
5 participants
@2opremio @bartekn @Shaptic @tamirms @ire-and-curses