-
Notifications
You must be signed in to change notification settings - Fork 37.8k
Improve logic for advertising blocks #8958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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! |
@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. |
a591a32
to
bccc2f3
Compare
Personally, I'd highly appreciate a constant variable above for readability. |
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. |
bccc2f3
to
1134b70
Compare
@dcousens Have amended since your comment - it should be more readable now. |
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! |
Needs rebase if still relevant. |
1134b70
to
1120d9d
Compare
@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. |
1120d9d
to
9bde417
Compare
@rebroad During IBD, nNewHeight is always larger than any pindexLastCommonBlock, so this removes the protection the original code was intended to give. |
Revisit #4846 (comments were to wait until headers first had been tested sufficiently first!)