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

troubleshoot the limits validation timing during start with local network #654

Closed
wants to merge 1 commit into from

Conversation

sreuland
Copy link
Contributor

@sreuland sreuland commented Dec 17, 2024

the 'limits' config file path validation when' local' network was referring to PROTOCOL_VERSION before start had a chance to fully qualify it when not explicitly set from env or param, i.e. it will then set PROTOCOL_VERSION based on current core runtime default version reported after process start.

resolves e2e test failures on stellar-rpc - https://github.com/stellar/stellar-rpc/actions/runs/12363207197/job/34504081784

@@ -91,7 +92,6 @@ function start() {
echo "network root account id: $NETWORK_ROOT_ACCOUNT_ID"

copy_defaults
validate_after_copy_defaults
Copy link
Contributor Author

@sreuland sreuland Dec 17, 2024

Choose a reason for hiding this comment

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

this appears to be running too soon since it references PROTOCOL_VERSION, as start has not finished conditionally setting PROTOCOL_VERSION if it's not set and local network, in that case PROTOCOL_VERSION is set later after core has been started dynamically to the default version that the core runtime reports in upgrade_local

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I don't think we should go ahead with this fix, at least based on what I know so far, but please let me know if I'm missing something.

Why: The PROTOCOL_VERSION is never empty anymore. We always set it to a value, and that value is PROTOCOL_VERSION_DEFAULT now.

There are a few lines which handle if the PROTOCOL_VERSION is empty, but we should probably remove them and instead introduce a check right at the beginning that hard errors if both PROTOCOL_VERSION_DEFAULT and PROTOCOL_VERSION are empty.

Asking core what its max supported version is unreliable as a way to know which protocol version is supported by the image as a whole. This is because core can support a version ahead of all the other software and it can take time to upgrade.

We no longer rely on auto-determining the version because otherwise those images would break every time we go through a protocol release.

@leighmcculloch
Copy link
Member

@sreuland I suggest we do this:

@sreuland
Copy link
Contributor Author

thanks @leighmcculloch for explanation on the current protocol version strategy that quickstart employs, closing this as obsolete.

@sreuland sreuland closed this Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants