Update hip-1046.md

[Mark-Swirlds]

Based on feedback I've edited the following:

• Updated Abstract to make problem statement clear.
• Change entries containing "grpc_web nodes" to "grpc_web proxies"
• Updated proposed changes to incorporate a recommendation from the mirror-node team.

[netlify]

✅ Deploy Preview for hedera-hips ready!

Name	Link
🔨 Latest commit	c975af7
🔍 Latest deploy log	https://app.netlify.com/sites/hedera-hips/deploys/67215b75c0aff900087c05c0
😎 Deploy Preview	https://deploy-preview-1066--hedera-hips.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[netopyr]

I would have expected a new field in node_create.proto and node_update.proto like:

repeated proto.ServiceEndpoint grpc_web_proxy_endpoint;

There are probably good reasons why this approach was not taken, even though it seems less error-prone to use. Can we please add an entry under "Rejected Ideas" explaining why this approach was not taken?

[netopyr]

Why do we want to change the API like this? This looks more complicated and error-prone than the current definition.

[steven-sheehy]

This 80 character limit seems too restrictive and conflicts with our Java coding standards to use 120. The standards just mention java so not sure about other formats. Is it documented somewhere that we should use 80 for markdown?

[jsync-swirlds]

There is no standard for markdown that I know of, but github enforces 80 characters in much of the review interface (particularly suggestions), so I habitually reformat HIP markdown to 80 characters for ease of review.
I do not think there is any requirement to do so, however.

[steven-sheehy]

These statements are not really true. A node can have more than one secure port on different proxy IPs. A node can also have both secure and insecure on the same IP since they are different ports. I imagine in the future with permissionless some nodes might even want multiple secure per IP or multiple insecure per IP. Here's node 0.0.3 service endpoints on mainnet right now:

"service_endpoints":[
{"domain_name":"","ip_address_v4":"34.239.82.6","port":50211},
{"domain_name":"","ip_address_v4":"34.239.82.6","port":50212},
{"domain_name":"","ip_address_v4":"35.237.200.180","port":50211},
{"domain_name":"","ip_address_v4":"35.237.200.180","port":50212}]

[jsync-swirlds]

It is permitted to do so (SHOULD is a recommendation keyword). It is not best practice, however.

[steven-sheehy]

I was commenting on both statements. There is a MUST NOT statement that is not true.

And I disagree with your SHOULD recommendation as it can't be considered a bad practice if every node in production does it. The statements should reflect the reality on mainnet.

[jsync-swirlds]

I see. That is not ideal (having both secure and insecure endpoints is bad security practice), but I can remove the specification text (once the author returns and can accept a PR).

[rbair23]

I don't understand the new proposal. The NodeCreateTransactionBody for example, which adds a node to the address book, has ServiceEndpoints such as gossip_endpoint and service_endpoint. This HIP doesn't change that. But it does add ServiceEndpointPurpose. So what happens if I set a ServiceEndpoint on gossip_endpoint with a purpose of SECURE_GRPC?

I'm assuming the node transaction will have to be updated in an incompatible way? That isn't a problem since it is not currently used, but I didn't see that change in the specification section.

[jsync-swirlds]

After this HIP, we would need to enforce the correct purpose value for nodeCreate and nodeUpdate endpoint lists in consensus node software.

All changes needed would be compatible from the perspective of protobuf. The node transaction definitions do not need to be changed here as we're just adding a new field.
During upgrade we might need perform a state migration to set the purpose for each endpoint (which can be determined from the containing list and port, as Mirror Node does currently).

[netopyr]

I think we should keep the different types of endpoints separated. Instead of putting everything into a single list and adding a type, I suggest defining the new field in NodeCreateTransactionBody and NodeUpdateTransactionBody as lists:

repeated proto.ServiceEndpoint grpc_proxy_endpoint;

Both solutions provide the same functionality. Everything that can be configured with a single list can also be configured with two lists and vice versa. Storing the gRPC proxies in a separate list has several advantages, though:

1. A single list makes errors possible that would otherwise not occur. As @rbair23, for example, notes, the API does not strongly discourage me from putting a gossip endpoint into the list of service endpoints. Another point of failure is the additional mandatory purpose field that one may forget.
2. It is inconsistent and unexpected that one type of ServiceEndpoint has a separate list while the others do not.
3. A single list makes it harder to run checks like "Does the node define at least one service point?". These checks will probably be implemented on the server and in all SDKs.

I agree with @steven-sheehy's observation that we should not rely on port ranges to determine if an endpoint is secured. How about adding a boolean flag in ServiceEndpoint?

[jsync-swirlds]

I think we should keep the different types of endpoints separated. Instead of putting everything into a single list and adding a type, I suggest defining the new field in NodeCreateTransactionBody and NodeUpdateTransactionBody as lists:

repeated proto.ServiceEndpoint grpc_proxy_endpoint;

Both solutions provide the same functionality. Everything that can be configured with a single list can also be configured with two lists and vice versa. Storing the gRPC proxies in a separate list has several advantages, though:

1. A single list makes errors possible that would otherwise not occur. As @rbair23, for example, notes, the API does not strongly discourage me from putting a gossip endpoint into the list of service endpoints. Another point of failure is the additional mandatory purpose field that one may forget.
2. It is inconsistent and unexpected that one type of ServiceEndpoint has a separate list while the others do not.
3. A single list makes it harder to run checks like "Does the node define at least one service point?". These checks will probably be implemented on the server and in all SDKs.

I agree with @steven-sheehy's observation that we should not rely on port ranges to determine if an endpoint is secured. How about adding a boolean flag in ServiceEndpoint?

This is exactly what the current HIP text does (aside from eliding the repeated keyword, which was requested during technical review, as we do not expect to have more than one proxy per node), prior to this update.

We probably also need to think about whether we expect to have more node types in the future. Already the Node object is very specific to consensus as currently implemented, and would be difficult to adjust to handle, for instance, a split between consensus and execution, or the addition of Block Nodes to the address book.

[netopyr]

This is exactly what the current HIP text does (aside from eliding the repeated keyword, which was requested during technical review, as we do not expect to have more than one proxy per node), prior to this update.

I know. I think the new proposal is worse than the current HIP.

We probably also need to think about whether we expect to have more node types in the future. Already the Node object is very specific to consensus as currently implemented, and would be difficult to adjust to handle, for instance, a split between consensus and execution, or the addition of Block Nodes to the address book.

I do not see how the split between consensus and execution would influence the transactions. For block nodes, we probably want specialized transaction types. I do not see much commonality between consensus nodes and block nodes.

[bugbytesinc]

In general this seems to be bad architecture to require nodes that only communicate via gRPC to maintain a list of endpoints that they do not control, nor can validate and attest to their correctness. If the problem is due to SDK design, the correction should be mitigated in that tech stack not the consensus node protocol.