Use RelevantServices instead of node_network in AttemptToEvict. #9052

Merged
merged 1 commit into from Nov 7, 2016

Projects

None yet

7 participants

@gmaxwell
Member
gmaxwell commented Nov 1, 2016

Use of node_network here is really meant to be a proxy of "likely to
send us blocks in the future". RelevantServices is the right criteria
now.

@gmaxwell gmaxwell Use RelevantServices instead of node_network in AttemptToEvict.
Use of node_network here is really meant to be a proxy of "likely to
 send us blocks in the future".  RelevantServices is the right criteria
 now.
d32036a
@instagibbs
Contributor

utACK d32036a

@hearbeat
hearbeat commented Nov 2, 2016

good

@laanwj laanwj added the P2P label Nov 2, 2016
@@ -850,7 +850,7 @@ static bool CompareNodeBlockTime(const NodeEvictionCandidate &a, const NodeEvict
{
// There is a fall-through here because it is common for a node to have many peers which have not yet relayed a block.
if (a.nLastBlockTime != b.nLastBlockTime) return a.nLastBlockTime < b.nLastBlockTime;
- if (a.fNetworkNode != b.fNetworkNode) return b.fNetworkNode;
+ if (a.fRelevantServices != b.fRelevantServices) return b.fRelevantServices;
@sipa
sipa Nov 3, 2016 Member

Shouldn't we return a.fRelevantServices here? The return value true indicates whether a is better than b.

@gmaxwell
gmaxwell Nov 4, 2016 Member

We want the best things to sort last, because the last entries from the sort are protected from eviction.

@sipa
sipa Nov 4, 2016 Member

Oh, right. I'm sure I've gone through this code already and made the same mistake.

@gmaxwell
gmaxwell Nov 5, 2016 edited Member

Yes. :)

@laanwj laanwj merged commit d32036a into bitcoin:master Nov 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Nov 7, 2016
@laanwj laanwj Merge #9052: Use RelevantServices instead of node_network in AttemptT…
…oEvict.


d32036a Use RelevantServices instead of node_network in AttemptToEvict. (Gregory Maxwell)
c113a65
@sdaftuar
Contributor

@gmaxwell Should this be backported to 0.13?

@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 2, 2016
@gmaxwell @luke-jr gmaxwell + luke-jr Use RelevantServices instead of node_network in AttemptToEvict.
Use of node_network here is really meant to be a proxy of "likely to
 send us blocks in the future".  RelevantServices is the right criteria
 now.

Github-Pull: #9052
Rebased-From: d32036a
3a3bcbf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment