Showing Local Addresses in Node Window #626

jadijadi · 2022-06-29T13:56:10Z

This change adds a new row to the Node Window (debugwindow.ui)
under the Network section which shows the LocalAddresses.

fixes #564

jadijadi · 2022-06-29T14:16:48Z

I've got this from the linter:

he locale dependent function std::to_string(...) appears to be used:
src/qt/rpcconsole.cpp:        localAddresses += item.first.ToString() + ":" + std::to_string(item.second.nPort) + ", ";

Unnecessary locale depedence can cause bugs that are very tricky to isolate and fix. Please avoid using locale dependent functions if possible.

Not sure how I should convert from int to string correctly. Any advice?

jarolrod

concept ack

full diff:

diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
index 01989b5a0..080f9b979 100644
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -961,15 +961,16 @@ void RPCConsole::updateNetworkState()
     ui->numberOfConnections->setText(connections);

     // localAddressess
-    std::string localAddresses="";
+    QString local_addresses;
     LOCK(g_maplocalhost_mutex);
     for (const std::pair<const CNetAddr, LocalServiceInfo> &item : mapLocalHost)
-        localAddresses += item.first.ToString() + ":" + std::to_string(item.second.nPort) + ", ";
-    if (localAddresses.length() > 2)
-        localAddresses[localAddresses.length()-2] = 0;
+        local_addresses += QString::fromStdString(item.first.ToString()) + ":" + QString::number(item.second.nPort) + ", ";
+    if (local_addresses.length() > 2)
+        local_addresses[local_addresses.length()-2] = 0;
     else
-        localAddresses = "None";
-    ui->localAddresses->setText(QString::fromStdString(localAddresses));
+        //: Signals to the user that they do not have any local addresses.
+        local_addresses = tr("None");
+    ui->localAddresses->setText(local_addresses);

 }

src/qt/rpcconsole.cpp

furszy

Aside from the code comments, there is an important topic to mention:

Let's not access fields from the net layer in a GUI widget/view, the sources architecture is (should be) widget -> model -> interface -> backend.

Better to create the proper flow to access the information that you are looking for:
rpcconsole -> clientModel -> node interface -> net layer.

In this way, the field access is encapsulated, layers responsibilities are respected and the same function can be used, and tested, from other user facing end points like the RPC server equally.

src/qt/forms/debugwindow.ui

src/qt/rpcconsole.cpp

furszy · 2022-06-30T18:28:24Z

src/qt/rpcconsole.cpp

+    std::string localAddresses="";
+    LOCK(g_maplocalhost_mutex);
+    for (const std::pair<const CNetAddr, LocalServiceInfo> &item : mapLocalHost)
+        localAddresses += item.first.ToString() + ":" + std::to_string(item.second.nPort) + ", ";
+    if (localAddresses.length() > 2)
+        localAddresses[localAddresses.length()-2] = 0;
+    else
+        localAddresses = "None";


Would suggest to write it differently:

Suggested change

std::string localAddresses="";

LOCK(g_maplocalhost_mutex);

for (const std::pair<const CNetAddr, LocalServiceInfo> &item : mapLocalHost)

localAddresses += item.first.ToString() + ":" + std::to_string(item.second.nPort) + ", ";

if (localAddresses.length() > 2)

localAddresses[localAddresses.length()-2] = 0;

else

localAddresses = "None";

std::string localAddresses = "";

LOCK(g_maplocalhost_mutex);

for (auto it = mapLocalHost.begin(); it != mapLocalHost.end();) {

localAddresses += it->first.ToString() + ":" + std::to_string(it->second.nPort);

if (++it != mapLocalHost.end()) localAddresses += ", ";

}

if (localAddresses.empty()) localAddresses = "None";

This is a review on an earlier state of the PR, the following would be the appropriate diff to express your idea:

diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 386b9373c..a249d7895 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -964,13 +964,12 @@ void RPCConsole::updateNetworkState() // localAddressess QString local_addresses; LOCK(g_maplocalhost_mutex); - for (const std::pair<const CNetAddr, LocalServiceInfo> &item : mapLocalHost) - local_addresses += QString::fromStdString(item.first.ToString()) + ":" + QString::number(item.second.nPort) + ", "; - if (local_addresses.length() > 2) - local_addresses[local_addresses.length()-2] = 0; - else - // Signals to the user that they do not have any local addresses. - local_addresses = tr("None"); + for (auto it = mapLocalHost.begin(); it != mapLocalHost.end();) { + local_addresses += QString::fromStdString(it->first.ToString()) + ":" + QString::number(it->second.nPort) + ", "; + if (++it != mapLocalHost.end()) local_addresses += ", "; + } + //: Signals to the user that they do not have any local addresses. + if (local_addresses.isEmpty()) local_addresses = tr("None"); ui->localAddresses->setText(local_addresses); }

