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

Support reading bootstrap_nodes.yaml #6751

Merged
merged 2 commits into from
Dec 9, 2024
Merged

Support reading bootstrap_nodes.yaml #6751

merged 2 commits into from
Dec 9, 2024

Conversation

etan-status
Copy link
Contributor

bootstrap_nodes.txt is retired in lieu of bootstrap_nodes.yaml, start reading .yaml format (similar to .enr).

`bootstrap_nodes.txt` is retired in lieu of `bootstrap_nodes.yaml`,
start reading `.yaml` format (similar to `.enr`).
@@ -126,7 +144,8 @@ proc loadEth2NetworkMetadata*(
deployBlockPath = path & "/deploy_block.txt"
depositContractBlockPath = path & "/deposit_contract_block.txt"
depositContractBlockHashPath = path & "/deposit_contract_block_hash.txt"
bootstrapNodesPath = path & "/bootstrap_nodes.txt"
bootstrapNodesLegacyPath = path & "/bootstrap_nodes.txt" # <= Dec 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good way to more safely transition especially given (a) a potentially early December release and (b) possible legacy-format-metadata custom networks people might have. Worth considering at some point removing the older versions. But or this PR, sure.

mapIt(it[2..^1].strip())
else:
@[]
for line in splitLines(readFile(path)):
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really a YAML parser, this is a parser for that specific still-basically-line-oriented format. Is that enshrined as what's necessary, or is it legal now to have true general-purpose YAML files?

We do have NimYAML if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in line with how we parse config.yaml for network configurations. We kinda parse what exists in the wild; the .yaml extension for both the boot_enr.yaml (previously existed, and did not work on Chiado file) and the new bootstrap_nodes.yaml (same format seems to work) suggests that this is general purpose YAML in theory.

NimYAML so far is solely used in testing, and if we add it to production it should be audited accordingly. For now, I think going with the existing parser (plus the fix for Chiado which quotes strings, which is valid in YAML) is probably alright.

@tersec tersec merged commit 7d81ee1 into unstable Dec 9, 2024
13 checks passed
@tersec tersec deleted the dev/etan/nw-yamlboot branch December 9, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants