Enable auto quorum set configuration by default#371
Conversation
Since stellar-core v22.3.0 shipped long ago, all stable images now support the SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING config option. This commit: 1. Changes --enable-relaxed-auto-qset-config default from false to true 2. Removes RequireExplicitQset and skipHighCriticalValidatorChecks=false overrides from old-image missions, allowing them to use auto quorum set configuration 3. Updates related comments in StellarCoreSet.fs Agent-Logs-Url: https://github.com/stellar/supercluster/sessions/dcaac6b7-fa02-4444-a8bc-a8ff62e60327 Co-authored-by: marta-lokhova <9428003+marta-lokhova@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR flips the default behavior of quorum set generation toward automatic quorum set configuration by enabling “relaxed auto-qset config” by default, and removes several mission-specific fallbacks that were only needed for older stellar-core images.
Changes:
- Default
--enable-relaxed-auto-qset-configtotrue(CLI option and defaultMissionContextinProgram.fs, plus the test context). - Remove per-mission overrides that forced explicit quorum sets / disabled validator-check skipping for mixed-image and upgrade/catchup missions.
- Refresh comments around quorum-set configuration modes and
skipHighCriticalValidatorChecks.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/FSLibrary/StellarCoreSet.fs | Updates docs/comments for quorum-set configuration and skipHighCriticalValidatorChecks. |
| src/FSLibrary/MissionVersionMixConsensus.fs | Removes old-image explicit-qset/validator-check workarounds. |
| src/FSLibrary/MissionMixedImageNetworkSurvey.fs | Removes old-image explicit-qset/validator-check workarounds. |
| src/FSLibrary/MissionMixedImageLoadGeneration.fs | Removes old-image explicit-qset/validator-check workarounds. |
| src/FSLibrary/MissionDatabaseInplaceUpgrade.fs | Removes old-image explicit-qset/validator-check workarounds. |
| src/FSLibrary/MissionCatchupHelpers.fs | Removes old-image explicit-qset/validator-check workarounds from catchup set construction. |
| src/FSLibrary.Tests/Tests.fs | Updates test MissionContext default for enableRelaxedAutoQsetConfig to true. |
| src/App/Program.fs | CLI flag default changed to true, and default mission contexts updated accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Prefer automatic quorum set configuration. Fall back on explicit quorum | ||
| // set configuration if automatic configuration is not possible, or if | ||
| // --enable-relaxed-auto-qset-config is not set. | ||
| // set configuration if automatic configuration is not possible. |
There was a problem hiding this comment.
The PreferAutoQset comment says it falls back to explicit quorum only when automatic configuration is not possible, but the implementation also forces an explicit quorum when missionContext.enableRelaxedAutoQsetConfig is false (see NetworkCfg.QuorumSet in StellarCoreCfg.fs). Consider updating the comment to reflect that PreferAutoQset only uses auto-qset when relaxed auto-qset is enabled (or otherwise clarify the meaning of “not possible”).
| // Require automatic quorum set configuration. Fail if automatic | ||
| // configuration is not possible. Uses automatic configuration even if | ||
| // --enable-relaxed-auto-qset-config is not set, so missions using this | ||
| // option *must* satisfy the HIGH quality validator checks present in | ||
| // stellar-core. | ||
| // configuration is not possible. Missions using this option *must* satisfy | ||
| // the HIGH quality validator checks present in stellar-core, or set | ||
| // `skipHighCriticalValidatorChecks` to `true`. |
There was a problem hiding this comment.
This comment suggests setting skipHighCriticalValidatorChecks is sufficient to bypass the HIGH/CRITICAL validator checks, but the config key is only emitted when missionContext.enableRelaxedAutoQsetConfig is also true (see StellarCoreCfg.fs where SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING is gated). Please clarify this requirement here to avoid misleading mission authors.
| // SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING in stellar-core config, | ||
| // which disables validator checks for HIGH and CRITICAL validators. |
There was a problem hiding this comment.
skipHighCriticalValidatorChecks is described as controlling whether SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING is set, but in the config generation it is additionally gated on missionContext.enableRelaxedAutoQsetConfig. Consider mentioning that this only takes effect when relaxed auto-qset config is enabled, so the field name/comment match runtime behavior.
| // SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING in stellar-core config, | |
| // which disables validator checks for HIGH and CRITICAL validators. | |
| // SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING in stellar-core config | |
| // when relaxed auto-qset config is enabled, which disables validator | |
| // checks for HIGH and CRITICAL validators. |
| @@ -524,7 +524,7 @@ type MissionOptions | |||
| [<Option("enable-relaxed-auto-qset-config", | |||
| HelpText = "Enables the use of automatic quorum set configuration on missions that may create core sets that do not satisfy the redundancy and history requirements placed on pubnet validators. Requires a stellar-core version that supports the SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING config option.", | |||
There was a problem hiding this comment.
With this option now defaulting to true, the help text reads like it is an opt-in flag. Consider amending the HelpText to explicitly state that it defaults to enabled and that users can opt out by passing --enable-relaxed-auto-qset-config=false (important if someone needs to run against an older stellar-core image that doesn’t support the config key).
| HelpText = "Enables the use of automatic quorum set configuration on missions that may create core sets that do not satisfy the redundancy and history requirements placed on pubnet validators. Requires a stellar-core version that supports the SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING config option.", | |
| HelpText = "Enabled by default. Uses automatic quorum set configuration on missions that may create core sets that do not satisfy the redundancy and history requirements placed on pubnet validators. Opt out with --enable-relaxed-auto-qset-config=false, which may be necessary when running against older stellar-core images that do not support the SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTING config option.", |
bboston7
left a comment
bboston7
left a comment
There was a problem hiding this comment.
We should probably run these missions manually before merging. This looks fine, but I also wouldn't be surprised if this breaks a mission.
|
By "we", I mean "I". After all, I opened this ticket and assigned it myself. |
SKIP_HIGH_CRITICAL_VALIDATOR_CHECKS_FOR_TESTINGhas been available in all stable stellar-core images since v22.3.0. The--enable-relaxed-auto-qset-configgate and old-image explicit quorum fallbacks are no longer needed.--enable-relaxed-auto-qset-configtotruein CLI option and internal default contextsskipHighCriticalValidatorChecks = false+quorumSetConfigType = RequireExplicitQset) from:MissionVersionMixConsensusMissionMixedImageLoadGenerationMissionCatchupHelpersMissionDatabaseInplaceUpgradeMissionMixedImageNetworkSurveyStellarCoreSet.fsforQuorumSetConfigurationandskipHighCriticalValidatorChecksThe CLI flag is retained (now defaulting to
true) for opt-out if needed. These missions now use the defaultPreferAutoQsetpath, which will use auto quorum when ahomeDomainis set and fall back to explicit quorum otherwise.