Pull-mode flooding by hidenori-shinohara · Pull Request #3479 · stellar/stellar-core

hidenori-shinohara · 2022-07-07T21:51:09Z

Description

Resolves #3384.

Based on #2432. Some differences in the design are:

Implementation of flow control for adverts and demands
Use of SHA-256 to hash txns.
Round-robin when creating demands

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

marta-lokhova

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.

src/overlay/Peer.h

src/overlay/Peer.cpp

src/overlay/Floodgate.cpp

src/overlay/Peer.cpp

src/overlay/Floodgate.cpp

src/overlay/Floodgate.h

src/overlay/Floodgate.cpp

src/overlay/Floodgate.h

marta-lokhova

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

src/overlay/Peer.h

src/overlay/Floodgate.cpp

src/overlay/Peer.cpp

src/overlay/OverlayManagerImpl.cpp

src/overlay/Peer.cpp

src/overlay/OverlayManagerImpl.cpp

src/overlay/Peer.cpp

src/overlay/OverlayMetrics.cpp

src/overlay/Peer.cpp

src/overlay/test/OverlayTests.cpp

src/overlay/Floodgate.cpp

src/overlay/OverlayManagerImpl.cpp

src/overlay/OverlayManagerImpl.h

src/overlay/Floodgate.h

src/overlay/OverlayManagerImpl.cpp

marta-lokhova

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.

src/overlay/OverlayManagerImpl.cpp

src/overlay/OverlayManagerImpl.h

src/overlay/Peer.cpp

src/main/Config.cpp

marta-lokhova · 2022-08-02T17:05:18Z

a few more things:

we're missing the outbound queue drop metrics in this PR. I made the change for testing yesterday, feel free to just grab this commit: marta-lokhova@7bca3c3
based on what we discussed, I think doing pure round-robin is not ideal for the demand logic. Specifically, I'm thinking about the current network conditions: we have a limit of 1000 operations per ledger, with 40 ops broadcasted every 200 ms. This means that nodes with many connections (which is very common on the network) may end up sending a lot of small demands to its peers. This defeats the purpose of batching, and can get inefficient. To avoid this, I think it's better to iterate over all shuffled peers, and for each peer either fill the demand completely or until it has no more hashes. This way we have proper batching regardless of the number of connections, and load balancing due to peer randomization.

hidenori-shinohara · 2022-08-03T15:04:00Z

I updated the demand function so it'll take advantage of both shuffling and batching with a simpler implementation. @marta-lokhova let me know what you think

MonsieurNicolas

Reviewed most of the code. Looks pretty good. Comments are mostly around cleanup.

MonsieurNicolas · 2022-08-04T18:01:54Z

src/protocol-curr/xdr/Stellar-overlay.x

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.

Makes sense!

need to open a PR in xdr vNext repo and bump version submodule in this PR after the other one is merged

I opened a PR stellar/stellar-xdr#24 (not ready to be merged yet)

I think we can merge that other PR (ahead of merging this change) -> 1 action item in that PR for you @hidenori-shinohara

docs/stellar-core_example.cfg

src/herder/TransactionQueue.cpp

src/herder/Herder.h

src/overlay/Peer.cpp

src/overlay/Floodgate.cpp

src/overlay/test/FloodTests.cpp

src/overlay/OverlayMetrics.cpp

src/herder/Herder.h

src/herder/TxQueueLimiter.h

src/overlay/OverlayManager.h

src/overlay/OverlayManagerImpl.h

src/main/Config.cpp

src/overlay/OverlayManagerImpl.h

src/protocol-curr/xdr/Stellar-overlay.x

docs/stellar-core_example.cfg

src/overlay/OverlayManagerImpl.cpp

src/overlay/Peer.cpp

src/overlay/OverlayManagerImpl.cpp

src/overlay/test/TxAdvertQueueTests.cpp

MonsieurNicolas · 2022-08-31T22:17:22Z

src/overlay/Peer.cpp

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/Peer.cpp

MonsieurNicolas · 2022-08-31T23:04:19Z

src/overlay/OverlayManagerImpl.cpp

getting warning: int64->double conversion (need explicit cast to avoid it)

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)

src/overlay/OverlayManagerImpl.cpp

src/overlay/Peer.cpp

src/overlay/OverlayManagerImpl.cpp

src/overlay/test/TxDemandQueueTests.cpp

src/util/test/TimerTests.cpp

MonsieurNicolas

looks very close!

MonsieurNicolas · 2022-09-02T00:40:00Z

src/overlay/Peer.cpp

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)

MonsieurNicolas · 2022-09-02T00:44:13Z

src/overlay/Peer.cpp

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)

marta-lokhova · 2022-09-03T03:38:58Z

src/overlay/Peer.cpp

demans was moved from, so this metric always reports 0

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

marta-lokhova · 2022-09-13T19:55:52Z

r+ 88da68c

marta-lokhova self-requested a review July 7, 2022 22:04

marta-lokhova reviewed Jul 8, 2022

View reviewed changes

