Add new time sliced overlay survey #4275

bboston7 · 2024-04-05T22:09:48Z

Description

Resolves #4169, #4164. Partially resolves #2592 (script update remains).

This pull request adds a new survey mode that aims to be more deterministic and easier to compare across runs. It also adds some additional fields to the survey schema. It is very faithful to the design outlined in the Unified Network Survey Work Proposal.

While this PR includes all of the new functionality, ~~it is still missing a few things I'd like to do before it's completely done~~:

Document new schema
Document new commands / how to run survey
Document new metrics in metrics.md
Change tests to use HTTP endpoints instead of calling into SurveyManager
Deduplicate shared code between tests
Update overlay survey script (or open an issue and update this script separately)
- Opened Update overlay survey script to support new time sliced survey #4302 to track this. This PR is huge as is and the script changes don't need to land with the release.
Merge Add new overlay types to support time sliced network survey stellar-xdr#181 and Add new overlay types to support time sliced network survey (for merge into next) stellar-xdr#186 and update XDR submodules to point to those instead of my own branch.

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

checkpointing here: I'm still reviewing data collection/relaying portion. Thanks for the changes! Appreciate how clean and meticulous the code is :) Most of my suggestions are minor; I did leave a few questions around backward compatibility when connecting to old versions of core as well as the future of old-style survey.

src/main/CommandHandler.cpp

src/overlay/SurveyDataManager.h

marta-lokhova · 2024-04-15T21:02:09Z

src/main/Application.h

@@ -221,6 +222,7 @@ class Application
    virtual WorkScheduler& getWorkScheduler() = 0;
    virtual BanManager& getBanManager() = 0;
    virtual StatusManager& getStatusManager() = 0;
+    virtual SurveyDataManager& getSurveyDataManager() = 0;


We need to bump OVERLAY_PROTOCOL_VERSION to indicate a version of core that supports new-style survey.

Done in a108620

Note: if this change lands in v21.1.0, we will need to bump overlay version again from 33 to 34, as we expect to bump it to 33 in v21.0.0.

Just a reminder to bump the version to 34.

Bumped in 0859d27

src/overlay/SurveyManager.cpp

src/overlay/Peer.cpp

src/overlay/SurveyDataManager.cpp

src/overlay/SurveyDataManager.h

src/overlay/SurveyDataManager.cpp

src/main/ApplicationImpl.h

src/overlay/SurveyManager.cpp

src/overlay/SurveyMessageLimiter.cpp

src/overlay/SurveyDataManager.cpp

src/overlay/SurveyManager.cpp

bboston7 · 2024-05-02T21:16:19Z

I just squashed and rebased this on top of master to pull in the new overlay changes.

src/overlay/SurveyDataManager.cpp

src/overlay/SurveyManager.h

src/overlay/SurveyDataManager.cpp

src/overlay/SurveyDataManager.h

marta-lokhova · 2024-05-03T01:40:17Z

src/main/Application.h

@@ -221,6 +222,7 @@ class Application
    virtual WorkScheduler& getWorkScheduler() = 0;
    virtual BanManager& getBanManager() = 0;
    virtual StatusManager& getStatusManager() = 0;
+    virtual SurveyDataManager& getSurveyDataManager() = 0;


Note: if this change lands in v21.1.0, we will need to bump overlay version again from 33 to 34, as we expect to bump it to 33 in v21.0.0.

marta-lokhova

thanks for the updates! I spent a bit more time thinking through non-happy-path scenarios, and left a few suggestions there. I'd say we should merge this PR as-is, then just do a follow-up with the last few tweaks (to make PR management easier). Looks like there are some merge conflicts, so let's just address those and squash commits. Thanks again!

src/overlay/test/OverlayManagerTests.cpp

marta-lokhova · 2024-05-15T22:39:48Z

src/main/Application.h

@@ -221,6 +222,7 @@ class Application
    virtual WorkScheduler& getWorkScheduler() = 0;
    virtual BanManager& getBanManager() = 0;
    virtual StatusManager& getStatusManager() = 0;
+    virtual SurveyDataManager& getSurveyDataManager() = 0;


Just a reminder to bump the version to 34.

src/overlay/SurveyDataManager.cpp

marta-lokhova · 2024-05-15T23:34:35Z

src/overlay/SurveyDataManager.h

+    SurveyPhase phase();
+
+    // Reset survey data. Intended to be called when survey data expires.
+    void reset();


Is expiration only detected when we call phase? If so, I think that means we don't know when survey data is going to be reset exactly. I think this is fine now since we call it every time we call modifyNodeData, but that seems like a footgun in case implementation of modifyNodeData changes. To be safe, can we check expiration and reset survey on a periodic basis? Simplest i think is to add the check to OverlayManager::tick, which is called every few seconds

Hmm, that's a good point about this being easy to screw up if/when we modify this in the future. I got rid of the phase() function and replaced it with a member variable mPhase that the various start/stop functions update. I also added a function updateSurveyPhase that tick calls that serves to reset the survey on expiration.

marta-lokhova · 2024-05-15T23:43:45Z

src/overlay/SurveyDataManager.cpp

