Set peers as HB peers upon full block validation #9400

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:maybesetfullblock changing 1 files +28 −27
  1. instagibbs commented at 2:02 pm on December 21, 2016: member

    We would rather select HB peers to be those that are getting us full valid blocks the fastest, versus merely letting us know about the next valid PoW block header the earliest. Currently a peer can just share a header, withhold the full block, and be selected as a HB peer, denying other less-malicious peers their slot. The attacker could possibly get 3 connections with the node, then block all useful CB propagation gains.

    After this fix the worst that can happen is malicious peers are first to give you valid full blocks to stay in the HB slots.

  2. fanquake added the label P2P on Dec 21, 2016
  3. rebroad commented at 4:18 pm on December 21, 2016: contributor
    Is it still the case that the first peer to send us the header is the only peer we’ll request the block from (with a timeout for non-response of 10 minutes)?
  4. instagibbs commented at 4:21 pm on December 21, 2016: member
    @rebroad This code doesn’t change any inv behavior.
  5. in src/net_processing.cpp: in ee43109e63 outdated 
    802@@ -798,6 +803,11 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta
803                 Misbehaving(it->second.first, nDoS);
804         }
805     }
806+    else if (state.IsValid() && !IsInitialBlockDownload()) {
807+        if (it != mapBlockSource.end() && State(it->second.first)) {
    TheBlueMatt commented at 8:12 am on December 22, 2016:
    nit: maybe move the State() check inside MaybeSet…?
  6. in src/net_processing.cpp: in ee43109e63 outdated 
    802@@ -798,6 +803,11 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta
803                 Misbehaving(it->second.first, nDoS);
804         }
805     }
806+    else if (state.IsValid() && !IsInitialBlockDownload()) {
807+        if (it != mapBlockSource.end() && State(it->second.first)) {
808+            MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, *connman);
    TheBlueMatt commented at 8:27 am on December 22, 2016:

    Ideally this would go in UpdatedBlockTip instead. mapBlockSource wouldn’t be available there, but BlockChecked isnt really capturing where we want this - we want to know that this is actually the best currently available block, not just a block that was just checked.

    Not really sure how to fix this - maybe just a mapBlocksInFlight.empty() is sufficient, but its still super ugly.

    instagibbs commented at 1:53 pm on December 22, 2016:
    Could we change the lifetime of mapBlockSource to keep it around until then? We could sweep it later.
  7. TheBlueMatt commented at 8:54 am on December 22, 2016: member

    On second thought, I suppose I could be convinced that using BlockChecked here was OK if there were very explicit documentation on when it is/isn’t called in validationinterface.h and maybe some flags passed in to further disambiguate.

    On December 21, 2016 8:22:02 AM PST, Gregory Sanders notifications@github.com wrote:

    @rebroad This code doesn’t change any inv behavior.

    – You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9400#issuecomment-268565152

  8. TheBlueMatt commented at 0:05 am on December 23, 2016: member

    Changing the lifetime is hard, as only BlockChecked is guaranteed to be called if the block’s PoW is invalid.

    On December 22, 2016 5:53:40 AM PST, Gregory Sanders notifications@github.com wrote:

    instagibbs commented on this pull request.

    @@ -798,6 +803,11 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta Misbehaving(it->second.first, nDoS); } }

    • else if (state.IsValid() && !IsInitialBlockDownload()) {
    •    if (it != mapBlockSource.end() && State(it->second.first)) {

    •        MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first,

    *connman);

    Could we change the lifetime of mapBlockSource to keep it around until then? We could sweep it later.

    – You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9400

  9. in src/net_processing.cpp: in ee43109e63 outdated 
    440+                lNodesAnnouncingHeaderAndIDs.pop_front();
441+            }
442+            fAnnounceUsingCMPCTBLOCK = true;
443+            connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SENDCMPCT, fAnnounceUsingCMPCTBLOCK, nCMPCTBLOCKVersion));
444+            lNodesAnnouncingHeaderAndIDs.push_back(pfrom->GetId());
445+            return true;
    rebroad commented at 2:54 am on December 26, 2016:
    why return true? it’s a void function.
    rebroad commented at 3:31 am on December 27, 2016:
    I was referring to MaybeSetPeerAsAnnouncingHeaderAndIDs()… was misreading the flow of the code. Sorry.
  10. rebroad commented at 5:19 pm on December 26, 2016: contributor

    Not sure if this is behaving as it’s supposed to:-

     02016-12-26 17:13:12.258123 UpdateTip: new best=00000000000000000153e83f7511f8b104912bc578ad69d5ee5f138bd418a4cf (445158) ver=0x20000000 age=10.2h workage=9.3h tx=448
 12016-12-26 17:13:13.525927 send sendcmpct (no announce) ver=1 peer=2
 22016-12-26 17:13:13.526175 send sendcmpct (announce) ver=1 peer=9
 32016-12-26 17:13:13.583144 UpdateTip: new best=00000000000000000185ee972011605697340a4a4fa8ba778a93bc61a95dc315 (445159) ver=0x20000000 age=9.8h workage=9.2h tx=2823
 42016-12-26 17:13:16.007449 UpdateTip: new best=0000000000000000028907ba1fd6f492cdccfd0be79d11c6c43352834964378e (445160) ver=0x20000000 age=9.8h workage=9.0h tx=929
 52016-12-26 17:13:16.143575 send sendcmpct (no announce) ver=1 peer=12
 62016-12-26 17:13:16.143707 send sendcmpct (announce) ver=1 peer=2
 72016-12-26 17:13:16.150024 UpdateTip: new best=0000000000000000034a8dca7fe1801ed65e03aaeff94eeb34236cda2808f19d (445161) ver=0x20000000 age=9.8h workage=8.8h tx=175
 82016-12-26 17:13:18.224032 send sendcmpct (no announce) ver=1 peer=8
 92016-12-26 17:13:18.224165 send sendcmpct (announce) ver=1 peer=4
