From f3d39ec680b9ddd57021373f9bffa44e044cfa6e Mon Sep 17 00:00:00 2001 From: Olek <115580134+oleks-rip@users.noreply.github.com> Date: Tue, 28 Jan 2025 10:45:59 -0500 Subject: [PATCH] Fix CI unit tests (#5196) - Add retries for rpc client - Add dynamic port allocation for rpc servers --- .github/workflows/macos.yml | 11 +++++- include/xrpl/basics/BasicConfig.h | 11 ++++-- include/xrpl/server/detail/ServerImpl.h | 17 +++++++-- src/libxrpl/basics/BasicConfig.cpp | 8 ++-- src/libxrpl/server/Port.cpp | 5 ++- src/test/app/LedgerReplay_test.cpp | 5 +-- src/test/app/XChain_test.cpp | 6 +-- src/test/core/Config_test.cpp | 32 +++++++++++++++- src/test/jtx/Env.h | 15 ++++++++ src/test/jtx/envconfig.h | 13 ------- src/test/jtx/impl/Env.cpp | 43 ++++++++++++---------- src/test/jtx/impl/JSONRPCClient.cpp | 4 ++ src/test/jtx/impl/WSClient.cpp | 4 ++ src/test/jtx/impl/envconfig.cpp | 47 +++++++----------------- src/test/ledger/View_test.cpp | 6 +-- src/test/rpc/AccountObjects_test.cpp | 6 +-- src/test/rpc/LedgerRPC_test.cpp | 6 +-- src/test/rpc/LedgerRequestRPC_test.cpp | 1 + src/test/rpc/ServerInfo_test.cpp | 12 +++--- src/test/rpc/Subscribe_test.cpp | 2 +- src/test/rpc/ValidatorInfo_test.cpp | 1 + src/test/rpc/ValidatorRPC_test.cpp | 1 + src/test/server/Server_test.cpp | 19 ++++++++-- src/test/unit_test/multi_runner.cpp | 6 --- src/xrpld/app/main/Application.cpp | 30 ++++++++++++++- src/xrpld/app/main/GRPCServer.cpp | 33 ++++++++++++++--- src/xrpld/app/main/GRPCServer.h | 10 ++++- src/xrpld/core/detail/Config.cpp | 30 +++++++++++++++ src/xrpld/overlay/detail/OverlayImpl.cpp | 2 +- src/xrpld/rpc/ServerHandler.h | 19 +++++----- src/xrpld/rpc/detail/ServerHandler.cpp | 34 ++++++++++++----- 31 files changed, 293 insertions(+), 146 deletions(-) diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml index c214f417e7..851e825bf3 100644 --- a/.github/workflows/macos.yml +++ b/.github/workflows/macos.yml @@ -56,6 +56,9 @@ jobs: else brew install cmake fi + - name: install nproc + run: | + brew install coreutils - name: check environment run: | env | sort @@ -63,6 +66,9 @@ jobs: python --version conan --version cmake --version + nproc --version + echo -n "nproc returns: " + nproc - name: configure Conan run : | conan profile new default --detect || true @@ -81,6 +87,9 @@ jobs: with: generator: ${{ matrix.generator }} configuration: ${{ matrix.configuration }} + cmake-args: ${{ matrix.cmake-args }} - name: test run: | - ${build_dir}/rippled --unittest + n=$(nproc) + echo "Using $n test jobs" + ${build_dir}/rippled --unittest --unittest-jobs $n diff --git a/include/xrpl/basics/BasicConfig.h b/include/xrpl/basics/BasicConfig.h index 8f522cdd2f..f7e90c7c9a 100644 --- a/include/xrpl/basics/BasicConfig.h +++ b/include/xrpl/basics/BasicConfig.h @@ -21,18 +21,21 @@ #define RIPPLE_BASICS_BASICCONFIG_H_INCLUDED #include + #include #include + #include -#include #include #include #include +#include #include namespace ripple { -using IniFileSections = std::map>; +using IniFileSections = + std::unordered_map>; //------------------------------------------------------------------------------ @@ -43,7 +46,7 @@ class Section { private: std::string name_; - std::map lookup_; + std::unordered_map lookup_; std::vector lines_; std::vector values_; bool had_trailing_comments_ = false; @@ -215,7 +218,7 @@ public: class BasicConfig { private: - std::map map_; + std::unordered_map map_; public: /** Returns `true` if a section with the given name exists. */ diff --git a/include/xrpl/server/detail/ServerImpl.h b/include/xrpl/server/detail/ServerImpl.h index 00a9a26384..fd0b082b46 100644 --- a/include/xrpl/server/detail/ServerImpl.h +++ b/include/xrpl/server/detail/ServerImpl.h @@ -22,18 +22,21 @@ #include #include -#include #include #include + #include + #include #include #include #include +#include namespace ripple { -using Endpoints = std::vector; +using Endpoints = + std::unordered_map; /** A multi-protocol server. @@ -168,11 +171,17 @@ ServerImpl::ports(std::vector const& ports) for (auto const& port : ports) { ports_.push_back(port); + auto& internalPort = ports_.back(); if (auto sp = ios_.emplace>( - handler_, io_service_, ports_.back(), j_)) + handler_, io_service_, internalPort, j_)) { list_.push_back(sp); - eps.push_back(sp->get_endpoint()); + + auto ep = sp->get_endpoint(); + if (!internalPort.port) + internalPort.port = ep.port(); + eps.emplace(port.name, std::move(ep)); + sp->run(); } } diff --git a/src/libxrpl/basics/BasicConfig.cpp b/src/libxrpl/basics/BasicConfig.cpp index 932eea346f..2ad47ab7ee 100644 --- a/src/libxrpl/basics/BasicConfig.cpp +++ b/src/libxrpl/basics/BasicConfig.cpp @@ -104,7 +104,7 @@ Section::append(std::vector const& lines) bool Section::exists(std::string const& name) const { - return lookup_.find(name) != lookup_.end(); + return lookup_.contains(name); } std::ostream& @@ -120,13 +120,13 @@ operator<<(std::ostream& os, Section const& section) bool BasicConfig::exists(std::string const& name) const { - return map_.find(name) != map_.end(); + return map_.contains(name); } Section& BasicConfig::section(std::string const& name) { - return map_[name]; + return map_.emplace(name, name).first->second; } Section const& @@ -163,7 +163,7 @@ BasicConfig::deprecatedClearSection(std::string const& section) void BasicConfig::legacy(std::string const& section, std::string value) { - map_[section].legacy(std::move(value)); + map_.emplace(section, section).first->second.legacy(std::move(value)); } std::string diff --git a/src/libxrpl/server/Port.cpp b/src/libxrpl/server/Port.cpp index 0554c8082a..9c1f16cda0 100644 --- a/src/libxrpl/server/Port.cpp +++ b/src/libxrpl/server/Port.cpp @@ -198,6 +198,7 @@ populate( void parse_Port(ParsedPort& port, Section const& section, std::ostream& log) { + port.name = section.name(); { auto const optResult = section.get("ip"); if (optResult) @@ -223,8 +224,8 @@ parse_Port(ParsedPort& port, Section const& section, std::ostream& log) { port.port = beast::lexicalCastThrow(*optResult); - // Port 0 is not supported - if (*port.port == 0) + // Port 0 is not supported for [server] + if ((*port.port == 0) && (port.name == "server")) Throw(); } catch (std::exception const&) diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index 3ebf059178..883aca7bce 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -584,10 +584,7 @@ public: PeerSetBehavior behavior = PeerSetBehavior::Good, InboundLedgersBehavior inboundBhvr = InboundLedgersBehavior::Good, PeerFeature peerFeature = PeerFeature::LedgerReplayEnabled) - : env(suite, - jtx::envconfig(jtx::port_increment, 3), - nullptr, - beast::severities::kDisabled) + : env(suite, jtx::envconfig(), nullptr, beast::severities::kDisabled) , app(env.app()) , ledgerMaster(env.app().getLedgerMaster()) , inboundLedgers( diff --git a/src/test/app/XChain_test.cpp b/src/test/app/XChain_test.cpp index 4f24d17601..3d25e9e989 100644 --- a/src/test/app/XChain_test.cpp +++ b/src/test/app/XChain_test.cpp @@ -201,11 +201,7 @@ struct SEnv template struct XEnv : public jtx::XChainBridgeObjects, public SEnv { - XEnv(T& s, bool side = false) - : SEnv( - s, - jtx::envconfig(jtx::port_increment, side ? 3 : 0), - features) + XEnv(T& s, bool side = false) : SEnv(s, jtx::envconfig(), features) { using namespace jtx; STAmount xrp_funds{XRP(10000)}; diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index eb64649748..2bf398c9a9 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -24,10 +24,13 @@ #include #include #include + #include #include + #include #include +#include namespace ripple { namespace detail { @@ -140,12 +143,15 @@ public: path subDir, path const& dbPath, path const& validatorsFile, - bool useCounter = true) + bool useCounter = true, + std::string confContents = "") : FileDirGuard( test, std::move(subDir), path(Config::configFileName), - configContents(dbPath.string(), validatorsFile.string()), + confContents.empty() + ? configContents(dbPath.string(), validatorsFile.string()) + : confContents, useCounter) , dataDir_(dbPath) { @@ -1068,6 +1074,27 @@ trustthesevalidators.gov BEAST_EXPECT(wss.admin_nets_v4.size() + wss.admin_nets_v6.size() == 1); } + void + testZeroPort() + { + auto const contents = std::regex_replace( + detail::configContents("", ""), + std::regex("port\\s*=\\s*\\d+"), + "port = 0"); + + try + { + detail::RippledCfgGuard const cfg( + *this, "testPort", "", "", true, contents); + BEAST_EXPECT(false); + } + catch (std::exception const& ex) + { + BEAST_EXPECT(std::string_view(ex.what()).starts_with( + "Invalid value '0' for key 'port'")); + } + } + void testWhitespace() { @@ -1478,6 +1505,7 @@ r.ripple.com:51235 testSetup(false); testSetup(true); testPort(); + testZeroPort(); testWhitespace(); testColons(); testComments(); diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index b3f49ef838..66845ae91d 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -419,6 +419,20 @@ public: app().checkSigs(false); } + // set rpc retries + void + set_retries(unsigned r = 5) + { + retries_ = r; + } + + // get rpc retries + unsigned + retries() const + { + return retries_; + } + /** Associate AccountID with account. */ void memoize(Account const& account); @@ -700,6 +714,7 @@ protected: uint256 txid_; TER ter_ = tesSUCCESS; bool parseFailureExpected_ = false; + unsigned retries_ = 5; Json::Value do_rpc( diff --git a/src/test/jtx/envconfig.h b/src/test/jtx/envconfig.h index 259b61b6ca..bb1716fffb 100644 --- a/src/test/jtx/envconfig.h +++ b/src/test/jtx/envconfig.h @@ -106,19 +106,6 @@ std::unique_ptr secure_gateway_localnet(std::unique_ptr); std::unique_ptr validator(std::unique_ptr, std::string const&); -/// @brief adjust the default configured server ports by a specified value -/// -/// This is intended for use with envconfig, as in -/// envconfig(port_increment, 5) -/// -/// @param cfg config instance to be modified -/// @param int amount by which to increment the existing server port -/// values in the config -/// -/// @return unique_ptr to Config instance -std::unique_ptr -port_increment(std::unique_ptr, int); - /// @brief add a grpc address and port to config /// /// This is intended for use with envconfig, for tests that require a grpc diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index 2ae5758858..c87b126024 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -326,24 +326,16 @@ Env::submit(JTx const& jt) auto const jr = [&]() { if (jt.stx) { - // We shouldn't need to retry, but it fixes the test on macOS for - // the moment. - int retries = 3; - do - { - txid_ = jt.stx->getTransactionID(); - Serializer s; - jt.stx->add(s); - auto const jr = rpc("submit", strHex(s.slice())); + txid_ = jt.stx->getTransactionID(); + Serializer s; + jt.stx->add(s); + auto const jr = rpc("submit", strHex(s.slice())); - parsedResult = parseResult(jr); - test.expect(parsedResult.ter, "ter uninitialized!"); - ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED); - if (ter_ != telENV_RPC_FAILED || - parsedResult.rpcCode != rpcINTERNAL || - jt.ter == telENV_RPC_FAILED || --retries <= 0) - return jr; - } while (true); + parsedResult = parseResult(jr); + test.expect(parsedResult.ter, "ter uninitialized!"); + ter_ = parsedResult.ter.value_or(telENV_RPC_FAILED); + + return jr; } else { @@ -576,8 +568,21 @@ Env::do_rpc( std::vector const& args, std::unordered_map const& headers) { - return rpcClient(args, app().config(), app().logs(), apiVersion, headers) - .second; + auto response = + rpcClient(args, app().config(), app().logs(), apiVersion, headers); + + for (unsigned ctr = 0; (ctr < retries_) and (response.first == rpcINTERNAL); + ++ctr) + { + JLOG(journal.error()) + << "Env::do_rpc error, retrying, attempt #" << ctr + 1 << " ..."; + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + + response = + rpcClient(args, app().config(), app().logs(), apiVersion, headers); + } + + return response.second; } void diff --git a/src/test/jtx/impl/JSONRPCClient.cpp b/src/test/jtx/impl/JSONRPCClient.cpp index 20f2149e4a..19c20b5bab 100644 --- a/src/test/jtx/impl/JSONRPCClient.cpp +++ b/src/test/jtx/impl/JSONRPCClient.cpp @@ -52,6 +52,10 @@ class JSONRPCClient : public AbstractClient if (pp.ip && pp.ip->is_unspecified()) *pp.ip = pp.ip->is_v6() ? address{address_v6::loopback()} : address{address_v4::loopback()}; + + if (!pp.port) + Throw("Use fixConfigPorts with auto ports"); + return {*pp.ip, *pp.port}; } Throw("Missing HTTP port"); diff --git a/src/test/jtx/impl/WSClient.cpp b/src/test/jtx/impl/WSClient.cpp index 185d0ea5db..03e2b2d608 100644 --- a/src/test/jtx/impl/WSClient.cpp +++ b/src/test/jtx/impl/WSClient.cpp @@ -69,6 +69,10 @@ class WSClientImpl : public WSClient if (pp.ip && pp.ip->is_unspecified()) *pp.ip = pp.ip->is_v6() ? address{address_v6::loopback()} : address{address_v4::loopback()}; + + if (!pp.port) + Throw("Use fixConfigPorts with auto ports"); + return {*pp.ip, *pp.port}; } Throw("Missing WebSocket port"); diff --git a/src/test/jtx/impl/envconfig.cpp b/src/test/jtx/impl/envconfig.cpp index c9788a6d70..1309a52393 100644 --- a/src/test/jtx/impl/envconfig.cpp +++ b/src/test/jtx/impl/envconfig.cpp @@ -25,22 +25,11 @@ namespace ripple { namespace test { -int port_base = 8000; -void -incPorts(int times) -{ - port_base += (4 * times); -} - std::atomic envUseIPv4{false}; void setupConfigForUnitTests(Config& cfg) { - std::string const port_peer = std::to_string(port_base); - std::string port_rpc = std::to_string(port_base + 1); - std::string port_ws = std::to_string(port_base + 2); - using namespace jtx; // Default fees to old values, so tests don't have to worry about changes in // Config.h @@ -58,19 +47,25 @@ setupConfigForUnitTests(Config& cfg) cfg.setupControl(true, true, true); cfg["server"].append(PORT_PEER); cfg[PORT_PEER].set("ip", getEnvLocalhostAddr()); - cfg[PORT_PEER].set("port", port_peer); + + // Using port 0 asks the operating system to allocate an unused port, which + // can be obtained after a "bind" call. + // Works for all system (Linux, Windows, Unix, Mac). + // Check https://man7.org/linux/man-pages/man7/ip.7.html + // "ip_local_port_range" section for more info + cfg[PORT_PEER].set("port", "0"); cfg[PORT_PEER].set("protocol", "peer"); cfg["server"].append(PORT_RPC); cfg[PORT_RPC].set("ip", getEnvLocalhostAddr()); cfg[PORT_RPC].set("admin", getEnvLocalhostAddr()); - cfg[PORT_RPC].set("port", port_rpc); + cfg[PORT_RPC].set("port", "0"); cfg[PORT_RPC].set("protocol", "http,ws2"); cfg["server"].append(PORT_WS); cfg[PORT_WS].set("ip", getEnvLocalhostAddr()); cfg[PORT_WS].set("admin", getEnvLocalhostAddr()); - cfg[PORT_WS].set("port", port_ws); + cfg[PORT_WS].set("port", "0"); cfg[PORT_WS].set("protocol", "ws"); cfg.SSL_VERIFY = false; } @@ -123,27 +118,11 @@ validator(std::unique_ptr cfg, std::string const& seed) return cfg; } -std::unique_ptr -port_increment(std::unique_ptr cfg, int increment) -{ - for (auto const sectionName : {PORT_PEER, PORT_RPC, PORT_WS}) - { - Section& s = (*cfg)[sectionName]; - auto const port = s.get("port"); - if (port) - { - s.set("port", std::to_string(*port + increment)); - } - } - return cfg; -} - std::unique_ptr addGrpcConfig(std::unique_ptr cfg) { - std::string port_grpc = std::to_string(port_base + 3); (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr()); - (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc); + (*cfg)[SECTION_PORT_GRPC].set("port", "0"); return cfg; } @@ -152,9 +131,11 @@ addGrpcConfigWithSecureGateway( std::unique_ptr cfg, std::string const& secureGateway) { - std::string port_grpc = std::to_string(port_base + 3); (*cfg)[SECTION_PORT_GRPC].set("ip", getEnvLocalhostAddr()); - (*cfg)[SECTION_PORT_GRPC].set("port", port_grpc); + + // Check https://man7.org/linux/man-pages/man7/ip.7.html + // "ip_local_port_range" section for using 0 ports + (*cfg)[SECTION_PORT_GRPC].set("port", "0"); (*cfg)[SECTION_PORT_GRPC].set("secure_gateway", secureGateway); return cfg; } diff --git a/src/test/ledger/View_test.cpp b/src/test/ledger/View_test.cpp index 29ceb54cc0..fc9dce7de7 100644 --- a/src/test/ledger/View_test.cpp +++ b/src/test/ledger/View_test.cpp @@ -1012,11 +1012,7 @@ class View_test : public beast::unit_test::suite // The two Env's can't share the same ports, so modify the config // of the second Env to use higher port numbers - Env eB{ - *this, - envconfig(port_increment, 3), - nullptr, - beast::severities::kDisabled}; + Env eB{*this, envconfig(), nullptr, beast::severities::kDisabled}; // Make ledgers that are incompatible with the first ledgers. Note // that bob is funded before alice. diff --git a/src/test/rpc/AccountObjects_test.cpp b/src/test/rpc/AccountObjects_test.cpp index ae1e3c195d..232a091114 100644 --- a/src/test/rpc/AccountObjects_test.cpp +++ b/src/test/rpc/AccountObjects_test.cpp @@ -759,7 +759,7 @@ public: { // Create a bridge test::jtx::XChainBridgeObjects x; - Env scEnv(*this, envconfig(port_increment, 3), features); + Env scEnv(*this, envconfig(), features); x.createScBridgeObjects(scEnv); auto scEnvAcctObjs = [&](Account const& acct, char const* type) { @@ -800,7 +800,7 @@ public: // Alice and Bob create a xchain sequence number that we can look // for in the ledger. test::jtx::XChainBridgeObjects x; - Env scEnv(*this, envconfig(port_increment, 3), features); + Env scEnv(*this, envconfig(), features); x.createScBridgeObjects(scEnv); scEnv( @@ -845,7 +845,7 @@ public: } { test::jtx::XChainBridgeObjects x; - Env scEnv(*this, envconfig(port_increment, 3), features); + Env scEnv(*this, envconfig(), features); x.createScBridgeObjects(scEnv); auto const amt = XRP(1000); diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 9352518dba..ebc6297fe4 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -64,7 +64,7 @@ class LedgerRPC_XChain_test : public beast::unit_test::suite, using namespace test::jtx; Env mcEnv{*this, features}; - Env scEnv(*this, envconfig(port_increment, 3), features); + Env scEnv(*this, envconfig(), features); createBridgeObjects(mcEnv, scEnv); @@ -157,7 +157,7 @@ class LedgerRPC_XChain_test : public beast::unit_test::suite, using namespace test::jtx; Env mcEnv{*this, features}; - Env scEnv(*this, envconfig(port_increment, 3), features); + Env scEnv(*this, envconfig(), features); createBridgeObjects(mcEnv, scEnv); @@ -218,7 +218,7 @@ class LedgerRPC_XChain_test : public beast::unit_test::suite, using namespace test::jtx; Env mcEnv{*this, features}; - Env scEnv(*this, envconfig(port_increment, 3), features); + Env scEnv(*this, envconfig(), features); // note: signers.size() and quorum are both 5 in createBridgeObjects createBridgeObjects(mcEnv, scEnv); diff --git a/src/test/rpc/LedgerRequestRPC_test.cpp b/src/test/rpc/LedgerRequestRPC_test.cpp index 8922cd3838..61905c99c1 100644 --- a/src/test/rpc/LedgerRequestRPC_test.cpp +++ b/src/test/rpc/LedgerRequestRPC_test.cpp @@ -347,6 +347,7 @@ public: auto const USD = gw["USD"]; env.fund(XRP(100000), gw); + env.set_retries(0); auto const result = env.rpc("ledger_request", "1")[jss::result]; // The current HTTP/S ServerHandler returns an HTTP 403 error code here // rather than a noPermission JSON error. The JSONRPCClient just eats diff --git a/src/test/rpc/ServerInfo_test.cpp b/src/test/rpc/ServerInfo_test.cpp index 5821ddb59c..2f0cdee77e 100644 --- a/src/test/rpc/ServerInfo_test.cpp +++ b/src/test/rpc/ServerInfo_test.cpp @@ -104,17 +104,17 @@ admin = 127.0.0.1 } { - auto config = makeValidatorConfig(); - auto const rpc_port = - (*config)["port_rpc"].get("port"); + Env env(*this, makeValidatorConfig()); + auto const& config = env.app().config(); + + auto const rpc_port = config["port_rpc"].get("port"); auto const grpc_port = - (*config)[SECTION_PORT_GRPC].get("port"); - auto const ws_port = (*config)["port_ws"].get("port"); + config[SECTION_PORT_GRPC].get("port"); + auto const ws_port = config["port_ws"].get("port"); BEAST_EXPECT(grpc_port); BEAST_EXPECT(rpc_port); BEAST_EXPECT(ws_port); - Env env(*this, std::move(config)); auto const result = env.rpc("server_info"); BEAST_EXPECT(!result[jss::result].isMember(jss::error)); BEAST_EXPECT(result[jss::result][jss::status] == "success"); diff --git a/src/test/rpc/Subscribe_test.cpp b/src/test/rpc/Subscribe_test.cpp index f1cb2f9a13..3f804adda6 100644 --- a/src/test/rpc/Subscribe_test.cpp +++ b/src/test/rpc/Subscribe_test.cpp @@ -614,7 +614,7 @@ public: } { - Env env_nonadmin{*this, no_admin(envconfig(port_increment, 3))}; + Env env_nonadmin{*this, no_admin(envconfig())}; Json::Value jv; jv[jss::url] = "no-url"; auto jr = diff --git a/src/test/rpc/ValidatorInfo_test.cpp b/src/test/rpc/ValidatorInfo_test.cpp index 603a0ad9d2..5ca1af7a3c 100644 --- a/src/test/rpc/ValidatorInfo_test.cpp +++ b/src/test/rpc/ValidatorInfo_test.cpp @@ -50,6 +50,7 @@ public: { using namespace test::jtx; Env env{*this, envconfig(no_admin)}; + env.set_retries(0); auto const info = env.rpc("validator_info")[jss::result]; BEAST_EXPECT(info.isNull()); } diff --git a/src/test/rpc/ValidatorRPC_test.cpp b/src/test/rpc/ValidatorRPC_test.cpp index 2bd4b69c37..b53b8d1f82 100644 --- a/src/test/rpc/ValidatorRPC_test.cpp +++ b/src/test/rpc/ValidatorRPC_test.cpp @@ -49,6 +49,7 @@ public: for (std::string cmd : {"validators", "validator_list_sites"}) { Env env{*this, isAdmin ? envconfig() : envconfig(no_admin)}; + env.set_retries(isAdmin ? 5 : 0); auto const jrr = env.rpc(cmd)[jss::result]; if (isAdmin) { diff --git a/src/test/server/Server_test.cpp b/src/test/server/Server_test.cpp index c7d7dc3b91..e0de7ddda5 100644 --- a/src/test/server/Server_test.cpp +++ b/src/test/server/Server_test.cpp @@ -299,8 +299,8 @@ public: serverPort.back().port = 0; serverPort.back().protocol.insert("http"); auto eps = s->ports(serverPort); - test_request(eps[0]); - test_keepalive(eps[0]); + test_request(eps.begin()->second); + test_keepalive(eps.begin()->second); // s->close(); s = nullptr; pass(); @@ -423,7 +423,20 @@ public: std::make_unique(&messages)}; }); BEAST_EXPECT( - messages.find("Invalid value '0' for key 'port' in [port_rpc]") != + messages.find("Invalid value '0' for key 'port' in [port_rpc]") == + std::string::npos); + + except([&] { + Env env{ + *this, + envconfig([](std::unique_ptr cfg) { + (*cfg)["server"].set("port", "0"); + return cfg; + }), + std::make_unique(&messages)}; + }); + BEAST_EXPECT( + messages.find("Invalid value '0' for key 'port' in [server]") != std::string::npos); except([&] { diff --git a/src/test/unit_test/multi_runner.cpp b/src/test/unit_test/multi_runner.cpp index 60487cadfb..0b7d08c5ae 100644 --- a/src/test/unit_test/multi_runner.cpp +++ b/src/test/unit_test/multi_runner.cpp @@ -32,9 +32,6 @@ namespace ripple { namespace test { -extern void -incPorts(int times); - namespace detail { std::string @@ -510,9 +507,6 @@ multi_runner_child::multi_runner_child( , quiet_{quiet} , print_log_{!quiet || print_log} { - // incPort twice (2*jobIndex_) because some tests need two envs - test::incPorts(2 * job_index_); - if (num_jobs_ > 1) { keep_alive_thread_ = std::thread([this] { diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index e2d1a1f682..bcacdbc375 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -90,6 +90,9 @@ namespace ripple { +static void +fixConfigPorts(Config& config, Endpoints const& endpoints); + // VFALCO TODO Move the function definitions into the class declaration class ApplicationImp : public Application, public BasicApp { @@ -1417,6 +1420,7 @@ ApplicationImp::setup(boost::program_options::variables_map const& cmdline) *config_, beast::logstream{m_journal.error()}); setup.makeContexts(); serverHandler_->setup(setup, m_journal); + fixConfigPorts(*config_, serverHandler_->endpoints()); } catch (std::exception const& e) { @@ -1538,7 +1542,11 @@ ApplicationImp::start(bool withTimers) m_shaMapStore->start(); if (overlay_) overlay_->start(); - grpcServer_->start(); + + if (grpcServer_->start()) + fixConfigPorts( + *config_, {{SECTION_PORT_GRPC, grpcServer_->getEndpoint()}}); + ledgerCleaner_->start(); perfLog_->start(); } @@ -2189,4 +2197,24 @@ make_Application( std::move(config), std::move(logs), std::move(timeKeeper)); } +void +fixConfigPorts(Config& config, Endpoints const& endpoints) +{ + for (auto const& [name, ep] : endpoints) + { + if (!config.exists(name)) + continue; + + auto& section = config[name]; + auto const optPort = section.get("port"); + if (optPort) + { + std::uint16_t const port = + beast::lexicalCast(*optPort); + if (!port) + section.set("port", std::to_string(ep.port())); + } + } +} + } // namespace ripple diff --git a/src/xrpld/app/main/GRPCServer.cpp b/src/xrpld/app/main/GRPCServer.cpp index a7280a7688..4bcd95ceb8 100644 --- a/src/xrpld/app/main/GRPCServer.cpp +++ b/src/xrpld/app/main/GRPCServer.cpp @@ -268,7 +268,7 @@ template std::optional GRPCServerImpl::CallData::getClientEndpoint() { - return getEndpoint(ctx_.peer()); + return ripple::getEndpoint(ctx_.peer()); } template @@ -447,8 +447,8 @@ GRPCServerImpl::handleRpcs() if (!ok) { - JLOG(journal_.debug()) - << "Request listener cancelled. " << "Destroying object"; + JLOG(journal_.debug()) << "Request listener cancelled. " + << "Destroying object"; erase(ptr); } else @@ -564,8 +564,12 @@ GRPCServerImpl::start() JLOG(journal_.info()) << "Starting gRPC server at " << serverAddress_; grpc::ServerBuilder builder; + // Listen on the given address without any authentication mechanism. - builder.AddListeningPort(serverAddress_, grpc::InsecureServerCredentials()); + // Actually binded port will be returned into "port" variable. + int port = 0; + builder.AddListeningPort( + serverAddress_, grpc::InsecureServerCredentials(), &port); // Register "service_" as the instance through which we'll communicate with // clients. In this case it corresponds to an *asynchronous* service. builder.RegisterService(&service_); @@ -574,11 +578,21 @@ GRPCServerImpl::start() cq_ = builder.AddCompletionQueue(); // Finally assemble the server. server_ = builder.BuildAndStart(); + serverPort_ = static_cast(port); - return true; + return static_cast(serverPort_); } -void +boost::asio::ip::tcp::endpoint +GRPCServerImpl::getEndpoint() const +{ + std::string const addr = + serverAddress_.substr(0, serverAddress_.find_last_of(':')); + return boost::asio::ip::tcp::endpoint( + boost::asio::ip::make_address(addr), serverPort_); +} + +bool GRPCServer::start() { // Start the server and setup listeners @@ -591,6 +605,7 @@ GRPCServer::start() this->impl_.handleRpcs(); }); } + return running_; } void @@ -609,4 +624,10 @@ GRPCServer::~GRPCServer() XRPL_ASSERT(!running_, "ripple::GRPCServer::~GRPCServer : is not running"); } +boost::asio::ip::tcp::endpoint +GRPCServer::getEndpoint() const +{ + return impl_.getEndpoint(); +} + } // namespace ripple diff --git a/src/xrpld/app/main/GRPCServer.h b/src/xrpld/app/main/GRPCServer.h index 39bfc0c976..0298a40493 100644 --- a/src/xrpld/app/main/GRPCServer.h +++ b/src/xrpld/app/main/GRPCServer.h @@ -84,6 +84,7 @@ private: Application& app_; std::string serverAddress_; + std::uint16_t serverPort_ = 0; std::vector secureGatewayIPs_; @@ -141,6 +142,10 @@ public: std::vector> setupListeners(); + // Obtaining actually binded endpoint (if port 0 was used for server setup). + boost::asio::ip::tcp::endpoint + getEndpoint() const; + private: // Class encompasing the state and logic needed to serve a request. template @@ -305,7 +310,7 @@ public: GRPCServer& operator=(const GRPCServer&) = delete; - void + bool start(); void @@ -313,6 +318,9 @@ public: ~GRPCServer(); + boost::asio::ip::tcp::endpoint + getEndpoint() const; + private: GRPCServerImpl impl_; std::thread thread_; diff --git a/src/xrpld/core/detail/Config.cpp b/src/xrpld/core/detail/Config.cpp index eb7bfcce8a..b6375f1a9b 100644 --- a/src/xrpld/core/detail/Config.cpp +++ b/src/xrpld/core/detail/Config.cpp @@ -420,6 +420,35 @@ Config::setup( get_if_exists(nodeDbSection, "fast_load", FAST_LOAD); } +// 0 ports are allowed for unit tests, but still not allowed to be present in +// config file +static void +checkZeroPorts(Config const& config) +{ + if (!config.exists("server")) + return; + + for (auto const& name : config.section("server").values()) + { + if (!config.exists(name)) + return; + + auto const& section = config[name]; + auto const optResult = section.get("port"); + if (optResult) + { + auto const port = beast::lexicalCast(*optResult); + if (!port) + { + std::stringstream ss; + ss << "Invalid value '" << *optResult << "' for key 'port' in [" + << name << "]"; + Throw(ss.str()); + } + } + } +} + void Config::load() { @@ -440,6 +469,7 @@ Config::load() } loadFromString(fileContents); + checkZeroPorts(*this); } void diff --git a/src/xrpld/overlay/detail/OverlayImpl.cpp b/src/xrpld/overlay/detail/OverlayImpl.cpp index 9c90c5f84a..b69a074e99 100644 --- a/src/xrpld/overlay/detail/OverlayImpl.cpp +++ b/src/xrpld/overlay/detail/OverlayImpl.cpp @@ -478,7 +478,7 @@ OverlayImpl::start() { PeerFinder::Config config = PeerFinder::Config::makeConfig( app_.config(), - serverHandler_.setup().overlay.port, + serverHandler_.setup().overlay.port(), app_.getValidationPublicKey().has_value(), setup_.ipLimit); diff --git a/src/xrpld/rpc/ServerHandler.h b/src/xrpld/rpc/ServerHandler.h index a8d4e900fc..032011ca77 100644 --- a/src/xrpld/rpc/ServerHandler.h +++ b/src/xrpld/rpc/ServerHandler.h @@ -28,9 +28,11 @@ #include #include #include + #include #include #include + #include #include #include @@ -71,15 +73,7 @@ public: client_t client; // Configuration for the Overlay - struct overlay_t - { - explicit overlay_t() = default; - - boost::asio::ip::address ip; - std::uint16_t port = 0; - }; - - overlay_t overlay; + boost::asio::ip::tcp::endpoint overlay; void makeContexts(); @@ -95,6 +89,7 @@ private: NetworkOPs& m_networkOPs; std::unique_ptr m_server; Setup setup_; + Endpoints endpoints_; JobQueue& m_jobQueue; beast::insight::Counter rpc_requests_; beast::insight::Event rpc_size_; @@ -145,6 +140,12 @@ public: return setup_; } + Endpoints const& + endpoints() const + { + return endpoints_; + } + void stop(); diff --git a/src/xrpld/rpc/detail/ServerHandler.cpp b/src/xrpld/rpc/detail/ServerHandler.cpp index 082cb6b74c..e5b7d9e327 100644 --- a/src/xrpld/rpc/detail/ServerHandler.cpp +++ b/src/xrpld/rpc/detail/ServerHandler.cpp @@ -44,9 +44,11 @@ #include #include #include + #include #include #include + #include #include @@ -130,7 +132,26 @@ void ServerHandler::setup(Setup const& setup, beast::Journal journal) { setup_ = setup; - m_server->ports(setup.ports); + endpoints_ = m_server->ports(setup.ports); + + // fix auto ports + for (auto& port : setup_.ports) + { + if (auto it = endpoints_.find(port.name); it != endpoints_.end()) + { + auto const endpointPort = it->second.port(); + if (!port.port) + port.port = endpointPort; + + if (!setup_.client.port && + (port.protocol.count("http") > 0 || + port.protocol.count("https") > 0)) + setup_.client.port = endpointPort; + + if (!setup_.overlay.port() && (port.protocol.count("peer") > 0)) + setup_.overlay.port(endpointPort); + } + } } //------------------------------------------------------------------------------ @@ -1094,11 +1115,6 @@ to_Port(ParsedPort const& parsed, std::ostream& log) log << "Missing 'port' in [" << p.name << "]"; Throw(); } - else if (*parsed.port == 0) - { - log << "Port " << *parsed.port << "in [" << p.name << "] is invalid"; - Throw(); - } p.port = *parsed.port; if (parsed.protocol.empty()) @@ -1157,7 +1173,6 @@ parse_Ports(Config const& config, std::ostream& log) continue; ParsedPort parsed = common; - parsed.name = name; parse_Port(parsed, config[name], log); result.push_back(to_Port(parsed, log)); } @@ -1232,11 +1247,10 @@ setup_Overlay(ServerHandler::Setup& setup) }); if (iter == setup.ports.cend()) { - setup.overlay.port = 0; + setup.overlay = {}; return; } - setup.overlay.ip = iter->ip; - setup.overlay.port = iter->port; + setup.overlay = {iter->ip, iter->port}; } ServerHandler::Setup