-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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
Refactor node configs to remove local-only addresses from genesis #6629
Conversation
ad0bb83
to
8915d8d
Compare
@tharbert are there configs in other repos that I'll need to update after this change to migrate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good. I wonder, for now, if we want to always configure worker/primary address for our own worker/primary to be localhost:{port in the committee}.
I also want someone from narwhal to double check this, maybe @akichidis?
narwhal_config::WorkerInfo { | ||
name: NetworkPublicKey::from_bytes(validator.worker_key().as_ref()) | ||
.expect("Can't get worker key"), | ||
transactions: consensus_config.address.clone(), | ||
worker_address, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a follow up PR maybe we should remove transactions
from the narwhal WorkerInfo (or allow for it to be empty) since its unused in Sui and this value put here is just a placeholder for all entries other than our worker.
I would opt not to hardcode localhost, since I think that we don't want that to be the long-term default anyway? My understanding is this is just an escape hatch for partners with broken routing? |
Yeah i do much prefer this as an escape hatch. |
.clone() | ||
}; | ||
let workers = [( | ||
0, // worker_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a concern of this PR, but since we are here the worker_id
should also be configurable in the future and not hardcoded - but we can refactor this when we deal with the story of spinning up multiple workers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea it doesn't work at all for >1 worker right now so I figured that should all be done at once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good, thanks @aschran ! Could you please ensure that Narwhal benchmarks are also working fine after the changes? (since we have now removed the internal_worker_address
? )
Also very good point about infra changes that need to be made - let's make sure that those will be addressed as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR!
referencing a slack conversation
I think the difference is that genesis is global/public and the other parts of NodeConfig aren't necessarily so
If that is the case, maybe we can rename the things that are called genesis
but actually just public to public
here?
8915d8d
to
073747e
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
For posterity: @tharbert said this should be fine to merge, the main thing that'll need updates is https://github.com/MystenLabs/sui-operations/tree/main/docker/config/generate/templates which he'll do before Monday PT. |
This removes the notion of internal address from the narwhal side, instead configuring that externally.
073747e
to
d7d8bf8
Compare
This removes the notion of internal address from the narwhal side, instead configuring that externally.