Showing Local Addresses in Node Window #626

jadijadi commented at 1:56 PM on June 29, 2022: contributor

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

fixes #564

jarolrod added the label UX on Jun 29, 2022

jadijadi commented at 2:16 PM on June 29, 2022: contributor

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?

in src/qt/forms/debugwindow.ui:264 in c21adfa620 outdated

 259 | +        <widget class="QLabel" name="localAddresses">
 260 | +         <property name="cursor">
 261 | +          <cursorShape>IBeamCursor</cursorShape>
 262 | +         </property>
 263 | +         <property name="text">
 264 | +          <string>N/A</string>

furszy commented at 6:04 PM on June 30, 2022:

missing notr="true" property

jadijadi commented at 6:33 AM on July 4, 2022:

thanks. fixed this. will push.

in src/qt/rpcconsole.cpp:953 in c21adfa620 outdated

 948 | @@ -949,6 +949,7 @@ void RPCConsole::message(int category, const QString &message, bool html)
 949 |  
 950 |  void RPCConsole::updateNetworkState()
 951 |  {
 952 | +    // numberOfConnections

furszy commented at 6:06 PM on June 30, 2022:

I don't see this comment helpful

jadijadi commented at 6:33 AM on July 4, 2022:

You are right. Variable names are self explanatory. will remove.

in src/qt/rpcconsole.cpp:971 in c21adfa620 outdated

 966 | +    for (const std::pair<const CNetAddr, LocalServiceInfo> &item : mapLocalHost)
 967 | +        localAddresses += item.first.ToString() + ":" + std::to_string(item.second.nPort) + ", ";
 968 | +    if (localAddresses.length() > 2)
 969 | +        localAddresses[localAddresses.length()-2] = 0;
 970 | +    else
 971 | +        localAddresses = "None";

furszy commented at 6:28 PM on June 30, 2022:

Would suggest to write it differently:

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";

jarolrod commented at 3:35 PM on July 1, 2022:

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);
 }

furszy commented at 5:09 PM on July 1, 2022:

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.

jadijadi commented at 9:51 AM on July 4, 2022:

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.

furszy commented at 11:44 PM on July 6, 2022:

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)

jadijadi commented at 6:45 AM on May 22, 2024:

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.

in src/qt/rpcconsole.cpp:964 in c21adfa620 outdated

 958 | @@ -958,6 +959,18 @@ void RPCConsole::updateNetworkState()
 959 |      }
 960 |  
 961 |      ui->numberOfConnections->setText(connections);
 962 | +
 963 | +    // localAddressess
 964 | +    std::string localAddresses="";

jarolrod commented at 3:04 AM on July 1, 2022:

you could just make this a QString, then item.first.ToString() -> QString::fromStdString(item.first.ToString())

jarolrod commented at 3:05 AM on July 1, 2022:

Should rename from localAddresses -> local_addresses to keep in line with our coding style

in src/qt/rpcconsole.cpp:967 in c21adfa620 outdated

 958 | @@ -958,6 +959,18 @@ void RPCConsole::updateNetworkState()
 959 |      }
 960 |  
 961 |      ui->numberOfConnections->setText(connections);
 962 | +
 963 | +    // localAddressess
 964 | +    std::string localAddresses="";
 965 | +    LOCK(g_maplocalhost_mutex);
 966 | +    for (const std::pair<const CNetAddr, LocalServiceInfo> &item : mapLocalHost)
 967 | +        localAddresses += item.first.ToString() + ":" + std::to_string(item.second.nPort) + ", ";

jarolrod commented at 3:08 AM on July 1, 2022:

to fix the linter issue arrising from std::to_string(item.second.nPort), and keeping in mind that local_addresses is now a QString:

        local_addresses += QString::fromStdString(item.first.ToString()) + ":" + QString::number(item.second.nPort) + ", ";

QString::number fixes the linter issue, from Qt Docs: "The formatting always uses QLocale::C, i.e., English/UnitedStates"

jadijadi commented at 5:30 AM on July 1, 2022:

Thank you very much. Greatly appreciate. Will do the changes & will commit.

jarolrod commented at 3:09 AM on July 1, 2022: member

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);

 }

jadijadi force-pushed on Jul 1, 2022

