-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version 70235
#6239
feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version 70235
#6239
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK fa6dd58
src/net_processing.cpp
Outdated
@@ -3110,7 +3110,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, | |||
} | |||
|
|||
// Consider fetching more headers. | |||
if (nCount == MAX_HEADERS_RESULTS) { | |||
if (nCount == (pfrom.GetCommonVersion() >= INCREASE_MAX_HEADERS_VERSION ? MAX_HEADERS_RESULTS_NEW : MAX_HEADERS_RESULTS_OLD)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you add instead a helper function?
such as pfrom.MaxHeadersResults()
TODO: tie increase to only apply to headers2 and not headers message |
src/validation.h
Outdated
@@ -85,10 +85,9 @@ static const int MAX_SCRIPTCHECK_THREADS = 15; | |||
static const int DEFAULT_SCRIPTCHECK_THREADS = 0; | |||
/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends | |||
* less than this number, we reached its tip. Changing this value is a protocol upgrade. */ | |||
static const unsigned int MAX_HEADERS_RESULTS = 8000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not use old name for better error resistance for backports. So, it won't be mistakenly used directly this constant instead helper function
pls check b2f3593 |
What about knst's concern around changing the name of the old variable to avoid incidental backport silent errors? |
agree, and this issue is addressed in my commit too |
How so? you still have the variable
|
diff for diff --git a/src/validation.h b/src/validation.h
index 8dc4d7e656..75b7590ffe 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -86,6 +86,7 @@ static const int DEFAULT_SCRIPTCHECK_THREADS = 0;
/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends
* less than this number, we reached its tip. Changing this value is a protocol upgrade. */
static const unsigned int MAX_HEADERS_RESULTS = 2000;
+static const unsigned int MAX_HEADERS2_RESULTS = 8000;
static const int64_t DEFAULT_MAX_TIP_AGE = 6 * 60 * 60; // ~144 blocks behind -> 2 x fork detection time, was 24 * 60 * 60 in bitcoin
|
Yes, but if bitcoin changes stuff, and uses |
oh, sorry, I misunderstood his comment. I thought his concerns were that we change MAX_HEADERS_RESULTS value and it might affect smth once we do backports that rely on it. My bad. |
yeah, then you need s/ |
MAX_HEADERS_UNCOMPRESSED_RESULTS ? |
please advise how ya'll think I should move forward :) |
b2f3593 +
I guess |
2a98834
to
3b142a4
Compare
8adfad9
to
07f6b4f
Compare
…nce to 8000 in protocol version `70234`
We can reduce the diff by keeping the name `MAX_HEADERS_RESULTS` for `MAX_HEADERS_RESULTS_NEW` as `MAX_HEADERS_RESULTS_OLD` is only referenced once (in `GetHeadersLimit()`)
…ESULTS` ; use `MAX_HEADERS_UNCOMPRESSED_RESULT` in RPC to avoid breaking changes
07f6b4f
to
b423f42
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls see 0e66be4 + below
also needs release notes I guess
70234
70235
Co-authored-by: UdjinM6 <[email protected]>
…or it to accept `compressed` instead of `msg_type`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
light ACK 48c7f98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 48c7f98 (with nits)
@@ -58,6 +58,9 @@ static const int NO_LEGACY_ISLOCK_PROTO_VERSION = 70231; | |||
//! Inventory type for DSQ messages added | |||
static const int DSQ_INV_VERSION = 70234; | |||
|
|||
//! Maximum header count for HEADRES2 message was increased from 2000 to 8000 in this version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
//! Maximum header count for HEADRES2 message was increased from 2000 to 8000 in this version | |
//! Maximum header count for HEADERS2 message was increased from 2000 to 8000 in this version |
/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends | ||
* less than this number, we reached its tip. Changing this value is a protocol upgrade. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/** Number of headers sent in one getheaders result. We rely on the assumption that if a peer sends | |
* less than this number, we reached its tip. Changing this value is a protocol upgrade. */ | |
/** Number of headers sent in one getheaders(2) result. We rely on the assumption that if a peer sends | |
* less than this number, we reached its tip. Changing these values is a protocol upgrade. */ |
@@ -4564,7 +4574,7 @@ void PeerManagerImpl::ProcessMessage( | |||
|
|||
// Bypass the normal CBlock deserialization, as we don't want to risk deserializing 2000 full blocks. | |||
unsigned int nCount = ReadCompactSize(vRecv); | |||
if (nCount > MAX_HEADERS_RESULTS) { | |||
if (nCount > GetHeadersLimit(pfrom, msg_type == NetMsgType::HEADERS2)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can commit result to variable and reuse it further down, also consider changing misbehavior text to mention headers2 when appropriate due to different limit.
if (nCount > GetHeadersLimit(pfrom, msg_type == NetMsgType::HEADERS2)) { | |
const bool is_compressed{msg_type == NetMsgType::HEADERS2}; | |
if (nCount > GetHeadersLimit(pfrom, is_compressed)) { |
----------------------- | ||
|
||
The max number of compressed block headers which can be requested at once has been increased from 2000 to 8000. This | ||
change activates with the protocol version `70235` and only applies to compressed block headers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: first line already mentions it's compressed-only
change activates with the protocol version `70235` and only applies to compressed block headers. | |
change activates with the protocol version `70235`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 48c7f98
p.s. CI failure (tsan) seems unrelated, I restarted it.
Issue being fixed or feature implemented
We did some testing quite a while ago that found that sending 8000 headers at a time could speed stuff up. But we wanted to wait until compressed headers were implemented. Well, they've been implemented!
What was done?
Bump 2000 -> 8000 triggered by protocol version
How Has This Been Tested?
Hasn't, we should setup a few nodes running this and sync them from each other
Breaking Changes
New protocol version, not breaking but should add notes? I should probably add release notes
Checklist:
Go over all the following points, and put an
x
in all the boxes that apply.