Skip to content

Conversation

rebroad
Copy link
Contributor

@rebroad rebroad commented Oct 18, 2016

Revisit #4846 (comments were to wait until headers first had been tested sufficiently first!)

@laanwj laanwj added the P2P label Oct 18, 2016
@paveljanik
Copy link
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!

@rebroad
Copy link
Contributor Author

rebroad commented Oct 19, 2016

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

@rebroad rebroad closed this Oct 19, 2016
@rebroad rebroad reopened this Oct 19, 2016
@rebroad rebroad force-pushed the BetterBroadcastLogic branch from a591a32 to bccc2f3 Compare October 19, 2016 00:33
@dcousens
Copy link
Contributor

Personally, I'd highly appreciate a constant variable above for readability.

@gmaxwell
Copy link
Contributor

gmaxwell commented Oct 19, 2016

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.

@rebroad
Copy link
Contributor Author

rebroad commented Oct 19, 2016

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

@rebroad
Copy link
Contributor Author

rebroad commented Oct 20, 2016

@dcousens Have amended since your comment - it should be more readable now.

@sdaftuar
Copy link
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!

@maflcko
Copy link
Member

maflcko commented Dec 6, 2016

Needs rebase if still relevant.

@rebroad rebroad changed the title Improve logic for advertising blocks WIP: Improve logic for advertising blocks Dec 11, 2016
@rebroad rebroad force-pushed the BetterBroadcastLogic branch from 1134b70 to 1120d9d Compare December 11, 2016 05:56
@rebroad rebroad changed the title WIP: Improve logic for advertising blocks Improve logic for advertising blocks Dec 11, 2016
@rebroad
Copy link
Contributor Author

rebroad commented Dec 11, 2016

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

@rebroad rebroad force-pushed the BetterBroadcastLogic branch from 1120d9d to 9bde417 Compare December 11, 2016 07:52
@sipa
Copy link
Member

sipa commented Apr 9, 2017

@rebroad During IBD, nNewHeight is always larger than any pindexLastCommonBlock, so this removes the protection the original code was intended to give.

@fanquake fanquake closed this May 17, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants