Change maxuploadtarget recommended minimum calculation #8559

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/08/max_ut changing 2 files +5 −4
  1. jonasschnelli commented at 11:39 am on August 22, 2016: contributor

    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.

  2. jonasschnelli added the label P2P on Aug 22, 2016
  3. in src/net.cpp: in 4cdd1a02be outdated 
    2192@@ -2193,7 +2193,7 @@ void CNode::RecordBytesSent(uint64_t bytes)
2193 void CNode::SetMaxOutboundTarget(uint64_t limit)
2194 {
2195     LOCK(cs_totalBytesSent);
2196-    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE;
2197+    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_BASE_SIZE;
2198     nMaxOutboundLimit = limit;
2199 
2200     if (limit > 0 && limit < recommendedMinimum)
    MarcoFalke commented at 12:28 pm on August 22, 2016:
    Nit: Can you also adjust the LogPrintf below to display the size in whatever unit -maxuploadtarget is.
    jonasschnelli commented at 12:31 pm on August 22, 2016:
    @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.
    MarcoFalke commented at 12:37 pm on August 22, 2016:
    Huh, I thought -maxuploadtarget was in MB or MiB?
    jonasschnelli commented at 12:39 pm on August 22, 2016:
    Yes. It is in MB. But the LogPrintf does print out the passed in MB values in bytes (nMaxOutboundLimit is -maxuploadtarget *1024 *1024)
    MarcoFalke commented at 12:44 pm on August 22, 2016:
    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.
  4. in src/net.cpp: in 4cdd1a02be outdated 
    2192@@ -2193,7 +2193,7 @@ void CNode::RecordBytesSent(uint64_t bytes)
2193 void CNode::SetMaxOutboundTarget(uint64_t limit)
2194 {
2195     LOCK(cs_totalBytesSent);
2196-    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE;
2197+    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_BASE_SIZE;
    MarcoFalke commented at 12:38 pm on August 22, 2016:

    Though, MAX_BLOCK_SERIALIZED_SIZE is your cost “on the wire”. This will be true after segwit is active.

    I’d rather keep this line as is and adjust the documentation instead.

    jonasschnelli commented at 12:42 pm on August 22, 2016:
    Right.. the recommended minimum must not reflect the historical blocks sizes, it should reflect the current blocks. Should’t this be switched from MAX_BLOCK_BASE_SIZE to MAX_BLOCK_SERIALIZED_SIZE when SW activates?
    MarcoFalke commented at 12:45 pm on August 22, 2016:
    Right, you could add a conditional to check if SW is active.
    MarcoFalke commented at 12:46 pm on August 22, 2016:
    But keep in mind that updating the doc is required in either case: If you keep MAX_BLOCK_SERIALIZED_SIZE or if you add the condition.
  5. jonasschnelli force-pushed on Aug 22, 2016
  6. jonasschnelli force-pushed on Aug 22, 2016
  7. jonasschnelli renamed this:
    Fix maxuploadtarget recommended minimum calculation
    Change maxuploadtarget recommended minimum calculation
    on Aug 22, 2016
  8. jonasschnelli commented at 1:25 pm on August 22, 2016: contributor

    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.

  9. MarcoFalke added this to the milestone 0.13.1 on Aug 22, 2016
  10. MarcoFalke added the label Needs backport on Aug 22, 2016
  11. MarcoFalke commented at 1:29 pm on August 22, 2016: member
    Thanks, looks better! utACK 5e872003e7243c4e02eec4f57f3a001dd20d925c
  12. ghost commented at 4:33 pm on August 22, 2016: none

    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.

  13. Fix maxuploadtarget recommended minimum calculation d2ce6d39f6
  14. in doc/reduce-traffic.md: in 5e872003e7 outdated 
    18@@ -19,8 +19,8 @@ This is *not* a hard limit; only a threshold to minimize the outbound
19 traffic. When the limit is about to be reached, the uploaded data is cut by no
20 longer serving historic blocks (blocks older than one week).
21 Keep in mind that new nodes require other nodes that are willing to serve
22-historic blocks. **The recommended minimum is 144 blocks per day (max. 144MB
23-per day)**
24+historic blocks.
25+**The current recommended minimum is 72 full blocks (by size "on the wire" per day)**
    MarcoFalke commented at 10:34 am on August 25, 2016:

    Nit: (to address the feedback by @wodry )

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

  15. in src/net.cpp: in 5e872003e7 outdated 
    2192@@ -2193,11 +2193,12 @@ void CNode::RecordBytesSent(uint64_t bytes)
2193 void CNode::SetMaxOutboundTarget(uint64_t limit)
2194 {
2195     LOCK(cs_totalBytesSent);
2196-    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE;
2197+    // set the recommended minimum to ~72 full blocks per day (maximal upstream full blocks worth of a ~1/2 day)
    MarcoFalke commented at 10:35 am on August 25, 2016:

    Nit: (as above)

    0// assume the average size of serialized blocks is half the maximum
  16. jonasschnelli force-pushed on Aug 26, 2016
  17. jonasschnelli commented at 7:24 am on August 26, 2016: contributor
    Nits addressed.
  18. sipa commented at 5:34 pm on August 26, 2016: member
    utACK
  19. MarcoFalke commented at 5:36 pm on August 26, 2016: member
    utACK d2ce6d3
  20. ghost commented at 7:05 pm on August 26, 2016: none
    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.
  21. MarcoFalke commented at 7:23 pm on August 26, 2016: member
    @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.
  22. in doc/reduce-traffic.md: in d2ce6d39f6 
    18@@ -19,8 +19,8 @@ This is *not* a hard limit; only a threshold to minimize the outbound
19 traffic. When the limit is about to be reached, the uploaded data is cut by no
20 longer serving historic blocks (blocks older than one week).
21 Keep in mind that new nodes require other nodes that are willing to serve
22-historic blocks. **The recommended minimum is 144 blocks per day (max. 144MB
23-per day)**
24+historic blocks.
25+**The current recommended minimum is 288MiB, assuming an average serialized block size of 2MB.**
    MarcoFalke commented at 7:25 pm on August 26, 2016: 
    0:%s/288/275 /g

    Per

    0$ src/qt/bitcoin-qt -maxuploadtarget=12 -regtest -printtoconsole|grep Recomm
12016-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.

  23. luke-jr commented at 4:01 am on August 27, 2016: member
    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?
  24. in src/net.cpp: in d2ce6d39f6 
    2192@@ -2193,11 +2193,12 @@ void CNode::RecordBytesSent(uint64_t bytes)
2193 void CNode::SetMaxOutboundTarget(uint64_t limit)
2194 {
2195     LOCK(cs_totalBytesSent);
2196-    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE;
2197+    // assume the average size of serialized blocks is half the maximum
2198+    uint64_t recommendedMinimum = (nMaxOutboundTimeframe / 600) * MAX_BLOCK_SERIALIZED_SIZE / 2;
    luke-jr commented at 4:05 am on August 27, 2016:
    Suggest defining MAX_EXPECTED_BLOCK_SIZE somewhere.
  25. MarcoFalke commented at 9:30 am on August 27, 2016: member
    @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.
  26. gmaxwell commented at 8:21 am on September 9, 2016: contributor

    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.

  27. MarcoFalke commented at 9:48 am on September 9, 2016: member
    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.
  28. MarcoFalke commented at 9:51 am on September 9, 2016: member
    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.
  29. jonasschnelli commented at 4:13 pm on September 13, 2016: contributor
    Closing in favor of #8712
  30. jonasschnelli closed this on Sep 13, 2016
  31. MarcoFalke removed this from the milestone 0.13.1 on Sep 13, 2016
  32. MarcoFalke removed the label Needs backport on Sep 13, 2016
  33. DrahtBot locked this on Sep 8, 2021


jonasschnelli MarcoFalke ghost sipa luke-jr gmaxwell

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me