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

server: Add bound addresses IPC events #3020

Merged
merged 1 commit into from
Nov 23, 2022

Conversation

matheusd
Copy link
Member

This adds a new set of IPC events sent over the TX pipe that report the locally bound addresses of the P2P and RPC interfaces.

The main goal for this feature is to enable binding dcrd to a random port (using for example, --listen :0) while enabling a controlling parent process to learn the address that was assigned to dcrd by the OS.

The events are only sent when the --boundaddrevents flag is specified.

Closes #3018

Copy link
Member

@davecgh davecgh left a comment

Choose a reason for hiding this comment

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

Shouldn't the protocol version over in serviceControlPipeTx be bumped as well?

@matheusd
Copy link
Member Author

It's not clear to me what the semantics are for the protocol version (whether it's only meant to guard the encoding or the actual contents/types of messages). Based on this comment in the original implementation I was under the impression it would only be used for encoding, so that's why I didn't bump it.

I'm ok with switching to using the IPC protocol version as a discriminant for what sorts of messages are emitted. If we do so, I'll improve the documentation a little bit to make it more explicit what the version refers to.

@davecgh
Copy link
Member

davecgh commented Nov 22, 2022

Shouldn't the protocol version over in serviceControlPipeTx be bumped as well?

After discussing this a bit with the original author, the protocol version there was only intended to refer to the encoding. Since adding new message types doesn't affect that and can safely be ignored, it doesn't need to be bumped. Depending on how the spawning processes handle it, bumping it could potentially break them.

That said, it does seem to me like giving the spawning process a way to determine which messages are available would be ideal, because it could otherwise spawn a slightly older version of dcrd (say the commit just prior to this one) and it wouldn't be available.

I realize that because this PR is introducing a CLI flag, attempting to launch that slightly older version of dcrd would fail due to an unknown flag, so it doesn't really apply in this case, but my guy tells me that, in the more general case, not gating behind a flag and instead allowing the spawning process to discover whether or not a specific capability is available would result in a nicer UX.

That would not be something for this PR however.

@davecgh
Copy link
Member

davecgh commented Nov 22, 2022

It's not clear to me what the semantics are for the protocol version (whether it's only meant to guard the encoding or the actual contents/types of messages). Based on this comment in the original implementation I was under the impression it would only be used for encoding, so that's why I didn't bump it.

Looks like we were both making a comment at the same time, but yes, I've confirmed with the original author that is the intent.

I'm ok with switching to using the IPC protocol version as a discriminant for what sorts of messages are emitted. If we do so, I'll improve the documentation a little bit to make it more explicit what the version refers to.

I discussed that a bit in my comment above that I was typing while you made this comment too. We can follow up on that separate since it's not something for this PR regardless.

ipc.go Outdated Show resolved Hide resolved
This adds a new set of IPC events sent over the TX pipe that report the
locally bound addresses of the P2P and RPC interfaces.

The main goal for this feature is to enable binding dcrd to a random
port (using for example, --listen :0) while enabling a controlling
parent process to learn the address that was assigned to dcrd by the OS.

The events are only sent when the --boundaddrevents flag is specified.
@davecgh davecgh merged commit fc017ce into decred:master Nov 23, 2022
@matheusd matheusd deleted the random-bind-port branch November 24, 2022 11:27
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.

Add IPC (pipetx) events to report listening ports
3 participants