From d9e5cab264df1bdb15b8f2329c4ab1bf064fb259 Mon Sep 17 00:00:00 2001 From: Stephane Janel Date: Sun, 25 Feb 2024 20:32:18 +0100 Subject: [PATCH] Fix race condition for simultaneous calls to queryTradableMarkets from private exchanges --- src/api/common/include/exchangepublicapi.hpp | 4 +- src/api/common/src/exchangepublicapi.cpp | 84 +++++++++++-------- .../common/test/exchangepublicapi_test.cpp | 6 +- src/api/exchanges/src/binanceprivateapi.cpp | 12 ++- .../include/staticcommandlineoptioncheck.hpp | 14 ++-- src/http-request/src/curlhandle.cpp | 10 ++- src/main/CMakeLists.txt | 32 +++---- src/tech/include/threadpool.hpp | 8 +- 8 files changed, 94 insertions(+), 76 deletions(-) diff --git a/src/api/common/include/exchangepublicapi.hpp b/src/api/common/include/exchangepublicapi.hpp index ad445d85..8f240894 100644 --- a/src/api/common/include/exchangepublicapi.hpp +++ b/src/api/common/include/exchangepublicapi.hpp @@ -137,9 +137,9 @@ class ExchangePublic : public ExchangeBase { std::optional computeAvgOrderPrice(Market mk, MonetaryAmount from, const PriceOptions &priceOptions); /// Retrieve the market in the correct order proposed by the exchange for given couple of currencies. - Market retrieveMarket(CurrencyCode c1, CurrencyCode c2, const MarketSet &markets); + std::optional retrieveMarket(CurrencyCode c1, CurrencyCode c2, const MarketSet &markets); - Market retrieveMarket(CurrencyCode c1, CurrencyCode c2) { return retrieveMarket(c1, c2, queryTradableMarkets()); } + std::optional retrieveMarket(CurrencyCode c1, CurrencyCode c2); /// Helper method to determine ordered Market from this exchange from a market string representation without currency /// separator (for instance, "BTCEUR" should be guessed as a market with BTC as base currency, and EUR as price diff --git a/src/api/common/src/exchangepublicapi.cpp b/src/api/common/src/exchangepublicapi.cpp index 5c1d8876..c5cb6ce0 100644 --- a/src/api/common/src/exchangepublicapi.cpp +++ b/src/api/common/src/exchangepublicapi.cpp @@ -270,24 +270,29 @@ std::optional ExchangePublic::computeAvgOrderPrice(Market mk, Mo return queryOrderBook(mk, depth).computeAvgPrice(from, priceOptions); } -Market ExchangePublic::retrieveMarket(CurrencyCode c1, CurrencyCode c2, const MarketSet &markets) { +std::optional ExchangePublic::retrieveMarket(CurrencyCode c1, CurrencyCode c2, const MarketSet &markets) { Market mk(c1, c2); if (!markets.contains(mk)) { mk = mk.reverse(); if (!markets.contains(mk)) { - throw exception("Cannot find {}-{} nor {}-{} markets on {}", c1, c2, c2, c1, _name); + return {}; } } return mk; } +std::optional ExchangePublic::retrieveMarket(CurrencyCode c1, CurrencyCode c2) { + std::lock_guard guard(_tradableMarketsMutex); + return retrieveMarket(c1, c2, queryTradableMarkets()); +} + MarketPriceMap ExchangePublic::MarketPriceMapFromMarketOrderBookMap(const MarketOrderBookMap &marketOrderBookMap) { MarketPriceMap marketPriceMap; marketPriceMap.reserve(marketOrderBookMap.size()); - for (const auto &it : marketOrderBookMap) { - std::optional optAmount = it.second.averagePrice(); + for (const auto &[market, marketOrderBook] : marketOrderBookMap) { + std::optional optAmount = marketOrderBook.averagePrice(); if (optAmount) { - marketPriceMap.insert_or_assign(it.first, *optAmount); + marketPriceMap.insert_or_assign(market, *optAmount); } } return marketPriceMap; @@ -295,9 +300,6 @@ MarketPriceMap ExchangePublic::MarketPriceMapFromMarketOrderBookMap(const Market std::optional ExchangePublic::determineMarketFromMarketStr(std::string_view marketStr, MarketSet &markets, CurrencyCode filterCur) { - std::optional ret; - static constexpr std::string_view::size_type kMinimalCryptoAcronymLen = 3; - if (!filterCur.isNeutral()) { std::size_t firstCurLen; auto curStr = filterCur.str(); @@ -307,45 +309,55 @@ std::optional ExchangePublic::determineMarketFromMarketStr(std::string_v } else { firstCurLen = marketStr.size() - curStr.size(); } - ret = Market( + return Market( _coincenterInfo.standardizeCurrencyCode(std::string_view(marketStr.data(), firstCurLen)), _coincenterInfo.standardizeCurrencyCode(std::string_view(marketStr.begin() + firstCurLen, marketStr.end()))); - } else if (markets.empty() && marketStr.size() == 2 * kMinimalCryptoAcronymLen) { - // optim (to avoid possible queryTradableMarkets): there is no crypto currency acronym shorter than 3 chars - we - // can split the "symbol" string currencies with 3 chars each - ret = Market(_coincenterInfo.standardizeCurrencyCode(std::string_view(marketStr.data(), kMinimalCryptoAcronymLen)), + } + + static constexpr std::string_view::size_type kMinimalCryptoAcronymLen = 3; + + if (markets.empty() && marketStr.size() == 2 * kMinimalCryptoAcronymLen) { + // optim (to avoid possible queryTradableMarkets): assuming there is no crypto currency acronym shorter than 3 chars + // - we can split the "symbol" string currencies with 3 chars each + return Market(_coincenterInfo.standardizeCurrencyCode(std::string_view(marketStr.data(), kMinimalCryptoAcronymLen)), + _coincenterInfo.standardizeCurrencyCode( + std::string_view(marketStr.data() + kMinimalCryptoAcronymLen, kMinimalCryptoAcronymLen))); + } + + std::optional ret; + + // General case, we need to query the markets + if (markets.empty()) { + // Without any currency, and because "marketStr" is returned without hyphen, there is no easy way to guess the + // currencies so we need to compare with the markets that exist + std::lock_guard guard(_tradableMarketsMutex); + markets = queryTradableMarkets(); + } + const auto symbolStrSize = marketStr.size(); + for (auto splitCurPos = kMinimalCryptoAcronymLen; splitCurPos < symbolStrSize; ++splitCurPos) { + ret = Market(_coincenterInfo.standardizeCurrencyCode(std::string_view(marketStr.data(), splitCurPos)), _coincenterInfo.standardizeCurrencyCode( - std::string_view(marketStr.data() + kMinimalCryptoAcronymLen, kMinimalCryptoAcronymLen))); - } else { // General case, we need to query the markets - if (markets.empty()) { - // Without any currency, and because "marketStr" is returned without hyphen, there is no easy way to guess the - // currencies so we need to compare with the markets that exist - markets = queryTradableMarkets(); - } - const auto symbolStrSize = marketStr.size(); - for (auto splitCurPos = kMinimalCryptoAcronymLen; splitCurPos < symbolStrSize; ++splitCurPos) { - ret = Market(_coincenterInfo.standardizeCurrencyCode(std::string_view(marketStr.data(), splitCurPos)), - _coincenterInfo.standardizeCurrencyCode( - std::string_view(marketStr.data() + splitCurPos, symbolStrSize - splitCurPos))); - if (markets.contains(*ret)) { - break; - } - ret = ret->reverse(); - if (markets.contains(*ret)) { - break; - } + std::string_view(marketStr.data() + splitCurPos, symbolStrSize - splitCurPos))); + if (markets.contains(*ret)) { + break; } - if (!ret || ret->quote().size() < kMinimalCryptoAcronymLen) { - log::error("Cannot determine market for {}, skipping", marketStr); - ret = std::nullopt; + ret = ret->reverse(); + if (markets.contains(*ret)) { + break; } } + if (!ret || ret->quote().size() < kMinimalCryptoAcronymLen) { + log::error("Cannot determine market for {}, skipping", marketStr); + ret = std::nullopt; + } + return ret; } Market ExchangePublic::determineMarketFromFilterCurrencies(MarketSet &markets, CurrencyCode filterCur1, CurrencyCode filterCur2) { if (markets.empty()) { + std::lock_guard guard(_tradableMarketsMutex); markets = queryTradableMarkets(); } @@ -353,7 +365,7 @@ Market ExchangePublic::determineMarketFromFilterCurrencies(MarketSet &markets, C const auto tryAppendBaseCurrency = [&](CurrencyCode cur) { if (!cur.isNeutral()) { - auto firstMarketIt = std::ranges::partition_point(markets, [cur](Market mk) { return mk.base() < cur; }); + const auto firstMarketIt = std::ranges::partition_point(markets, [cur](Market mk) { return mk.base() < cur; }); if (firstMarketIt != markets.end() && firstMarketIt->base() == cur) { ret = Market(cur, CurrencyCode()); return true; diff --git a/src/api/common/test/exchangepublicapi_test.cpp b/src/api/common/test/exchangepublicapi_test.cpp index 08e2eba2..c6fdc457 100644 --- a/src/api/common/test/exchangepublicapi_test.cpp +++ b/src/api/common/test/exchangepublicapi_test.cpp @@ -78,9 +78,9 @@ TEST_F(ExchangePublicTest, FindCurrenciesPath) { TEST_F(ExchangePublicTest, RetrieveMarket) { EXPECT_CALL(exchangePublic, queryTradableMarkets()).WillOnce(::testing::Return(markets)); - EXPECT_EQ(exchangePublic.retrieveMarket("BTC", "KRW"), Market("BTC", "KRW")); - EXPECT_EQ(exchangePublic.retrieveMarket("KRW", "BTC", markets), Market("BTC", "KRW")); - EXPECT_THROW(exchangePublic.retrieveMarket("EUR", "EOS", markets), exception); + EXPECT_EQ(exchangePublic.retrieveMarket("BTC", "KRW").value(), Market("BTC", "KRW")); + EXPECT_EQ(exchangePublic.retrieveMarket("KRW", "BTC", markets).value(), Market("BTC", "KRW")); + EXPECT_FALSE(exchangePublic.retrieveMarket("EUR", "EOS", markets).has_value()); } class ExchangePublicConvertTest : public ExchangePublicTest { diff --git a/src/api/exchanges/src/binanceprivateapi.cpp b/src/api/exchanges/src/binanceprivateapi.cpp index dbc75c07..12720dce 100644 --- a/src/api/exchanges/src/binanceprivateapi.cpp +++ b/src/api/exchanges/src/binanceprivateapi.cpp @@ -34,6 +34,7 @@ #include "exchangename.hpp" #include "exchangeprivateapi.hpp" #include "exchangeprivateapitypes.hpp" +#include "exchangepublicapi.hpp" #include "exchangepublicapitypes.hpp" #include "httprequesttype.hpp" #include "market.hpp" @@ -275,14 +276,11 @@ Wallet BinancePrivate::DepositWalletFunc::operator()(CurrencyCode currencyCode) } bool BinancePrivate::checkMarketAppendSymbol(Market mk, CurlPostData& params) { - MarketSet markets = _exchangePublic.queryTradableMarkets(); - if (!markets.contains(mk)) { - mk = mk.reverse(); - if (!markets.contains(mk)) { - return false; - } + const auto optMarket = _exchangePublic.retrieveMarket(mk.base(), mk.quote()); + if (!optMarket) { + return false; } - params.append("symbol", mk.assetsPairStrUpper()); + params.append("symbol", optMarket->assetsPairStrUpper()); return true; } diff --git a/src/engine/include/staticcommandlineoptioncheck.hpp b/src/engine/include/staticcommandlineoptioncheck.hpp index 4fa6d26e..3edb4aaa 100644 --- a/src/engine/include/staticcommandlineoptioncheck.hpp +++ b/src/engine/include/staticcommandlineoptioncheck.hpp @@ -18,16 +18,16 @@ consteval bool StaticCommandLineOptionsDuplicatesCheck(std::array... ar) { auto all = ComputeAllCommandLineOptions(ar...); // Check short names equality with a bitset hashmap of presence - // (std::bitset is unfortunately not constexpr yet) + // (std::bitset is unfortunately not constexpr in C++20) uint64_t charPresenceBmp[8]{}; - for (const auto& f : all) { - if (f.hasShortName()) { - uint8_t c = static_cast(f.shortNameChar()); - uint64_t& subBmp = charPresenceBmp[c / 64]; - if ((subBmp & (static_cast(1) << (c % 64)))) { + for (const auto& commandLineOption : all) { + if (commandLineOption.hasShortName()) { + uint8_t shortNameChar = static_cast(commandLineOption.shortNameChar()); + uint64_t& subBmp = charPresenceBmp[shortNameChar / 64]; + if ((subBmp & (static_cast(1) << (shortNameChar % 64)))) { return false; } - subBmp |= (static_cast(1) << (c % 64)); + subBmp |= (static_cast(1) << (shortNameChar % 64)); } } diff --git a/src/http-request/src/curlhandle.cpp b/src/http-request/src/curlhandle.cpp index d5f575ed..30d9b969 100644 --- a/src/http-request/src/curlhandle.cpp +++ b/src/http-request/src/curlhandle.cpp @@ -69,10 +69,18 @@ void CurlSetLogIfError(CURL *curl, CURLoption curlOption, T value) { string GetCurlVersionInfo() { const curl_version_info_data &curlVersionInfo = *curl_version_info(CURLVERSION_NOW); + string curlVersionInfoStr("curl "); curlVersionInfoStr.append(curlVersionInfo.version); + if (curlVersionInfo.ssl_version == nullptr) { + throw exception("Invalid curl install - no open ssl support"); + } curlVersionInfoStr.append(" ssl ").append(curlVersionInfo.ssl_version); - curlVersionInfoStr.append(" libz ").append(curlVersionInfo.libz_version); + if (curlVersionInfo.libz_version != nullptr) { + curlVersionInfoStr.append(" libz ").append(curlVersionInfo.libz_version); + } else { + curlVersionInfoStr.append(" NO SSL SUPPORT"); + } return curlVersionInfoStr; } diff --git a/src/main/CMakeLists.txt b/src/main/CMakeLists.txt index d97cde5b..9198d607 100644 --- a/src/main/CMakeLists.txt +++ b/src/main/CMakeLists.txt @@ -1,30 +1,30 @@ aux_source_directory(src MAIN_SRC) if(CCT_BUILD_EXEC) - add_executable(coincenter ${MAIN_SRC}) + add_executable(coincenter ${MAIN_SRC}) - # Enable LTO with coincenter in Release mode - if(CMAKE_BUILD_TYPE STREQUAL "Release") - set_property(TARGET coincenter PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE) - message(STATUS "Activate LTO for coincenter") - endif() + # Enable LTO with coincenter in Release mode + if(CMAKE_BUILD_TYPE STREQUAL "Release") + set_property(TARGET coincenter PROPERTY INTERPROCEDURAL_OPTIMIZATION TRUE) + message(STATUS "Activate LTO for coincenter") + endif() else() - list(REMOVE_ITEM ENGINE_SRC "src/main.cpp") - add_library(coincenter STATIC ${MAIN_SRC}) + list(REMOVE_ITEM ENGINE_SRC "src/main.cpp") + add_library(coincenter STATIC ${MAIN_SRC}) endif() target_link_libraries(coincenter PUBLIC coincenter_engine) set_target_properties(coincenter PROPERTIES - VERSION ${PROJECT_VERSION} - COMPILE_DEFINITIONS_DEBUG "JSON_DEBUG;JSON_SAFE;JSON_ISO_STRICT" - RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR} + VERSION ${PROJECT_VERSION} + COMPILE_DEFINITIONS_DEBUG "JSON_DEBUG;JSON_SAFE;JSON_ISO_STRICT" + RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR} ) add_unit_test( - processcommandsfromcli_test - src/processcommandsfromcli.cpp - test/processcommandsfromcli_test.cpp - LIBRARIES - coincenter_engine + processcommandsfromcli_test + src/processcommandsfromcli.cpp + test/processcommandsfromcli_test.cpp + LIBRARIES + coincenter_engine ) \ No newline at end of file diff --git a/src/tech/include/threadpool.hpp b/src/tech/include/threadpool.hpp index 778c3be5..319cb5c2 100644 --- a/src/tech/include/threadpool.hpp +++ b/src/tech/include/threadpool.hpp @@ -40,7 +40,7 @@ class ThreadPool { // add new work item to the pool template - std::future::type> enqueue(Func&& f, Args&&... args); + std::future> enqueue(Func&& func, Args&&... args); // Parallel version of std::transform with unary operation. // This function will first enqueue all the tasks at one, using waiting threads of the thread pool, @@ -104,11 +104,11 @@ inline ThreadPool::~ThreadPool() { } template -inline std::future::type> ThreadPool::enqueue(Func&& f, Args&&... args) { - using return_type = typename std::invoke_result::type; +inline std::future> ThreadPool::enqueue(Func&& func, Args&&... args) { + using return_type = std::invoke_result_t; auto task = std::make_shared>( - std::bind(std::forward(f), std::forward(args)...)); + std::bind(std::forward(func), std::forward(args)...)); std::future res = task->get_future(); {