I actually pointed to the previous state on purpose. My main suggestion was to encapsulate the mutex and mapLocalHost access in the backend, which is Qt/GUI independent. So cannot use QString from there. Need to craft a plain std::string, then convert it to QString once it passes through the model class or directly in the widget.

Dear @furszy thanks for the valuable input. Applied most of them except this encapsulation part. I understand your rational and I think it's a better design. But since the number of connections are also calculated here, I did the same with Local Address.
Since I'm leaving for a 10 day off the grid trip, I submitted a new commit (squashed) with these chagnes:

removed the extra new line

used your better loop for ", " additions

removed unnecessary comments (kept the one from @jarolrod)

Thanks @jarolrod for the inputs and fixes and reviews.

Dear @furszy thanks for the valuable input. Applied most of them except this encapsulation part. I understand your rational and I think it's a better design. But since the number of connections are also calculated here, I did the same with Local Address.

The connections aren't calculated here (on the widget, as you are doing for the external addresses). It's encapsulated inside the connections manager. The flow is the following one: widget -> model -> node -> conman.

updateNetworkState calls clientModel->getNumConnections which calls the interface node.getNodeCount, which calls the connection manager.

Let's try to respect the different layers, encapsulating access where is possible, to get cleaner code to work with. (If you need a hand with it, feel very welcome to ping me on IRC)

Thanks. Did the flow as you said. Now we are querying layer by layer. Please comment if it still needs improvement. I decided to pass the std::map<CNetAddr, LocalServiceInfo> to keep it flexible enough for different use cases; for example in this specific usecase (showing IPs in GUI), i was able to do check for "IsI2P" on CNetAddrs.

src/qt/rpcconsole.cpp

shaavan

Concept ACK

I agree with showing the Local Address on the Information Tab.
I successfully compiled the PR on Ubuntu 22.04 with Qt version 5.15.3.
I was able to verify that the Local Address option is displayed correctly on the Information Tab.

Screenshot:

Currently, I am figuring out how to add local addresses to check if they are displayed correctly. It would be awesome if someone could point towards documentation on how to do so.

shaavan · 2022-07-05T12:36:45Z

src/qt/forms/debugwindow.ui

@@ -249,6 +249,29 @@
        </widget>
       </item>
       <item row="9" column="0">
+        <widget class="QLabel" name="label_14">


nit:

Suggested change

<widget class="QLabel" name="label_14">

<widget class="QLabel" name="labelLocalAddresses">

thanks for the checks & ACK @shaavan . I kept the label_14 in accordance in most of other fields which had the same pattern.

regarding the listening port, you can test with something like (for testnet):

testnet=1 blocksonly=1 [test] #prune=1000 # this has no effect during syncing externalip=100.200.100.10 rpcbind=127.0.0.1 rpcallowip=0.0.0.0/0 rpcuser=user rpcpassword=pass

as your bitcoin.conf to force a listening IP.

shaavan

ACK c47f01b

Continuing on my previous review:

I was able to add local addresses successfully and tested that those were displayed perfectly on the GUI under the Local Address option.

Method of testing:

I added the following options in the bitcoin.conf file.

[signet]
externalip=100.200.100.11
externalip=100.200.100.10
externalip=100.200.100.12

The IP addresses, along with the port number, appeared in the GUI.

Screenshot:

Note: The IP:port appears in the sorted order irrespective of the order provided in the bitcoin.conf file. This behavior is the same as the -netinfo RPC command.

Screenshot:

Thanks, @jadijadi, for the help on adding the listening ports!

jarolrod

Code Review ACK c47f01b

Some nits

jarolrod · 2022-08-01T04:22:56Z

src/qt/forms/debugwindow.ui

@@ -249,6 +249,29 @@
        </widget>
       </item>
       <item row="9" column="0">
+        <widget class="QLabel" name="label_14">
+         <property name="text">
+          <string>Local Address</string>


we could add a translator comment here by adding in the extracomment field:

Suggested change

<string>Local Address</string>

<string extracomment="nice translator comment">Local Address</string>

While here, if there's more than one local address; I guess this should take the plural form, no?

Or could be just changed to "Local Address(es)" maybe?

DrahtBot · 2023-03-07T17:40:46Z

qt/rpcconsole.cpp: In member function ‘void RPCConsole::updateNetworkState()’:
qt/rpcconsole.cpp:965:61: error: ‘const class CNetAddr’ has no member named ‘ToString’; did you mean ‘ToStringAddr’?
         local_addresses += QString::fromStdString(it->first.ToString()) + ":" + QString::number(it->second.nPort);
                                                             ^~~~~~~~
                                                             ToStringAddr

hebasto · 2023-03-27T14:20:53Z

Please rebase due to the conflict with the current master branch (b968424).

DrahtBot · 2023-03-27T14:21:44Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	RandyMcMillan, pablomartin4btc, kristapsk
Stale ACK	shaavan, jarolrod

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

RandyMcMillan · 2023-03-27T14:42:33Z

Concept ACK

Will test on macos x86/Arm64 today

luke-jr · 2023-07-25T20:22:03Z

src/qt/rpcconsole.cpp

+    LOCK(g_maplocalhost_mutex);
+    for (auto it = mapLocalHost.begin(); it != mapLocalHost.end();) {
+        local_addresses += QString::fromStdString(it->first.ToString()) + ":" + QString::number(it->second.nPort);
+        if (++it != mapLocalHost.end()) local_addresses += ", ";


Kinda ugly to do the increment here. Maybe make the loop normal, and check for it != mapLocalHost.begin() at the start of it?

changed to a normal loop and checking for .begin().

pablomartin4btc

Concept ACK and tested c47f01b

Thanks @furszy for emphasizing the importance of a good code design and architectural integrity. These principles are crucial for the long-term maintainability and scalability of our project. I totally agree with his code change recommendation in terms of the flow/ pattern to follow.

In terms of the UI itself, I'd use the plural form "Local Addresses", as @jarolrod also pointed it out, to be consistent with the getnetworkinfo rpc command output.

kristapsk · 2023-09-22T00:30:48Z

Concept ACK, kinda works.

Minor nit noticed - could avoid adding ":0" as a port for I2P addresses.

kristapsk · 2023-09-22T14:35:29Z

Another thing - list is currently comma separated, without wrapping. Wouldn't it be too long if somebody has IPv4, IPv6, CJDNS, Tor and I2P addresses at the same time?

hebasto · 2024-05-15T17:01:51Z

This PR still have a couple of unaddressed comments: #626 (comment) and #626 (comment).

@jadijadi Are you still working on this?

jadijadi · 2024-05-18T08:30:35Z

This PR still have a couple of unaddressed comments: #626 (comment) and #626 (comment).

@jadijadi Are you still working on this?

Sorry for forgetting this... will check and comment soon

jadijadi · 2024-05-22T06:22:49Z

Concept ACK, kinda works.

Minor nit noticed - could avoid adding ":0" as a port for I2P addresses.

thanks for the suggestion, did this.

jadijadi · 2024-05-22T06:42:13Z

Another thing - list is currently comma separated, without wrapping. Wouldn't it be too long if somebody has IPv4, IPv6, CJDNS, Tor and I2P addresses at the same time?

Thanks for this point, it is fixed and the widget worswraps and extends when needed.

jadijadi · 2024-05-22T08:13:15Z

This PR still have a couple of unaddressed comments: #626 (comment) and #626 (comment).

Dear @hebasto , addressed all issues, updated the code & replied to queries. Thanks for the follow up.

furszy

thanks for the encapsulation jadijadi.

furszy · 2024-05-22T14:56:51Z

src/interfaces/node.h

+    //! Get num blocks.
+    virtual std::map<CNetAddr, LocalServiceInfo> getLocalAddresses() = 0;


wrong description

furszy · 2024-05-22T14:58:57Z

src/node/interfaces.cpp

+        std::map<CNetAddr, LocalServiceInfo> empty;
+
+        return m_context->connman ? m_context->connman->getLocalAddresses() : empty;


Suggested change

std::map<CNetAddr, LocalServiceInfo> empty;

return m_context->connman ? m_context->connman->getLocalAddresses() : empty;

return m_context->connman ? m_context->connman->getLocalAddresses() : {};

Thanks for all the support and patient. Applying them. The {} was also my first try but I got:
error: initializer list cannot be used on the right hand side of operator ':'
Now I changed it to

if (m_context->connman) return m_context->connman->getLocalAddresses(); else return {};

to prevent that ugly empty variable of mine.

furszy · 2024-05-22T15:07:23Z

src/qt/rpcconsole.cpp

+
+    QString local_addresses;
+    std::map<CNetAddr, LocalServiceInfo> mapHosts = clientModel->getLocalAddresses();
+    for (auto it = mapHosts.begin(); it != mapHosts.end(); it++) {
+        if (it != mapHosts.begin()) local_addresses += ", ";
+        if (it->first.IsI2P())
+            local_addresses += QString::fromStdString(it->first.ToStringAddr());
+        else
+            local_addresses += QString::fromStdString(it->first.ToStringAddr()) + ":" + QString::number(it->second.nPort);
+    }


Maybe?

QString local_addresses; std::map<CNetAddr, LocalServiceInfo> hosts = clientModel->getLocalAddresses(); for (const auto& [addr, info] : hosts) { local_addresses += QString::fromStdString(it->first.ToStringAddr()); if (!addr.IsPrivacyNet()) local_addresses += ":" + QString::number(it->second.nPort); local_addresses += ", "; } local_addresses.chop(2); // remove last ", "

This is more beautiful and understandable. Replaced the it->first & it->second with addr & info:

for (const auto& [addr, info] : hosts) { local_addresses += QString::fromStdString(addr.ToStringAddr()); if (!addr.IsPrivacyNet()) local_addresses += ":" + QString::number(info.nPort); local_addresses += ", "; } local_addresses.chop(2); // remove last ", "

luke-jr · 2024-05-22T23:59:12Z

src/interfaces/node.h

@@ -168,6 +168,9 @@ class Node
    //! Get num blocks.
    virtual int getNumBlocks() = 0;

+    //! Get local addresses.
+    virtual std::map<CNetAddr, LocalServiceInfo> getLocalAddresses() = 0;


nit

Suggested change

virtual std::map<CNetAddr, LocalServiceInfo> getLocalAddresses() = 0;

virtual std::map<CNetAddr, LocalServiceInfo> getNetLocalAddresses() = 0;

Agree with this. Will change because "LocalAddress" can be confusing in BTC context.

luke-jr · 2024-05-23T00:05:54Z

src/qt/rpcconsole.cpp

+    std::map<CNetAddr, LocalServiceInfo> hosts = clientModel->getLocalAddresses();
+    for (const auto& [addr, info] : hosts) {
+        local_addresses += QString::fromStdString(addr.ToStringAddr());
+        if (!addr.IsPrivacyNet()) local_addresses += ":" + QString::number(info.nPort);


AFAIK Tor hidden services do still have port numbers

luke-jr · 2024-05-23T00:10:02Z

Also, please split the interfaces changes into a different commit than the GUI changes.

luke-jr · 2024-05-23T01:19:55Z

Might be a good improvement to cherry-pick e4fddc9f395819d0bdf74003676aa563929756fa and use it to update; in testing, I found myself watching for None to change, but it never did because the string isn't updated.

Contributes to bitcoin-core#564 by providing an interface for mapLocalHost through net -> node interface -> clientModel. Later this value can be read by GUI to show the local addresses.

jadijadi · 2024-05-23T14:22:22Z

Also, please split the interfaces changes into a different commit than the GUI changes.

did that. cleaner and atomic. thanks.

luke-jr · 2024-05-23T15:21:48Z

did that. cleaner and atomic. thanks.

Hmm, it's not that way in your new push tho

jadijadi · 2024-05-23T16:48:13Z

did that. cleaner and atomic. thanks.

Hmm, it's not that way in your new push tho

Ouch... pushed. thanks.

luke-jr · 2024-05-23T23:11:32Z

Somehow you introduced a typo in the commit message >_<

Adds a new row to the Node Window (debugwindow.ui) under the Network section which shows the LocalAddresses. fixes bitcoin-core#564

jadijadi · 2024-05-24T06:18:59Z

Somehow you introduced a typo in the commit message >_<

Oh.. sorry. fixed and added a spellchecker.

jarolrod added the UX All about "how to get things done" label Jun 29, 2022

jarolrod reviewed Jul 1, 2022

View reviewed changes

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved

jadijadi force-pushed the issue564-add-localaddress branch from c21adfa to e3c2e03 Compare July 1, 2022 05:46

furszy reviewed Jul 1, 2022

View reviewed changes

jadijadi force-pushed the issue564-add-localaddress branch from e3c2e03 to c47f01b Compare July 4, 2022 07:58

shaavan reviewed Jul 5, 2022

View reviewed changes

shaavan approved these changes Jul 9, 2022

View reviewed changes

hebasto changed the title ~~gui: Showing Local Addresses in Node Window~~ Showing Local Addresses in Node Window Jul 10, 2022

jarolrod reviewed Aug 1, 2022

View reviewed changes

hebasto added the Needs rebase label Mar 27, 2023

DrahtBot removed the Needs rebase label Mar 27, 2023

luke-jr reviewed Jul 25, 2023

View reviewed changes

pablomartin4btc reviewed Sep 19, 2023

View reviewed changes

DrahtBot mentioned this pull request Jan 21, 2024

refactor: Readable widget naming for debug window #790

Closed

jadijadi force-pushed the issue564-add-localaddress branch from c47f01b to c3b5638 Compare May 22, 2024 06:51

DrahtBot added the CI failed label May 22, 2024

furszy reviewed May 22, 2024

View reviewed changes

jadijadi force-pushed the issue564-add-localaddress branch from c3b5638 to 56bdeb2 Compare May 22, 2024 16:13

luke-jr reviewed May 23, 2024

View reviewed changes

net: Providing an interface for mapLocalHost

ca35d47

Contributes to bitcoin-core#564 by providing an interface for mapLocalHost through net -> node interface -> clientModel. Later this value can be read by GUI to show the local addresses.

jadijadi force-pushed the issue564-add-localaddress branch from 56bdeb2 to 283c855 Compare May 23, 2024 14:19

jadijadi force-pushed the issue564-add-localaddress branch from 283c855 to 2116e4a Compare May 23, 2024 16:45

Showing local addresses on the Node Window

ea576b6

Adds a new row to the Node Window (debugwindow.ui) under the Network section which shows the LocalAddresses. fixes bitcoin-core#564

jadijadi force-pushed the issue564-add-localaddress branch from 2116e4a to ea576b6 Compare May 24, 2024 06:17

	<widget class="QLabel" name="label_14">
	<widget class="QLabel" name="labelLocalAddresses">

	<string>Local Address</string>
	<string extracomment="nice translator comment">Local Address</string>

		//! Get num blocks.
		virtual std::map<CNetAddr, LocalServiceInfo> getLocalAddresses() = 0;

		std::map<CNetAddr, LocalServiceInfo> empty;

		return m_context->connman ? m_context->connman->getLocalAddresses() : empty;

Showing Local Addresses in Node Window #626

Are you sure you want to change the base?

Showing Local Addresses in Node Window #626

Conversation

jadijadi commented Jun 29, 2022 • edited

jadijadi commented Jun 29, 2022

jarolrod left a comment

Choose a reason for hiding this comment

furszy left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy Jul 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furszy Jul 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaavan left a comment

Choose a reason for hiding this comment

jarolrod left a 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

DrahtBot commented Mar 7, 2023

hebasto commented Mar 27, 2023

DrahtBot commented Mar 27, 2023 • edited

Reviews

Conflicts

RandyMcMillan commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablomartin4btc left a comment • edited

Choose a reason for hiding this comment

kristapsk commented Sep 22, 2023

kristapsk commented Sep 22, 2023 • edited

hebasto commented May 15, 2024

jadijadi commented May 18, 2024 • edited

jadijadi commented May 22, 2024

jadijadi commented May 22, 2024

jadijadi commented May 22, 2024

furszy left a 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

luke-jr commented May 23, 2024

luke-jr commented May 23, 2024

jadijadi commented May 23, 2024

luke-jr commented May 23, 2024

jadijadi commented May 23, 2024

luke-jr commented May 23, 2024

jadijadi commented May 24, 2024

jadijadi commented Jun 29, 2022 •

edited

furszy left a comment •

edited

furszy Jul 1, 2022 •

edited

furszy Jul 6, 2022 •

edited

DrahtBot commented Mar 27, 2023 •

edited

pablomartin4btc left a comment •

edited

kristapsk commented Sep 22, 2023 •

edited

jadijadi commented May 18, 2024 •

edited