Skip to content

Introduce a maximum size for locators. #13907

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

Merged
merged 1 commit into from
Aug 10, 2018
Merged

Introduce a maximum size for locators. #13907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/net.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ static const int TIMEOUT_INTERVAL = 20 * 60;
static const int FEELER_INTERVAL = 120;
/** The maximum number of entries in an 'inv' protocol message */
static const unsigned int MAX_INV_SZ = 50000;
/** The maximum number of entries in a locator */
static const unsigned int MAX_LOCATOR_SZ = 101;
/** The maximum number of new addresses to accumulate before announcing. */
static const unsigned int MAX_ADDR_TO_SEND = 1000;
/** Maximum length of incoming protocol messages (no message over 4 MB is currently acceptable). */
Expand Down
12 changes: 12 additions & 0 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2018,6 +2018,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
uint256 hashStop;
vRecv >> locator >> hashStop;

if (locator.vHave.size() > MAX_LOCATOR_SZ) {
Copy link
Member

@maflcko maflcko Aug 8, 2018

Choose a reason for hiding this comment

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

Note that your BIP says: "A locator included in a getblock or getheaders message may include no more
than 64 hashes, including the final hash_stop hash", so unless I am mistaken, the > should be replaced with a >=?

Copy link
Contributor Author

@gmaxwell gmaxwell Aug 8, 2018

Choose a reason for hiding this comment

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

Yes, if we end up following the BIP. I'm giving it about equal odds that we need to use a threshold higher than the BIP for now, after we see whats showing up on the network.

LogPrint(BCLog::NET, "getblocks locator size %lld > %d, disconnect peer=%d\n", locator.vHave.size(), MAX_LOCATOR_SZ, pfrom->GetId());
pfrom->fDisconnect = true;
Copy link
Contributor

@domob1812 domob1812 Aug 8, 2018

Choose a reason for hiding this comment

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

Is there a specific reason why you use fDisconnect here, rather than Misbehaving as in many other places around here? If there is, perhaps adding a comment would be helpful to future readers of the code (it would certainly be useful to me right now).

Copy link
Contributor Author

@gmaxwell gmaxwell Aug 8, 2018

Choose a reason for hiding this comment

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

Because I don't see any particular reason to ban the peer. We only need to disconnect rather than just ignoring the message because the peer might be counting on our response and ignoring it could make it get stuck when otherwise it might make progress with someone else.

We similarly disconnect for some messges when OutboundTargetReached, when we're limited and someone requests a block before the limited horizon, when the peer requests BIP37 services that we're not offering, etc.

Copy link
Contributor

@domob1812 domob1812 Aug 10, 2018

Choose a reason for hiding this comment

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

Makes sense, thanks for the clarification!

return true;
}

// We might have announced the currently-being-connected tip using a
// compact block, which resulted in the peer sending a getblocks
// request, which we would otherwise respond to without the new block.
Expand Down Expand Up @@ -2131,6 +2137,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
uint256 hashStop;
vRecv >> locator >> hashStop;

if (locator.vHave.size() > MAX_LOCATOR_SZ) {
LogPrint(BCLog::NET, "getheaders locator size %lld > %d, disconnect peer=%d\n", locator.vHave.size(), MAX_LOCATOR_SZ, pfrom->GetId());
pfrom->fDisconnect = true;
return true;
}

LOCK(cs_main);
if (IsInitialBlockDownload() && !pfrom->fWhitelisted) {
LogPrint(BCLog::NET, "Ignoring getheaders from peer=%d because node is in initial block download\n", pfrom->GetId());
Expand Down