-
Notifications
You must be signed in to change notification settings - Fork 969
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
Pull-mode flooding #3479
Pull-mode flooding #3479
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a preliminary pass, thanks for putting this together! Couple of questions, and I think we need to move the code around to appropriate modules.
9913e46
to
ac30b5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for the updates! I did another round, and I think there are a few things that can be causing instabilities you're seeing
98adf39
to
c238aff
Compare
fd66a5b
to
d8965a6
Compare
ed17d4b
to
44db5f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look good to me, thank you! Just a few minor things. At this point I think @MonsieurNicolas should take a look.
I have one remaining question regarding tx queue flooding that I'd like to get input on. Right now because we have a timer for adverts and a timer for tx flooding, we may end up in an awkward situation where advert flooding is delayed by FLOOD_TX_PERIOD_MS + FLOOD_ADVERT_PERIOD_MS in the worst case scenario, which doesn't seem ideal, but I'm not sure how big of a problem it is. The reason is that we wanted tx advertising to be self-contained in overlay. Otherwise, tx queue needed to trigger advert flushing manually, which would violate abstraction barriers. Touching the tx queue timer itself didn't seem like an option as well, since there's non-trivial functionality to order txs there.
a few more things:
|
I updated the |
bc35de7
to
4b2c8b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed most of the code. Looks pretty good. Comments are mostly around cleanup.
// Pull mode | ||
case FLOOD_ADVERT: | ||
FloodAdvert floodAdvert; | ||
case FLOOD_DEMAND: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am getting an error trying to pull your branch because of the commit referenced by the protocol-next xdr.
fatal: Fetched in submodule path 'src/protocol-next/xdr', but it did not contain c59b1a13a587aa5aaf3bed97c01a5de559ff6650. Direct fetching of that commit failed.
I think we should just merge the xdr changes in master in xdr next when we're ready to merge this PR, that way we can point to a stable commit hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to open a PR in xdr vNext repo and bump version submodule in this PR after the other one is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a PR stellar/stellar-xdr#24 (not ready to be merged yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge that other PR (ahead of merging this change) -> 1 action item in that PR for you @hidenori-shinohara
401fee6
to
e55a2c6
Compare
1db66f8
to
f06c787
Compare
a7848ba
to
bdded33
Compare
src/overlay/Peer.cpp
Outdated
while (mAdvertQueueTxHashCount > limit) | ||
{ | ||
dropped++; | ||
mAdvertQueueTxHashCount -= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why you made it signed instead of just following the pattern above: you still need to add asserts in the code base, and now you also introduce a bunch of warnings because int
and size_t
don't mix well
src/overlay/OverlayManagerImpl.cpp
Outdated
OverlayManagerImpl::getMaxDemandSize() const | ||
{ | ||
auto const& cfg = mApp.getConfig(); | ||
double ledgerCloseTime = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting warning: int64->double conversion (need explicit cast to avoid it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why you used long
here, but now the warning is int64 -> long
; as you're now using bigDivide
you can just use int64
here.
(same problem in getMaxAdvertSize
)
bdded33
to
ae4ee23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks very close!
src/overlay/Peer.cpp
Outdated
{ | ||
msgQInd = 2; | ||
size_t s = msg->floodDemand().txHashes.size(); | ||
releaseAssert(s <= INT32_MAX - mDemandQueueTxHashCount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
condition is wrong (wrong boundary), and I'm still getting warnings here because you made mDemandQueueTxHashCount
an uint32
and the compiler does not like += size_t
. We can remove this assert and use size_t instead (pushed a commit for you with those changes as we've been bouncing over warnings that you cannot see)
src/overlay/Peer.cpp
Outdated
while (mAdvertQueueTxHashCount > limit) | ||
{ | ||
dropped++; | ||
mAdvertQueueTxHashCount -= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still getting warnings because you're using uint32 and doing things like u32 -= u64
. I am pushing an update to your branch. there is no need to use u32 when u64 is perfectly fine here (and we don't need to worry about overflow)
80c9c47
to
01ed73e
Compare
src/overlay/Peer.cpp
Outdated
} | ||
}, | ||
"sendTxDemand"); | ||
getOverlayMetrics().mMessagesDemanded.Mark(demands.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demans
was moved from, so this metric always reports 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed it for CLOG_TRACE
above when I was looking at logs, but I completely missed this. Thank you! Fixed it.
send demands appropriately
01ed73e
to
88da68c
Compare
r+ 88da68c |
Description
Resolves #3384.
Based on #2432. Some differences in the design are:
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)