feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version `70235` #6239

PastaPastaPasta · 2024-08-28T17:13:58Z

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.

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

kwvg

Consider f69f410 and 08878d6

knst

utACK fa6dd58

knst · 2024-09-04T12:47:42Z

src/net_processing.cpp

@@ -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)) {


nit: can you add instead a helper function?

such as pfrom.MaxHeadersResults()

PastaPastaPasta · 2024-09-04T17:14:33Z

TODO: tie increase to only apply to headers2 and not headers message

knst · 2024-09-04T18:54:17Z

src/validation.h

@@ -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;


Should not use old name for better error resistance for backports. So, it won't be mistakenly used directly this constant instead helper function

UdjinM6 · 2024-09-04T18:58:33Z

TODO: tie increase to only apply to headers2 and not headers message

pls check b2f3593

PastaPastaPasta · 2024-09-04T19:03:23Z

TODO: tie increase to only apply to headers2 and not headers message

pls check b2f3593

What about knst's concern around changing the name of the old variable to avoid incidental backport silent errors?

UdjinM6 · 2024-09-04T19:06:01Z

TODO: tie increase to only apply to headers2 and not headers message

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

PastaPastaPasta · 2024-09-04T19:07:44Z

How so? you still have the variable

static const unsigned int MAX_HEADERS_RESULTS = 2000;

UdjinM6 · 2024-09-04T19:10:39Z

diff for src/validation.h with my commit applied looks like this

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

PastaPastaPasta · 2024-09-04T19:12:28Z

Yes, but if bitcoin changes stuff, and uses MAX_HEADERS_RESULTS we may actually want to be using MAX_HEADERS2_RESULTS for some reason, but the backport will merge w/o issue. If we change the name of MAX_HEADERS_RESULTS to something else, then we'll have to make a manual decision as to which applies in that situation during the merge. I guess it is better that in your commit the default logic would be the conservative one. Just want to make sure you don't think it'd be valuable to change it to something else and force us to evaluate it.

UdjinM6 · 2024-09-04T19:16:27Z

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.

UdjinM6 · 2024-09-04T19:24:02Z

yeah, then you need s/MAX_HEADERS_RESULTS/<some new name>/ on top of my commit indeed. I don't like _OLD suffix though... maybe simply MAX_HEADERS_RESULTS_ so that it would look close enough but still cause backport conflicts/issues?

knst · 2024-09-04T20:25:46Z

I don't like _OLD suffix though... maybe simply MAX_HEADERS_RESULT

MAX_HEADERS_UNCOMPRESSED_RESULTS
MAX_HEADERS_COMPRESSED_RESULT

?

PastaPastaPasta · 2024-09-05T19:05:28Z

please advise how ya'll think I should move forward :)

UdjinM6 · 2024-09-05T20:34:09Z

please advise how ya'll think I should move forward :)

b2f3593 +

MAX_HEADERS_UNCOMPRESSED_RESULTS and MAX_HEADERS_COMPRESSED_RESULT

I guess

UdjinM6 · 2024-09-06T21:20:51Z

pls see ac8fa39 and 5a28ca0

also, Gitlab CI won't pick the branch... try rebasing maybe?

…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

UdjinM6

pls see 0e66be4 + below

also needs release notes I guess

test/functional/test_framework/p2p.py

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

…or it to accept `compressed` instead of `msg_type`

PastaPastaPasta added this to the 21.2 milestone Aug 29, 2024

PastaPastaPasta marked this pull request as ready for review August 29, 2024 15:33

kwvg suggested changes Aug 30, 2024

View reviewed changes

knst previously approved these changes Sep 4, 2024

View reviewed changes

PastaPastaPasta self-assigned this Sep 4, 2024

PastaPastaPasta dismissed knst’s stale review via 8c8f767 September 4, 2024 17:12

knst reviewed Sep 4, 2024

View reviewed changes

PastaPastaPasta force-pushed the feat-introduce-increased-block-headers-requests branch from 2a98834 to 3b142a4 Compare September 6, 2024 20:13

PastaPastaPasta force-pushed the feat-introduce-increased-block-headers-requests branch from 8adfad9 to 07f6b4f Compare September 10, 2024 03:04

PastaPastaPasta and others added 8 commits September 10, 2024 10:23

feat: increase the number of block headers able to be downloaded at o…

993c7c0

…nce to 8000 in protocol version `70234`

trivial: move the headers limit determination to GetHeadersLimit()

bcf0320

trivial: rename MAX_HEADERS_RESULTS_NEW to MAX_HEADERS_RESULTS

e23410f

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()`)

fix: increase it for headers2 only

303bc7a

change to _COMPRESSED or _UNCOMPRESSED

b137280

use MAX_HEADERS_UNCOMPRESSED_RESULT not `MAX_HEADERS_UNCOMPRESSED_R…

07876b2

…ESULTS` ; use `MAX_HEADERS_UNCOMPRESSED_RESULT` in RPC to avoid breaking changes

refactor: simplify _compute_requested_block_headers

f6c68ba

refactor: sort imports

b423f42

PastaPastaPasta force-pushed the feat-introduce-increased-block-headers-requests branch from 07f6b4f to b423f42 Compare September 10, 2024 15:25

UdjinM6 reviewed Sep 10, 2024

View reviewed changes

test/functional/test_framework/p2p.py Outdated Show resolved Hide resolved

PastaPastaPasta changed the title ~~feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version 70234~~ feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version 70235 Sep 10, 2024

PastaPastaPasta and others added 3 commits September 10, 2024 12:56

bump p2p_version in tests

b224f3f

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

fix: GetHeadersLimit is used for getheaders(2) and headers(2), refact…

a6bbaac

…or it to accept `compressed` instead of `msg_type`

docs: add release notes for 6239

cfe99fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

PastaPastaPasta commented Aug 28, 2024 •

edited

Loading

kwvg left a comment

knst left a comment

knst Sep 4, 2024

PastaPastaPasta commented Sep 4, 2024

knst Sep 4, 2024

UdjinM6 commented Sep 4, 2024

PastaPastaPasta commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024

PastaPastaPasta commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024 •

edited

Loading

PastaPastaPasta commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024 •

edited

Loading

knst commented Sep 4, 2024

PastaPastaPasta commented Sep 5, 2024

UdjinM6 commented Sep 5, 2024

UdjinM6 commented Sep 6, 2024

UdjinM6 left a comment

feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version 70235 #6239

Are you sure you want to change the base?

feat: increase the number of block headers able to be downloaded at once to 8000 in protocol version 70235 #6239

Conversation

PastaPastaPasta commented Aug 28, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

kwvg left a comment

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

knst Sep 4, 2024

Choose a reason for hiding this comment

PastaPastaPasta commented Sep 4, 2024

knst Sep 4, 2024

Choose a reason for hiding this comment

UdjinM6 commented Sep 4, 2024

PastaPastaPasta commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024

PastaPastaPasta commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024 • edited Loading

PastaPastaPasta commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024

UdjinM6 commented Sep 4, 2024 • edited Loading

knst commented Sep 4, 2024

PastaPastaPasta commented Sep 5, 2024

UdjinM6 commented Sep 5, 2024

UdjinM6 commented Sep 6, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

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

PastaPastaPasta commented Aug 28, 2024 •

edited

Loading

UdjinM6 commented Sep 4, 2024 •

edited

Loading

UdjinM6 commented Sep 4, 2024 •

edited

Loading