Skip to content

addrman: log AS only when using asmap #28729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fanquake merged 1 commit into from
Nov 1, 2023
Merged

addrman: log AS only when using asmap #28729

fanquake merged 1 commit into from
Nov 1, 2023

Conversation

@brunoerg
Copy link
Contributor

@brunoerg brunoerg commented Oct 25, 2023

This PR changes the log to just print the ASN when using asmap, same logic presented in other logs:

bitcoin/src/net_processing.cpp

Lines 3552 to 3556 in afa081a

const auto mapped_as{m_connman.GetMappedAS(pfrom.addr)};
LogPrint(BCLog::NET, "receive version message: %s: version %d, blocks=%d, us=%s, txrelay=%d, peer=%d%s%s\n",
cleanSubVer, pfrom.nVersion,
peer->m_starting_height, addrMe.ToStringAddrPort(), fRelay, pfrom.GetId(),
remoteAddr, (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : ""));

bitcoin/src/net_processing.cpp

Lines 3598 to 3604 in afa081a

const auto mapped_as{m_connman.GetMappedAS(pfrom.addr)};
LogPrintf("New %s %s peer connected: version: %d, blocks=%d, peer=%d%s%s\n",
pfrom.ConnectionTypeAsString(),
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(), peer->m_starting_height,
pfrom.GetId(), (fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToStringAddrPort()) : ""),
(mapped_as ? strprintf(", mapped_as=%d", mapped_as) : ""));

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 25, 2023 •
edited
Loading

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK naumenkogs, mzumsande
Stale ACK sipa

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

@DrahtBot DrahtBot added the P2P label Oct 25, 2023
@maflcko
Copy link
Member

maflcko commented Oct 25, 2023

Can you share an example log before and after?

@brunoerg
Copy link
Contributor Author

brunoerg commented Oct 25, 2023

Can you share an example log before and after?

This PR (without asmap):
[addrman] Added 172.56.55.248:8333 to new[401][28]

master (without asmap):
[addrman] Added 172.56.55.248:8333 mapped to AS0 to new[401][28]

@brunoerg brunoerg force-pushed the 2023-10-addrman-log-as branch from 1cf8440 to 3e3f5dd Compare October 25, 2023 12:49
@naumenkogs
Copy link
Contributor

naumenkogs commented Oct 27, 2023

ACK 3e3f5ddb9a8ac27ea509fe3dd09b783472c1ce9a
No need to print a confusing mapped-nothing. Hopefully, we don't break much log-parsers.

@sipa
Copy link
Member

sipa commented Oct 28, 2023

utACK 3e3f5ddb9a8ac27ea509fe3dd09b783472c1ce9a

mzumsande
mzumsande reviewed Oct 30, 2023
View reviewed changes
Copy link
Contributor

@mzumsande mzumsande left a comment

Choose a reason for hiding this comment

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

Concept ACK

Would be good to do the same thing to the log in AddrManImpl::Good_().

@brunoerg
Copy link
Contributor Author

brunoerg commented Oct 30, 2023

Would be good to do the same thing to the log in AddrManImpl::Good_().

Nice find, addressing it.

@brunoerg brunoerg force-pushed the 2023-10-addrman-log-as branch from 3e3f5dd to 02a4f1a Compare October 30, 2023 23:19
@brunoerg
Copy link
Contributor Author

brunoerg commented Oct 30, 2023

Force-pushed changing the log in AddrManImpl::Good_().

@naumenkogs
Copy link
Contributor

naumenkogs commented Oct 31, 2023

ACK 02a4f1a

@DrahtBot DrahtBot requested review from mzumsande and sipa October 31, 2023 07:29
@mzumsande
Copy link
Contributor

mzumsande commented Oct 31, 2023

Code Review ACK 02a4f1a

@DrahtBot DrahtBot removed the request for review from mzumsande October 31, 2023 20:57
@fanquake fanquake merged commit 4733de3 into bitcoin:master Nov 1, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sipa sipa Awaiting requested review from sipa

1 more reviewer

@mzumsande mzumsande mzumsande left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

P2P

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@brunoerg @DrahtBot @maflcko @naumenkogs @sipa @mzumsande @fanquake