102016-12-26 17:13:18.280457 UpdateTip: new best=000000000000000001bfb4c8427fd5d8256d899d60411c339c52b1c87e6df049 (445162) ver=0x20000002 age=9.5h workage=8.7h tx=1608
112016-12-26 17:13:19.570130 UpdateTip: new best=00000000000000000005546a59fcc5f489a6f0ea15111a137ebc9be21b646b8e (445163) ver=0x20000000 age=9.1h workage=8.5h tx=2579
122016-12-26 17:13:20.929863 send sendcmpct (no announce) ver=1 peer=9
132016-12-26 17:13:20.929987 send sendcmpct (announce) ver=1 peer=8
142016-12-26 17:13:20.985947 UpdateTip: new best=0000000000000000003c52ad03a94a8da08e2bce3c76430d798fba39d1083ff7 (445164) ver=0x20000000 age=9.1h workage=8.3h tx=2110

    (there are no blocks being received, it’s just catching up with the chain using blocks already downloaded quite some time ago)

  11. TheBlueMatt commented at 5:39 pm on December 26, 2016: member

    @rebroad can you try adding a mapBlocksInFlight.empty() check prior to sending the sendcmpct message in the function? I believe that should (at least mostly) fix the issue.

    On December 26, 2016 12:19:27 PM EST, R E Broadley notifications@github.com wrote:

    Not sure if this is behaving as it’s supposed to:-

     02016-12-26 17:13:12.258123 UpdateTip: new
 1best=00000000000000000153e83f7511f8b104912bc578ad69d5ee5f138bd418a4cf
 2(445158) ver=0x20000000 age=10.2h workage=9.3h tx=448
 32016-12-26 17:13:13.525927 send sendcmpct (no announce) ver=1 peer=2
 42016-12-26 17:13:13.526175 send sendcmpct (announce) ver=1 peer=9
 52016-12-26 17:13:13.583144 UpdateTip: new
 6best=00000000000000000185ee972011605697340a4a4fa8ba778a93bc61a95dc315
 7(445159) ver=0x20000000 age=9.8h workage=9.2h tx=2823
 82016-12-26 17:13:16.007449 UpdateTip: new
 9best=0000000000000000028907ba1fd6f492cdccfd0be79d11c6c43352834964378e
10(445160) ver=0x20000000 age=9.8h workage=9.0h tx=929
112016-12-26 17:13:16.143575 send sendcmpct (no announce) ver=1 peer=12
122016-12-26 17:13:16.143707 send sendcmpct (announce) ver=1 peer=2
132016-12-26 17:13:16.150024 UpdateTip: new
14best=0000000000000000034a8dca7fe1801ed65e03aaeff94eeb34236cda2808f19d
15(445161) ver=0x20000000 age=9.8h workage=8.8h tx=175
162016-12-26 17:13:18.224032 send sendcmpct (no announce) ver=1 peer=8
172016-12-26 17:13:18.224165 send sendcmpct (announce) ver=1 peer=4
182016-12-26 17:13:18.280457 UpdateTip: new
19best=000000000000000001bfb4c8427fd5d8256d899d60411c339c52b1c87e6df049
20(445162) ver=0x20000002 age=9.5h workage=8.7h tx=1608
212016-12-26 17:13:19.570130 UpdateTip: new
22best=00000000000000000005546a59fcc5f489a6f0ea15111a137ebc9be21b646b8e
23(445163) ver=0x20000000 age=9.1h workage=8.5h tx=2579
242016-12-26 17:13:20.929863 send sendcmpct (no announce) ver=1 peer=9
252016-12-26 17:13:20.929987 send sendcmpct (announce) ver=1 peer=8
262016-12-26 17:13:20.985947 UpdateTip: new
27best=0000000000000000003c52ad03a94a8da08e2bce3c76430d798fba39d1083ff7
28(445164) ver=0x20000000 age=9.1h workage=8.3h tx=2110

    – You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9400#issuecomment-269228342

  12. instagibbs force-pushed on Dec 26, 2016
  13. instagibbs commented at 6:14 pm on December 26, 2016: member
    Updated to hopefully fix issue @rebroad
  14. in src/net_processing.cpp: in bbdf9563b4 outdated 
    397@@ -398,7 +398,7 @@ void UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
398 void MaybeSetPeerAsAnnouncingHeaderAndIDs(NodeId nodeid, CConnman& connman) {
399     AssertLockHeld(cs_main);
400     CNodeState* nodestate = State(nodeid);
401-    if (!nodestate->fSupportsDesiredCmpctVersion) {
402+    if (!nodestate || !nodestate->fSupportsDesiredCmpctVersion) {
    rebroad commented at 3:52 am on December 27, 2016:
    nodestate can be NULL…?
    instagibbs commented at 2:01 pm on December 27, 2016:
    yes?
    TheBlueMatt commented at 2:06 pm on December 27, 2016:
    If the peer is disconnected between when it provides the block and when the block is connected it can, sure.
  15. sipa commented at 7:04 pm on December 27, 2016: member
    utACK bbdf9563b41ce9a7f032e19afbba1f38fabefa7e
  16. morcos commented at 8:32 pm on December 27, 2016: member

    I think we should have more discussion on whether this is the right logic to implement and not just is the code correct for implementing it. My guess is there is little benefit from this code and its maybe not worth the risk of introducing a change until we have something that will provide a more significant benefit.

    It’s already the case that the first peer to announce anything to you is the only peer that you will allow to deliver you the full block (barring a full timeout), so for the most part this doesn’t change who can be considered for HB peer. The exception to this is peers that are already HB peers and give you cmpctblocks that get optimistically reconstructed. Those peers are already HB so it doesn’t help them, but it does prevent any peer who had previously headered you but not yet delivered the block from being considered. But this is a minor roadblock for any attacker (or just inadvertent bad behavior) because as soon as you have a cmpctblock that doesn’t optimistically reconstruct, you’re back to the base case.

    That said I think this is the general right idea, but I’d just prefer to see it in the context of a larger change so that we’re not hacking around in this code repeatedly and at risk of introducing another bug. There is definitely an improvement needed in better maintaining good HB peers.

    But perhaps I’m wrong and the benefit from this code is worth it on its own?

  17. rebroad commented at 2:31 am on December 28, 2016: contributor

    @morcos completely agree - and I think I’m testing this code only because it potentially compliments other code I have added myself (e.g. requesting blocktxns from more then one node), but without such further changes, this code alone, at this stage, doesn’t really provide any benefit from what I can see either, and needs to be combined with other improvements (i.e. more refined versions of what I’m also hackily producing).

    Having said that, I don’t think using the PR system to share our hacks (that we may often think are more complete than they are) is intrinsically a bad thing, and the review system is designed to help reveal the level of completeness of a PR raised, or dependencies needed, as we are doing.

  18. rebroad commented at 11:15 am on December 28, 2016: contributor

    @instagibbs ok, looking at this debug.log output:-

    02016-12-28 09:05:41.928636 recv new best header 00000000000000000320a325788953e769b47205654f55ea80a4c2e108a1189a (445473) age=21s peer=813
12016-12-28 09:05:41.928868 send getdata cmpctblock 00000000000000000320a325788953e769b47205654f55ea80a4c2e108a1189a (445473) age=21s peer=813
22016-12-28 09:05:42.085294 recv best cmpctblock 00000000000000000320a325788953e769b47205654f55ea80a4c2e108a1189a (445473) age=22s size=11814 peer=736
32016-12-28 09:05:42.089384 send getblocktxn 00000000000000000320a325788953e769b47205654f55ea80a4c2e108a1189a (445473) age=22s indexes=1/1933 size=60 peer=736
42016-12-28 09:05:42.479448 recv best cmpctblock 00000000000000000320a325788953e769b47205654f55ea80a4c2e108a1189a (445473) age=22s size=11814 peer=813
52016-12-28 09:05:42.587522 recv blocktxn 00000000000000000320a325788953e769b47205654f55ea80a4c2e108a1189a (445473) age=22s indexes=1 size=256 expected-peer=813 peer=736
62016-12-28 09:05:42.612230 made block 00000000000000000320a325788953e769b47205654f55ea80a4c2e108a1189a size=998115 txs: mempool=1931 req=1

    we can see that peer 813 is the first to advertise the block, but if we change the logic to fetch blocktxns from the first peer to provide the compact block, then often the one that provided the header will not be the one to provide the block as it requries an extra round trip to request it, so it’ll often not be asked to announce with compact blocks, even though this might mean it then becomes the best block provider - how do we deal with this situation? (Does it need dealing with?) I think the only way to determine if peer 813 would be better would be to measure it’s bandwidth and work out whether it would have sent the larger message (the compact block of 11814 bytes) before the other peers were able to.

  19. morcos commented at 12:50 pm on December 28, 2016: member
    @rebroad are you also running modified logic above? In master, we would not have requested blocktxn from peer 736 in your example.
  20. rebroad commented at 7:58 am on December 29, 2016: contributor

    @morcos Yes, I am running this PR combined with some of my changes - in an aim to make the changes this PR provides more useful, although I think further refinement is still needed as mentioned in my previous comment.

    …ah. I see what you are saying now… yes without my changes, peer 813 would have provided the block and therefore been requested to announce as compact blocks going forward, but at the cost of receiving the block more slowly. My proposal in my previous comment was a way in which we could get the block more quickly AND ensure we are requested compact block announcements from the correct peers - assuming both these things are important.

  21. in src/net_processing.cpp: in bbdf9563b4 outdated 
    802@@ -798,6 +803,11 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta
803                 Misbehaving(it->second.first, nDoS);
804         }
805     }
806+    else if (state.IsValid() && !IsInitialBlockDownload() && mapBlocksInFlight.empty()) {
    morcos commented at 5:27 pm on December 29, 2016:
    I don’t think this check of mapBlocksInFlight.empty() will play nicely with #9352 which doesn’t MarkBlockReceived until after ProcessNewBlock
    morcos commented at 7:49 pm on December 29, 2016:
    perhaps mapBlocksInFlight.count(hash) == mapBlocksInFlight.size() instead?
  22. gmaxwell commented at 2:39 am on December 30, 2016: contributor

    @morcos I think this is the right logic: We should pick for HB usage peers that actually helped us get the block, not one that merely said they would.

    Beyond the slightly improved misbehavior resistance, this should reduce a small bias towards non-HB (or non-predicting HB) peers that arises because their announcement has less bytes on the wire, so it transmits somewhat faster even though it results in block recovery somewhat slower. Perhaps it over corrects for it somewhat: in a world without attacks you’d promote to HB status based on time of announcement adjusted by the serialization size (“who would have been first if all were sending CBs?”)– but we don’t know the adjustment and we’re not in a world without attacks. The net result of the over-correction is just a little hysteresis– we’re more likely to keep our existing HB set unless someone was faster by at least a RTT–, easily justified by the fact that changing HB selection isn’t completely free.

    Calling maybe on a peer that is already HB is not a no-op: they get moved to the front of the list, which makes them less likely to get dropped.

    I do think this is more interesting once we have multiple reconstructions in flight.

  23. morcos commented at 2:23 pm on December 30, 2016: member
    @gmaxwell @instagibbs Yes, after spending more time reviewing this, I agree it is the right approach, and its simpler than I thought. Still suggest the fix above, but otherwise ACK.
  24. Set peers as HB peers upon full block validation d4781ac6c2
  25. instagibbs force-pushed on Dec 30, 2016
  26. instagibbs commented at 7:29 pm on December 30, 2016: member
    @morcos fixed and squashed
  27. morcos commented at 3:54 pm on January 14, 2017: member

    tested ACK d4781ac

    I think this would be nice for 0.14

  28. TheBlueMatt commented at 8:47 pm on January 14, 2017: member
    utACK d4781ac6c26399d316a2b25f656d9a4c0990c419
  29. gmaxwell commented at 3:48 pm on January 15, 2017: contributor
    utACK.
  30. sipa merged this on Jan 15, 2017
  31. sipa closed this on Jan 15, 2017
  32. sipa referenced this in commit 8a445c5651 on Jan 15, 2017
  33. gladcow referenced this in commit 5a75b8de73 on Mar 8, 2018
  34. gladcow referenced this in commit 30e2ed4cec on Mar 13, 2018
  35. gladcow referenced this in commit 0a05fbf1a1 on Mar 14, 2018
  36. gladcow referenced this in commit 7be5158b79 on Mar 15, 2018
  37. gladcow referenced this in commit a362a9d947 on Mar 15, 2018
  38. gladcow referenced this in commit 3fc45d2c2a on Mar 15, 2018
  39. gladcow referenced this in commit c105d76e1e on Mar 15, 2018
  40. gladcow referenced this in commit b046bd3b5b on Mar 24, 2018
  41. gladcow referenced this in commit 2f8f47557e on Apr 4, 2018
  42. UdjinM6 referenced this in commit bc45a2f87a on Apr 11, 2018
  43. andvgal referenced this in commit fd5c50bc2b on Jan 6, 2019
  44. CryptoCentric referenced this in commit dd3fd51204 on Feb 28, 2019
  45. MarcoFalke locked this on Sep 8, 2021


instagibbs rebroad TheBlueMatt MarcoFalke sipa morcos 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-07-06 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me