hidenori-shinohara force-pushed the pull-mode-decent-perf branch from 9913e46 to ac30b5c Compare July 13, 2022 18:58

marta-lokhova reviewed Jul 15, 2022

View reviewed changes

hidenori-shinohara force-pushed the pull-mode-decent-perf branch 4 times, most recently from 98adf39 to c238aff Compare July 25, 2022 14:35

hidenori-shinohara force-pushed the pull-mode-decent-perf branch 3 times, most recently from fd66a5b to d8965a6 Compare July 26, 2022 21:18

marta-lokhova reviewed Jul 27, 2022

View reviewed changes

src/overlay/OverlayManagerImpl.cpp Outdated Show resolved Hide resolved

marta-lokhova reviewed Jul 27, 2022

View reviewed changes

src/overlay/OverlayManagerImpl.cpp Outdated Show resolved Hide resolved

hidenori-shinohara force-pushed the pull-mode-decent-perf branch from ed17d4b to 44db5f0 Compare July 29, 2022 16:57

hidenori-shinohara marked this pull request as ready for review August 1, 2022 18:24

hidenori-shinohara changed the title ~~Draft: Pull-mode flooding~~ Pull-mode flooding Aug 1, 2022

marta-lokhova reviewed Aug 1, 2022

View reviewed changes

hidenori-shinohara force-pushed the pull-mode-decent-perf branch 2 times, most recently from bc35de7 to 4b2c8b1 Compare August 3, 2022 17:52

MonsieurNicolas requested changes Aug 6, 2022

View reviewed changes

dmkozh reviewed Aug 8, 2022

View reviewed changes

hidenori-shinohara force-pushed the pull-mode-decent-perf branch from 401fee6 to e55a2c6 Compare August 15, 2022 17:49

hidenori-shinohara force-pushed the pull-mode-decent-perf branch 3 times, most recently from 1db66f8 to f06c787 Compare August 23, 2022 19:43

This was referenced Aug 23, 2022

Pull mode assumes 1 txn = 1 op #3514

Open

Pull mode advertises a transaction to a node even if it has advertised it to the node #3515

Closed

hidenori-shinohara force-pushed the pull-mode-decent-perf branch 4 times, most recently from a7848ba to bdded33 Compare August 31, 2022 20:22

marta-lokhova reviewed Aug 31, 2022

View reviewed changes

src/overlay/OverlayManagerImpl.h Outdated Show resolved Hide resolved

src/protocol-curr/xdr/Stellar-overlay.x Outdated Show resolved Hide resolved

marta-lokhova reviewed Aug 31, 2022

View reviewed changes

docs/stellar-core_example.cfg Outdated Show resolved Hide resolved

src/overlay/OverlayManagerImpl.cpp Outdated Show resolved Hide resolved

src/overlay/Peer.cpp Outdated Show resolved Hide resolved

marta-lokhova reviewed Aug 31, 2022

View reviewed changes

src/overlay/Peer.cpp Outdated Show resolved Hide resolved

src/overlay/Peer.cpp Outdated Show resolved Hide resolved

src/overlay/OverlayManagerImpl.cpp Outdated Show resolved Hide resolved

MonsieurNicolas requested changes Sep 1, 2022

View reviewed changes

hidenori-shinohara force-pushed the pull-mode-decent-perf branch from bdded33 to ae4ee23 Compare September 1, 2022 22:23

MonsieurNicolas requested changes Sep 2, 2022

View reviewed changes

hidenori-shinohara force-pushed the pull-mode-decent-perf branch 3 times, most recently from 80c9c47 to 01ed73e Compare September 2, 2022 21:03

marta-lokhova reviewed Sep 3, 2022

View reviewed changes

Hidenori Shinohara and others added 11 commits September 12, 2022 15:49

Enable tracy by default for a local docker image

653826c

Add new metrics for pull mode

5c5d4cb

Add new config values for pull mode

df06231

Add new XDR structs & fields for pull mode

7f14ee7

TxAdvertQueue stores incoming adverts in a specific order so we can

6aa2040

send demands appropriately

Convert hash -> txn using tx queue

d313a4f

Implement pull mode

e9cad25

Pull mode tests

f41acee

Update visual studio files

dd2e69b

warning police

7839ef9

fix the demand size metric

88da68c

hidenori-shinohara force-pushed the pull-mode-decent-perf branch from 01ed73e to 88da68c Compare September 12, 2022 20:27

latobarita merged commit 0d0cdc8 into stellar:master Sep 13, 2022

MonsieurNicolas mentioned this pull request Sep 14, 2022

[EXPERIMENT] pull-mode flooding #2432

Closed

hidenori-shinohara mentioned this pull request Oct 6, 2022

Pull mode may pull transactions that ItemFetcher has fetched #3568

Open

Conversation

hidenori-shinohara commented Jul 7, 2022

Description

Checklist

Uh oh!

marta-lokhova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marta-lokhova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marta-lokhova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marta-lokhova commented Aug 2, 2022

Uh oh!

hidenori-shinohara commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MonsieurNicolas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hidenori-shinohara commented Aug 3, 2022 •

edited

Loading