in src/qt/rpcconsole.cpp:52 in e3c2e0337e outdated

  48 | @@ -49,6 +49,7 @@
  49 |  
  50 |  #include <chrono>
  51 |  
  52 | +

furszy commented at 12:04 PM on July 1, 2022:

Can remove this extra added space

jadijadi commented at 6:34 AM on July 4, 2022:

done. will push.

furszy commented at 12:19 PM on July 1, 2022: member

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.

jadijadi force-pushed on Jul 4, 2022

in src/qt/forms/debugwindow.ui:252 in c47f01bf25 outdated

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

shaavan commented at 12:36 PM on July 5, 2022:

nit:

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

jadijadi commented at 6:46 AM on July 6, 2022:

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 commented at 12:37 PM on July 5, 2022: contributor

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 approved

shaavan commented at 2:18 PM on July 9, 2022: contributor

ACK c47f01bf25e1353b51c24b9b31260a539a394b1a

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: Screenshot from 2022-07-09 19-36-14

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!

hebasto renamed this:
~~gui: Showing Local Addresses in Node Window~~
Showing Local Addresses in Node Window
on Jul 10, 2022

in src/qt/forms/debugwindow.ui:254 in c47f01bf25 outdated

 248 | @@ -249,6 +249,29 @@
 249 |          </widget>
 250 |         </item>
 251 |         <item row="9" column="0">
 252 | +        <widget class="QLabel" name="label_14">
 253 | +         <property name="text">
 254 | +          <string>Local Address</string>

jarolrod commented at 4:22 AM on August 1, 2022:

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

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

jarolrod commented at 4:23 AM on August 1, 2022:

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

kristapsk commented at 12:29 AM on September 22, 2023:

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

jarolrod commented at 4:23 AM on August 1, 2022: member

Code Review ACK c47f01bf25e1353b51c24b9b31260a539a394b1a

Some nits

DrahtBot commented at 5:40 PM on March 7, 2023: contributor

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 commented at 2:20 PM on March 27, 2023: member

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

hebasto added the label Needs rebase on Mar 27, 2023

DrahtBot commented at 2:21 PM on March 27, 2023: contributor

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
ACK	pablomartin4btc, furszy
Concept ACK	RandyMcMillan, 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 commented at 2:42 PM on March 27, 2023: contributor

Concept ACK

Will test on macos x86/Arm64 today

DrahtBot removed the label Needs rebase on Mar 27, 2023

in src/qt/rpcconsole.cpp:966 in c47f01bf25 outdated

 957 | @@ -958,6 +958,17 @@ void RPCConsole::updateNetworkState()
 958 |      }
 959 |  
 960 |      ui->numberOfConnections->setText(connections);
 961 | +
 962 | +    QString local_addresses;
 963 | +    LOCK(g_maplocalhost_mutex);
 964 | +    for (auto it = mapLocalHost.begin(); it != mapLocalHost.end();) {
 965 | +        local_addresses += QString::fromStdString(it->first.ToString()) + ":" + QString::number(it->second.nPort);
 966 | +        if (++it != mapLocalHost.end()) local_addresses += ", ";

luke-jr commented at 8:22 PM on July 25, 2023:

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

jadijadi commented at 6:47 AM on May 22, 2024:

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

pablomartin4btc commented at 9:13 PM on September 19, 2023: contributor

Concept ACK and tested c47f01bf25e1353b51c24b9b31260a539a394b1a

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 commented at 12:30 AM on September 22, 2023: contributor

Concept ACK, kinda works.

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

kristapsk commented at 2:35 PM on September 22, 2023: contributor

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 commented at 5:01 PM on May 15, 2024: member

This PR still have a couple of unaddressed comments: #626 (review) and #626 (review). @jadijadi Are you still working on this?

jadijadi commented at 8:30 AM on May 18, 2024: contributor

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 commented at 6:22 AM on May 22, 2024: contributor

Concept ACK, kinda works.

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

thanks for the suggestion, did this.

jadijadi commented at 6:42 AM on May 22, 2024: contributor

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 force-pushed on May 22, 2024

jadijadi commented at 8:13 AM on May 22, 2024: contributor

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.

DrahtBot added the label CI failed on May 22, 2024

in src/interfaces/node.h:172 in c3b5638b75 outdated

 167 | @@ -168,6 +168,9 @@ class Node
 168 |      //! Get num blocks.
 169 |      virtual int getNumBlocks() = 0;
 170 |  
 171 | +    //! Get num blocks.
 172 | +    virtual std::map<CNetAddr, LocalServiceInfo> getLocalAddresses() = 0;

furszy commented at 2:56 PM on May 22, 2024:

wrong description

in src/node/interfaces.cpp:288 in c3b5638b75 outdated

 280 | @@ -281,6 +281,13 @@ class NodeImpl : public Node
 281 |          }
 282 |          return false;
 283 |      }
 284 | +    std::map<CNetAddr, LocalServiceInfo> getLocalAddresses() override
 285 | +    {
 286 | +        std::map<CNetAddr, LocalServiceInfo> empty;
 287 | +
 288 | +        return m_context->connman ? m_context->connman->getLocalAddresses() : empty;

furszy commented at 2:58 PM on May 22, 2024:

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

jadijadi commented at 4:08 PM on May 22, 2024:

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.

in src/qt/rpcconsole.cpp:985 in c3b5638b75 outdated

 982 | +        if (it != mapHosts.begin()) local_addresses += ", ";
 983 | +        if (it->first.IsI2P())
 984 | +            local_addresses += QString::fromStdString(it->first.ToStringAddr());
 985 | +        else
 986 | +            local_addresses += QString::fromStdString(it->first.ToStringAddr()) + ":" + QString::number(it->second.nPort);
 987 | +    }