+bool
+SurveyDataManager::nonceIsReporting(uint32_t nonce)
+{
+    return phase() == SurveyPhase::REPORTING && mNonce == nonce;


Should these two conditions be swapped? This way we won't reset the current survey if another surveyor erroneously sends a survey request.

(actually, this probably suggests that phase shouldn't be modifying state, and is related to my question around periodically checking state)

With the removal of the phase() function I don't think order matters here anymore

marta-lokhova · 2024-05-16T00:20:59Z

src/overlay/SurveyDataManager.cpp

+{
+    ZoneScoped;
+
+    if (phase() == SurveyPhase::REPORTING && mNonce == request.nonce &&


Similarly here, should we check nonce first?

This also no longer matters with the removal of the phase() function

marta-lokhova · 2024-05-16T17:40:49Z

src/overlay/SurveyManager.cpp

@@ -16,6 +18,74 @@ namespace stellar

 uint32_t const SurveyManager::SURVEY_THROTTLE_TIMEOUT_MULT(3);

+uint32_t constexpr TIME_SLICED_SURVEY_MIN_OVERLAY_PROTOCOL_VERSION = 33;


Reminder: this should be 34.

Done in 0859d27

marta-lokhova · 2024-05-16T18:18:02Z

src/overlay/SurveyDataManager.cpp

+// Collecting phase is limited to 2 hours. If 2 hours pass without receiving
+// a StopSurveyCollecting message the `SurveyDataManager` will reset all data
+// and transition to the `INACTIVE` phase.
+constexpr std::chrono::hours COLLECTING_PHASE_MAX_DURATION{2};


Is there value in using this timeout to mark the end of collecting phase, and still allow surveyors to query results? Wondering if this should set to something we'd expect to use in reality, like 30 minutes? This may help maximize chances of more nodes responding to survey, as I imagine there'll be situations where messages are dropped due to our filtering policies (we'd still use the stop message to end survey earlier when needed)

Good idea. I shortened the max collecting phase duration to 30 minutes and changed the code to automatically transition to the reporting phase if the collecting phase expires in 160d1f8

bboston7 · 2024-05-18T00:17:00Z

I've rebased and squashed this down to a single commit in preparation for merging!

marta-lokhova · 2024-05-21T14:38:59Z

r+ 075df81

bboston7 requested a review from marta-lokhova April 5, 2024 22:10

bboston7 mentioned this pull request Apr 5, 2024

Add new overlay types to support time sliced network survey stellar/stellar-xdr#181

Merged

marta-lokhova reviewed Apr 15, 2024

View reviewed changes

marta-lokhova reviewed Apr 17, 2024

View reviewed changes

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

marta-lokhova requested changes Apr 18, 2024

View reviewed changes

bboston7 force-pushed the survey branch from 63666f7 to 9ec0ec8 Compare April 27, 2024 00:49

marta-lokhova mentioned this pull request Apr 30, 2024

Avoid copying StellarMessage in flooding paths #4299

Merged

bboston7 mentioned this pull request May 2, 2024

Update overlay survey script to support new time sliced survey #4302

Closed

bboston7 force-pushed the survey branch from 4e273b6 to d83130b Compare May 2, 2024 21:15

sisuresh reviewed May 2, 2024

View reviewed changes

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

marta-lokhova requested changes May 3, 2024

View reviewed changes

marta-lokhova approved these changes May 16, 2024

View reviewed changes

bboston7 marked this pull request as ready for review May 16, 2024 23:19

bboston7 force-pushed the survey branch from 160d1f8 to cc87de6 Compare May 17, 2024 21:05

Add new time sliced overlay survey

075df81

bboston7 force-pushed the survey branch from eb24905 to 075df81 Compare May 18, 2024 00:15

latobarita merged commit 7676070 into stellar:master May 21, 2024
15 checks passed

This was referenced May 28, 2024

Remove V1 survey XDR and supporting code #4332

Open

Deprecate old-style survey responses after v20 upgrade #4164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new time sliced overlay survey #4275

Add new time sliced overlay survey #4275

bboston7 commented Apr 5, 2024 •

edited

marta-lokhova left a comment

marta-lokhova Apr 15, 2024

bboston7 Apr 22, 2024

marta-lokhova May 3, 2024

marta-lokhova May 15, 2024

bboston7 May 16, 2024

bboston7 commented May 2, 2024

marta-lokhova May 3, 2024

marta-lokhova left a comment

marta-lokhova May 15, 2024

marta-lokhova May 15, 2024

bboston7 May 17, 2024

marta-lokhova May 15, 2024

marta-lokhova May 16, 2024

bboston7 May 17, 2024

marta-lokhova May 16, 2024

bboston7 May 17, 2024

marta-lokhova May 16, 2024

bboston7 May 16, 2024

marta-lokhova May 16, 2024

bboston7 May 17, 2024

bboston7 commented May 18, 2024

marta-lokhova commented May 21, 2024

		@@ -16,6 +18,74 @@ namespace stellar

		uint32_t const SurveyManager::SURVEY_THROTTLE_TIMEOUT_MULT(3);

		uint32_t constexpr TIME_SLICED_SURVEY_MIN_OVERLAY_PROTOCOL_VERSION = 33;

Add new time sliced overlay survey #4275

Add new time sliced overlay survey #4275

Conversation

bboston7 commented Apr 5, 2024 • edited

Description

Checklist

marta-lokhova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboston7 commented May 2, 2024

Choose a reason for hiding this comment

marta-lokhova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboston7 commented May 18, 2024

marta-lokhova commented May 21, 2024

bboston7 commented Apr 5, 2024 •

edited