Fix debug rpc ports localnet #8583 #8866

ghost · 2023-04-01T05:11:14Z

Problem

This is only an issue on localnet and not production (mainnet, testnet) For more information: https://github.com//issues/8583

Solution

Hence, prefer having this fix within the frontend webserver itself. Alternatively, could have an optional approach for nodes to fetch rpc address from each peer. However, this introduces both complexity and potentially additional network bandwidth that will only be useful for localnet testing.

The code currently uses the assumption on the ports that the nodes will be started in the ranges:

Network address : 24567 + [0, numNodes - 1]
RPC address: 3030 + [0, numNodes - 1]
Reference: https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94

If this assumption is ever violated in future,
only the debug pages would break and can easily be fixed without affecting the actual core network itself.

Testing

Ran linter

npm run lint

Open both old and new debug pages and manually click on the links

cd nearcore
make debug
nearup run localnet --binary-path ~/Github/near/nearcore/target/debug/
open http://localhost:3030/debug

cd nearcore/tools/debug-ui
npm install
npm start
open http://localhost:3000/localhost:3030/cluster

<h1> Problem </h1> This is only an issue on localnet and not production (mainnet, testnet) Hence, prefer having this fix within the frontend webserver itself. Alternatively, could have an optional approach for nodes to fetch rpc address from each peer. However, this introduces both complexity and additional bandwidth that will only be useful for localnet testing. <h1> Solution </h1> The code currently uses the assumption on the ports that the nodes will be started in the ranges: - Network address : 24567 + [0, numNodes - 1] - RPC address: 3030 + [0, numNodes - 1] - Reference: https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94 If this assumption is ever violated in future, only the debug pages would break and can easily be fixed without affecting the actual core network itself. <h1> Testing </h1> - Ran linter ``` npm run lint ``` - Open both old and new debug pages and manually click on the links ``` cd nearcore make debug nearup run localnet --binary-path ~/Github/near/nearcore/target/debug/ open http://localhost:3030/debug cd nearcore/tools/debug-ui npm install npm start open http://localhost:3000/localhost:3030/cluster ```

nikurt · 2023-04-03T10:17:32Z

chain/jsonrpc/res/network_info.js

+    // Then, each subsequence neard process on the same machine will are strict increments of a fix constant from both ports
+    // peer_num should only be > 0 for nearup's localnet and 0 otherwise
+    // Reference to nearup's localnet port assumptions
+    // https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94


Please don't use nearup as reference.

