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

New connection strategy #1911

Merged
merged 13 commits into from Jan 31, 2019

Conversation

Projects
4 participants
@vogel
Copy link
Contributor

vogel commented Jan 15, 2019

Description

Partially resolves #1753

This PR adds new outboundvalid field to PeerRecord that is stored in database. outboundvalid means that at least once it was possible to make valid outbound connection to that peer. Peers with outboundvalid flags are first to try to join to and first to send to other peers as a response to GETPEERS message.

This PR also makes MAX_PENDING_PEERS setting only applicable to incoming connections, as OverlayManagerImpl already has logic for initiating only proper number of outgoing connections at any time, and it would be bad to have all PENDING slots taken by inbound connection from misbehaving peers.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document
@MonsieurNicolas
Copy link
Contributor

MonsieurNicolas left a comment

overall a lot better than the previous PR. A few comments, biggest change needed is around configuration: we have to preserve backward compatibility with this change (and have a sane default behavior for people using an existing configuration file)

Show resolved Hide resolved src/overlay/OverlayManagerImpl.cpp Outdated
Show resolved Hide resolved src/overlay/OverlayManagerImpl.cpp Outdated
Show resolved Hide resolved src/overlay/OverlayManager.h
return peers.size() < static_cast<size_t>(maxNum);
});
// don't connect to too many peers at once
auto peers = mPeerManager.getRandomPeers(PeerManager::nextAttemptCutoff(),

This comment has been minimized.

@MonsieurNicolas

MonsieurNicolas Jan 16, 2019

Contributor

the move to getRandomPeers doesn't belong to this commit

This comment has been minimized.

@vogel

vogel Jan 21, 2019

Author Contributor

that can be fixed during rebasing

});
newMsg.peers().reserve(peerList.size());
for (auto const& pr : peerList)
auto peers = mApp.getOverlayManager().getPeerManager().getRandomPeers(

This comment has been minimized.

@MonsieurNicolas

MonsieurNicolas Jan 16, 2019

Contributor

same thing: move to random doesn't belong to this commit

This comment has been minimized.

@vogel

vogel Jan 21, 2019

Author Contributor

that can be fixed during rebasing

Show resolved Hide resolved src/main/Config.cpp Outdated
Show resolved Hide resolved src/main/Config.cpp Outdated
Show resolved Hide resolved src/main/Config.cpp
Show resolved Hide resolved src/overlay/PeerManager.cpp Outdated
Show resolved Hide resolved src/overlay/PeerManager.cpp
@vogel

This comment has been minimized.

Copy link
Contributor Author

vogel commented Jan 17, 2019

What do you think of different database change? Currently preferred implies that peer is outbound, and we could have this implication in database:

flags -> type, where type = 0 (inbound), 1 (outbound), 2 (preferred).

We could add index on new type column and do not create outbound one.

@MonsieurNicolas

This comment has been minimized.

Copy link
Contributor

MonsieurNicolas commented Jan 18, 2019

yes, this is a good idea @vogel : so basically repurpose the "flags" column and not add the new one

@MonsieurNicolas

This comment has been minimized.

Copy link
Contributor

MonsieurNicolas commented Jan 18, 2019

and yes, we probably need the index on this type column

@MonsieurNicolas

This comment has been minimized.

Copy link
Contributor

MonsieurNicolas commented Jan 22, 2019

@vogel : reviewing a different PR, I ran into the sqlite documentation for schema updates, and it looks like renaming a column is supported in sqlite https://www.sqlite.org/lang_altertable.html

@vogel

This comment has been minimized.

Copy link
Contributor Author

vogel commented Jan 23, 2019

I see, it was added to newer version of sqlite. I can do that after next rebase, as current master contains new version of sqlite: aa65986

@MonsieurNicolas
Copy link
Contributor

MonsieurNicolas left a comment

a few small changes needed - getting close!

Show resolved Hide resolved src/main/Config.cpp Outdated
Show resolved Hide resolved src/main/Config.cpp Outdated
Show resolved Hide resolved src/main/Config.cpp Outdated
Show resolved Hide resolved src/overlay/PeerManager.cpp
Show resolved Hide resolved src/overlay/OverlayTests.cpp Outdated
Show resolved Hide resolved src/overlay/OverlayManagerImpl.cpp
Show resolved Hide resolved src/overlay/OverlayManagerImpl.cpp
Show resolved Hide resolved src/main/Config.cpp
Show resolved Hide resolved src/main/Config.cpp
Show resolved Hide resolved src/overlay/OverlayManagerImpl.cpp
@MonsieurNicolas

This comment has been minimized.

Copy link
Contributor

MonsieurNicolas commented Jan 26, 2019

as you've removed MAX_PEER_CONNECTIONS
you also need to update https://github.com/stellar/docker-stellar-core/blob/master/confd/templates/stellar-core.cfg.tmpl to stop using that config (and use MAX_ADDITIONAL_PEER_CONNECTIONS instead), this is supported by master and this new PR so we should be good then.

I noticed that many tests in acceptance fail because of that.

@vogel

This comment has been minimized.

Copy link
Contributor Author

vogel commented Jan 28, 2019

Done. I have not found another instance of MAX_PEER_CONNECTIONS in our docker, core commander or acceptance tests, so it should be OK now.

@MonsieurNicolas
Copy link
Contributor

MonsieurNicolas left a comment

looking good, only minor things to update + need rebase & squash commits

Show resolved Hide resolved src/overlay/OverlayManagerImpl.cpp Outdated
Show resolved Hide resolved src/overlay/OverlayManagerImpl.cpp Outdated
Show resolved Hide resolved src/overlay/RandomPeerSource.cpp Outdated
Show resolved Hide resolved src/overlay/PeerManager.h Outdated
Show resolved Hide resolved src/main/Config.cpp
@MonsieurNicolas

This comment has been minimized.

Copy link
Contributor

MonsieurNicolas commented Jan 29, 2019

when you rebase you will have to re-add the schema version change as it was rolled back as part of #1933

vogel added some commits Jan 10, 2019

test if preferred wins in both ways
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
test limits of pending peers for both ways
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
add DropMode for each drop call
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
rename PeerRecord files to PeerManager
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>

vogel added some commits Jan 10, 2019

use PeerManager to get access to stored peer data
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
replace peers.flags column with peers.type
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
split peer data in OverlayManager into inbound and outbound
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
bettter split available connections between inbound and outbound
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
use random peers for connection
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
allow promotion to outbound peer
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
allow inbound pending peers over max if possibly preferred
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>
only reset backoff to 0 at start of the application
Signed-off-by: Rafał Malinowski <rafal.przemyslaw.malinowski@gmail.com>

@vogel vogel force-pushed the vogel:new-connection-strategy branch from 735efe2 to ada57ee Jan 30, 2019

@MonsieurNicolas

This comment has been minimized.

Copy link
Contributor

MonsieurNicolas commented Jan 31, 2019

r+ ada57ee

@latobarita

This comment has been minimized.

Copy link
Contributor

latobarita commented on ada57ee Jan 31, 2019

saw approval from MonsieurNicolas
at vogel@ada57ee

This comment has been minimized.

Copy link
Contributor

latobarita replied Jan 31, 2019

merging vogel/stellar-core/new-connection-strategy = ada57ee into auto

This comment has been minimized.

Copy link
Contributor

latobarita replied Jan 31, 2019

vogel/stellar-core/new-connection-strategy = ada57ee merged ok, testing candidate = 13d36e5

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

latobarita replied Jan 31, 2019

fast-forwarding master to auto = 13d36e5

latobarita added a commit that referenced this pull request Jan 31, 2019

Merge pull request #1911 from vogel/new-connection-strategy
New connection strategy

Reviewed-by: MonsieurNicolas

@latobarita latobarita merged commit ada57ee into stellar:master Jan 31, 2019

1 check passed

default all tests passed
Details

v10.2.0 automation moved this from In progress to Done Jan 31, 2019

@vogel vogel deleted the vogel:new-connection-strategy branch Feb 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment