Don't do mempool lookups for "mempool" command without a filter #7174

Merged
merged 1 commit into from Dec 5, 2015

Conversation

Projects
None yet
8 participants
@TheBlueMatt
Contributor

TheBlueMatt commented Dec 5, 2015

No description provided.

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 5, 2015

Contributor

utACK

Contributor

pstratem commented Dec 5, 2015

utACK

@pstratem

This comment has been minimized.

Show comment
Hide comment
@pstratem

pstratem Dec 5, 2015

Contributor

This performance regression was introduced in 319b116

Contributor

pstratem commented Dec 5, 2015

This performance regression was introduced in 319b116

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Dec 5, 2015

Contributor

utACK

Contributor

petertodd commented Dec 5, 2015

utACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Dec 5, 2015

Contributor

utACK

Contributor

jgarzik commented Dec 5, 2015

utACK

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Dec 5, 2015

Member

ACK. Good catch, it makes a huge performance difference with a large mempool. The problem still exists with a filter set, but it's a nice step forward. This change also makes the code simpler and clearer, which is always a bonus. I think we should take this back to 0.12 as well.

Member

gmaxwell commented Dec 5, 2015

ACK. Good catch, it makes a huge performance difference with a large mempool. The problem still exists with a filter set, but it's a nice step forward. This change also makes the code simpler and clearer, which is always a bonus. I think we should take this back to 0.12 as well.

@gmaxwell gmaxwell merged commit 96918a2 into bitcoin:master Dec 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

gmaxwell added a commit that referenced this pull request Dec 5, 2015

Merge pull request #7174
96918a2 Don't do mempool lookups for "mempool" command without a filter (Matt Corallo)

@gmaxwell gmaxwell added this to the 0.12.0 milestone Dec 6, 2015

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 7, 2015

Contributor

utACK

Contributor

dcousens commented Dec 7, 2015

utACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 7, 2015

Member

Posthumous ACK on the code change.
But this should have had a description and motivation in the commit message and PR description.

Member

laanwj commented Dec 7, 2015

Posthumous ACK on the code change.
But this should have had a description and motivation in the commit message and PR description.

laanwj added a commit that referenced this pull request Dec 7, 2015

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 7, 2015

Member

Agreed, I was confused as to its behaviour for a while. The description given sounds like "mempool" wouldn't work at all with non-filter connections.

Member

luke-jr commented Dec 7, 2015

Agreed, I was confused as to its behaviour for a while. The description given sounds like "mempool" wouldn't work at all with non-filter connections.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Dec 7, 2015

Contributor

Da. The description confused several devs...

Contributor

jgarzik commented Dec 7, 2015

Da. The description confused several devs...

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Dec 7, 2015

Member

Ah, I didn't see the potential for confusion it all. Though the description was complete and accurate, what "mempool lookup" means and is ambigious I could see it how you might extract that from the description without reading the code.

For short patches I try to read the code before the description, so as to not be biased by what it says it does and fail to see what it actually does. I would have expected with three acks from regulars that someone would have pointed out something like that earlier. Live and learn.

Member

gmaxwell commented Dec 7, 2015

Ah, I didn't see the potential for confusion it all. Though the description was complete and accurate, what "mempool lookup" means and is ambigious I could see it how you might extract that from the description without reading the code.

For short patches I try to read the code before the description, so as to not be biased by what it says it does and fail to see what it actually does. I would have expected with three acks from regulars that someone would have pointed out something like that earlier. Live and learn.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Dec 8, 2015

Contributor

Title made sense to me, I can understand how the misconception could exist, but, upon reading the code it was clear that it was just moving the branching logic around such that mempool.lookup(...) is only being done when necessary instead of pre-emptively.

Contributor

dcousens commented Dec 8, 2015

Title made sense to me, I can understand how the misconception could exist, but, upon reading the code it was clear that it was just moving the branching logic around such that mempool.lookup(...) is only being done when necessary instead of pre-emptively.

+ bool fInMemPool = mempool.lookup(hash, tx);
+ if (!fInMemPool) continue; // another thread removed since queryHashes, maybe...
+ if (!pfrom->pfilter->IsRelevantAndUpdate(tx)) continue;
+ }

This comment has been minimized.

@dcousens

dcousens Dec 8, 2015

Contributor

There is 1 other change that wasn't described here, which is !IsRelevantAndUpdate and vInv.size() === MAX_INV_SZ would previously cause a "inv" message to be pushed.
This does not occur now.

However, given the logic that vInv.size() is only increased if isRelevantAndUpdate was true anyway, means AFAIK this shouldn't be an issue.

I guess, that is a minor performance optimization then 👍 😃

@dcousens

dcousens Dec 8, 2015

Contributor

There is 1 other change that wasn't described here, which is !IsRelevantAndUpdate and vInv.size() === MAX_INV_SZ would previously cause a "inv" message to be pushed.
This does not occur now.

However, given the logic that vInv.size() is only increased if isRelevantAndUpdate was true anyway, means AFAIK this shouldn't be an issue.

I guess, that is a minor performance optimization then 👍 😃

@str4d str4d referenced this pull request in zcash/zcash Jul 14, 2017

Open

Bitcoin 0.12 P2P/Net PRs 1 #2534

@str4d str4d referenced this pull request in zcash/zcash Dec 19, 2017

Merged

Add NODE_BLOOM service bit #2814

zkbot added a commit to zcash/zcash that referenced this pull request Apr 5, 2018

Auto merge of #2814 - str4d:2074-node-bloom, r=str4d
Add NODE_BLOOM service bit

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6579
  - Zcash equivalent of BIP 111
- bitcoin/bitcoin#6652
  - Docs for BIP 111
- bitcoin/bitcoin#7087
- bitcoin/bitcoin#7174
- bitcoin/bitcoin#8709

Part of #2074. Closes #2738.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment