Nomination prototype #3856

JuI3s · 2023-07-26T08:49:19Z

Draft working off Hidenori's branch on the nomination protocol prototype. Replace transaction sets with their hashes.

Description

Resolves #X

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

SirTyson

I think you're off to a good start! I'm not super knowledgeable on specific SCP issues so this is more of a high-level overview for now, but I'll come back through and validate the actual logic a little more once the unit tests are passing. It might seem like a bunch of comments, but most of the stuff is more code style and git related issues which is always a bit tricky to get right the first couple times :).

SirTyson · 2023-08-08T21:39:54Z

lib/asio

I've noticed in a few of your PR's that you've been adding changes to non-source files and changes to src files that shouldn't be included like white space changes. No big deal, I ran into this issue when submitting my first couple of PR's too because I used git add . to add my changes. I'm not sure exactly what git frontend you're using, but it looks like you might be picking up some unwanted changes if you're doing something similar. This change shouldn't update any of the submodule versions (which is what all the lib/* changes are) and it makes it a little difficult for reviews to help you debug because it leads to build errors on other people's machines and rebase conflicts with master. If you use VSCode, I'd recommend using the source control tab to view changes and add files to a commit individually after reviewing the diffs, or doing something similar with whatever git flavor you like.

Easiest way to undo this (or at least what I do) is probably to run git reset --soft HEAD^, then unstage the lib changes. git reset --soft ~commit-hash~ resets the commit history, but leaves all the changes from the commit staged. HEAD^ is just shorthand for the last commit hash. I use this a lot when I need to fix a change I've already pushed when I don't want to send another commit. After you've reset the commit history, you can call git restore --staged file/path on individual files to remove them from the PR. Once you've removed the files you can redo the commit with git commit --amend --no-edit and push the edited commit with git push -f.

src/herder/HerderImpl.cpp

src/herder/PendingEnvelopes.cpp

SirTyson · 2023-08-08T22:48:39Z

src/overlay/Tracker.h

+    // Overwrite this with TX_SET_BACKOFF_DELAY_MS
+    // For the prototype, I made this a regular variable (not a const)
+    // but I'm not sure if it's a good idea.
+    static std::chrono::milliseconds MS_TO_WAIT_FOR_FETCH_REPLY;


For constants like this, we often have the value as a static constexpr, but have a get function. For testing purposes, we have a BUILD_TEST ifdef inside the get function that can return a variable value that you can set for testing. This is helpful because you can change the value in test environments but can ensure it's a proper static constexpr in prod. See SorobanNetworkConfig::getBucketListSizeSnapshotPeriod() and BUCKETLIST_SIZE_SNAPSHOT_PERIOD for an example.

SirTyson · 2023-08-08T22:49:54Z

src/overlay/test/LoopbackPeer.h

@@ -110,6 +119,12 @@ class LoopbackPeer : public Peer
    double getReorderProbability() const;
    void setReorderProbability(double d);

+    void


If this is only for testing, you should wrap this in #ifdef BUILD_TESTS for safety purposes. Same with the bool mSuspendTxFlooding{false}; above.

I'm not sure if this should be be only used for testing purposes (Hide made this change)? Is there any benefit in the real environment we might want to suspend flooding?

LoopbackPeer is only used for testing, so no need for BUILD_TESTS here.

src/protocol-next/xdr

SirTyson · 2023-08-08T22:58:06Z

src/overlay/OverlayManagerImpl.cpp

@@ -1256,6 +1257,12 @@ OverlayManagerImpl::getMaxAdvertSize() const
    return res;
 }

+UnorderedMap<Hash, std::vector<std::weak_ptr<Peer>>>
+OverlayManagerImpl::getPendingTxSetRequests()


Nit: OverlayManagerImpl::getPendingTxSetRequests() const

SirTyson · 2023-08-08T22:58:30Z

src/overlay/OverlayManagerImpl.cpp

@@ -1256,6 +1257,12 @@ OverlayManagerImpl::getMaxAdvertSize() const
    return res;
 }

+UnorderedMap<Hash, std::vector<std::weak_ptr<Peer>>>


This should return a const&

SirTyson · 2023-08-08T23:06:37Z

src/overlay/Peer.cpp

+    auto pendingTxSetRequests =
+        mApp.getOverlayManager().getPendingTxSetRequests();
+
+    for (auto& weakPeer : pendingTxSetRequests[frame->getContentsHash()])


I'm not 100% percent sure, but I think this unchecked map access may open up a DOS angle. Probably want to check with @marta-lokhova, but it looks like this assumes the incoming message asks for a pending TxSet hash that we currently have in pendingTxSetRequests. I don't think this assumption is valid given byzantine nodes or dropped messages from a well intentioned node. You should check that the hash exists in the map and not do an unchecked access.

Update the TransactionSet fetching

marta-lokhova · 2023-09-28T18:12:11Z

src/overlay/Peer.cpp