furszy commented at 3:07 PM on May 22, 2024:

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 ", "

jadijadi commented at 4:10 PM on May 22, 2024:

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 ", "

furszy commented at 3:08 PM on May 22, 2024: member

thanks for the encapsulation jadijadi.

jadijadi force-pushed on May 22, 2024

in src/interfaces/node.h:172 in 56bdeb2aa0 outdated

 167 | @@ -168,6 +168,9 @@ class Node
 168 |      //! Get num blocks.
 169 |      virtual int getNumBlocks() = 0;
 170 |  
 171 | +    //! Get local addresses.
 172 | +    virtual std::map<CNetAddr, LocalServiceInfo> getLocalAddresses() = 0;

luke-jr commented at 11:59 PM on May 22, 2024:

nit

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

jadijadi commented at 1:07 PM on May 23, 2024:

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

in src/qt/rpcconsole.cpp:983 in 56bdeb2aa0 outdated

 974 | @@ -975,6 +975,18 @@ void RPCConsole::updateNetworkState()
 975 |      }
 976 |  
 977 |      ui->numberOfConnections->setText(connections);
 978 | +
 979 | +    QString local_addresses;
 980 | +    std::map<CNetAddr, LocalServiceInfo> hosts = clientModel->getLocalAddresses();
 981 | +    for (const auto& [addr, info] : hosts) {
 982 | +        local_addresses += QString::fromStdString(addr.ToStringAddr());
 983 | +        if (!addr.IsPrivacyNet()) local_addresses += ":" + QString::number(info.nPort);

luke-jr commented at 12:05 AM on May 23, 2024:

AFAIK Tor hidden services do still have port numbers

luke-jr commented at 12:10 AM on May 23, 2024: member

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

luke-jr commented at 1:19 AM on May 23, 2024: member

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.

jadijadi force-pushed on May 23, 2024

jadijadi commented at 2:22 PM on May 23, 2024: contributor

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

did that. cleaner and atomic. thanks.

luke-jr commented at 3:21 PM on May 23, 2024: member

did that. cleaner and atomic. thanks.

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

jadijadi force-pushed on May 23, 2024

jadijadi commented at 4:48 PM on May 23, 2024: contributor

did that. cleaner and atomic. thanks.

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

Ouch... pushed. thanks.

luke-jr commented at 11:11 PM on May 23, 2024: member

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

jadijadi force-pushed on May 24, 2024

jadijadi commented at 6:18 AM on May 24, 2024: contributor

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

Oh.. sorry. fixed and added a spellchecker.

jadijadi commented at 3:59 PM on July 3, 2024: contributor

@luke-jr May I kindly ask for a review on this PR?

hebasto commented at 4:00 PM on July 30, 2024: member

cc @furszy

in src/qt/clientmodel.h:16 in ca35d47318 outdated

  12 | @@ -13,6 +13,9 @@
  13 |  #include <sync.h>
  14 |  #include <uint256.h>
  15 |  
  16 | +#include <net.h>

furszy commented at 6:27 PM on July 30, 2024:

Could remove the need for adding this dependency here:

diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h
--- a/src/qt/clientmodel.h	(revision ea576b65592c23895f80c9116f881ff063253ff8)
+++ b/src/qt/clientmodel.h	(date 1722363555212)
@@ -13,7 +13,6 @@
 #include <sync.h>
 #include <uint256.h>
 
-#include <net.h>
 #include <netaddress.h>
 
 class BanTableModel;
@@ -22,6 +21,7 @@
 class PeerTableModel;
 class PeerTableSortProxy;
 enum class SynchronizationState;
+struct LocalServiceInfo;
 
 namespace interfaces {
 class Handler;

This will avoid including the backend net.h file in all the gui files that are already including clientmodel.h.

Ideally, net.h structs/classes shouldn't be accesible by the GUI widgets. But no prob, it seems we are already including it in rpcconsole.h too.

jadijadi commented at 1:50 PM on August 1, 2024:

Thanks @furszy , this looks better. Applied it and pushed the new version with not net.h and with the struct only; as you suggested.

luke-jr commented at 6:31 PM on August 1, 2024:

You changed the net.h stuff in the second commit, after adding it in the first

jadijadi commented at 7:20 AM on August 2, 2024:

thanks! rebased.

jadijadi force-pushed on Aug 1, 2024

furszy commented at 1:57 PM on August 1, 2024: member

utACK

Maybe add a QLabel hint describing what "local addresses" are for GUI users? (and/or use another term for them).

jadijadi commented at 2:36 PM on August 1, 2024: contributor

utACK

Maybe add a QLabel hint describing what "local addresses" are for GUI users? (and/or use another term for them).

Any suggestion? English is not my first language. Something like "Addresses where Bitcoin Core binds to"? On DataDir we have the tooltip on the value column. I'll also keep it like that.

DrahtBot removed the label CI failed on Aug 1, 2024

pablomartin4btc commented at 4:44 PM on August 1, 2024: contributor

Maybe add a QLabel hint describing what "local addresses" are for GUI users? (and/or use another term for them).

Any suggestion? English is not my first language. Something like "Addresses where Bitcoin Core binds to"? On DataDir we have the tooltip on the value column. I'll also keep it like that.

Perhaps any of these (or a combination):

Each local address broadcast by this node.
IP addresses of your local machine or network interface that the node uses to connect to the Bitcoin network
Network addresses that your Bitcoin node is currently using to communicate with other nodes.

pablomartin4btc commented at 5:01 PM on August 1, 2024: contributor

<details> <summary>re tACK bb5bb2001cf805301f56aa1ec15ed1035febab4b</summary>

</details>

DrahtBot requested review from shaavan on Aug 1, 2024

DrahtBot requested review from kristapsk on Aug 1, 2024

DrahtBot requested review from furszy on Aug 1, 2024

DrahtBot requested review from jarolrod on Aug 1, 2024

jadijadi force-pushed on Aug 2, 2024

jadijadi commented at 6:54 AM on August 2, 2024: contributor

Network addresses that your Bitcoin node is currently using to communicate with other nodes.

Thanks @pablomartin4btc . Added this as the tooltip & thanks for the tACK

net: Providing an interface for mapLocalHost

Contributes to #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.

a5d7aff867

Showing local addresses on the Node Window

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

fixes #564

189c987386

jadijadi force-pushed on Aug 2, 2024

pablomartin4btc approved

pablomartin4btc commented at 9:14 AM on August 2, 2024: contributor

re-ACK 189c987386a0da132d7ef076cdf539f9eb75fc3c

This has been fixed since my last review:

You changed the net.h stuff in the second commit, after adding it in the first

furszy commented at 1:10 PM on August 2, 2024: member

utACK 189c987

hebasto merged this on Aug 2, 2024

hebasto closed this on Aug 2, 2024

hebasto added this to the milestone 28.0 on Aug 2, 2024

bitcoin-core locked this on Aug 2, 2025