When using -asmap, you can check the ASN assigned to the peers only with the RPC command getpeerinfo (check mapped_as field), however, it's not possible to check it in logs (e.g. see in logs the ASN of the peers when a new outbound peer has been connected). This PR includes the peers' ASN in debug output when using -asmap.
Obs: Open this primarily to chase some Concept ACK, I've been using this on my node to facilitate to track the peers' ASN especially when reading the logs.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, jamesob, achow101 |
| Concept ACK | Ayush170-Future |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
3409 | @@ -3410,9 +3410,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, 3410 | } 3411 | 3412 | if (!pfrom.IsInboundConn()) { 3413 | - LogPrintf("New outbound peer connected: version: %d, blocks=%d, peer=%d%s (%s)\n", 3414 | + LogPrintf("New outbound peer connected: version: %d, blocks=%d, peer=%d%s%s (%s)\n", 3415 | pfrom.nVersion.load(), peer->m_starting_height, 3416 | pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToStringAddrPort()) : ""), 3417 | + (log_asmap ? strprintf(", mapped_as=%d", m_connman.GetMappedAS(pfrom.addr)) : ""),
Concept ACK, but would there be any downside to not adding a config option and just logging the asmap if it is non-zero? Edit: this would also avoid the need for a release note.
Yeah, that does make more sense. I don't see any harm in automatically logging the asmap information if the fLogIPs flag is true.
Yeah, I think it would be simpler to remove the new flag I created and log it when using -asmap.
Concept ACK.
Just updated the description and changed the approach to leave it simpler. Before I had created a flag -logasmap (similar to -logips), now I removed it and when using -asmap it will includes the peers' ASN in debug output.
thanks @jonatack and @Ayush170-Future for your thoughts.
Before I had created a flag
-logasmap(similar to-logips), now I removed it
Thanks! It looks like logasmap is still added in the last push, though.
It looks like logasmap is still added in the last push, though.
Sorry it was a leftover, fixed it.
tACK 7b302e1
The second commit message could be improved: the current implementation only logs outbound.
Would be nice to log this for inbound connections too, I guess the receive version message: is most suitable for that?
When using `-asmap`, it will log the ASN from the peer on some logs (e.g. when a new outbound peer has been connected).
Would be nice to log this for inbound connections too, I guess the receive version message: is most suitable for that?
Yes, it would be great to have this for both ones, just updated it!
Big concept ACK. Will test next week (post-honeymoon).
tACK 0076bed45eb2b42111fa3f4c95181393c685a42e
During review I noticed fLogIPs which is controlled by -logips which is off by default. This setting seems to be ignored in a bunch of places. I don't know what it's original purpose is. If the issue was peer privacy, then even though an ASN is also not an IP, we should probably still hide it when -logips is false.
There's some discussion in #3764 about this. Hack/subpoena risk being one argument, but that mainly applies to transaction relay, not connect messages). We should probably clarify the docs for -logips to say "Log IP addresses for privacy sensitive events such as transaction relay." And then maybe make it default to true for -debug=net, adding to the help Implied for -debug=net.
I agree with @Sjors here. In my last review comment, I suggested the same idea. Though I'm not sure how the peer privacy issue is relevant here. But, it makes more sense to only log ASN when <code>fLogIPs</code> is already true.
Maybe changing the name of <code>-logips</code> flag to something like <code>-lognetinfo</code> will better reflect its use and avoid confusion, but that would require a release note. (just a school of thought)
Reading that original discussion I think the current code is fine. The main concern was exposing the origin of transactions in log files under the default configuration. Since we don't log transaction relay by default (it's extremely noisy) we can safely log the ASN (or even IP) of a node when it connects.
We can clean up the documentation and/or behavior of -logips in another PR.
We can clean up the documentation and/or behavior of -logips in another PR.
For sure, I can work on that after this one.
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e (jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from)
Reviewed code, built locally. Change looks good. Going to test with -asmap today.
<details><summary>Show signature data</summary> <p>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e ([`jamesob/ackr/27412.1.brunoerg.logging_net_add_asn_from`](https://github.com/jamesob/bitcoin/tree/ackr/27412.1.brunoerg.logging_net_add_asn_from))
Reviewed code, built locally. Change looks good. Going to test with -asmap today.
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmQ1m4YACgkQepNdrbLE
TwWHLg/9EbvTm+b4IWsdpytUtiN2Y/v8+hX9dVZooWnvdOw5Bviryj5jr7eaUvhf
UpOBnwH/CH52kBpSZf5qGmDJKRECThSGYKeSnGzuDUhcfOuT6vax6/DIdy7qWsva
xx+t6KvRVcnkAif0yNVtKdV9+mng+W6KqmC0AndeqZ5H7VK+d/5Dp+W8r0tFSJsk
Bh5pTOYfaH53BZdjzQ00WWQJ8ljahuS3uDgTSIIAyJd3rtMv+vsvBN9dy5PQoMdT
gPWarbthuT1lQ80on/zD8SwKOPTcfyBwZbv74i5fzv8M71O8OBpQUrUcgPkI8oHI
2mzIgE1PaoN6zlnEtVcFjVLJgztNphbH5pmmzlqDCoOxoQ5fz7ZRIl0ck4Cz+8vv
DlawDyf7yRefZL3CKbgLWjOzXUOz5yrytj/YBGH6KzCyFUiv11kQTx73AtMnKKLY
M7tP0/7tsWVgCzl1SU0pL443q7ncw6S46UAoEL7aWmLT6HQ8My0tzMXUlZ+jNqIl
RroZRMCFoXC1B5dKe7o65V3PgWVW3JZbRZv5a5X7wKA5WWr5M8UK9q5PAyzq4MVY
DW87ozXrApaFilR+51beS2CHyVV2ysXkdq1hasBS0y3FNcYP66CFB4uiQeiUhd7U
RHpXx/05dL0hzOR4GTJoI4bHFpgNY8TSfOfZMxGFDTiwfmRWyK4=
=Sq2n
-----END PGP SIGNATURE-----
</p></details>
TIL you can quite easily verify from the clipboard (at least on macOS, probably on Linux too). No need to put things in a file. gpg --verify will show a prompt. Paste everything from -----BEGIN to the end, hit enter and then ctrl + d.
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e