Improve logic for advertising blocks #8958

1. 
   rebroad commented at 3:42 pm on October 18, 2016: contributor

   Revisit #4846 (comments were to wait until headers first had been tested sufficiently first!)
2. laanwj added the label P2P on Oct 18, 2016
3. 
   paveljanik commented at 7:54 pm on October 18, 2016: contributor

   OMG, do we really use the similar code somewhere else? Using ternary operator twice on one line and without parens is getting closer to the programmers hell!
4. 
   rebroad commented at 0:29 am on October 19, 2016: contributor

   @paveljanik It’s similar to 2 * 3 + 1 which, as far as I know, most people don’t use brackets for as it’s understood what order the numbers are combined.

   Rebased.

5. rebroad closed this on Oct 19, 2016

   ---

6. rebroad reopened this on Oct 19, 2016

   ---

7. rebroad force-pushed on Oct 19, 2016
8. 
   dcousens commented at 0:58 am on October 19, 2016: contributor

   Personally, I’d highly appreciate a constant variable above for readability.
9. 
   gmaxwell commented at 1:13 am on October 19, 2016: contributor

   This won’t compile, it’s reintroducing a checkpoints dependency (nBlockEstimate) that was recently removed by 0278fb5 in PR #8865 (the whole block is not entered if we’re in initial block download, and if we’re not… then any block we’d consider would already be past the estimator that it gives us).

   For future PR’s it would be much more helpful to write what a change intends to accomplish (and, especially, WHY) in terms more specific than “improve”. :) Hopefully every change is an improvement.

10. rebroad force-pushed on Oct 19, 2016
11. 
    rebroad commented at 3:29 am on October 19, 2016: contributor

    @gmaxwell Yes, I realise that #8865 broke it. Have now rebased correctly. Apologies for this.

    I also agree that the word improve is entirely redundant if we assume that all PR are intended to improve :) (I tend to avoid assuming to the point of not even making common sense ones!)

12. 
    rebroad commented at 2:25 am on October 20, 2016: contributor

    @dcousens Have amended since your comment - it should be more readable now.
13. dcousens approved
14. 
    sdaftuar commented at 1:19 pm on October 23, 2016: member

    NACK. This is thoroughly busted – first you need cs_main here to do what you’re trying, second you can’t skip announcing a block to a peer just because you think it’s on a longer (not necessarily more work) chain than yours!
15. 
    MarcoFalke commented at 6:09 pm on December 6, 2016: member

    Needs rebase if still relevant.
16. rebroad renamed this:
    Improve logic for advertising blocks
    WIP: Improve logic for advertising blocks
    on Dec 11, 2016
17. rebroad force-pushed on Dec 11, 2016
18. rebroad renamed this:
    WIP: Improve logic for advertising blocks
    Improve logic for advertising blocks
    on Dec 11, 2016
19. 
    rebroad commented at 7:40 am on December 11, 2016: contributor

    @sdaftuar Not sure it’s “thoroughly” busted but your comment has helped me to notice a mistake in the coding - it should be pindexLastCommonBlock not pindexBestKnownBlock that I am using. Making this change now.
20. Improve logic for advertising blocks 9bde4175c6
21. rebroad force-pushed on Dec 11, 2016
22. 
    sipa commented at 3:35 pm on April 9, 2017: member

    @rebroad During IBD, nNewHeight is always larger than any pindexLastCommonBlock, so this removes the protection the original code was intended to give.
23. fanquake closed this on May 17, 2017

    ---

24. DrahtBot locked this on Sep 8, 2021

Contributors
rebroad paveljanik dcousens gmaxwell sdaftuar MarcoFalke sipa

Labels
P2P