[P2P] Refactor/consolidate P2P modules #576
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,6 +76,10 @@ spec: | |
| # Any host that is visible and connected to the cluster can be arbitrarily selected as the RPC host | ||
| - name: RPC_HOST | ||
| value: v1-validator001 | ||
| # TECHDEBT(#678): debug client requires hostname to participate | ||
| # in P2P networking. | ||
| - name: POCKET_P2P_HOSTNAME | ||
| value: "127.0.0.1" | ||
|
Comment on lines
+81
to
+82
There was a problem hiding this comment. I added this to resolve this error when using the debug cli in the tilt localnet (however, the value isn't actually used by the debug CLI): I read this as, it tried to resolve DNS for an empty string I hypothesize that the reason this is necessary in tilt and not in docker compose is because the config manifest has - name: POCKET_P2P_IS_EMPTY_CONNECTION_TYPE
value: "true"(Thoughts on this one @okdas?) There was a problem hiding this comment.
There was a problem hiding this comment. @bryanchriswhite I think you don't see that behavior on docker-compose because by default CLI uses pocket/app/client/cli/debug.go Line 80 in 39d16cd Which has node1.consensus set as a hostname.
I need to look deeper to figure out why it tries to resolve the empty host string when the value is I think requiring such parameters (hostname, Postgres, etc.) is counterintuitive when the user only needs to use CLI. In the current v0 world, users often use CLI to query pocket RPC endpoints to get the latest height, information about nodes, etc. We definitely should not require unnecessary configs. I'll see if such a ticket exists to add more information or create a new one tomorrow. There was a problem hiding this comment. @okdas Apologies for any confusion, to be clear, setting There was a problem hiding this comment. Comment added: # TECHDEBT(#678): debug client requires hostname to participate
# in P2P networking.
- name: POCKET_P2P_HOSTNAME
value: "127.0.0.1"
There was a problem hiding this comment.
👍 |
||
| volumeMounts: | ||
| # IMPROVE: should probably go in /etc/pocket and have Viper read from there as a default path | ||
| - mountPath: /var/pocket/config | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,6 @@ data: | |
| { | ||
| "root_directory": "/go/src/github.com/pocket-network", | ||
| "private_key": "", | ||
| "consensus": { | ||
| "max_mempool_bytes": 500000000, | ||
| "pacemaker_config": { | ||
|
|
@@ -36,6 +35,7 @@ data: | |
| }, | ||
| "p2p": { | ||
| "port": 42069, | ||
| "hostname": "", | ||
|
There was a problem hiding this comment. Rationale
Is there an issue for this / should there be?There was a problem hiding this comment. Nope, just add a comment with the rationale There was a problem hiding this comment. 🤔 Unfortunately I don't think it's possible to add a comment inline with the JSON as that would produce invalid JSON. Comments were introduced in JSON5, which has been slow to catch on 😅 This seems to be fairly conventional at this point as there are other env vars which are being set to empty string, presumably for the same reason. Perhaps this would be better represented in the docs? |
||
| "use_rain_tree": true, | ||
| "is_empty_connection_type": false, | ||
| "private_key": "0ca1a40ddecdab4f5b04fa0bfed1d235beaa2b8082e7554425607516f0862075dfe357de55649e6d2ce889acf15eb77e94ab3c5756fe46d3c7538d37f27f115e", | ||
|
|
||
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.
NB: While
mod's interface does define#Start(), it does not define other members ofmodules.P2Pmodule(e.g.#Send()) which are called on the global varp2pModin other functions in this module.