This issue mainly addresses localnet configurations and this issue does not exist in mainnet/testnet/mocknet (any network that runs only 1 neard process per computer as it doesn't end up with clashes in port usage (3030 and 24567)).
What alternatives can I use as reference for localnet port configurations?

Something like that:

nearcore/chain/network/src/tcp.rs

Line 186 in cc38c88

guard.bind("[::1]:0".parse().unwrap()).unwrap();

The issue with nearup is that it's use is discouraged for anything other than betanet and localnet. Pagoda is going to stop using nearup for betanet. And recently neard localnet command received some care and the value of using nearup for localnet is even smaller.

Will remove any mention of nearup or links to it in code

nikurt · 2023-04-03T10:17:42Z

chain/jsonrpc/res/network_info.js

-function add_debug_port_link(peer_addr) {
+function add_debug_port_link(peer_network_addr) {
+    // Assume rpc port is always 3030 and network port is always 24567 for the first neard process on a machine
+    // Then, each subsequence neard process on the same machine will are strict increments of a fix constant from both ports


typo: "will are"

thanks, will fix grammar to 'will be' instead

nikurt · 2023-04-03T10:22:45Z

tools/debug-ui/src/utils.tsx

+    // https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94
+    // Reference to mainnet's assumption of network port
+    // https://github.com/near/nearcore/blob/e61da3d26e3614d3f6c1b669d31fca7a12d55296/chain/network/src/raw/connection.rs#L153
+    const rpc_port_assumption = 3030;


I'm surprised that the code needs to be duplicated.
But if it's actually the case, then fine to leave it as is.

I believe it's because the newer version is written in typescript whereas the older version is written in javascript. I believe we're aiming to deprecate the older version in future, hence will also remove any duplicated code then.

nikurt · 2023-04-03T14:43:46Z

chain/jsonrpc/res/network_info.js

+    // Then, each subsequence neard process on the same machine will are strict increments of a fix constant from both ports
+    // peer_num should only be > 0 for nearup's localnet and 0 otherwise
+    // Reference to nearup's localnet port assumptions
+    // https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94


Something like that:

nearcore/chain/network/src/tcp.rs

Line 186 in cc38c88

guard.bind("[::1]:0".parse().unwrap()).unwrap();

The issue with nearup is that it's use is discouraged for anything other than betanet and localnet. Pagoda is going to stop using nearup for betanet. And recently neard localnet command received some care and the value of using nearup for localnet is even smaller.

saketh-are

Thanks for looking into this @soonnear.

Were you able to look into including the debug port as part of the config?

Given that the network address is a modifiable part of the config: https://github.com/near/nearcore/blob/master/chain/network/src/config_json.rs#L82, we should avoid assuming that it is always 24567.

chain/jsonrpc/res/network_info.js

saketh-are · 2023-04-03T18:41:20Z

As discussed offline, because we don't know how a peer node’s debug port is configured (and even if such information were to be added to the config, we don't have access to other nodes' configs), we will move forward with some heuristics on the front-end to deduce it.

By default, assume that the debug RPC port is 3030
If a peer node has the same network address as the current node (excluding the port number), we detect a localnet situation. In this case, the neighbor's debug port is calculated from the expectation that the ith localnet node has network port 24567+i and debug port 3030+i.

saketh-are · 2023-04-03T18:50:20Z

chain/jsonrpc/res/network_info.js

+    // https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94
+    // Reference to mainnet's assumption of network port
+    // https://github.com/near/nearcore/blob/e61da3d26e3614d3f6c1b669d31fca7a12d55296/chain/network/src/raw/connection.rs#L153
+    rpc_port_assumption = 3030


style nit: I'd suggest naming constants in upper-case and using the word default rather than assumption. For example, DEFAULT_RPC_PORT here.

updated as suggested

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const

- Added conditional to only perform arithmetic for localnet nodes. - Remove any mention of nearup - use DEFAULT_UPPER_CASE for constants instead of lower_case_assumption - - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const - Remove link to 24567 as irrelevant (used for PING) - Include comment on how peer_rpc_address aren't shared in PeerInfo

ghost · 2023-04-04T13:21:23Z

Added doc as comment for this task
#8583 (comment)

saketh-are · 2023-04-04T14:00:29Z

chain/jsonrpc/res/network_info.js

-function add_debug_port_link(peer_addr) {
+function add_debug_port_link(peer_network_addr) {
+    // Default rpc port is 3030 and default network port is 24567 for the first neard process on a machine
+    // Then, each subsequence neard process on the same machine will be strict increments of a fix constant from both ports


nit: "subsequent"

"strict increments of a fix constant from both ports" is unclear. Do you think you can combine this sentence and the next one into a more simple statement? Something along the lines of "the ith node running on the same machine is typically assigned ports 24567+i and 3030+i".

I prefer not adding + i, although it is true, the code itself works for any constant c 24567+c , 3030+c so it's more general

Updated to 1 line in next commit

saketh-are · 2023-04-04T14:03:58Z

chain/jsonrpc/res/network_info.js

+    peer_num = 0;
+    localhost_ips = ["127.0.0.1", "localhost", "0.0.0.0"]
+    for (const localhost_ip of localhost_ips) {
+        if (peer_network_ip.includes(localhost_ip)) {


Isn't peer_network_ip always 127.0.0.1 in localnet?

No idea how IP works, if it is, the code doesn't run any slower as it's the first condition on the for loop.

If it is we could remove the for loop and just keep an if statement, but I'm not sure, so I'll leave it for now unless you're 100% positive it's always going to be 127.0.0.1

It is not a performance concern but rather a simplicity concern. Let's just check for 127.0.0.1.

alright, will update to just this check before merge, thanks!

<h1> Problem </h1> This is only an issue on localnet and not production (mainnet, testnet) Hence, prefer having this fix within the frontend webserver itself. Alternatively, could have an optional approach for nodes to fetch rpc address from each peer. However, this introduces both complexity and additional bandwidth that will only be useful for localnet testing. <h1> Solution </h1> The code currently uses the assumption on the ports that the nodes will be started in the ranges: - Network address : 24567 + [0, numNodes - 1] - RPC address: 3030 + [0, numNodes - 1] - Reference: https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94 If this assumption is ever violated in future, only the debug pages would break and can easily be fixed without affecting the actual core network itself. <h1> Testing </h1> - Ran linter ``` npm run lint ``` - Open both old and new debug pages and manually click on the links ``` cd nearcore make debug nearup run localnet --binary-path ~/Github/near/nearcore/target/debug/ open http://localhost:3030/debug cd nearcore/tools/debug-ui npm install npm start open http://localhost:3000/localhost:3030/cluster ```

- Added conditional to only perform arithmetic for localnet nodes. - Remove any mention of nearup - use DEFAULT_UPPER_CASE for constants instead of lower_case_assumption - - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const - Remove link to 24567 as irrelevant (used for PING) - Include comment on how peer_rpc_address aren't shared in PeerInfo

saketh-are · 2023-04-04T21:46:15Z

chain/jsonrpc/res/network_info.js

+function add_debug_port_link(peer_network_addr) {
+    // Each node running in a machine is assigned ports 24567 + peer_num and 3030 + peer_num, whereby peer_num is a whole number
+    // peer_rpc_address is not shared between peer nodes. Hence, it cannot be programmatically fetched.
+    // https://github.com/near/nearcore/blob/700ec29270f72f2e78a17029b4799a8228926c07/chain/network/src/network_protocol/peer.rs#L13-L19


Linking just the peer_info here is confusing without additional context. The reader needs deeper knowledge of the network crate to understand that the peer_info is what is shared between nodes. I would suggest just deleting this line, the above explanation that RPC addresses are not shared is sufficient.

Took me a few hours to figure what (and where in the code) exactly is exchanged between peer nodes is the peer info. Think this line would be helpful for anyone modifying this code in future or those trying to understand why the numbers were hacked in.

Also difficult for future developers to know if this comment is outdated, so this line allows them to view the master branch of that file to see if this comment of what exactly is being exchanged is still up to date. Hence, leaving this line in there.

The first page itself explains the definition of 'peer' of being other peer nodes, so understanding the context of peer info after that shouldn't be too difficult
https://near.github.io/nearcore/architecture/index.html

* Fix debug rpc ports localnet near#8583 <h1> Problem </h1> This is only an issue on localnet and not production (mainnet, testnet) Hence, prefer having this fix within the frontend webserver itself. Alternatively, could have an optional approach for nodes to fetch rpc address from each peer. However, this introduces both complexity and additional bandwidth that will only be useful for localnet testing. <h1> Solution </h1> The code currently uses the assumption on the ports that the nodes will be started in the ranges: - Network address : 24567 + [0, numNodes - 1] - RPC address: 3030 + [0, numNodes - 1] - Reference: https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94 If this assumption is ever violated in future, only the debug pages would break and can easily be fixed without affecting the actual core network itself. <h1> Testing </h1> - Ran linter ``` npm run lint ``` - Open both old and new debug pages and manually click on the links ``` cd nearcore make debug nearup run localnet --binary-path ~/Github/near/nearcore/target/debug/ open http://localhost:3030/debug cd nearcore/tools/debug-ui npm install npm start open http://localhost:3000/localhost:3030/cluster ``` * Updated typo from 'will be' to 'will are' * Address 1st code review comments - Added conditional to only perform arithmetic for localnet nodes. - Remove any mention of nearup - use DEFAULT_UPPER_CASE for constants instead of lower_case_assumption - - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const - Remove link to 24567 as irrelevant (used for PING) - Include comment on how peer_rpc_address aren't shared in PeerInfo * Updated comments to be more concise * Fix debug rpc ports localnet near#8583 <h1> Problem </h1> This is only an issue on localnet and not production (mainnet, testnet) Hence, prefer having this fix within the frontend webserver itself. Alternatively, could have an optional approach for nodes to fetch rpc address from each peer. However, this introduces both complexity and additional bandwidth that will only be useful for localnet testing. <h1> Solution </h1> The code currently uses the assumption on the ports that the nodes will be started in the ranges: - Network address : 24567 + [0, numNodes - 1] - RPC address: 3030 + [0, numNodes - 1] - Reference: https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94 If this assumption is ever violated in future, only the debug pages would break and can easily be fixed without affecting the actual core network itself. <h1> Testing </h1> - Ran linter ``` npm run lint ``` - Open both old and new debug pages and manually click on the links ``` cd nearcore make debug nearup run localnet --binary-path ~/Github/near/nearcore/target/debug/ open http://localhost:3030/debug cd nearcore/tools/debug-ui npm install npm start open http://localhost:3000/localhost:3030/cluster ``` * Address 1st code review comments - Added conditional to only perform arithmetic for localnet nodes. - Remove any mention of nearup - use DEFAULT_UPPER_CASE for constants instead of lower_case_assumption - - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const - Remove link to 24567 as irrelevant (used for PING) - Include comment on how peer_rpc_address aren't shared in PeerInfo - only check 127.0.0.1 for localnet to dive into localnet only code paths

walnut-the-cat · 2023-04-05T19:00:16Z

chain/jsonrpc/res/network_info.js

+    peer_rpc_port = DEFAULT_RPC_PORT + peer_num;
+    peer_rpc_address = "http://" + peer_network_addr.replace(/:.*/, ":") + peer_rpc_port + "/debug"


can these be const?

prolly, linter isn't set up for this part of code because it's getting deprecated. autolinter could probably add constants everywhere allowed as well.

walnut-the-cat · 2023-04-05T19:02:28Z

tools/debug-ui/src/utils.tsx

+    const peer_network_addr_array = peer_network_addr.split(":");
+    const peer_network_port = parseInt(peer_network_addr_array.pop() || '24567');


why not using DEFAULT_NETWORK_PORT instead of '24567'?

Also wonder if tsx takes the syntax peer_network_addr_array.pop() ?? '24567'

Ah good point, but code has been merged and I rather not push a full refactoring commit just for this.

git blame for this line would point to a refactoring commit instead of issue commit.

It's running on a client side with < 1TPS, no need to hyper optimize this code path.

* Fix debug rpc ports localnet #8583 <h1> Problem </h1> This is only an issue on localnet and not production (mainnet, testnet) Hence, prefer having this fix within the frontend webserver itself. Alternatively, could have an optional approach for nodes to fetch rpc address from each peer. However, this introduces both complexity and additional bandwidth that will only be useful for localnet testing. <h1> Solution </h1> The code currently uses the assumption on the ports that the nodes will be started in the ranges: - Network address : 24567 + [0, numNodes - 1] - RPC address: 3030 + [0, numNodes - 1] - Reference: https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94 If this assumption is ever violated in future, only the debug pages would break and can easily be fixed without affecting the actual core network itself. <h1> Testing </h1> - Ran linter ``` npm run lint ``` - Open both old and new debug pages and manually click on the links ``` cd nearcore make debug nearup run localnet --binary-path ~/Github/near/nearcore/target/debug/ open http://localhost:3030/debug cd nearcore/tools/debug-ui npm install npm start open http://localhost:3000/localhost:3030/cluster ``` * Updated typo from 'will be' to 'will are' * Address 1st code review comments - Added conditional to only perform arithmetic for localnet nodes. - Remove any mention of nearup - use DEFAULT_UPPER_CASE for constants instead of lower_case_assumption - - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const - Remove link to 24567 as irrelevant (used for PING) - Include comment on how peer_rpc_address aren't shared in PeerInfo * Updated comments to be more concise * Fix debug rpc ports localnet #8583 <h1> Problem </h1> This is only an issue on localnet and not production (mainnet, testnet) Hence, prefer having this fix within the frontend webserver itself. Alternatively, could have an optional approach for nodes to fetch rpc address from each peer. However, this introduces both complexity and additional bandwidth that will only be useful for localnet testing. <h1> Solution </h1> The code currently uses the assumption on the ports that the nodes will be started in the ranges: - Network address : 24567 + [0, numNodes - 1] - RPC address: 3030 + [0, numNodes - 1] - Reference: https://github.com/near/nearup/blob/0b9a7b60236f3164dd32677b6fa58c531a586200/nearuplib/localnet.py#L93-L94 If this assumption is ever violated in future, only the debug pages would break and can easily be fixed without affecting the actual core network itself. <h1> Testing </h1> - Ran linter ``` npm run lint ``` - Open both old and new debug pages and manually click on the links ``` cd nearcore make debug nearup run localnet --binary-path ~/Github/near/nearcore/target/debug/ open http://localhost:3030/debug cd nearcore/tools/debug-ui npm install npm start open http://localhost:3000/localhost:3030/cluster ``` * Address 1st code review comments - Added conditional to only perform arithmetic for localnet nodes. - Remove any mention of nearup - use DEFAULT_UPPER_CASE for constants instead of lower_case_assumption - - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const - Remove link to 24567 as irrelevant (used for PING) - Include comment on how peer_rpc_address aren't shared in PeerInfo - only check 127.0.0.1 for localnet to dive into localnet only code paths

ghost self-requested a review as a code owner April 1, 2023 05:11

ghost requested a review from nikurt April 1, 2023 05:11

ghost assigned saketh-are Apr 1, 2023

This was referenced Apr 1, 2023

Fix debug rpc ports localnet near/nearcore#8583 #8865

Closed

Fix debug rpc ports localnet near/nearcore#8583 #8864

Closed

nikurt approved these changes Apr 3, 2023

View reviewed changes

Soon Near and others added 2 commits April 3, 2023 10:12

Updated typo from 'will be' to 'will are'

00c506f

Merge branch 'master' into master

ff969a4

nikurt approved these changes Apr 3, 2023

View reviewed changes

ghost requested a review from saketh-are April 3, 2023 15:02

ghost assigned ghost and unassigned saketh-are Apr 3, 2023

saketh-are requested changes Apr 3, 2023

View reviewed changes

chain/jsonrpc/res/network_info.js Outdated Show resolved Hide resolved

saketh-are reviewed Apr 3, 2023

View reviewed changes

Soon Near and others added 3 commits April 4, 2023 08:40

Merge branch 'master' of ssh://github.com/soonnear/nearcore

4ae3cba

Merge branch 'master' into master

33b1611

ghost mentioned this pull request Apr 4, 2023

Network debug page assumes that debug RPC port is 3030 #8583

Closed

saketh-are approved these changes Apr 4, 2023

View reviewed changes

Soon Near added 6 commits April 4, 2023 13:18

Updated comments to be more concise

bf4b110

Merge branch 'master' of ssh://github.com/soonnear/nearcore

5165a27

Updated typo from 'will be' to 'will are'

3b74066

Updated comments to be more concise

6c1769d

saketh-are reviewed Apr 4, 2023

View reviewed changes

only check 127.0.0.1

c814a00

Merge branch 'master' of ssh://github.com/soonnear/nearcore

af95f18

ghost merged commit 136f5b0 into near:master Apr 4, 2023

walnut-the-cat reviewed Apr 5, 2023

View reviewed changes

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix debug rpc ports localnet #8583 #8866

Fix debug rpc ports localnet #8583 #8866

ghost commented Apr 1, 2023 •

edited by ghost

Loading

nikurt Apr 3, 2023

ghost Apr 3, 2023 •

edited by ghost

Loading

nikurt Apr 3, 2023

ghost Apr 4, 2023

nikurt Apr 3, 2023

ghost Apr 3, 2023

nikurt Apr 3, 2023

ghost Apr 3, 2023

nikurt Apr 3, 2023

saketh-are left a comment

saketh-are commented Apr 3, 2023 •

edited

Loading

saketh-are Apr 3, 2023

ghost Apr 4, 2023

ghost commented Apr 4, 2023

saketh-are Apr 4, 2023

ghost Apr 4, 2023

ghost Apr 4, 2023

saketh-are Apr 4, 2023

ghost Apr 4, 2023

ghost Apr 4, 2023

saketh-are Apr 4, 2023

ghost Apr 4, 2023

saketh-are Apr 4, 2023

ghost Apr 4, 2023

walnut-the-cat Apr 5, 2023

ghost Apr 5, 2023

walnut-the-cat Apr 5, 2023

ghost Apr 5, 2023 •

edited by ghost

Loading

		peer_rpc_port = DEFAULT_RPC_PORT + peer_num;
		peer_rpc_address = "http://" + peer_network_addr.replace(/:.*/, ":") + peer_rpc_port + "/debug"

		const peer_network_addr_array = peer_network_addr.split(":");
		const peer_network_port = parseInt(peer_network_addr_array.pop() \|\| '24567');

Fix debug rpc ports localnet #8583 #8866

Fix debug rpc ports localnet #8583 #8866

Conversation

ghost commented Apr 1, 2023 • edited by ghost Loading

Problem

Solution

Testing

Choose a reason for hiding this comment

ghost Apr 3, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saketh-are left a comment

Choose a reason for hiding this comment

saketh-are commented Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 5, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Apr 1, 2023 •

edited by ghost

Loading

ghost Apr 3, 2023 •

edited by ghost

Loading

saketh-are commented Apr 3, 2023 •

edited

Loading

ghost Apr 5, 2023 •

edited by ghost

Loading