Additional field to RPC getpeerinfo output: addrlocal #2929

Krellan commented at 8:16 AM on August 24, 2013: contributor

This simple patch gives user visibility to see the contents of the existing CNode::addrLocal member. No existing behavior is changed, this is read-only. If addrLocal is invalid (not filled in yet), the field will not be included in the output.

Diapolo commented at 1:33 PM on August 24, 2013: none

You should squash into one single commit.

Krellan commented at 9:18 PM on August 24, 2013: contributor

Thanks for the feedback! Done. Nicely squashed now.

jgarzik commented at 2:30 AM on August 25, 2013: contributor

ACK the code change... what is the use case? How is this useful?

Krellan commented at 9:46 AM on August 25, 2013: contributor

Thanks! Use case is to help network troubleshooting. In my example, I was having a tough time because I couldn't tell if my external IP address was being seen correctly by the outside world. Bitcoin exchanges this information during the "version" command handling, and stores it in the "addrLocal" member of CNode, but doesn't expose this to the user, so unless the user is lucky and sees the debug text scroll by at the moment a connection is made, the user won't be able to easily learn this information.

Also, it might be nice in the future to have a table of network connections in Bitcoin-Qt or something like that, and this would make it easy to have both local and remote addresses appear in the table (for completeness).

in src/net.cpp:None in 83fa518212 outdated

 609 | @@ -610,6 +610,10 @@ void CNode::copyStats(CNodeStats &stats)
 610 |      X(nSendBytes);
 611 |      X(nRecvBytes);
 612 |      stats.fSyncNode = (this == pnodeSync);
 613 | +    
 614 | +    // Leave string empty if addrLocal invalid (not filled in yet)
 615 | +    CAddress addr(addrLocal);

sipa commented at 2:53 PM on August 25, 2013:

Nit: this can be a CService instead of a full CAddress, I suppose.

sipa commented at 2:54 PM on August 25, 2013: member

ACK

Krellan commented at 7:35 PM on August 25, 2013: contributor

Thanks. I found the methods worked the same when simply reusing the existing CService, so there's no need to construct a CAddress here. I updated the commit, removing that CAddress.

Krellan commented at 8:52 AM on September 4, 2013: contributor

Found and removed a needless usage of c_str().

in src/net.cpp:None in fd19c143f0 outdated

 609 | @@ -610,6 +610,9 @@ void CNode::copyStats(CNodeStats &stats)
 610 |      X(nSendBytes);
 611 |      X(nRecvBytes);
 612 |      stats.fSyncNode = (this == pnodeSync);
 613 | +    
 614 | +    // Leave string empty if addrLocal invalid (not filled in yet)

Diapolo commented at 9:44 AM on September 6, 2013:

I'm not sure if an empty string, n/a or just to hide the addrlocal filed would be best...

Krellan commented at 9:00 PM on September 7, 2013:

Thanks for the thought. I actually hide the addrlocal field from the output entirely, in the getpeerinfo handler, if it's blank. If local address is not known yet, the output is unchanged from upstream.

Thought about adding the field anyway but leaving it as an empty string, but that just added bloat to the output. Using n/a instead introduces a magic constant string that conveys no more information than an empty string.

Krellan commented at 10:59 PM on September 7, 2013: contributor

No change made, just rebased this branch to catch it up to the latest master.

gavinandresen commented at 2:22 AM on October 21, 2013: contributor

Rebase needed again.

Adding new "addrlocal" field to RPC getpeerinfo.

The existing CNode::addrLocal member is revealed to the user,
as an address string, similar to the existing "addr" field.
Instead of showing garbage or empty string,
it simply will not appear in the output if local address not known yet.

547c61f8d8

Krellan commented at 9:43 AM on October 21, 2013: contributor

Rebased! Thanks for the reminder.

BitcoinPullTester commented at 10:18 AM on October 21, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/547c61f8d8b42296fd0a51bad4a2e3a3765aa7fd for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj referenced this in commit 0c1222ff84 on Oct 21, 2013

laanwj merged this on Oct 21, 2013

laanwj closed this on Oct 21, 2013

IntegralTeam referenced this in commit 9482f77e57 on Jun 4, 2019

DrahtBot locked this on Sep 8, 2021