+
+        if (slotIndex == 0)
+        {
+            CLOG_INFO(Overlay,


I think we should just send DONT_HAVE immediately here, since we're not tracking this tx set.

PR fix No.1

…ntation" This reverts commit da29888.

marta-lokhova · 2023-10-02T19:51:10Z

src/overlay/Peer.cpp

+//      │          ┌────────┴───────┐     │              ▲
+// Local node      │Local node recvs│     │              │
+//  receives       │   GetTxnSet    │◀────┘              │
+//  new txn        │    request.    │                   Y│


What does "Retry after timeout" mean here? It would be the remote node retrying, right?

Fix format

marta-lokhova · 2024-05-30T17:16:44Z

Closing this as stale, we'll probably open a new PR picking up this work

JuI3s force-pushed the nomination-prototype branch 3 times, most recently from 7b73bb9 to ccf54de Compare August 2, 2023 04:07

JuI3s force-pushed the nomination-prototype branch from 6058299 to d53b9c6 Compare August 2, 2023 13:05

marta-lokhova self-requested a review August 8, 2023 22:06

SirTyson suggested changes Aug 8, 2023

View reviewed changes

JuI3s force-pushed the nomination-prototype branch from d53b9c6 to 6fa76ff Compare August 10, 2023 03:57

JuI3s force-pushed the nomination-prototype branch 3 times, most recently from 7d7c1d7 to 1c38c02 Compare August 28, 2023 14:47

JuI3s force-pushed the nomination-prototype branch 2 times, most recently from 6b71e20 to 2e375bb Compare September 5, 2023 14:51

JuI3s force-pushed the nomination-prototype branch 5 times, most recently from bb0277b to 2701942 Compare September 18, 2023 04:46

Hidenori Shinohara and others added 13 commits September 26, 2023 21:37

SCP change

83c46a8

Update the TransactionSet fetching

move mPendingTxRequests to private variable in implementation

4c11fd0

fix clang format

e29bb86

Debugging tx fetching

5ac20c0

Debug tx queue source account limit test

0e6cf73

Node adds pending tx set requests for missing tx sets

3478fba

Remove debug info

5883118

Remove debug CLOG_INFO

da136a7

Make SEND_DONT_HAVE_DELAY configurable

541f632

Wait before sending DONT_HAVE for tx set hash request

1db7bc2

Set default SEND_DONT_HAVE_DELAY to 2 seconds

46236b5

Set SEND_DONT_HAVE_DELAY to 100 milliseconds

cee418e

Add delay sending DONT_HAVE test

b8222e2

marta-lokhova reviewed Sep 28, 2023

View reviewed changes

PR fix No.1

9f534db

PR fix No.1

stellar deleted a comment from JuI3s Sep 28, 2023

fix pr 2

2acba40

JuI3s force-pushed the nomination-prototype branch 3 times, most recently from 2745014 to 5e5edd2 Compare September 28, 2023 20:52

PR fix 3

1651120

JuI3s force-pushed the nomination-prototype branch from 5e5edd2 to 1651120 Compare September 28, 2023 21:42

JuI3s added 2 commits September 29, 2023 00:10

PR fix 3. Use generalized tx sets instead of tx sets.

5e43b7a

Working on generating load for test 'overlay tx set fetching test'

1d71931

JuI3s requested a review from marta-lokhova September 29, 2023 17:04

JuI3s added 9 commits September 30, 2023 01:42

Do not send tx set back to peers we've previously sent DONT_HAVE to

254fc6f

fix delay sending DONT_HAVE test

5e5445d

Send DONT_HAVE back immediately for slot index not currently tracking

555b9e0

Update recvGetTxSet diagram

86cd20b

Change add recvGetTxSet clearing request logic and add documentation

da29888

Reject untracked tx set message in recvGetTxSet

a94237d

temp

fe80739

Revert "Change add recvGetTxSet clearing request logic and add docume…

cf90561

…ntation" This reverts commit da29888.

Fix ambiguities in and errors recvGetTxSet flow diagram

5da884e

marta-lokhova reviewed Oct 2, 2023

View reviewed changes

JuI3s added 2 commits October 3, 2023 06:29

Moving delay sending DONT_HAVE logic to Peer::maybeSendDontHave

101e0ae

Fix format

Fix bugs in maybeSendDontHaveAfterDelay

73dba79

JuI3s force-pushed the nomination-prototype branch from 613974c to 73dba79 Compare October 3, 2023 07:12

JuI3s added 2 commits October 3, 2023 18:59

Turn log info msg for sending DONT_HAVE to trace

a7ca10e

Remove logs

122363e

JuI3s requested a review from marta-lokhova October 5, 2023 21:06

marta-lokhova closed this May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nomination prototype #3856

Nomination prototype #3856

JuI3s commented Jul 26, 2023 •

edited

SirTyson left a comment

SirTyson Aug 8, 2023

SirTyson Aug 8, 2023

SirTyson Aug 8, 2023

SirTyson Aug 8, 2023

JuI3s Aug 26, 2023

marta-lokhova Oct 3, 2023

SirTyson Aug 8, 2023

SirTyson Aug 8, 2023

SirTyson Aug 8, 2023

marta-lokhova Sep 28, 2023

marta-lokhova Oct 2, 2023

marta-lokhova commented May 30, 2024

Nomination prototype #3856

Nomination prototype #3856

Conversation

JuI3s commented Jul 26, 2023 • edited

Description

Checklist

SirTyson 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

marta-lokhova commented May 30, 2024

JuI3s commented Jul 26, 2023 •

edited