Change maxuploadtarget recommended minimum calculation #8559

jonasschnelli · 2016-08-22T11:39:40Z

Addresses #8555.

Changes the recommended -maxuploadtarget minimum from 144 blocks per day to 72 blocks per day. With the SWs MAX_BLOCK_SERIALIZED_SIZE of 4MiB, 144 blocks per day would result in a recommended minimum of 576MiB per day = ~17GB per Month in uploading blocks. IMO this is too hight for a recommended minimum.

This PR changes the minimum calculation from 144 to 72 blocks per day ("half day of full blocks").
Anyway, the calculation is just a form of an vague estimation of how many MiB in historical blocks we should recommend to broadcast upstream when -maxuploadtarget is set. It does not take -maxconnection etc. into the calculation.

The current maxuploadtarget recommendation is calculated using MAX_BLOCK_SERIALIZED_SIZE (4MB) instead of MAX_BLOCK_BASE_SIZE (1MB). Given that the function is there to reduce uploading historical blocks, using MAX_BLOCK_BASE_SIZE makes more sense.

maflcko · 2016-08-22T12:28:56Z

src/net.cpp

@@ -2193,7 +2193,7 @@ void CNode::RecordBytesSent(uint64_t bytes)
 void CNode::SetMaxOutboundTarget(uint64_t limit)
 {
    LOCK(cs_totalBytesSent);
-    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE;
+    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_BASE_SIZE;
    nMaxOutboundLimit = limit;

    if (limit > 0 && limit < recommendedMinimum)


Nit: Can you also adjust the LogPrintf below to display the size in whatever unit -maxuploadtarget is.

@MarcoFalke: I can't parse your sentence. :)
Right now we have: LogPrintf("Max outbound target is very small (%s bytes) and will be overshot. Recommended minimum is %s bytes.\n", nMaxOutboundLimit, recommendedMinimum); which prints the desired output limit (1:1 from -maxuploadtraget and the recommended minimum, both are stated to be in bytes.

Huh, I thought -maxuploadtarget was in MB or MiB?

Yes. It is in MB. But the LogPrintf does print out the passed in MB values in bytes (nMaxOutboundLimit is -maxuploadtarget *1024 *1024)

I think it would be more user friendly to print this out in MiB, so the user can see what value for -maxuploadtarget should be used instead.

jonasschnelli · 2016-08-22T13:25:24Z

Changed the recommended minimum "calculation" (it's not really a calculation) from 144 blocks to 72 full blocks (on the wire size) per day.
Not doing this, would result in a recommended upload minimum (after SW has been deployed) of ~17GiB per month (which is too high IMO for a minimum).

Updated the docs, removed the MiB values.

maflcko · 2016-08-22T13:29:07Z

Thanks, looks better! utACK 5e872003e7243c4e02eec4f57f3a001dd20d925c

ghost · 2016-08-22T16:33:59Z

Feedback from a rather "normal user":

In reduce-traffic.md would now be written:
"1. Use -maxuploadtarget= [...] The current recommended minimum is 72 full blocks (by size "on the wire" per day)" [...]"

In my opinion, this is not understandable for a normal user- what value should that be in MiB ??

I think it would be better to recommend a concrete value in the unit of the parameter (MiB), and adapt that recommendation with increasing population of Segwit (and therefore the increasing ammount of bytes stored in one block, as I understand?) accross the upcoming (minor) releases.

At least, if the current text should remain, there should be more explanation of how could one calculate "72 full blocks (by size "on the wire" per day)"" to MiB.

maflcko · 2016-08-25T10:34:00Z

doc/reduce-traffic.md

-historic blocks. **The recommended minimum is 144 blocks per day (max. 144MB
-per day)**
+historic blocks.
+**The current recommended minimum is 72 full blocks (by size "on the wire" per day)**


Nit: (to address the feedback by @wodry )

The current recommended minimum is -maxuploadtarget=$recommendedMinimum, assuming an average serialized block size of 2MB.

jonasschnelli · 2016-08-26T07:24:03Z

Nits addressed.

sipa · 2016-08-26T17:34:57Z

utACK

maflcko · 2016-08-26T17:36:38Z

utACK d2ce6d3

ghost · 2016-08-26T19:05:02Z

Instead of
"The current recommended minimum is 288MiB, assuming an average serialized block size of 2MB."
I would suggest
"The current recommended minimum is 288 MiB, assuming an average serialized block size of 2 MiB."
, to have a consistent unit of MiB and correct spelling.

maflcko · 2016-08-26T19:23:40Z

@wodry The block size limit is denoted in the "decimal" MB (Like most other sizes in Bitcoin Core). Unfortunately some palaces use MiB, but there is nothing you can do about it.

maflcko · 2016-08-26T19:25:16Z

doc/reduce-traffic.md

-historic blocks. **The recommended minimum is 144 blocks per day (max. 144MB
-per day)**
+historic blocks.
+**The current recommended minimum is 288MiB, assuming an average serialized block size of 2MB.**


:%s/288/275 /g

Per

$ src/qt/bitcoin-qt -maxuploadtarget=12 -regtest -printtoconsole|grep Recomm 2016-08-26 19:22:09 Max outbound target is very small (12 MiB) and will be overshot. Recommended minimum is 274.658 MiB.

But consider it a nit. Can be merged now.

luke-jr · 2016-08-27T04:01:56Z

There are constantly 144 blocks per day. Segwit blocks can be up to 4 MB. If users set a value lower than this, what happens when that assumption is violated?

luke-jr · 2016-08-27T04:05:06Z

src/net.cpp

@@ -2193,11 +2193,12 @@ void CNode::RecordBytesSent(uint64_t bytes)
 void CNode::SetMaxOutboundTarget(uint64_t limit)
 {
    LOCK(cs_totalBytesSent);
-    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE;
+    // assume the average size of serialized blocks is half the maximum
+    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE / 2;


Suggest defining MAX_EXPECTED_BLOCK_SIZE somewhere.

maflcko · 2016-08-27T09:30:07Z

@luke-jr IIRC, nothing will happen. You just stop to serve historical blocks earlier. I think you can even set -maxuploadtarget=1 to effectively never send historical blocks.

gmaxwell · 2016-09-09T08:21:58Z

uh. so the reason that it is a full day is because that is the reset interval.

Since the limiting will not prevent it from sending blocks, the purpose of the minimum was that it was the true minimum, and setting lower would pretty much guarantee overshoot.

maflcko · 2016-09-09T09:48:10Z

Though, -maxuploadtarget only applies to "historic blocks", so implying that it will not overshoot when you set it to maximum block chain growth in one day is misleading.
Regardless of the value that is set (may it be 0 or 100000), there is no guarantee that it will not overshoot.

maflcko · 2016-09-09T09:51:09Z

So we might as well just remove the "recommended" minimum and call it done. Effectively you are still not helping IBD if you set the target to 144 or 288 instead of 0.

jonasschnelli · 2016-09-13T16:13:32Z

Closing in favor of #8712

jonasschnelli added the P2P label Aug 22, 2016

maflcko reviewed Aug 22, 2016
View reviewed changes

jonasschnelli force-pushed the 2016/08/max_ut branch 2 times, most recently from b42a065 to 5e87200 Compare August 22, 2016 13:16

jonasschnelli changed the title ~~Fix maxuploadtarget recommended minimum calculation~~ Change maxuploadtarget recommended minimum calculation Aug 22, 2016

maflcko added this to the 0.13.1 milestone Aug 22, 2016

maflcko added the Needs backport label Aug 22, 2016

maflcko mentioned this pull request Aug 25, 2016

Segregated witness #7910

Closed

7 tasks

maflcko reviewed Aug 25, 2016
View reviewed changes

Fix maxuploadtarget recommended minimum calculation

d2ce6d3

jonasschnelli force-pushed the 2016/08/max_ut branch from 5e87200 to d2ce6d3 Compare August 26, 2016 07:23

maflcko reviewed Aug 26, 2016
View reviewed changes

luke-jr mentioned this pull request Aug 27, 2016

mining: Optimise for typical mining use with blockmaxsize #8386

Closed

luke-jr reviewed Aug 27, 2016
View reviewed changes

jonasschnelli mentioned this pull request Sep 13, 2016

Remove maxuploadtargets recommended minimum #8712

Merged

jonasschnelli closed this Sep 13, 2016

maflcko removed this from the 0.13.1 milestone Sep 13, 2016

maflcko removed the Needs backport label Sep 13, 2016

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Change maxuploadtarget recommended minimum calculation #8559

Change maxuploadtarget recommended minimum calculation #8559

Uh oh!

Conversation

jonasschnelli commented Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Aug 22, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Aug 22, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 22, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Aug 22, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 22, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 22, 2016

Uh oh!

maflcko commented Aug 22, 2016

Uh oh!

ghost commented Aug 22, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Aug 25, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Aug 26, 2016

Uh oh!

sipa commented Aug 26, 2016

Uh oh!

maflcko commented Aug 26, 2016

Uh oh!

ghost commented Aug 26, 2016

Uh oh!

maflcko commented Aug 26, 2016

Uh oh!

maflcko Aug 26, 2016

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Aug 27, 2016

Uh oh!

luke-jr Aug 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 27, 2016

Uh oh!

gmaxwell commented Sep 9, 2016

Uh oh!

maflcko commented Sep 9, 2016

Uh oh!

maflcko commented Sep 9, 2016

Uh oh!

jonasschnelli commented Sep 13, 2016

Uh oh!

Uh oh!

jonasschnelli commented Aug 22, 2016 •

edited

Loading

ghost commented Aug 22, 2016 •

edited by ghost

Loading

luke-jr Aug 27, 2016 •

edited

Loading