improve addr/inv trickle logic #5989
Conversation
|
Any possibility to use the timer stuff being added in #5964 ? |
|
@jgarzik maybe later, porting wont be difficult |
src/main.cpp
Outdated
There was a problem hiding this comment.
Could that magic number be defined somehwere as constant? IMHO it's used quite often in this pull.
There was a problem hiding this comment.
At a later date in a different PR
|
Concept ACK. I love getting rid of the vSendTrickle logic... |
|
I'm with @jgarzik: this could be much cleaner built on top of #5964. All of the pnode->nNext* variables could go away; just schedule a task that takes a NodeId and Does The Right Thing at some (random) time in the future. (the tasks would lock cs_vNodes and do the NodeId to CNode* lookup... probably don't need to optimize that lookup, looping through even a 1,000-peer vector isn't a significant amount of CPU time). |
|
Unsure. I don't think running the scheduled operations makes sense if there
is nothing else to do for a node, and there almost always is, at which time
we already hold the necessary locks.
Small benefit, small cost.
|
|
@sipa : benefit would be clearer logic. I find the "set a variable here, test it over there and then do something" hard to review/follow. |
|
@gavinandresen As I said to @jgarzik it's easy enough to back port this logic into CSchedule. I would rather not tie the two PR's together. |
src/main.cpp
Outdated
There was a problem hiding this comment.
Integer overflow on this line. You probably want to cast to int64_t first.
|
@gavinandresen Yes, that's the small benefit. The small cost is extra locking/work. Not opposed to it, but it's not as clear an advantage as replacing the several pieces of code we had that were already using their own thread. This one doesn't need it. |
|
Can you fix the integer overflow, and perhaps rebase on top of #6066? |
|
Rebase please? |
|
Needs rebase, and fixing the integer overflow. |
|
@pstratem ping |
|
@sipa pong |
clarify expression remove fSendTrickle flag shuffle inventory list to send (for good measure) slight peak memory improvement user timestamps for transaction trickle logic always send MSG_TX to whitelisted nodes
improves the reoslution to the polling frequency from 1 second
|
setAddrKnown was replaced with a CRollingBloomFilter d81cff3 Given this it seems reasonable to slowly unset bits of the bloomfilter rather than clearing the filter entirely every 24 hours. Thoughts? |
|
Scratch that. There's no need to clear the filter explicitly with the rolling bloom filter. |
|
@pstratem rebase/close? |
|
rebase please...
|
|
See #7125. |
|
we have better ideas on how to achieve the properties of the trickle logic (such as mempool sync at random interval) |
7 participants
The addr/msg_tx logic currently assumes that ProcessMEssage/SendMessage are called on a regular timer.
This was (sort of) true in the past but is not longer true after #5971
Critical analysis of this PR would be helpful as this goes beyond just fixing the timing issue.