diff --git a/Builds/CMake/RippledCore.cmake b/Builds/CMake/RippledCore.cmake index 5a8f469d2..3f0cd5c81 100644 --- a/Builds/CMake/RippledCore.cmake +++ b/Builds/CMake/RippledCore.cmake @@ -157,7 +157,9 @@ install ( src/ripple/basics/MathUtilities.h src/ripple/basics/safe_cast.h src/ripple/basics/Slice.h + src/ripple/basics/spinlock.h src/ripple/basics/StringUtilities.h + src/ripple/basics/ThreadSafetyAnalysis.h src/ripple/basics/ToString.h src/ripple/basics/UnorderedContainers.h src/ripple/basics/XRPAmount.h diff --git a/Builds/CMake/RippledDocs.cmake b/Builds/CMake/RippledDocs.cmake index 7d9ef90fd..6cb8f730d 100644 --- a/Builds/CMake/RippledDocs.cmake +++ b/Builds/CMake/RippledDocs.cmake @@ -31,7 +31,7 @@ if (tests) # find_path sets a CACHE variable, so don't try using a "local" variable. find_path (${variable} "${name}" ${ARGN}) if (NOT ${variable}) - message (WARNING "could not find ${name}") + message (NOTICE "could not find ${name}") else () message (STATUS "found ${name}: ${${variable}}/${name}") endif () diff --git a/Builds/linux/README.md b/Builds/linux/README.md index bb5bc7618..001a3705c 100644 --- a/Builds/linux/README.md +++ b/Builds/linux/README.md @@ -239,3 +239,32 @@ change the `/opt/local` module path above to match your chosen installation pref `rippled` builds a set of unit tests into the server executable. To run these unit tests after building, pass the `--unittest` option to the compiled `rippled` executable. The executable will exit with summary info after running the unit tests. + +## Workaround for a compile error in soci + +Compilation errors have been observed with Apple Clang 13.1.6+ and soci v4.x. soci compiles with the `-Werror` flag which causes warnings to be treated as errors. These warnings pertain to style (not correctness). However, they cause the cmake process to fail. + +Here's an example of how this looks: +``` +.../rippled/.nih_c/unix_makefiles/AppleClang_13.1.6.13160021/Debug/src/soci/src/core/session.cpp:450:66: note: in instantiation of function template specialization 'soci::use' requested here + return prepare << backEnd_->get_column_descriptions_query(), use(table_name, "t"); + ^ +1 error generated. +``` + +Please apply the below patch (courtesy of Scott Determan) to remove these errors. `.nih_c/unix_makefiles/AppleClang_13.1.6.13160021/Debug/src/soci/cmake/SociConfig.cmake` file needs to be edited. This file is an example for Mac OS and it might be slightly different for other OS/Architectures. + +``` +diff --git a/cmake/SociConfig.cmake b/cmake/SociConfig.cmake +index 97d907e4..11bcd1f3 100644 +--- a/cmake/SociConfig.cmake ++++ b/cmake/SociConfig.cmake +@@ -58,8 +58,8 @@ if (MSVC) + + else() + +- set(SOCI_GCC_CLANG_COMMON_FLAGS +- "-pedantic -Werror -Wno-error=parentheses -Wall -Wextra -Wpointer-arith -Wcast-align -Wcast-qual -Wfloat-equal -Woverloaded-virtual -Wredundant-decls -Wno-long-long") ++ set(SOCI_GCC_CLANG_COMMON_FLAGS "") ++ # "-pedantic -Werror -Wno-error=parentheses -Wall -Wextra -Wpointer-arith -Wcast-align -Wcast-qual -Wfloat-equal -Woverloaded-virtual -Wredundant-decls -Wno-long-long") +``` diff --git a/CMakeLists.txt b/CMakeLists.txt index 8b05f7216..2c4c643d5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -23,6 +23,12 @@ if(Git_FOUND) endif() endif() #git +if (thread_safety_analysis) + add_compile_options(-Wthread-safety -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS -DRIPPLE_ENABLE_THREAD_SAFETY_ANNOTATIONS) + add_compile_options("-stdlib=libc++") + add_link_options("-stdlib=libc++") +endif() + list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/Builds/CMake") list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/Builds/CMake/deps") diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 000000000..44530b4f7 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,67 @@ +# Contributing +The XRP Ledger has many and diverse stakeholders, and everyone deserves a chance to contribute meaningful changes to the code that runs the XRPL. +To contribute, please: +1. Fork the repository under your own user. +2. Create a new branch on which to write your changes. Please note that changes which alter transaction processing must be composed via and guarded using [Amendments](https://xrpl.org/amendments.html). Changes which are _read only_ i.e. RPC, or changes which are only refactors and maintain the existing behaviour do not need to be made through an Amendment. +3. Write and test your code. +4. Ensure that your code compiles with the provided build engine and update the provided build engine as part of your PR where needed and where appropriate. +5. Write test cases for your code and include those in `src/test` such that they are runnable from the command line using `./rippled -u`. (Some changes will not be able to be tested this way.) +6. Ensure your code passes automated checks (e.g. clang-format and levelization.) +7. Squash your commits (i.e. rebase) into as few commits as is reasonable to describe your changes at a high level (typically a single commit for a small change.) +8. Open a PR to the main repository onto the _develop_ branch, and follow the provided template. + +# Major Changes +If your code change is a major feature, a breaking change or in some other way makes a significant alteration to the way the XRPL will operate, then you must first write an XLS document (XRP Ledger Standard) describing your change. +To do this: +1. Go to [XLS Standards](https://github.com/XRPLF/XRPL-Standards/discussions). +2. Choose the next available standard number. +3. Open a discussion with the appropriate title to propose your draft standard. +4. Link your XLS in your PR. + +# Style guide +This is a non-exhaustive list of recommended style guidelines. These are not always strictly enforced and serve as a way to keep the codebase coherent rather than a set of _thou shalt not_ commandments. + +## Formatting +All code must conform to `clang-format` version 10, unless the result would be unreasonably difficult to read or maintain. +To change your code to conform use `clang-format -i `. + +## Avoid +1. Proliferation of nearly identical code. +2. Proliferation of new files and classes. +3. Complex inheritance and complex OOP patterns. +4. Unmanaged memory allocation and raw pointers. +5. Macros and non-trivial templates (unless they add significant value.) +6. Lambda patterns (unless these add significant value.) +7. CPU or architecture-specific code unless there is a good reason to include it, and where it is used guard it with macros and provide explanatory comments. +8. Importing new libraries unless there is a very good reason to do so. + +## Seek to +9. Extend functionality of existing code rather than creating new code. +10. Prefer readability over terseness where important logic is concerned. +11. Inline functions that are not used or are not likely to be used elsewhere in the codebase. +12. Use clear and self-explanatory names for functions, variables, structs and classes. +13. Use TitleCase for classes, structs and filenames, camelCase for function and variable names, lower case for namespaces and folders. +14. Provide as many comments as you feel that a competent programmer would need to understand what your code does. + +# Maintainers +Maintainers are ecosystem participants with elevated access to the repository. They are able to push new code, make decisions on when a release should be made, etc. + +## Code Review +New contributors' PRs must be reviewed by at least two of the maintainers. Well established prior contributors can be reviewed by a single maintainer. + +## Adding and Removing +New maintainers can be proposed by two existing maintainers, subject to a vote by a quorum of the existing maintainers. A minimum of 50% support and a 50% participation is required. In the event of a tie vote, the addition of the new maintainer will be rejected. + +Existing maintainers can resign, or be subject to a vote for removal at the behest of two existing maintainers. A minimum of 60% agreement and 50% participation are required. The XRP Ledger Foundation will have the ability, for cause, to remove an existing maintainer without a vote. + +## Existing Maintainers +* [JoelKatz](https://github.com/JoelKatz) (Ripple) +* [Manojsdoshi](https://github.com/manojsdoshi) (Ripple) +* [N3tc4t](https://github.com/n3tc4t) (XRPL Labs) +* [Nikolaos D Bougalis](https://github.com/nbougalis) (Ripple) +* [Nixer89](https://github.com/nixer89) (XRP Ledger Foundation) +* [RichardAH](https://github.com/RichardAH) (XRPL Labs + XRP Ledger Foundation) +* [Seelabs](https://github.com/seelabs) (Ripple) +* [Silkjaer](https://github.com/Silkjaer) (XRP Ledger Foundation) +* [WietseWind](https://github.com/WietseWind) (XRPL Labs + XRP Ledger Foundation) +* [Ximinez](https://github.com/ximinez) (Ripple) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 9412e13ef..61ac91631 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -5,7 +5,70 @@ This document contains the release notes for `rippled`, the reference server implementation of the XRP Ledger protocol. To learn more about how to build, run or update a `rippled` server, visit https://xrpl.org/install-rippled.html -Have new ideas? Need help with setting up your node? Come visit us [here](https://github.com/ripple/rippled/issues/new/choose) +Have new ideas? Need help with setting up your node? Come visit us [here](https://github.com/xrplf/rippled/issues/new/choose) + +# Introducing XRP Ledger version 1.9.2 + +Version 1.9.2 of `rippled`, the reference server implementation of the XRP Ledger protocol, is now available. This release includes several fixes and improvements, including a second new fix amendment to correct a bug in Non-Fungible Tokens (NFTs) code, a new API method for order book changes, less noisy logging, and other small fixes. + + + + +## Action Required + +This release introduces a two new amendments to the XRP Ledger protocol. The first, **fixNFTokenNegOffer**, fixes a bug in code associated with the **NonFungibleTokensV1** amendment, originally introduced in [version 1.9.0](https://xrpl.org/blog/2022/rippled-1.9.0.html). The second, **NonFungibleTokensV1_1**, is a "roll-up" amendment that enables the **NonFungibleTokensV1** feature plus the two fix amendments associated with it, **fixNFTokenDirV1** and **fixNFTokenNegOffer**. + +If you want to enable NFT code on the XRP Ledger Mainnet, you can vote in favor of only the **NonFungibleTokensV1_1** amendment to support enabling the feature and fixes together, without risk that the unfixed NFT code may become enabled first. + +These amendments are now open for voting according to the XRP Ledger's [amendment process](https://xrpl.org/amendments.html), which enables protocol changes following two weeks of >80% support from trusted validators. + +If you operate an XRP Ledger server, then you should upgrade to version 1.9.2 within two weeks, to ensure service continuity. The exact time that protocol changes take effect depends on the voting decisions of the decentralized network. + +For more information about NFTs on the XRP Ledger, see [NFT Conceptual Overview](https://xrpl.org/nft-conceptual-overview.html). + +## Install / Upgrade + +On supported platforms, see the [instructions on installing or updating `rippled`](https://xrpl.org/install-rippled.html). + +## Changelog + +This release contains the following features and improvements. + +- **Introduce fixNFTokenNegOffer amendment.** This amendment fixes a bug in the Non-Fungible Tokens (NFTs) functionality provided by the NonFungibleTokensV1 amendment (not currently enabled on Mainnet). The bug allowed users to place offers to buy tokens for negative amounts of money when using Brokered Mode. Anyone who accepted such an offer would transfer the token _and_ pay money. This amendment explicitly disallows offers to buy or sell NFTs for negative amounts of money, and returns an appropriate error code. This also corrects the error code returned when placing offers to buy or sell NFTs for negative amounts in Direct Mode. ([8266d9d](https://github.com/XRPLF/rippled/commit/8266d9d598d19f05e1155956b30ca443c27e119e)) +- **Introduce `NonFungibleTokensV1_1` amendment.** This amendment encompasses three NFT-related amendments: the original NonFungibleTokensV1 amendment (from version 1.9.0), the fixNFTokenDirV1 amendment (from version 1.9.1), and the new fixNFTokenNegOffer amendment from this release. This amendment contains no changes other than enabling those three amendments together; this allows validators to vote in favor of _only_ enabling the feature and fixes at the same time. ([59326bb](https://github.com/XRPLF/rippled/commit/59326bbbc552287e44b3a0d7b8afbb1ddddb3e3b)) +- **Handle invalid port numbers.** If the user specifies a URL with an invalid port number, the server would silently attempt to use port 0 instead. Now it raises an error instead. This affects admin API methods and config file parameters for downloading history shards and specifying validator list sites. ([#4213](https://github.com/XRPLF/rippled/pull/4213)) +- **Reduce log noisiness.** Decreased the severity of benign log messages in several places: "addPathsForType" messages during regular operation, expected errors during unit tests, and missing optional documentation components when compiling from source. ([#4178](https://github.com/XRPLF/rippled/pull/4178), [#4166](https://github.com/XRPLF/rippled/pull/4166), [#4180](https://github.com/XRPLF/rippled/pull/4180)) +- **Fix race condition in history shard implementation and support clang's ThreadSafetyAnalysis tool.** Added build settings so that developers can use this feature of the clang compiler to analyze the code for correctness, and fix an error found by this tool, which was the source of rare crashes in unit tests. ([#4188](https://github.com/XRPLF/rippled/pull/4188)) +- **Prevent crash when rotating a database with missing data.** When rotating databases, a missing entry could cause the server to crash. While there should never be a missing database entry, this change keeps the server running by aborting database rotation. ([#4182](https://github.com/XRPLF/rippled/pull/4182)) +- **Fix bitwise comparison in OfferCreate.** Fixed an expression that incorrectly used a bitwise comparison for two boolean values rather than a true boolean comparison. The outcome of the two comparisons is equivalent, so this is not a transaction processing change, but the bitwise comparison relied on compilers to implicitly fix the expression. ([#4183](https://github.com/XRPLF/rippled/pull/4183)) +- **Disable cluster timer when not in a cluster.** Disabled a timer that was unused on servers not running in clustered mode. The functionality of clustered servers is unchanged. ([#4173](https://github.com/XRPLF/rippled/pull/4173)) +- **Limit how often to process peer discovery messages.** In the peer-to-peer network, servers periodically share IP addresses of their peers with each other to facilitate peer discovery. It is not necessary to process these types of messages too often; previously, the code tracked whether it needed to process new messages of this type but always processed them anyway. With this change, the server no longer processes peer discovery messages if it has done so recently. ([#4202](https://github.com/XRPLF/rippled/pull/4202)) +- **Improve STVector256 deserialization.** Optimized the processing of this data type in protocol messages. This data type is used in several types of ledger entry that are important for bookkeeping, including directory pages that track other ledger types, amendments tracking, and the ledger hashes history. ([#4204](https://github.com/XRPLF/rippled/pull/4204)) +- **Fix and refactor spinlock code.** The spinlock code, which protects the `SHAMapInnerNode` child lists, had a mistake that allowed the same child to be repeatedly locked under some circumstances. Fixed this bug and improved the spinlock code to make it easier to use correctly and easier to verify that the code works correctly. ([#4201](https://github.com/XRPLF/rippled/pull/4201)) +- **Improve comments and contributor documentation.** Various minor documentation changes including some to reflect the fact that the source code repository is now owned by the XRP Ledger Foundation. ([#4214](https://github.com/XRPLF/rippled/pull/4214), [#4179](https://github.com/XRPLF/rippled/pull/4179), [#4222](https://github.com/XRPLF/rippled/pull/4222)) +- **Introduces a new API book_changes to provide information in a format that is useful for building charts that highlight DEX activity at a per-ledger level.** ([#4212](https://github.com/XRPLF/rippled/pull/4212)) + +## Contributions + +### GitHub + +The public source code repository for `rippled` is hosted on GitHub at . + +We welcome contributions, big and small, and invite everyone to join the community of XRP Ledger developers and help us build the Internet of Value. + +### Credits + +The following people contributed directly to this release: + +- Chenna Keshava B S +- Ed Hennis +- Ikko Ashimine +- Nik Bougalis +- Richard Holland +- Scott Schurr +- Scott Determan + +For a real-time view of all lifetime contributors, including links to the commits made by each, please visit the "Contributors" section of the GitHub repository: . # Introducing XRP Ledger version 1.9.1 diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 1bd635654..631d86da0 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -920,7 +920,10 @@ void NetworkOPsImp::setStateTimer() { setHeartbeatTimer(); - setClusterTimer(); + + // Only do this work if a cluster is configured + if (app_.cluster().size() != 0) + setClusterTimer(); } void @@ -973,6 +976,7 @@ void NetworkOPsImp::setClusterTimer() { using namespace std::chrono_literals; + setTimer( clusterTimer_, 10s, @@ -1058,7 +1062,11 @@ NetworkOPsImp::processHeartbeatTimer() void NetworkOPsImp::processClusterTimer() { + if (app_.cluster().size() == 0) + return; + using namespace std::chrono_literals; + bool const update = app_.cluster().update( app_.nodeIdentity().first, "", @@ -2384,10 +2392,6 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters) if (!app_.config().SERVER_DOMAIN.empty()) info[jss::server_domain] = app_.config().SERVER_DOMAIN; - if (!app_.config().reporting()) - if (auto const netid = app_.overlay().networkID()) - info[jss::network_id] = static_cast(*netid); - info[jss::build_version] = BuildInfo::getVersionString(); info[jss::server_state] = strOperatingMode(admin); @@ -2530,6 +2534,9 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters) if (!app_.config().reporting()) { + if (auto const netid = app_.overlay().networkID()) + info[jss::network_id] = static_cast(*netid); + auto const escalationMetrics = app_.getTxQ().getMetrics(*app_.openLedger().current()); diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 70519fc92..af568d027 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -17,17 +17,18 @@ */ //============================================================================== +#include + #include #include -#include #include #include #include #include #include -#include - #include +#include +#include #include @@ -363,11 +364,24 @@ SHAMapStoreImp::run() JLOG(journal_.debug()) << "copying ledger " << validatedSeq; std::uint64_t nodeCount = 0; - validatedLedger->stateMap().snapShot(false)->visitNodes(std::bind( - &SHAMapStoreImp::copyNode, - this, - std::ref(nodeCount), - std::placeholders::_1)); + + try + { + validatedLedger->stateMap().snapShot(false)->visitNodes( + std::bind( + &SHAMapStoreImp::copyNode, + this, + std::ref(nodeCount), + std::placeholders::_1)); + } + catch (SHAMapMissingNode const& e) + { + JLOG(journal_.error()) + << "Missing node while copying ledger before rotate: " + << e.what(); + continue; + } + if (stopping()) return; // Only log if we completed without a "health" abort diff --git a/src/ripple/app/paths/Pathfinder.cpp b/src/ripple/app/paths/Pathfinder.cpp index 71a4afa75..556622ee7 100644 --- a/src/ripple/app/paths/Pathfinder.cpp +++ b/src/ripple/app/paths/Pathfinder.cpp @@ -796,8 +796,8 @@ Pathfinder::addPathsForType( PathType const& pathType, std::function const& continueCallback) { - JLOG(j_.warn()) << "addPathsForType " - << CollectionAndDelimiter(pathType, ", "); + JLOG(j_.debug()) << "addPathsForType " + << CollectionAndDelimiter(pathType, ", "); // See if the set of paths for this type already exists. auto it = mPaths.find(pathType); if (it != mPaths.end()) diff --git a/src/ripple/app/rdb/impl/Wallet.cpp b/src/ripple/app/rdb/impl/Wallet.cpp index c6040964b..24715404c 100644 --- a/src/ripple/app/rdb/impl/Wallet.cpp +++ b/src/ripple/app/rdb/impl/Wallet.cpp @@ -254,7 +254,10 @@ readAmendments( soci::transaction tr(session); std::string sql = - "SELECT AmendmentHash, AmendmentName, Veto FROM FeatureVotes"; + "SELECT AmendmentHash, AmendmentName, Veto FROM " + "( SELECT AmendmentHash, AmendmentName, Veto, RANK() OVER " + "( PARTITION BY AmendmentHash ORDER BY ROWID DESC ) " + "as rnk FROM FeatureVotes ) WHERE rnk = 1"; // SOCI requires boost::optional (not std::optional) as parameters. boost::optional amendment_hash; boost::optional amendment_name; diff --git a/src/ripple/app/reporting/P2pProxy.cpp b/src/ripple/app/reporting/P2pProxy.cpp index 8e4fc3a97..ee04b68e6 100644 --- a/src/ripple/app/reporting/P2pProxy.cpp +++ b/src/ripple/app/reporting/P2pProxy.cpp @@ -72,7 +72,7 @@ shouldForwardToP2p(RPC::JsonContext& context) if (params.isMember(jss::ledger_index)) { auto indexValue = params[jss::ledger_index]; - if (!indexValue.isNumeric()) + if (indexValue.isString()) { auto index = indexValue.asString(); return index == "current" || index == "closed"; diff --git a/src/ripple/app/tx/impl/CreateOffer.cpp b/src/ripple/app/tx/impl/CreateOffer.cpp index fb7d49208..6a8223e81 100644 --- a/src/ripple/app/tx/impl/CreateOffer.cpp +++ b/src/ripple/app/tx/impl/CreateOffer.cpp @@ -745,7 +745,7 @@ CreateOffer::flowCross( // additional path with XRP as the intermediate between two books. // This second path we have to build ourselves. STPathSet paths; - if (!takerAmount.in.native() & !takerAmount.out.native()) + if (!takerAmount.in.native() && !takerAmount.out.native()) { STPath path; path.emplace_back(std::nullopt, xrpCurrency(), std::nullopt); diff --git a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp index 7c78f175f..fb5f51c72 100644 --- a/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenAcceptOffer.cpp @@ -75,6 +75,12 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) if (hasExpired(ctx.view, (*offerSLE)[~sfExpiration])) return {nullptr, tecEXPIRED}; + // The initial implementation had a bug that allowed a negative + // amount. The fixNFTokenNegOffer amendment fixes that. + if ((*offerSLE)[sfAmount].negative() && + ctx.view.rules().enabled(fixNFTokenNegOffer)) + return {nullptr, temBAD_OFFER}; + return {std::move(offerSLE), tesSUCCESS}; } return {nullptr, tesSUCCESS}; @@ -103,6 +109,14 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) if ((*so)[sfAmount] > (*bo)[sfAmount]) return tecINSUFFICIENT_PAYMENT; + // If the buyer specified a destination, that destination must be + // the seller or the broker. + if (auto const dest = bo->at(~sfDestination)) + { + if (*dest != so->at(sfOwner) && *dest != ctx.tx[sfAccount]) + return tecNFTOKEN_BUY_SELL_MISMATCH; + } + // If the seller specified a destination, that destination must be // the buyer or the broker. if (auto const dest = so->at(~sfDestination)) @@ -142,6 +156,15 @@ NFTokenAcceptOffer::preclaim(PreclaimContext const& ctx) !nft::findToken(ctx.view, ctx.tx[sfAccount], (*bo)[sfNFTokenID])) return tecNO_PERMISSION; + // If not in bridged mode... + if (!so) + { + // If the offer has a Destination field, the acceptor must be the + // Destination. + if (auto const dest = bo->at(~sfDestination); + dest.has_value() && *dest != ctx.tx[sfAccount]) + return tecNO_PERMISSION; + } // The account offering to buy must have funds: auto const needed = bo->at(sfAmount); diff --git a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp index 9a6ea5da4..f6f89633c 100644 --- a/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp +++ b/src/ripple/app/tx/impl/NFTokenCreateOffer.cpp @@ -46,7 +46,11 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx) auto const nftFlags = nft::getFlags(ctx.tx[sfNFTokenID]); { - auto const amount = ctx.tx[sfAmount]; + STAmount const amount = ctx.tx[sfAmount]; + + if (amount.negative() && ctx.rules.enabled(fixNFTokenNegOffer)) + // An offer for a negative amount makes no sense. + return temBAD_AMOUNT; if (!isXRP(amount)) { @@ -78,9 +82,14 @@ NFTokenCreateOffer::preflight(PreflightContext const& ctx) if (auto dest = ctx.tx[~sfDestination]) { - // The destination field is only valid on a sell offer; it makes no - // sense in a buy offer. - if (!isSellOffer) + // Some folks think it makes sense for a buy offer to specify a + // specific broker using the Destination field. This change doesn't + // deserve it's own amendment, so we're piggy-backing on + // fixNFTokenNegOffer. + // + // Prior to fixNFTokenNegOffer any use of the Destination field on + // a buy offer was malformed. + if (!isSellOffer && !ctx.rules.enabled(fixNFTokenNegOffer)) return temMALFORMED; // The destination can't be the account executing the transaction. diff --git a/src/ripple/basics/Slice.h b/src/ripple/basics/Slice.h index 67c954bb7..0ba6a94b6 100644 --- a/src/ripple/basics/Slice.h +++ b/src/ripple/basics/Slice.h @@ -48,7 +48,8 @@ private: std::size_t size_ = 0; public: - using const_iterator = std::uint8_t const*; + using value_type = std::uint8_t; + using const_iterator = value_type const*; /** Default constructed Slice has length 0. */ Slice() noexcept = default; @@ -75,13 +76,13 @@ public: This may be zero for an empty range. */ /** @{ */ - std::size_t + [[nodiscard]] std::size_t size() const noexcept { return size_; } - std::size_t + [[nodiscard]] std::size_t length() const noexcept { return size_; diff --git a/src/ripple/basics/ThreadSafetyAnalysis.h b/src/ripple/basics/ThreadSafetyAnalysis.h new file mode 100644 index 000000000..b1889d5b4 --- /dev/null +++ b/src/ripple/basics/ThreadSafetyAnalysis.h @@ -0,0 +1,63 @@ +#ifndef RIPPLE_BASICS_THREAD_SAFTY_ANALYSIS_H_INCLUDED +#define RIPPLE_BASICS_THREAD_SAFTY_ANALYSIS_H_INCLUDED + +#ifdef RIPPLE_ENABLE_THREAD_SAFETY_ANNOTATIONS +#define THREAD_ANNOTATION_ATTRIBUTE__(x) __attribute__((x)) +#else +#define THREAD_ANNOTATION_ATTRIBUTE__(x) // no-op +#endif + +#define CAPABILITY(x) THREAD_ANNOTATION_ATTRIBUTE__(capability(x)) + +#define SCOPED_CAPABILITY THREAD_ANNOTATION_ATTRIBUTE__(scoped_lockable) + +#define GUARDED_BY(x) THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x)) + +#define PT_GUARDED_BY(x) THREAD_ANNOTATION_ATTRIBUTE__(pt_guarded_by(x)) + +#define ACQUIRED_BEFORE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquired_before(__VA_ARGS__)) + +#define ACQUIRED_AFTER(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquired_after(__VA_ARGS__)) + +#define REQUIRES(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_capability(__VA_ARGS__)) + +#define REQUIRES_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(requires_shared_capability(__VA_ARGS__)) + +#define ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_capability(__VA_ARGS__)) + +#define ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(acquire_shared_capability(__VA_ARGS__)) + +#define RELEASE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_capability(__VA_ARGS__)) + +#define RELEASE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_shared_capability(__VA_ARGS__)) + +#define RELEASE_GENERIC(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(release_generic_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_capability(__VA_ARGS__)) + +#define TRY_ACQUIRE_SHARED(...) \ + THREAD_ANNOTATION_ATTRIBUTE__(try_acquire_shared_capability(__VA_ARGS__)) + +#define EXCLUDES(...) THREAD_ANNOTATION_ATTRIBUTE__(locks_excluded(__VA_ARGS__)) + +#define ASSERT_CAPABILITY(x) THREAD_ANNOTATION_ATTRIBUTE__(assert_capability(x)) + +#define ASSERT_SHARED_CAPABILITY(x) \ + THREAD_ANNOTATION_ATTRIBUTE__(assert_shared_capability(x)) + +#define RETURN_CAPABILITY(x) THREAD_ANNOTATION_ATTRIBUTE__(lock_returned(x)) + +#define NO_THREAD_SAFETY_ANALYSIS \ + THREAD_ANNOTATION_ATTRIBUTE__(no_thread_safety_analysis) + +#endif diff --git a/src/ripple/basics/base_uint.h b/src/ripple/basics/base_uint.h index ccbb24a13..8f277c300 100644 --- a/src/ripple/basics/base_uint.h +++ b/src/ripple/basics/base_uint.h @@ -26,6 +26,7 @@ #define RIPPLE_BASICS_BASE_UINT_H_INCLUDED #include +#include #include #include #include @@ -56,6 +57,11 @@ struct is_contiguous_container< { }; +template <> +struct is_contiguous_container : std::true_type +{ +}; + } // namespace detail /** Integers of any length that is a multiple of 32-bits diff --git a/src/ripple/basics/impl/StringUtilities.cpp b/src/ripple/basics/impl/StringUtilities.cpp index 8036cc3bf..bebbe1ef8 100644 --- a/src/ripple/basics/impl/StringUtilities.cpp +++ b/src/ripple/basics/impl/StringUtilities.cpp @@ -90,6 +90,13 @@ parseUrl(parsedURL& pUrl, std::string const& strUrl) if (!port.empty()) { pUrl.port = beast::lexicalCast(port); + + // For inputs larger than 2^32-1 (65535), lexicalCast returns 0. + // parseUrl returns false for such inputs. + if (pUrl.port == 0) + { + return false; + } } pUrl.path = smMatch[6]; diff --git a/src/ripple/basics/spinlock.h b/src/ripple/basics/spinlock.h new file mode 100644 index 000000000..85a2ac41d --- /dev/null +++ b/src/ripple/basics/spinlock.h @@ -0,0 +1,223 @@ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright 2022, Nikolaos D. Bougalis + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ + +#ifndef RIPPLE_BASICS_SPINLOCK_H_INCLUDED +#define RIPPLE_BASICS_SPINLOCK_H_INCLUDED + +#include +#include +#include +#include + +#ifndef __aarch64__ +#include +#endif + +namespace ripple { + +namespace detail { +/** Inform the processor that we are in a tight spin-wait loop. + + Spinlocks caught in tight loops can result in the processor's pipeline + filling up with comparison operations, resulting in a misprediction at + the time the lock is finally acquired, necessitating pipeline flushing + which is ridiculously expensive and results in very high latency. + + This function instructs the processor to "pause" for some architecture + specific amount of time, to prevent this. + */ +inline void +spin_pause() noexcept +{ +#ifdef __aarch64__ + asm volatile("yield"); +#else + _mm_pause(); +#endif +} + +} // namespace detail + +/** @{ */ +/** Classes to handle arrays of spinlocks packed into a single atomic integer: + + Packed spinlocks allow for tremendously space-efficient lock-sharding + but they come at a cost. + + First, the implementation is necessarily low-level and uses advanced + features like memory ordering and highly platform-specific tricks to + maximize performance. This imposes a significant and ongoing cost to + developers. + + Second, and perhaps most important, is that the packing of multiple + locks into a single integer which, albeit space-efficient, also has + performance implications stemming from data dependencies, increased + cache-coherency traffic between processors and heavier loads on the + processor's load/store units. + + To be sure, these locks can have advantages but they are definitely + not general purpose locks and should not be thought of or used that + way. The use cases for them are likely few and far between; without + a compelling reason to use them, backed by profiling data, it might + be best to use one of the standard locking primitives instead. Note + that in most common platforms, `std::mutex` is so heavily optimized + that it can, usually, outperform spinlocks. + + @tparam T An unsigned integral type (e.g. std::uint16_t) + */ + +/** A class that grabs a single packed spinlock from an atomic integer. + + This class meets the requirements of Lockable: + https://en.cppreference.com/w/cpp/named_req/Lockable + */ +template +class packed_spinlock +{ + // clang-format off + static_assert(std::is_unsigned_v); + static_assert(std::atomic::is_always_lock_free); + static_assert( + std::is_same_v&>().fetch_or(0)), T> && + std::is_same_v&>().fetch_and(0)), T>, + "std::atomic::fetch_and(T) and std::atomic::fetch_and(T) are required by packed_spinlock"); + // clang-format on + +private: + std::atomic& bits_; + T const mask_; + +public: + packed_spinlock(packed_spinlock const&) = delete; + packed_spinlock& + operator=(packed_spinlock const&) = delete; + + /** A single spinlock packed inside the specified atomic + + @param lock The atomic integer inside which the spinlock is packed. + @param index The index of the spinlock this object acquires. + + @note For performance reasons, you should strive to have `lock` be + on a cacheline by itself. + */ + packed_spinlock(std::atomic& lock, int index) + : bits_(lock), mask_(static_cast(1) << index) + { + assert(index >= 0 && (mask_ != 0)); + } + + [[nodiscard]] bool + try_lock() + { + return (bits_.fetch_or(mask_, std::memory_order_acquire) & mask_) == 0; + } + + void + lock() + { + while (!try_lock()) + { + // The use of relaxed memory ordering here is intentional and + // serves to help reduce cache coherency traffic during times + // of contention by avoiding writes that would definitely not + // result in the lock being acquired. + while ((bits_.load(std::memory_order_relaxed) & mask_) != 0) + detail::spin_pause(); + } + } + + void + unlock() + { + bits_.fetch_and(~mask_, std::memory_order_release); + } +}; + +/** A spinlock implemented on top of an atomic integer. + + @note Using `packed_spinlock` and `spinlock` against the same underlying + atomic integer can result in `spinlock` not being able to actually + acquire the lock during periods of high contention, because of how + the two locks operate: `spinlock` will spin trying to grab all the + bits at once, whereas any given `packed_spinlock` will only try to + grab one bit at a time. Caveat emptor. + + This class meets the requirements of Lockable: + https://en.cppreference.com/w/cpp/named_req/Lockable + */ +template +class spinlock +{ + static_assert(std::is_unsigned_v); + static_assert(std::atomic::is_always_lock_free); + +private: + std::atomic& lock_; + +public: + spinlock(spinlock const&) = delete; + spinlock& + operator=(spinlock const&) = delete; + + /** Grabs the + + @param lock The atomic integer to spin against. + + @note For performance reasons, you should strive to have `lock` be + on a cacheline by itself. + */ + spinlock(std::atomic& lock) : lock_(lock) + { + } + + [[nodiscard]] bool + try_lock() + { + T expected = 0; + + return lock_.compare_exchange_weak( + expected, + std::numeric_limits::max(), + std::memory_order_acquire, + std::memory_order_relaxed); + } + + void + lock() + { + while (!try_lock()) + { + // The use of relaxed memory ordering here is intentional and + // serves to help reduce cache coherency traffic during times + // of contention by avoiding writes that would definitely not + // result in the lock being acquired. + while (lock_.load(std::memory_order_relaxed) != 0) + detail::spin_pause(); + } + } + + void + unlock() + { + lock_.store(0, std::memory_order_release); + } +}; +/** @} */ + +} // namespace ripple + +#endif diff --git a/src/ripple/consensus/Consensus.h b/src/ripple/consensus/Consensus.h index a86144c38..1d02e0f13 100644 --- a/src/ripple/consensus/Consensus.h +++ b/src/ripple/consensus/Consensus.h @@ -295,7 +295,7 @@ class Consensus using Result = ConsensusResult; - // Helper class to ensure adaptor is notified whenver the ConsensusMode + // Helper class to ensure adaptor is notified whenever the ConsensusMode // changes class MonitoredMode { diff --git a/src/ripple/nodestore/impl/Database.cpp b/src/ripple/nodestore/impl/Database.cpp index 15aad0a02..70416c873 100644 --- a/src/ripple/nodestore/impl/Database.cpp +++ b/src/ripple/nodestore/impl/Database.cpp @@ -68,11 +68,14 @@ Database::Database( decltype(read_) read; - while (!isStopping()) + while (true) { { std::unique_lock lock(readLock_); + if (isStopping()) + break; + if (read_.empty()) { runningThreads_--; @@ -81,10 +84,10 @@ Database::Database( } if (isStopping()) - continue; + break; - // If configured, extract multiple object at a time to - // minimize the overhead of acquiring the mutex. + // extract multiple object at a time to minimize the + // overhead of acquiring the mutex. for (int cnt = 0; !read_.empty() && cnt != requestBundle_; ++cnt) @@ -120,6 +123,7 @@ Database::Database( read.clear(); } + --runningThreads_; --readThreads_; }, i); @@ -160,15 +164,34 @@ Database::maxLedgers(std::uint32_t shardIndex) const noexcept void Database::stop() { - if (!readStopping_.exchange(true, std::memory_order_relaxed)) { std::lock_guard lock(readLock_); - read_.clear(); - readCondVar_.notify_all(); + + if (!readStopping_.exchange(true, std::memory_order_relaxed)) + { + JLOG(j_.debug()) << "Clearing read queue because of stop request"; + read_.clear(); + readCondVar_.notify_all(); + } } + JLOG(j_.debug()) << "Waiting for stop request to complete..."; + + using namespace std::chrono; + + auto const start = steady_clock::now(); + while (readThreads_.load() != 0) + { + assert(steady_clock::now() - start < 30s); std::this_thread::yield(); + } + + JLOG(j_.debug()) << "Stop request completed in " + << duration_cast( + steady_clock::now() - start) + .count() + << " millseconds"; } void @@ -177,10 +200,13 @@ Database::asyncFetch( std::uint32_t ledgerSeq, std::function const&)>&& cb) { - // Post a read std::lock_guard lock(readLock_); - read_[hash].emplace_back(ledgerSeq, std::move(cb)); - readCondVar_.notify_one(); + + if (!isStopping()) + { + read_[hash].emplace_back(ledgerSeq, std::move(cb)); + readCondVar_.notify_one(); + } } void diff --git a/src/ripple/nodestore/impl/Shard.cpp b/src/ripple/nodestore/impl/Shard.cpp index 911eedef6..030fbf4aa 100644 --- a/src/ripple/nodestore/impl/Shard.cpp +++ b/src/ripple/nodestore/impl/Shard.cpp @@ -223,6 +223,7 @@ Shard::storeNodeObject(std::shared_ptr const& nodeObject) try { + std::lock_guard lock(mutex_); backend_->store(nodeObject); } catch (std::exception const& e) @@ -249,6 +250,7 @@ Shard::fetchNodeObject(uint256 const& hash, FetchReport& fetchReport) Status status; try { + std::lock_guard lock(mutex_); status = backend_->fetch(hash.data(), &nodeObject); } catch (std::exception const& e) @@ -331,6 +333,7 @@ Shard::storeLedger( try { + std::lock_guard lock(mutex_); backend_->storeBatch(batch); } catch (std::exception const& e) @@ -538,6 +541,7 @@ Shard::getWriteLoad() auto const scopedCount{makeBackendCount()}; if (!scopedCount) return 0; + std::lock_guard lock(mutex_); return backend_->getWriteLoad(); } @@ -572,6 +576,8 @@ Shard::finalize(bool writeSQLite, std::optional const& referenceHash) try { + std::lock_guard lock(mutex_); + state_ = ShardState::finalizing; progress_ = 0; @@ -759,8 +765,11 @@ Shard::finalize(bool writeSQLite, std::optional const& referenceHash) try { - // Store final key's value, may already be stored - backend_->store(nodeObject); + { + // Store final key's value, may already be stored + std::lock_guard lock(mutex_); + backend_->store(nodeObject); + } // Do not allow all other threads work with the shard busy_ = true; @@ -819,7 +828,7 @@ Shard::open(std::lock_guard const& lock) using namespace boost::filesystem; Config const& config{app_.config()}; auto preexist{false}; - auto fail = [this, &preexist](std::string const& msg) { + auto fail = [this, &preexist](std::string const& msg) REQUIRES(mutex_) { backend_->close(); lgrSQLiteDB_.reset(); txSQLiteDB_.reset(); @@ -837,7 +846,7 @@ Shard::open(std::lock_guard const& lock) } return false; }; - auto createAcquireInfo = [this, &config]() { + auto createAcquireInfo = [this, &config]() REQUIRES(mutex_) { DatabaseCon::Setup setup; setup.startUp = config.standalone() ? config.LOAD : config.START_UP; setup.standAlone = config.standalone(); @@ -1024,6 +1033,8 @@ Shard::storeSQLite(std::shared_ptr const& ledger) try { + std::lock_guard lock(mutex_); + auto res = updateLedgerDBs( *txSQLiteDB_->checkoutDb(), *lgrSQLiteDB_->checkoutDb(), @@ -1186,6 +1197,7 @@ Shard::verifyFetch(uint256 const& hash) const try { + std::lock_guard lock(mutex_); switch (backend_->fetch(hash.data(), &nodeObject)) { case ok: diff --git a/src/ripple/nodestore/impl/Shard.h b/src/ripple/nodestore/impl/Shard.h index b7516e5f1..210bdd54a 100644 --- a/src/ripple/nodestore/impl/Shard.h +++ b/src/ripple/nodestore/impl/Shard.h @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -210,6 +211,7 @@ public: std::string getStoredSeqs() { + std::lock_guard lock(mutex_); if (!acquireInfo_) return ""; @@ -316,29 +318,30 @@ private: boost::filesystem::path const dir_; // Storage space utilized by the shard - std::uint64_t fileSz_{0}; + GUARDED_BY(mutex_) std::uint64_t fileSz_{0}; // Number of file descriptors required by the shard - std::uint32_t fdRequired_{0}; + GUARDED_BY(mutex_) std::uint32_t fdRequired_{0}; // NuDB key/value store for node objects - std::unique_ptr backend_; + std::unique_ptr backend_ GUARDED_BY(mutex_); std::atomic backendCount_{0}; // Ledger SQLite database used for indexes - std::unique_ptr lgrSQLiteDB_; + std::unique_ptr lgrSQLiteDB_ GUARDED_BY(mutex_); // Transaction SQLite database used for indexes - std::unique_ptr txSQLiteDB_; + std::unique_ptr txSQLiteDB_ GUARDED_BY(mutex_); // Tracking information used only when acquiring a shard from the network. // If the shard is finalized, this member will be null. - std::unique_ptr acquireInfo_; + std::unique_ptr acquireInfo_ GUARDED_BY(mutex_); + ; // Older shard without an acquire database or final key // Eventually there will be no need for this and should be removed - bool legacy_{false}; + GUARDED_BY(mutex_) bool legacy_{false}; // Determines if the shard needs to stop processing for shutdown std::atomic stop_{false}; @@ -356,16 +359,17 @@ private: std::atomic removeOnDestroy_{false}; // The time of the last access of a shard with a finalized state - std::chrono::steady_clock::time_point lastAccess_; + std::chrono::steady_clock::time_point lastAccess_ GUARDED_BY(mutex_); + ; // Open shard databases [[nodiscard]] bool - open(std::lock_guard const& lock); + open(std::lock_guard const& lock) REQUIRES(mutex_); // Open/Create SQLite databases // Lock over mutex_ required [[nodiscard]] bool - initSQLite(std::lock_guard const&); + initSQLite(std::lock_guard const&) REQUIRES(mutex_); // Write SQLite entries for this ledger [[nodiscard]] bool @@ -374,7 +378,7 @@ private: // Set storage and file descriptor usage stats // Lock over mutex_ required void - setFileStats(std::lock_guard const&); + setFileStats(std::lock_guard const&) REQUIRES(mutex_); // Verify this ledger by walking its SHAMaps and verifying its Merkle trees // Every node object verified will be stored in the deterministic shard diff --git a/src/ripple/overlay/impl/OverlayImpl.h b/src/ripple/overlay/impl/OverlayImpl.h index 5f23b9150..2ba7999cb 100644 --- a/src/ripple/overlay/impl/OverlayImpl.h +++ b/src/ripple/overlay/impl/OverlayImpl.h @@ -125,8 +125,6 @@ private: // Peer IDs expecting to receive a last link notification std::set csIDs_; - std::optional networkID_; - reduce_relay::Slots slots_; // Transaction reduce-relay metrics @@ -391,7 +389,7 @@ public: std::optional networkID() const override { - return networkID_; + return setup_.networkID; } Json::Value diff --git a/src/ripple/peerfinder/impl/Logic.h b/src/ripple/peerfinder/impl/Logic.h index 4e69c8bd3..ca14a5111 100644 --- a/src/ripple/peerfinder/impl/Logic.h +++ b/src/ripple/peerfinder/impl/Logic.h @@ -782,10 +782,14 @@ public: // Must be handshaked! assert(slot->state() == Slot::active); - preprocess(slot, list); - clock_type::time_point const now(m_clock.now()); + // Limit how often we accept new endpoints + if (slot->whenAcceptEndpoints > now) + return; + + preprocess(slot, list); + for (auto const& ep : list) { assert(ep.hops != 0); diff --git a/src/ripple/protocol/Feature.h b/src/ripple/protocol/Feature.h index e930c0f0c..a66fd75b4 100644 --- a/src/ripple/protocol/Feature.h +++ b/src/ripple/protocol/Feature.h @@ -338,6 +338,8 @@ extern uint256 const featureExpandedSignerList; extern uint256 const featureNonFungibleTokensV1; extern uint256 const fixNFTokenDirV1; extern uint256 const featurePaychanAndEscrowForTokens; +extern uint256 const fixNFTokenNegOffer; +extern uint256 const featureNonFungibleTokensV1_1; } // namespace ripple diff --git a/src/ripple/protocol/impl/BuildInfo.cpp b/src/ripple/protocol/impl/BuildInfo.cpp index 1d4c4bdda..6c67c4dd7 100644 --- a/src/ripple/protocol/impl/BuildInfo.cpp +++ b/src/ripple/protocol/impl/BuildInfo.cpp @@ -33,7 +33,7 @@ namespace BuildInfo { // and follow the format described at http://semver.org/ //------------------------------------------------------------------------------ // clang-format off -char const* const versionString = "1.9.1" +char const* const versionString = "1.9.2" // clang-format on "-hooks" diff --git a/src/ripple/protocol/impl/Feature.cpp b/src/ripple/protocol/impl/Feature.cpp index fc59512c1..67b398a81 100644 --- a/src/ripple/protocol/impl/Feature.cpp +++ b/src/ripple/protocol/impl/Feature.cpp @@ -431,7 +431,7 @@ REGISTER_FEATURE(RequireFullyCanonicalSig, Supported::yes, DefaultVote::yes REGISTER_FIX (fix1781, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(HardenedValidations, Supported::yes, DefaultVote::yes); REGISTER_FIX (fixAmendmentMajorityCalc, Supported::yes, DefaultVote::yes); -REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::no); +REGISTER_FEATURE(NegativeUNL, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(TicketBatch, Supported::yes, DefaultVote::yes); REGISTER_FEATURE(FlowSortStrands, Supported::yes, DefaultVote::yes); REGISTER_FIX (fixSTAmountCanonicalize, Supported::yes, DefaultVote::yes); @@ -442,6 +442,8 @@ REGISTER_FEATURE(NonFungibleTokensV1, Supported::yes, DefaultVote::no) REGISTER_FEATURE(ExpandedSignerList, Supported::yes, DefaultVote::no); REGISTER_FIX (fixNFTokenDirV1, Supported::yes, DefaultVote::no); REGISTER_FEATURE(PaychanAndEscrowForTokens, Supported::yes, DefaultVote::no); +REGISTER_FIX (fixNFTokenNegOffer, Supported::yes, DefaultVote::no); +REGISTER_FEATURE(NonFungibleTokensV1_1, Supported::yes, DefaultVote::no); // The following amendments have been active for at least two years. Their // pre-amendment code has been removed and the identifiers are deprecated. diff --git a/src/ripple/protocol/impl/Rules.cpp b/src/ripple/protocol/impl/Rules.cpp index 3736764fc..35a09b856 100644 --- a/src/ripple/protocol/impl/Rules.cpp +++ b/src/ripple/protocol/impl/Rules.cpp @@ -17,10 +17,9 @@ */ //============================================================================== +#include #include -#include - namespace ripple { class Rules::Impl @@ -40,9 +39,8 @@ public: std::unordered_set> const& presets, std::optional const& digest, STVector256 const& amendments) - : presets_(presets) + : digest_(digest), presets_(presets) { - digest_ = digest; set_.reserve(amendments.size()); set_.insert(amendments.begin(), amendments.end()); } @@ -83,6 +81,18 @@ bool Rules::enabled(uint256 const& feature) const { assert(impl_); + + // The functionality of the "NonFungibleTokensV1_1" amendment is + // precisely the functionality of the following three amendments + // so if their status is ever queried individually, we inject an + // extra check here to simplify the checking elsewhere. + if (feature == featureNonFungibleTokensV1 || + feature == fixNFTokenNegOffer || feature == fixNFTokenDirV1) + { + if (impl_->enabled(featureNonFungibleTokensV1_1)) + return true; + } + return impl_->enabled(feature); } diff --git a/src/ripple/protocol/impl/STVector256.cpp b/src/ripple/protocol/impl/STVector256.cpp index f74670ac0..0ef1295b1 100644 --- a/src/ripple/protocol/impl/STVector256.cpp +++ b/src/ripple/protocol/impl/STVector256.cpp @@ -26,19 +26,19 @@ namespace ripple { STVector256::STVector256(SerialIter& sit, SField const& name) : STBase(name) { - Blob data = sit.getVL(); - auto const count = data.size() / (256 / 8); - mValue.reserve(count); - Blob::iterator begin = data.begin(); - unsigned int uStart = 0; - for (unsigned int i = 0; i != count; i++) - { - unsigned int uEnd = uStart + (256 / 8); - // This next line could be optimized to construct a default - // uint256 in the vector and then copy into it - mValue.push_back(uint256(Blob(begin + uStart, begin + uEnd))); - uStart = uEnd; - } + auto const slice = sit.getSlice(sit.getVLDataLength()); + + if (slice.size() % uint256::size() != 0) + Throw( + "Bad serialization for STVector256: " + + std::to_string(slice.size())); + + auto const cnt = slice.size() / uint256::size(); + + mValue.reserve(cnt); + + for (std::size_t i = 0; i != cnt; ++i) + mValue.emplace_back(slice.substr(i * uint256::size(), uint256::size())); } STBase* diff --git a/src/ripple/protocol/jss.h b/src/ripple/protocol/jss.h index ab1059d70..57ab0087e 100644 --- a/src/ripple/protocol/jss.h +++ b/src/ripple/protocol/jss.h @@ -201,8 +201,8 @@ JSS(converge_time_s); // out: NetworkOPs JSS(cookie); // out: NetworkOPs JSS(count); // in: AccountTx*, ValidatorList JSS(counters); // in/out: retrieve counters -JSS(cur_a); // out: BookChanges -JSS(cur_b); // out: BookChanges +JSS(currency_a); // out: BookChanges +JSS(currency_b); // out: BookChanges JSS(currentShard); // out: NodeToShardStatus JSS(currentShardIndex); // out: NodeToShardStatus JSS(currency); // in: paths/PathRequest, STAmount @@ -655,8 +655,8 @@ JSS(validator_sites); // out: ValidatorSites JSS(value); // out: STAmount JSS(version); // out: RPCVersion JSS(vetoed); // out: AmendmentTableImpl -JSS(vol_a); // out: BookChanges -JSS(vol_b); // out: BookChanges +JSS(volume_a); // out: BookChanges +JSS(volume_b); // out: BookChanges JSS(vote); // in: Feature JSS(warning); // rpc: JSS(warnings); // out: server_info, server_state diff --git a/src/ripple/rpc/BookChanges.h b/src/ripple/rpc/BookChanges.h index c754cbde7..5aef1c693 100644 --- a/src/ripple/rpc/BookChanges.h +++ b/src/ripple/rpc/BookChanges.h @@ -18,7 +18,7 @@ //============================================================================== #ifndef RIPPLE_RPC_BOOKCHANGES_H_INCLUDED -#define RIPPLE_RPC_BOOKCAHNGES_H_INCLUDED +#define RIPPLE_RPC_BOOKCHANGES_H_INCLUDED namespace Json { class Value; @@ -188,14 +188,14 @@ computeBookChanges(std::shared_ptr const& lpAccepted) STAmount volA = std::get<0>(entry.second); STAmount volB = std::get<1>(entry.second); - inner[jss::cur_a] = + inner[jss::currency_a] = (isXRP(volA) ? "XRP_drops" : to_string(volA.issue())); - inner[jss::cur_b] = + inner[jss::currency_b] = (isXRP(volB) ? "XRP_drops" : to_string(volB.issue())); - inner[jss::vol_a] = + inner[jss::volume_a] = (isXRP(volA) ? to_string(volA.xrp()) : to_string(volA.iou())); - inner[jss::vol_b] = + inner[jss::volume_b] = (isXRP(volB) ? to_string(volB.xrp()) : to_string(volB.iou())); inner[jss::high] = to_string(std::get<2>(entry.second).iou()); diff --git a/src/ripple/rpc/impl/ServerHandlerImp.cpp b/src/ripple/rpc/impl/ServerHandlerImp.cpp index 3ac5f04a9..cb70fdcab 100644 --- a/src/ripple/rpc/impl/ServerHandlerImp.cpp +++ b/src/ripple/rpc/impl/ServerHandlerImp.cpp @@ -873,9 +873,23 @@ ServerHandlerImp::processRequest( params, {user, forwardedFor}}; Json::Value result; + auto start = std::chrono::system_clock::now(); - RPC::doCommand(context, result); + + try + { + RPC::doCommand(context, result); + } + catch (std::exception const& ex) + { + result = RPC::make_error(rpcINTERNAL); + JLOG(m_journal.error()) << "Internal error : " << ex.what() + << " when processing request: " + << Json::Compact{Json::Value{params}}; + } + auto end = std::chrono::system_clock::now(); + logDuration(params, end - start, m_journal); usage.charge(loadType); diff --git a/src/ripple/shamap/impl/SHAMapInnerNode.cpp b/src/ripple/shamap/impl/SHAMapInnerNode.cpp index eb00f8587..d408fe195 100644 --- a/src/ripple/shamap/impl/SHAMapInnerNode.cpp +++ b/src/ripple/shamap/impl/SHAMapInnerNode.cpp @@ -22,102 +22,19 @@ #include #include #include +#include #include #include #include #include #include -#include - #include #include #include -#ifndef __aarch64__ -// This is used for the _mm_pause instruction: -#include -#endif - namespace ripple { -/** A specialized 16-way spinlock used to protect inner node branches. - - This class packs 16 separate spinlocks into a single 16-bit value. It makes - it possible to lock any one lock at once or, alternatively, all together. - - The implementation tries to use portable constructs but has to be low-level - for performance. - */ -class SpinBitlock -{ -private: - std::atomic& bits_; - std::uint16_t mask_; - -public: - SpinBitlock(std::atomic& lock) : bits_(lock), mask_(0xFFFF) - { - } - - SpinBitlock(std::atomic& lock, int index) - : bits_(lock), mask_(1 << index) - { - assert(index >= 0 && index < 16); - } - - [[nodiscard]] bool - try_lock() - { - // If we want to grab all the individual bitlocks at once we cannot - // use `fetch_or`! To see why, imagine that `lock_ == 0x0020` which - // means that the `fetch_or` would return `0x0020` but all the bits - // would already be (incorrectly!) set. Oops! - std::uint16_t expected = 0; - - if (mask_ != 0xFFFF) - return (bits_.fetch_or(mask_, std::memory_order_acquire) & mask_) == - expected; - - return bits_.compare_exchange_weak( - expected, - mask_, - std::memory_order_acquire, - std::memory_order_relaxed); - } - - void - lock() - { - // Testing suggests that 99.9999% of the time this will succeed, so - // we try to optimize the fast path. - if (try_lock()) - return; - - do - { - // We try to spin for a few times: - for (int i = 0; i != 100; ++i) - { - if (try_lock()) - return; - -#ifndef __aarch64__ - _mm_pause(); -#endif - } - - std::this_thread::yield(); - } while ((bits_.load(std::memory_order_relaxed) & mask_) == 0); - } - - void - unlock() - { - bits_.fetch_and(~mask_, std::memory_order_release); - } -}; - SHAMapInnerNode::SHAMapInnerNode( std::uint32_t cowid, std::uint8_t numAllocatedChildren) @@ -185,7 +102,7 @@ SHAMapInnerNode::clone(std::uint32_t cowid) const }); } - SpinBitlock sl(lock_); + spinlock sl(lock_); std::lock_guard lock(sl); if (thisIsSparse) @@ -422,7 +339,7 @@ SHAMapInnerNode::getChildPointer(int branch) auto const index = *getChildIndex(branch); - SpinBitlock sl(lock_, index); + packed_spinlock sl(lock_, index); std::lock_guard lock(sl); return hashesAndChildren_.getChildren()[index].get(); } @@ -435,7 +352,7 @@ SHAMapInnerNode::getChild(int branch) auto const index = *getChildIndex(branch); - SpinBitlock sl(lock_, index); + packed_spinlock sl(lock_, index); std::lock_guard lock(sl); return hashesAndChildren_.getChildren()[index]; } @@ -462,7 +379,7 @@ SHAMapInnerNode::canonicalizeChild( auto [_, hashes, children] = hashesAndChildren_.getHashesAndChildren(); assert(node->getHash() == hashes[childIndex]); - SpinBitlock sl(lock_, childIndex); + packed_spinlock sl(lock_, childIndex); std::lock_guard lock(sl); if (children[childIndex]) diff --git a/src/test/app/LedgerLoad_test.cpp b/src/test/app/LedgerLoad_test.cpp index 7175df345..d78d25ea0 100644 --- a/src/test/app/LedgerLoad_test.cpp +++ b/src/test/app/LedgerLoad_test.cpp @@ -21,11 +21,12 @@ #include #include #include +#include +#include + #include #include #include -#include -#include namespace ripple { @@ -111,7 +112,9 @@ class LedgerLoad_test : public beast::unit_test::suite Env env( *this, envconfig( - ledgerConfig, sd.dbPath, sd.ledgerFile, Config::LOAD_FILE)); + ledgerConfig, sd.dbPath, sd.ledgerFile, Config::LOAD_FILE), + nullptr, + beast::severities::kDisabled); auto jrb = env.rpc("ledger", "current", "full")[jss::result]; BEAST_EXPECT( sd.ledger[jss::ledger][jss::accountState].size() == @@ -129,7 +132,9 @@ class LedgerLoad_test : public beast::unit_test::suite except([&] { Env env( *this, - envconfig(ledgerConfig, sd.dbPath, "", Config::LOAD_FILE)); + envconfig(ledgerConfig, sd.dbPath, "", Config::LOAD_FILE), + nullptr, + beast::severities::kDisabled); }); // file does not exist @@ -137,10 +142,9 @@ class LedgerLoad_test : public beast::unit_test::suite Env env( *this, envconfig( - ledgerConfig, - sd.dbPath, - "badfile.json", - Config::LOAD_FILE)); + ledgerConfig, sd.dbPath, "badfile.json", Config::LOAD_FILE), + nullptr, + beast::severities::kDisabled); }); // make a corrupted version of the ledger file (last 10 bytes removed). @@ -168,7 +172,9 @@ class LedgerLoad_test : public beast::unit_test::suite ledgerConfig, sd.dbPath, ledgerFileCorrupt.string(), - Config::LOAD_FILE)); + Config::LOAD_FILE), + nullptr, + beast::severities::kDisabled); }); } @@ -183,7 +189,9 @@ class LedgerLoad_test : public beast::unit_test::suite boost::erase_all(ledgerHash, "\""); Env env( *this, - envconfig(ledgerConfig, sd.dbPath, ledgerHash, Config::LOAD)); + envconfig(ledgerConfig, sd.dbPath, ledgerHash, Config::LOAD), + nullptr, + beast::severities::kDisabled); auto jrb = env.rpc("ledger", "current", "full")[jss::result]; BEAST_EXPECT(jrb[jss::ledger][jss::accountState].size() == 97); BEAST_EXPECT( @@ -199,7 +207,10 @@ class LedgerLoad_test : public beast::unit_test::suite // create a new env with the ledger "latest" specified for startup Env env( - *this, envconfig(ledgerConfig, sd.dbPath, "latest", Config::LOAD)); + *this, + envconfig(ledgerConfig, sd.dbPath, "latest", Config::LOAD), + nullptr, + beast::severities::kDisabled); auto jrb = env.rpc("ledger", "current", "full")[jss::result]; BEAST_EXPECT( sd.ledger[jss::ledger][jss::accountState].size() == @@ -213,7 +224,11 @@ class LedgerLoad_test : public beast::unit_test::suite using namespace test::jtx; // create a new env with specific ledger index at startup - Env env(*this, envconfig(ledgerConfig, sd.dbPath, "43", Config::LOAD)); + Env env( + *this, + envconfig(ledgerConfig, sd.dbPath, "43", Config::LOAD), + nullptr, + beast::severities::kDisabled); auto jrb = env.rpc("ledger", "current", "full")[jss::result]; BEAST_EXPECT( sd.ledger[jss::ledger][jss::accountState].size() == diff --git a/src/test/app/LedgerReplay_test.cpp b/src/test/app/LedgerReplay_test.cpp index 549495d40..cff94ee04 100644 --- a/src/test/app/LedgerReplay_test.cpp +++ b/src/test/app/LedgerReplay_test.cpp @@ -465,7 +465,7 @@ struct LedgerServer assert(param.initLedgers > 0); createAccounts(param.initAccounts); createLedgerHistory(); - app.logs().threshold(beast::severities::Severity::kWarning); + app.logs().threshold(beast::severities::kWarning); } /** @@ -567,7 +567,10 @@ public: PeerSetBehavior behavior = PeerSetBehavior::Good, InboundLedgersBehavior inboundBhvr = InboundLedgersBehavior::Good, PeerFeature peerFeature = PeerFeature::LedgerReplayEnabled) - : env(suite, jtx::envconfig(jtx::port_increment, 3)) + : env(suite, + jtx::envconfig(jtx::port_increment, 3), + nullptr, + beast::severities::kDisabled) , app(env.app()) , ledgerMaster(env.app().getLedgerMaster()) , inboundLedgers( diff --git a/src/test/app/NFTokenDir_test.cpp b/src/test/app/NFTokenDir_test.cpp index ae7eeeaf6..d50bd1584 100644 --- a/src/test/app/NFTokenDir_test.cpp +++ b/src/test/app/NFTokenDir_test.cpp @@ -388,7 +388,12 @@ class NFTokenDir_test : public beast::unit_test::suite auto exerciseFixNFTokenDirV1 = [this, &features](std::initializer_list seeds) { - Env env{*this, features}; + Env env{ + *this, + envconfig(), + features, + nullptr, + beast::severities::kDisabled}; // Eventually all of the NFTokens will be owned by buyer. Account const buyer{"buyer"}; @@ -1070,7 +1075,8 @@ public: { using namespace test::jtx; FeatureBitset const all{supported_amendments()}; - FeatureBitset const fixNFTDir{fixNFTokenDirV1}; + FeatureBitset const fixNFTDir{ + fixNFTokenDirV1, featureNonFungibleTokensV1_1}; testWithFeats(all - fixNFTDir); testWithFeats(all); diff --git a/src/test/app/NFToken_test.cpp b/src/test/app/NFToken_test.cpp index de7a67148..04f4d920c 100644 --- a/src/test/app/NFToken_test.cpp +++ b/src/test/app/NFToken_test.cpp @@ -96,7 +96,10 @@ class NFToken_test : public beast::unit_test::suite { // If the NFT amendment is not enabled, you should not be able // to create or burn NFTs. - Env env{*this, features - featureNonFungibleTokensV1}; + Env env{ + *this, + features - featureNonFungibleTokensV1 - + featureNonFungibleTokensV1_1}; Account const& master = env.master; BEAST_EXPECT(ownerCount(env, master) == 0); @@ -2666,7 +2669,7 @@ class NFToken_test : public beast::unit_test::suite env.close(); // Test how adding a Destination field to an offer affects permissions - // for cancelling offers. + // for canceling offers. { uint256 const offerMinterToIssuer = keylet::nftoffer(minter, env.seq(minter)).key; @@ -2680,14 +2683,20 @@ class NFToken_test : public beast::unit_test::suite token::destination(buyer), txflags(tfSellNFToken)); - // buy offers cannot contain a Destination, so this attempt fails. + uint256 const offerIssuerToMinter = + keylet::nftoffer(issuer, env.seq(issuer)).key; env(token::createOffer(issuer, nftokenID, drops(1)), token::owner(minter), - token::destination(minter), - ter(temMALFORMED)); + token::destination(minter)); + + uint256 const offerIssuerToBuyer = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftokenID, drops(1)), + token::owner(minter), + token::destination(buyer)); env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == 2); BEAST_EXPECT(ownerCount(env, minter) == 3); BEAST_EXPECT(ownerCount(env, buyer) == 0); @@ -2702,8 +2711,12 @@ class NFToken_test : public beast::unit_test::suite ter(tecNO_PERMISSION)); env(token::cancelOffer(buyer, {offerMinterToIssuer}), ter(tecNO_PERMISSION)); + env(token::cancelOffer(buyer, {offerIssuerToMinter}), + ter(tecNO_PERMISSION)); + env(token::cancelOffer(minter, {offerIssuerToBuyer}), + ter(tecNO_PERMISSION)); env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, issuer) == 2); BEAST_EXPECT(ownerCount(env, minter) == 3); BEAST_EXPECT(ownerCount(env, buyer) == 0); @@ -2711,6 +2724,8 @@ class NFToken_test : public beast::unit_test::suite // cancel the offers. env(token::cancelOffer(buyer, {offerMinterToBuyer})); env(token::cancelOffer(minter, {offerMinterToIssuer})); + env(token::cancelOffer(buyer, {offerIssuerToBuyer})); + env(token::cancelOffer(issuer, {offerIssuerToMinter})); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); @@ -2720,7 +2735,7 @@ class NFToken_test : public beast::unit_test::suite // Test how adding a Destination field to a sell offer affects // accepting that offer. { - uint256 const offerMinterToBuyer = + uint256 const offerMinterSellsToBuyer = keylet::nftoffer(minter, env.seq(minter)).key; env(token::createOffer(minter, nftokenID, drops(1)), token::destination(buyer), @@ -2732,7 +2747,7 @@ class NFToken_test : public beast::unit_test::suite // issuer cannot accept a sell offer where they are not the // destination. - env(token::acceptSellOffer(issuer, offerMinterToBuyer), + env(token::acceptSellOffer(issuer, offerMinterSellsToBuyer), ter(tecNO_PERMISSION)); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); @@ -2740,36 +2755,61 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, buyer) == 0); // However buyer can accept the sell offer. - env(token::acceptSellOffer(buyer, offerMinterToBuyer)); + env(token::acceptSellOffer(buyer, offerMinterSellsToBuyer)); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 0); BEAST_EXPECT(ownerCount(env, buyer) == 1); } - // You can't add a Destination field to a buy offer. + // Test how adding a Destination field to a buy offer affects + // accepting that offer. { - env(token::createOffer(minter, nftokenID, drops(1)), - token::owner(buyer), - token::destination(buyer), - ter(temMALFORMED)); - env.close(); - BEAST_EXPECT(ownerCount(env, issuer) == 0); - BEAST_EXPECT(ownerCount(env, minter) == 0); - BEAST_EXPECT(ownerCount(env, buyer) == 1); - - // However without the Destination the buy offer works fine. - uint256 const offerMinterToBuyer = + uint256 const offerMinterBuysFromBuyer = keylet::nftoffer(minter, env.seq(minter)).key; env(token::createOffer(minter, nftokenID, drops(1)), - token::owner(buyer)); + token::owner(buyer), + token::destination(buyer)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + + // issuer cannot accept a buy offer where they are the + // destination. + env(token::acceptBuyOffer(issuer, offerMinterBuysFromBuyer), + ter(tecNO_PERMISSION)); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); BEAST_EXPECT(ownerCount(env, buyer) == 1); // Buyer accepts minter's offer. - env(token::acceptBuyOffer(buyer, offerMinterToBuyer)); + env(token::acceptBuyOffer(buyer, offerMinterBuysFromBuyer)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 0); + + // If a destination other than the NFToken owner is set, that + // destination must act as a broker. The NFToken owner may not + // simply accept the offer. + uint256 const offerBuyerBuysFromMinter = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftokenID, drops(1)), + token::owner(minter), + token::destination(broker)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + + env(token::acceptBuyOffer(minter, offerBuyerBuysFromMinter), + ter(tecNO_PERMISSION)); + env.close(); + + // Clean up the unused offer. + env(token::cancelOffer(buyer, {offerBuyerBuysFromMinter})); env.close(); BEAST_EXPECT(ownerCount(env, issuer) == 0); BEAST_EXPECT(ownerCount(env, minter) == 1); @@ -2856,6 +2896,47 @@ class NFToken_test : public beast::unit_test::suite BEAST_EXPECT(ownerCount(env, issuer) == 1); BEAST_EXPECT(ownerCount(env, minter) == 1); BEAST_EXPECT(ownerCount(env, buyer) == 0); + + // Clean out the unconsumed offer. + env(token::cancelOffer(issuer, {offerIssuerToBuyer})); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 1); + BEAST_EXPECT(ownerCount(env, buyer) == 0); + } + + // Show that if a buy and a sell offer both have the same destination, + // then that destination can broker the offers. + { + uint256 const offerMinterToBroker = + keylet::nftoffer(minter, env.seq(minter)).key; + env(token::createOffer(minter, nftokenID, drops(1)), + token::destination(broker), + txflags(tfSellNFToken)); + + uint256 const offerBuyerToBroker = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftokenID, drops(1)), + token::owner(minter), + token::destination(broker)); + + // Cannot broker offers when the sell destination is not the buyer + // or the broker. + env(token::brokerOffers( + issuer, offerBuyerToBroker, offerMinterToBroker), + ter(tecNFTOKEN_BUY_SELL_MISMATCH)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 2); + BEAST_EXPECT(ownerCount(env, buyer) == 1); + + // Broker is successful if they are the destination of both offers. + env(token::brokerOffers( + broker, offerBuyerToBroker, offerMinterToBroker)); + env.close(); + BEAST_EXPECT(ownerCount(env, issuer) == 0); + BEAST_EXPECT(ownerCount(env, minter) == 0); + BEAST_EXPECT(ownerCount(env, buyer) == 1); } } @@ -4557,6 +4638,243 @@ class NFToken_test : public beast::unit_test::suite checkOffers("nft_buy_offers", 501, 2, __LINE__); } + void + testFixNFTokenNegOffer(FeatureBitset features) + { + // Exercise changes introduced by fixNFTokenNegOffer. + using namespace test::jtx; + + testcase("fixNFTokenNegOffer"); + + Account const issuer{"issuer"}; + Account const buyer{"buyer"}; + Account const gw{"gw"}; + IOU const gwXAU(gw["XAU"]); + + // Test both with and without fixNFTokenNegOffer + for (auto const& tweakedFeatures : + {features - fixNFTokenNegOffer - featureNonFungibleTokensV1_1, + features | fixNFTokenNegOffer}) + { + // There was a bug in the initial NFT implementation that + // allowed offers to be placed with negative amounts. Verify + // that fixNFTokenNegOffer addresses the problem. + Env env{*this, tweakedFeatures}; + + env.fund(XRP(1000000), issuer, buyer, gw); + env.close(); + + env(trust(issuer, gwXAU(2000))); + env(trust(buyer, gwXAU(2000))); + env.close(); + + env(pay(gw, issuer, gwXAU(1000))); + env(pay(gw, buyer, gwXAU(1000))); + env.close(); + + // Create an NFT that we'll make XRP offers for. + uint256 const nftID0{ + token::getNextID(env, issuer, 0u, tfTransferable)}; + env(token::mint(issuer, 0), txflags(tfTransferable)); + env.close(); + + // Create an NFT that we'll make IOU offers for. + uint256 const nftID1{ + token::getNextID(env, issuer, 1u, tfTransferable)}; + env(token::mint(issuer, 1), txflags(tfTransferable)); + env.close(); + + TER const offerCreateTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(temBAD_AMOUNT) + : static_cast(tesSUCCESS); + + // Make offers with negative amounts for the NFTs + uint256 const sellNegXrpOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID0, XRP(-2)), + txflags(tfSellNFToken), + ter(offerCreateTER)); + env.close(); + + uint256 const sellNegIouOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID1, gwXAU(-2)), + txflags(tfSellNFToken), + ter(offerCreateTER)); + env.close(); + + uint256 const buyNegXrpOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID0, XRP(-1)), + token::owner(issuer), + ter(offerCreateTER)); + env.close(); + + uint256 const buyNegIouOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID1, gwXAU(-1)), + token::owner(issuer), + ter(offerCreateTER)); + env.close(); + + { + // Now try to accept the offers. + // 1. If fixNFTokenNegOffer is NOT enabled get tecINTERNAL. + // 2. If fixNFTokenNegOffer IS enabled get tecOBJECT_NOT_FOUND. + TER const offerAcceptTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(tecOBJECT_NOT_FOUND) + : static_cast(tecINTERNAL); + + // Sell offers. + env(token::acceptSellOffer(buyer, sellNegXrpOfferIndex), + ter(offerAcceptTER)); + env.close(); + env(token::acceptSellOffer(buyer, sellNegIouOfferIndex), + ter(offerAcceptTER)); + env.close(); + + // Buy offers. + env(token::acceptBuyOffer(issuer, buyNegXrpOfferIndex), + ter(offerAcceptTER)); + env.close(); + env(token::acceptBuyOffer(issuer, buyNegIouOfferIndex), + ter(offerAcceptTER)); + env.close(); + } + { + // 1. If fixNFTokenNegOffer is NOT enabled get tecSUCCESS. + // 2. If fixNFTokenNegOffer IS enabled get tecOBJECT_NOT_FOUND. + TER const offerAcceptTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(tecOBJECT_NOT_FOUND) + : static_cast(tesSUCCESS); + + // Brokered offers. + env(token::brokerOffers( + gw, buyNegXrpOfferIndex, sellNegXrpOfferIndex), + ter(offerAcceptTER)); + env.close(); + env(token::brokerOffers( + gw, buyNegIouOfferIndex, sellNegIouOfferIndex), + ter(offerAcceptTER)); + env.close(); + } + } + + // Test what happens if NFTokenOffers are created with negative amounts + // and then fixNFTokenNegOffer goes live. What does an acceptOffer do? + { + Env env{ + *this, + features - fixNFTokenNegOffer - featureNonFungibleTokensV1_1}; + + env.fund(XRP(1000000), issuer, buyer, gw); + env.close(); + + env(trust(issuer, gwXAU(2000))); + env(trust(buyer, gwXAU(2000))); + env.close(); + + env(pay(gw, issuer, gwXAU(1000))); + env(pay(gw, buyer, gwXAU(1000))); + env.close(); + + // Create an NFT that we'll make XRP offers for. + uint256 const nftID0{ + token::getNextID(env, issuer, 0u, tfTransferable)}; + env(token::mint(issuer, 0), txflags(tfTransferable)); + env.close(); + + // Create an NFT that we'll make IOU offers for. + uint256 const nftID1{ + token::getNextID(env, issuer, 1u, tfTransferable)}; + env(token::mint(issuer, 1), txflags(tfTransferable)); + env.close(); + + // Make offers with negative amounts for the NFTs + uint256 const sellNegXrpOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID0, XRP(-2)), + txflags(tfSellNFToken)); + env.close(); + + uint256 const sellNegIouOfferIndex = + keylet::nftoffer(issuer, env.seq(issuer)).key; + env(token::createOffer(issuer, nftID1, gwXAU(-2)), + txflags(tfSellNFToken)); + env.close(); + + uint256 const buyNegXrpOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID0, XRP(-1)), + token::owner(issuer)); + env.close(); + + uint256 const buyNegIouOfferIndex = + keylet::nftoffer(buyer, env.seq(buyer)).key; + env(token::createOffer(buyer, nftID1, gwXAU(-1)), + token::owner(issuer)); + env.close(); + + // Now the amendment passes. + env.enableFeature(fixNFTokenNegOffer); + env.close(); + + // All attempts to accept the offers with negative amounts + // should fail with temBAD_OFFER. + env(token::acceptSellOffer(buyer, sellNegXrpOfferIndex), + ter(temBAD_OFFER)); + env.close(); + env(token::acceptSellOffer(buyer, sellNegIouOfferIndex), + ter(temBAD_OFFER)); + env.close(); + + // Buy offers. + env(token::acceptBuyOffer(issuer, buyNegXrpOfferIndex), + ter(temBAD_OFFER)); + env.close(); + env(token::acceptBuyOffer(issuer, buyNegIouOfferIndex), + ter(temBAD_OFFER)); + env.close(); + + // Brokered offers. + env(token::brokerOffers( + gw, buyNegXrpOfferIndex, sellNegXrpOfferIndex), + ter(temBAD_OFFER)); + env.close(); + env(token::brokerOffers( + gw, buyNegIouOfferIndex, sellNegIouOfferIndex), + ter(temBAD_OFFER)); + env.close(); + } + + // Test buy offers with a destination with and without + // fixNFTokenNegOffer. + for (auto const& tweakedFeatures : + {features - fixNFTokenNegOffer - featureNonFungibleTokensV1_1, + features | fixNFTokenNegOffer}) + { + Env env{*this, tweakedFeatures}; + + env.fund(XRP(1000000), issuer, buyer); + + // Create an NFT that we'll make offers for. + uint256 const nftID{ + token::getNextID(env, issuer, 0u, tfTransferable)}; + env(token::mint(issuer, 0), txflags(tfTransferable)); + env.close(); + + TER const offerCreateTER = tweakedFeatures[fixNFTokenNegOffer] + ? static_cast(tesSUCCESS) + : static_cast(temMALFORMED); + + env(token::createOffer(buyer, nftID, drops(1)), + token::owner(issuer), + token::destination(issuer), + ter(offerCreateTER)); + env.close(); + } + } + void testWithFeats(FeatureBitset features) { @@ -4584,6 +4902,7 @@ class NFToken_test : public beast::unit_test::suite testNFTokenWithTickets(features); testNFTokenDeleteAccount(features); testNftXxxOffers(features); + testFixNFTokenNegOffer(features); } public: diff --git a/src/test/app/ValidatorKeys_test.cpp b/src/test/app/ValidatorKeys_test.cpp index 18061c7e4..3943fd858 100644 --- a/src/test/app/ValidatorKeys_test.cpp +++ b/src/test/app/ValidatorKeys_test.cpp @@ -23,8 +23,9 @@ #include #include #include +#include + #include -#include namespace ripple { namespace test { @@ -75,7 +76,14 @@ public: void run() override { - SuiteJournal journal("ValidatorKeys_test", *this); + // We're only using Env for its Journal. That Journal gives better + // coverage in unit tests. + test::jtx::Env env{ + *this, + test::jtx::envconfig(), + nullptr, + beast::severities::kDisabled}; + beast::Journal journal{env.app().journal("ValidatorKeys_test")}; // Keys/ID when using [validation_seed] SecretKey const seedSecretKey = diff --git a/src/test/app/ValidatorList_test.cpp b/src/test/app/ValidatorList_test.cpp index 860b8fc17..fead5563f 100644 --- a/src/test/app/ValidatorList_test.cpp +++ b/src/test/app/ValidatorList_test.cpp @@ -29,9 +29,10 @@ #include #include #include -#include #include +#include + namespace ripple { namespace test { @@ -217,7 +218,8 @@ private: { testcase("Config Load"); - jtx::Env env(*this); + jtx::Env env( + *this, jtx::envconfig(), nullptr, beast::severities::kDisabled); auto& app = env.app(); PublicKey emptyLocalKey; std::vector const emptyCfgKeys; diff --git a/src/test/app/ValidatorSite_test.cpp b/src/test/app/ValidatorSite_test.cpp index db9f5c977..8ec86fead 100644 --- a/src/test/app/ValidatorSite_test.cpp +++ b/src/test/app/ValidatorSite_test.cpp @@ -69,7 +69,7 @@ private: using namespace jtx; - Env env(*this); + Env env(*this, envconfig(), nullptr, beast::severities::kDisabled); auto trustedSites = std::make_unique(env.app(), env.journal); @@ -282,9 +282,6 @@ private: if (u.cfg.failFetch) { using namespace std::chrono; - log << " -- Msg: " - << myStatus[jss::last_refresh_message].asString() - << std::endl; std::stringstream nextRefreshStr{ myStatus[jss::next_refresh_time].asString()}; system_clock::time_point nextRefresh; @@ -357,9 +354,6 @@ private: sink.messages().str().find(u.expectMsg) != std::string::npos, sink.messages().str()); - log << " -- Msg: " - << myStatus[jss::last_refresh_message].asString() - << std::endl; } } } diff --git a/src/test/basics/PerfLog_test.cpp b/src/test/basics/PerfLog_test.cpp index a79dded90..79944e0ed 100644 --- a/src/test/basics/PerfLog_test.cpp +++ b/src/test/basics/PerfLog_test.cpp @@ -24,12 +24,13 @@ #include #include #include +#include + #include #include #include #include #include -#include #include //------------------------------------------------------------------------------ @@ -44,7 +45,11 @@ class PerfLog_test : public beast::unit_test::suite // We're only using Env for its Journal. That Journal gives better // coverage in unit tests. - test::jtx::Env env_{*this}; + test::jtx::Env env_{ + *this, + test::jtx::envconfig(), + nullptr, + beast::severities::kDisabled}; beast::Journal j_{env_.app().journal("PerfLog_test")}; struct Fixture diff --git a/src/test/basics/StringUtilities_test.cpp b/src/test/basics/StringUtilities_test.cpp index fc6d54c63..6146a3dcd 100644 --- a/src/test/basics/StringUtilities_test.cpp +++ b/src/test/basics/StringUtilities_test.cpp @@ -289,6 +289,13 @@ public: BEAST_EXPECT(!parseUrl(pUrl, "nonsense")); BEAST_EXPECT(!parseUrl(pUrl, "://")); BEAST_EXPECT(!parseUrl(pUrl, ":///")); + BEAST_EXPECT( + !parseUrl(pUrl, "scheme://user:pass@domain:65536/abc:321")); + BEAST_EXPECT(!parseUrl(pUrl, "UPPER://domain:23498765/")); + BEAST_EXPECT(!parseUrl(pUrl, "UPPER://domain:0/")); + BEAST_EXPECT(!parseUrl(pUrl, "UPPER://domain:+7/")); + BEAST_EXPECT(!parseUrl(pUrl, "UPPER://domain:-7234/")); + BEAST_EXPECT(!parseUrl(pUrl, "UPPER://domain:@#$56!/")); } { diff --git a/src/test/beast/beast_io_latency_probe_test.cpp b/src/test/beast/beast_io_latency_probe_test.cpp index 9ae6a1341..b2bf67b10 100644 --- a/src/test/beast/beast_io_latency_probe_test.cpp +++ b/src/test/beast/beast_io_latency_probe_test.cpp @@ -200,9 +200,6 @@ class io_latency_probe_test : public beast::unit_test::suite, duration_cast(probe_duration).count()) / static_cast(tt.getMean()); #endif - log << "expected_probe_count_min: " << expected_probe_count_min << "\n"; - log << "expected_probe_count_max: " << expected_probe_count_max << "\n"; - test_sampler io_probe{interval, get_io_service()}; io_probe.start(); MyTimer timer{get_io_service(), probe_duration}; diff --git a/src/test/core/ClosureCounter_test.cpp b/src/test/core/ClosureCounter_test.cpp index 478816a89..c4199a0b0 100644 --- a/src/test/core/ClosureCounter_test.cpp +++ b/src/test/core/ClosureCounter_test.cpp @@ -19,9 +19,10 @@ #include #include +#include + #include #include -#include #include namespace ripple { @@ -31,9 +32,14 @@ namespace test { class ClosureCounter_test : public beast::unit_test::suite { - // We're only using Env for its Journal. - jtx::Env env{*this}; - beast::Journal j{env.app().journal("ClosureCounter_test")}; + // We're only using Env for its Journal. That Journal gives better + // coverage in unit tests. + test::jtx::Env env_{ + *this, + jtx::envconfig(), + nullptr, + beast::severities::kDisabled}; + beast::Journal j{env_.app().journal("ClosureCounter_test")}; void testConstruction() diff --git a/src/test/core/Config_test.cpp b/src/test/core/Config_test.cpp index 45afaf6cb..da29fafac 100644 --- a/src/test/core/Config_test.cpp +++ b/src/test/core/Config_test.cpp @@ -154,7 +154,7 @@ public: rmDataDir_ = !exists(dataDir_); config_.setup( file_.string(), - /*bQuiet*/ true, + /* bQuiet */ true, /* bSilent */ false, /* bStandalone */ false); } @@ -190,9 +190,6 @@ public: using namespace boost::filesystem; if (rmDataDir_) rmDir(dataDir_); - else - test_.log << "Skipping rm dir: " << dataDir_.string() - << std::endl; } catch (std::exception& e) { diff --git a/src/test/jtx/Env_test.cpp b/src/test/jtx/Env_test.cpp index b1a1a8125..6f09f49ed 100644 --- a/src/test/jtx/Env_test.cpp +++ b/src/test/jtx/Env_test.cpp @@ -888,10 +888,14 @@ public: testExceptionalShutdown() { except([this] { - jtx::Env env{*this, jtx::envconfig([](std::unique_ptr cfg) { - (*cfg).deprecatedClearSection("port_rpc"); - return cfg; - })}; + jtx::Env env{ + *this, + jtx::envconfig([](std::unique_ptr cfg) { + (*cfg).deprecatedClearSection("port_rpc"); + return cfg; + }), + nullptr, + beast::severities::kDisabled}; }); pass(); } diff --git a/src/test/ledger/View_test.cpp b/src/test/ledger/View_test.cpp index 45c8f007e..bbb1eec8f 100644 --- a/src/test/ledger/View_test.cpp +++ b/src/test/ledger/View_test.cpp @@ -130,6 +130,8 @@ class View_test : public beast::unit_test::suite void testLedger() { + testcase("Ledger"); + using namespace jtx; Env env(*this); Config config; @@ -165,6 +167,8 @@ class View_test : public beast::unit_test::suite void testMeta() { + testcase("Meta"); + using namespace jtx; Env env(*this); wipe(env.app().openLedger()); @@ -196,6 +200,8 @@ class View_test : public beast::unit_test::suite void testMetaSucc() { + testcase("Meta succ"); + using namespace jtx; Env env(*this); wipe(env.app().openLedger()); @@ -260,6 +266,8 @@ class View_test : public beast::unit_test::suite void testStacked() { + testcase("Stacked"); + using namespace jtx; Env env(*this); wipe(env.app().openLedger()); @@ -325,6 +333,8 @@ class View_test : public beast::unit_test::suite void testContext() { + testcase("Context"); + using namespace jtx; using namespace std::chrono; { @@ -387,6 +397,8 @@ class View_test : public beast::unit_test::suite void testUpperAndLowerBound() { + testcase("Upper and lower bound"); + using namespace jtx; Env env(*this); Config config; @@ -654,6 +666,8 @@ class View_test : public beast::unit_test::suite void testSles() { + testcase("Sles"); + using namespace jtx; Env env(*this); Config config; @@ -786,6 +800,8 @@ class View_test : public beast::unit_test::suite void testFlags() { + testcase("Flags"); + using namespace jtx; Env env(*this); @@ -949,6 +965,8 @@ class View_test : public beast::unit_test::suite void testTransferRate() { + testcase("Transfer rate"); + using namespace jtx; Env env(*this); @@ -975,12 +993,14 @@ class View_test : public beast::unit_test::suite // construct and manage two different Env instances at the same // time. So we can use two Env instances to produce mutually // incompatible ledgers. + testcase("Are compatible"); + using namespace jtx; auto const alice = Account("alice"); auto const bob = Account("bob"); // The first Env. - Env eA(*this); + Env eA(*this, envconfig(), nullptr, beast::severities::kDisabled); eA.fund(XRP(10000), alice); eA.close(); @@ -990,9 +1010,13 @@ class View_test : public beast::unit_test::suite eA.close(); auto const rdViewA4 = eA.closed(); - // The two Env's can't share the same ports, so modifiy the config + // 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)}; + Env eB{ + *this, + envconfig(port_increment, 3), + nullptr, + beast::severities::kDisabled}; // Make ledgers that are incompatible with the first ledgers. Note // that bob is funded before alice. @@ -1029,6 +1053,8 @@ class View_test : public beast::unit_test::suite void testRegressions() { + testcase("Regressions"); + using namespace jtx; // Create a ledger with 1 item, put a diff --git a/src/test/overlay/short_read_test.cpp b/src/test/overlay/short_read_test.cpp index dd649bfd1..434b41008 100644 --- a/src/test/overlay/short_read_test.cpp +++ b/src/test/overlay/short_read_test.cpp @@ -195,8 +195,6 @@ private: { acceptor_.listen(); server_.endpoint_ = acceptor_.local_endpoint(); - test_.log << "[server] up on port: " << server_.endpoint_.port() - << std::endl; } void diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 1692b9806..2580c4bfe 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -1250,13 +1250,10 @@ class LedgerRPC_test : public beast::unit_test::suite // no amendments env.fund(XRP(10000), "alice"); env.close(); - log << env.closed()->info().hash; env.fund(XRP(10000), "bob"); env.close(); - log << env.closed()->info().hash; env.fund(XRP(10000), "jim"); env.close(); - log << env.closed()->info().hash; env.fund(XRP(10000), "jill"); { diff --git a/src/test/rpc/NodeToShardRPC_test.cpp b/src/test/rpc/NodeToShardRPC_test.cpp index edfaf6c20..07d8d8953 100644 --- a/src/test/rpc/NodeToShardRPC_test.cpp +++ b/src/test/rpc/NodeToShardRPC_test.cpp @@ -276,7 +276,8 @@ public: sectionNode.set("ledgers_per_shard", "256"); c->setupControl(true, true, true); - return jtx::Env(*this, std::move(c)); + return jtx::Env( + *this, std::move(c), nullptr, beast::severities::kDisabled); }(); std::uint8_t const numberOfShards = 10; diff --git a/src/test/rpc/ShardArchiveHandler_test.cpp b/src/test/rpc/ShardArchiveHandler_test.cpp index 37c9d0168..ee0bec1ea 100644 --- a/src/test/rpc/ShardArchiveHandler_test.cpp +++ b/src/test/rpc/ShardArchiveHandler_test.cpp @@ -173,7 +173,8 @@ public: } c->setupControl(true, true, true); - jtx::Env env(*this, std::move(c)); + jtx::Env env( + *this, std::move(c), nullptr, beast::severities::kDisabled); std::uint8_t const numberOfDownloads = 10; @@ -276,7 +277,8 @@ public: } c->setupControl(true, true, true); - jtx::Env env(*this, std::move(c)); + jtx::Env env( + *this, std::move(c), nullptr, beast::severities::kDisabled); std::uint8_t const numberOfDownloads = 10; @@ -380,7 +382,8 @@ public: } c->setupControl(true, true, true); - jtx::Env env(*this, std::move(c)); + jtx::Env env( + *this, std::move(c), nullptr, beast::severities::kDisabled); std::uint8_t const numberOfDownloads = 10; // Create some ledgers so that the ShardArchiveHandler diff --git a/src/test/server/Server_test.cpp b/src/test/server/Server_test.cpp index 35f9149ca..b5eb71f36 100644 --- a/src/test/server/Server_test.cpp +++ b/src/test/server/Server_test.cpp @@ -299,7 +299,6 @@ public: serverPort.back().port = 0; serverPort.back().protocol.insert("http"); auto eps = s->ports(serverPort); - log << "server listening on port " << eps[0].port() << std::endl; test_request(eps[0]); test_keepalive(eps[0]); // s->close(); diff --git a/src/test/shamap/SHAMapSync_test.cpp b/src/test/shamap/SHAMapSync_test.cpp index ba32f6e80..6b2648a96 100644 --- a/src/test/shamap/SHAMapSync_test.cpp +++ b/src/test/shamap/SHAMapSync_test.cpp @@ -184,7 +184,6 @@ public: BEAST_EXPECT(source.deepCompare(destination)); - log << "Checking destination invariants..." << std::endl; destination.invariants(); } }; diff --git a/src/test/unit_test/FileDirGuard.h b/src/test/unit_test/FileDirGuard.h index 6337365f0..3c79fb11b 100644 --- a/src/test/unit_test/FileDirGuard.h +++ b/src/test/unit_test/FileDirGuard.h @@ -86,9 +86,6 @@ public: if (rmSubDir_) rmDir(subDir_); - else - test_.log << "Skipping rm dir: " << subDir_.string() - << std::endl; } catch (std::exception& e) {