Skip to content

Additional field to RPC getpeerinfo output: addrlocal#2929

Merged
laanwj merged 1 commit intobitcoin:masterfrom
Krellan:addrlocal
Oct 21, 2013
Merged

Additional field to RPC getpeerinfo output: addrlocal#2929
laanwj merged 1 commit intobitcoin:masterfrom
Krellan:addrlocal

Conversation

@Krellan
Copy link
Copy Markdown
Contributor

@Krellan Krellan commented Aug 24, 2013

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
Copy link
Copy Markdown

Diapolo commented Aug 24, 2013

You should squash into one single commit.

@Krellan
Copy link
Copy Markdown
Contributor Author

Krellan commented Aug 24, 2013

Thanks for the feedback! Done. Nicely squashed now.

@jgarzik
Copy link
Copy Markdown
Contributor

jgarzik commented Aug 25, 2013

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

@Krellan
Copy link
Copy Markdown
Contributor Author

Krellan commented Aug 25, 2013

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

Comment thread src/net.cpp Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sipa
Copy link
Copy Markdown
Member

sipa commented Aug 25, 2013

ACK

@Krellan
Copy link
Copy Markdown
Contributor Author

Krellan commented Aug 25, 2013

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
Copy link
Copy Markdown
Contributor Author

Krellan commented Sep 4, 2013

Found and removed a needless usage of c_str().

Comment thread src/net.cpp Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

Krellan commented Sep 7, 2013

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

@gavinandresen
Copy link
Copy Markdown
Contributor

Rebase needed again.

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.
@Krellan
Copy link
Copy Markdown
Contributor Author

Krellan commented Oct 21, 2013

Rebased! Thanks for the reminder.

@BitcoinPullTester
Copy link
Copy Markdown

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 added a commit that referenced this pull request Oct 21, 2013
Additional field to RPC getpeerinfo output: addrlocal
@laanwj laanwj merged commit 0c1222f into bitcoin:master Oct 21, 2013
IntegralTeam pushed a commit to IntegralTeam/bitcoin that referenced this pull request Jun 4, 2019
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants