Relocate RPCErr to ripple/protocol - fix previous PR #4940

legleux · 2024-03-06T03:22:59Z

High Level Overview of Change

Adds STCurrency.h to libxrpl for Clio
Fixes PR #4885 botched by force push

Context of Change

Force push in updating PR #4885 undid intended changes.
Latest Clio build requires STCurrency.h in libxrpl package.

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

Adds STCurrency.h to libxrpl for Clio Fixes PR XRPLF#4885 botched by force push

codecov-commenter · 2024-03-06T03:43:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.62%. Comparing base (2ffead7) to head (b8f55c8).

❗ Current head b8f55c8 differs from pull request most recent head d985810. Consider uploading reports for the commit d985810 to get more accurate results

Additional details and impacted files

@@             Coverage Diff              @@
##           develop    #4940       +/-   ##
============================================
- Coverage    76.94%   61.62%   -15.32%     
============================================
  Files         1127      806      -321     
  Lines       131675    70967    -60708     
  Branches     39591    36690     -2901     
============================================
- Hits        101313    43732    -57581     
+ Misses       24448    19882     -4566     
- Partials      5914     7353     +1439

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

conanfile.py

Builds/CMake/RippledCore.cmake

thejohnfreeman

This PR undoes this commit. Please remove that change.

ximinez

Ok, it took me way longer than expected, but I built the install target using develop and this branch, then compared the results:

$ diff -r install.develop/ install
Binary files install.develop/bin/rippled and install/bin/rippled differ
Only in install.develop/include/ripple/beast/unit_test: main.cpp
Only in install.develop/include/ripple: net
Only in install/include/ripple/protocol: RPCErr.h
Only in install/include/ripple/resource: Consumer.h
Only in install/include/ripple/resource: Disposition.h
Only in install/include/ripple/resource: Gossip.h
Only in install/include/ripple/resource: impl
Only in install/include/ripple/resource: ResourceManager.h
Only in install/include/ripple/resource: Types.h
Only in install/include/ripple/server/impl: JSONRPCUtil.h
diff -r install.develop/lib/cmake/ripple/RippleConfigVersion.cmake install/lib/cmake/ripple/RippleConfigVersion.cmake
12c12
< set(PACKAGE_VERSION "2.2.0-b1")
---
> set(PACKAGE_VERSION "2.2.0-b2")
18c18
<   if("2.2.0-b1" MATCHES "^([0-9]+)\\.")
---
>   if("2.2.0-b2" MATCHES "^([0-9]+)\\.")
24c24
<     set(CVF_VERSION_MAJOR "2.2.0-b1")
---
>     set(CVF_VERSION_MAJOR "2.2.0-b2")
Binary files install.develop/lib/libxrpl_core.a and install/lib/libxrpl_core.a differ

The differences in the binary files are expected, since I had to recompile.
I notice that beast/unit_test/main.cpp was removed from the install list. This may have been originally included intentionally because it is not built into rippled, so there's no other way to get it. Is anybody going to miss it?
The removal of net and addition of protocol/RPCErr.h are expected since that's the main point of this PR.
The addition of the other .h files in resource is probably good - they were probably overlooked before. However the newly added impl files aren't even exposed to the rest of rippled. Are we sure we want them exposed to external projects?
server/impl/JSONRPCUtil.h is already exposed in rippled to other modules, so this is probably fine.
Finally, the RippleConfigVersion.cmake changes are due the the version number change I've already complained about below.

It's probably beyond the scope of this PR, but we really should audit the exposed impl files to determine if they really need to be included in the install set, and if they do, move them out of the impl directory....

.github/workflows/windows.yml

src/ripple/protocol/impl/BuildInfo.cpp

legleux · 2024-03-07T22:52:21Z

2. beast/unit_test/main.cpp

@ximinez This is one thing I was unsure about whether it actually belonged or not. What would be the utility of providing this or not whether installed to a system or provided in a Conan package with libxrpl?

I think @thejohnfreeman has the intention to export all these via globs so that none are forgotten about anymore but that would entail the audit Ed refers to.
Should I just close this PR or is that audit/export on too hazy of a timeline?

ximinez · 2024-03-08T19:28:10Z

This is one thing I was unsure about whether it actually belonged or not. What would be the utility of providing this or not whether installed to a system or provided in a Conan package with libxrpl?

I can't think of much utility. IIRC, if one were to build and run beast/unit_test/main.cpp, it would only be able run the unit tests that are part of beast. Those are all already included in rippled AFAIK. It might be useful as sample / example code, but that can just as easily be obtained from Github.

Should I just close this PR or is that audit/export on too hazy of a timeline?

I vote to keep this PR open. It's a smaller subset of changes that can be reviewed and merged quickly, and I believe unblocks something that @thejohnfreeman is working on.

ximinez

Looks good, and all the extraneous changes have been undone.

Here's what the install before and after looks like:

$ diff -r install.develop/ install
Binary files install.develop/bin/rippled.exe and install/bin/rippled.exe differ
Only in install.develop/include/ripple/beast/unit_test: main.cpp
Only in install.develop/include/ripple: net
Only in install/include/ripple/protocol: RPCErr.h
Only in install/include/ripple/resource: Consumer.h
Only in install/include/ripple/resource: Disposition.h
Only in install/include/ripple/resource: Gossip.h
Only in install/include/ripple/resource: ResourceManager.h
Only in install/include/ripple/resource: Types.h
Only in install/include/ripple/resource: impl
Only in install/include/ripple/server/impl: JSONRPCUtil.h
Binary files install.develop/lib/ed25519.lib and install/lib/ed25519.lib differ
Binary files install.develop/lib/secp256k1.lib and install/lib/secp256k1.lib differ
Binary files install.develop/lib/xrpl_core.lib and install/lib/xrpl_core.lib differ

thejohnfreeman · 2024-03-12T04:51:42Z

Builds/CMake/RippledCore.cmake

      src/ripple/beast/unit_test/suite_info.hpp
      src/ripple/beast/unit_test/suite_list.hpp
+      src/ripple/beast/unit_test/suite.hpp


The surrounding command, from line 427 (if(tests)) to line 442 (DESTINATION ...), should likely be removed. I don't know why we're installing to path ripple/beast/extras/unit_test/. Nothing should depend on that. We're already installing all of these headers to ripple/beast/unit_test/ (lines 383-410).

thejohnfreeman · 2024-03-12T04:51:55Z

Builds/CMake/RippledCore.cmake

+    src/ripple/beast/utility/WrappedSink.h
+    src/ripple/beast/utility/WrappedSink.h


thejohnfreeman · 2024-03-12T04:57:26Z

@ximinez we're already installing some headers in impl directories. I would like to hold off on moving them around. My plan is to remove all impl directories in the PR that separates libxrpl and xrpld sources.

ximinez · 2024-03-12T16:33:02Z

@ximinez we're already installing some headers in impl directories. I would like to hold off on moving them around. My plan is to remove all impl directories in the PR that separates libxrpl and xrpld sources.

That's fine with me

* Price Oracle (XLS-47d): (XRPLF#4789) (XRPLF#4789) Implement native support for Price Oracles. A Price Oracle is used to bring real-world data, such as market prices, onto the blockchain, enabling dApps to access and utilize information that resides outside the blockchain. Add Price Oracle functionality: - OracleSet: create or update the Oracle object - OracleDelete: delete the Oracle object To support this functionality add: - New RPC method, `get_aggregate_price`, to calculate aggregate price for a token pair of the specified oracles - `ltOracle` object The `ltOracle` object maintains: - Oracle Owner's account - Oracle's metadata - Up to ten token pairs with the scaled price - The last update time the token pairs were updated Add Oracle unit-tests * fix compile error on gcc 13: (XRPLF#4932) The compilation fails due to an issue in the initializer list of an optional argument, which holds a vector of pairs. The code compiles correctly on earlier gcc versions, but fails on gcc 13. * Set version to 2.2.0-b1 * Remove default ctors from SecretKey and PublicKey: (XRPLF#4607) * It is now an invariant that all constructed Public Keys are valid, non-empty and contain 33 bytes of data. * Additionally, the memory footprint of the PublicKey class is reduced. The size_ data member is declared as static. * Distinguish and identify the PublisherList retrieved from the local config file, versus the ones obtained from other validators. * Fixes XRPLF#2942 * Fast base58 codec: (XRPLF#4327) This algorithm is about an order of magnitude faster than the existing algorithm (about 10x faster for encoding and about 15x faster for decoding - including the double hash for the checksum). The algorithms use gcc's int128 (fast MS version will have to wait, in the meantime MS falls back to the slow code). * feat: add user version of `feature` RPC (XRPLF#4781) * uses same formatting as admin RPC * hides potentially sensitive data * build: add STCurrency.h to xrpl_core to fix clio build (XRPLF#4939) * Embed patched recipe for RocksDB 6.29.5 (XRPLF#4947) * fix: order book update variable swap: (XRPLF#4890) This is likely the result of a typo when the code was simplified. * Fix workflows (XRPLF#4948) The problem was `CONAN_USERNAME` environment variable, which Conan 1.x uses as the default user in package references. * Upgrade to xxhash 0.8.2 as a Conan requirement, enable SIMD hashing (XRPLF#4893) We are currently using old version 0.6.2 of `xxhash`, as a verbatim copy and paste of its header file `xxhash.h`. Switch to the more recent version 0.8.2. Since this version is in Conan Center (and properly protects its ABI by keeping the state object incomplete), add it as a Conan requirement. Switch to the SIMD instructions (in the new `XXH3` family) supported by the new version. * Update remaining actions (XRPLF#4949) Downgrade {upload,download}-artifact action to v3 because of unreliability with v4. * Install more public headers (XRPLF#4940) Fixes some mistakes in XRPLF#4885 * test: Env unit test RPC errors return a unique result: (XRPLF#4877) * telENV_RPC_FAILED is a new code, reserved exclusively for unit tests when RPC fails. This will make those types of errors distinct and easier to test for when expected and/or diagnose when not. * Output RPC command result when result is not expected. * Fix workflows (XRPLF#4951) - Update container for Doxygen workflow. Matches Linux workflow, with newer GLIBC version required by newer actions. - Fixes macOS workflow to install and configure Conan correctly. Still fails on tests, but that does not seem attributable to the workflow. * perf: improve `account_tx` SQL query: (XRPLF#4955) The witness server makes heavily use of the `account_tx` RPC command. Perf testing showed that the SQL query used by `account_tx` became unacceptably slow when the DB was large and there was a `marker` parameter. The plan for the query showed only indexed reads. This appears to be an issue with the internal SQLite optimizer. This patch rewrote the query to use `UNION` instead of `OR` and significantly improves performance. See RXI-896 and RIPD-1847 for more details. * `fixEmptyDID`: fix amendment to handle empty DID edge case: (XRPLF#4950) This amendment fixes an edge case where an empty DID object can be created. It adds an additional check to ensure that DIDs are non-empty when created, and returns a `tecEMPTY_DID` error if the DID would be empty. * Enforce no duplicate slots from incoming connections: (XRPLF#4944) We do not currently enforce that incoming peer connection does not have remote_endpoint which is already used (either by incoming or outgoing connection), hence already stored in slots_. If we happen to receive a connection from such a duplicate remote_endpoint, it will eventually result in a crash (when disconnecting) or weird behavior (when updating slot state), as a result of an apparently matching remote_endpoint in slots_ being used by a different connection. * Remove zaphod.alloy.ee hub from default server list: (XRPLF#4903) Remove the zaphod.alloy.ee hubs from the bootstrap and default configuration after 5 years. It has been an honor to run these servers, but it is now time for another entity to step into this role. The zaphod servers will be taken offline in a phased manner keeping all those who have peering arrangements informed. These would be the preferred attributes of a boostrap set of hubs: 1. Commitment to run the hubs for a minimum of 2 years 2. Highly available 3. Geographically dispersed 4. Secure and up to date 5. Committed to ensure that peering information is kept private * Write improved `forAllApiVersions` used in NetworkOPs (XRPLF#4833) * Don't reach consensus as quickly if no other proposals seen: (XRPLF#4763) This fixes a case where a peer can desync under a certain timing circumstance--if it reaches a certain point in consensus before it receives proposals. This was noticed under high transaction volumes. Namely, when we arrive at the point of deciding whether consensus is reached after minimum establish phase duration but before having received any proposals. This could be caused by finishing the previous round slightly faster and/or having some delay in receiving proposals. Existing behavior arrives at consensus immediately after the minimum establish duration with no proposals. This causes us to desync because we then close a non-validated ledger. The change in this PR causes us to wait for a configured threshold before making the decision to arrive at consensus with no proposals. This allows validators to catch up and for brief delays in receiving proposals to be absorbed. There should be no drawback since, with no proposals coming in, we needn't be in a huge rush to jump ahead. * fixXChainRewardRounding: round reward shares down: (XRPLF#4933) When calculating reward shares, the amount should always be rounded down. If the `fixUniversalNumber` amendment is not active, this works correctly. If it is not active, then the amount is incorrectly rounded up. This patch introduces an amendment so it will be rounded down. * Remove unused files * Remove packaging scripts * Consolidate external libraries * Simplify protobuf generation * Rename .hpp to .h * Format formerly .hpp files * Rewrite includes $ find src/ripple/ src/test/ -type f -exec sed -i 's:include\s*["<]ripple/$.*$\.h$pp$\?[">]:include <ripple/\1.h>:' {} + * Fix source lists * Add markers around source lists * fix: improper handling of large synthetic AMM offers: A large synthetic offer was not handled correctly in the payment engine. This patch fixes that issue and introduces a new invariant check while processing synthetic offers. * Set version to 2.1.1 * chore: change Github Action triggers for build/test jobs (XRPLF#4956) Github Actions for the build/test jobs (nix.yml, mac.yml, windows.yml) will only run on branches that build packages (develop, release, master), and branches with names starting with "ci/". This is intended as a compromise between disabling CI jobs on personal forks entirely, and having the jobs run as a free-for-all. Note that it will not affect PR jobs at all. * Address compiler warnings * Fix search for protoc * chore: Default validator-keys-tool to master branch: (XRPLF#4943) * master is the default branch for that project. There's no point in using develop. * Remove unused lambdas from MultiApiJson_test * fix Conan component reference typo * Set version to 2.2.0-b2 * bump version * 2.2.3 * 2.2.4 * 2.2.5 --------- Co-authored-by: Gregory Tsipenyuk <[email protected]> Co-authored-by: seelabs <[email protected]> Co-authored-by: Chenna Keshava B S <[email protected]> Co-authored-by: Mayukha Vadari <[email protected]> Co-authored-by: John Freeman <[email protected]> Co-authored-by: Bronek Kozicki <[email protected]> Co-authored-by: Ed Hennis <[email protected]> Co-authored-by: Olek <[email protected]> Co-authored-by: Alloy Networks <[email protected]> Co-authored-by: Mark Travis <[email protected]> Co-authored-by: Gregory Tsipenyuk <[email protected]>

Fixes some mistakes in XRPLF#4885

Relocate RPCErr to ripple/protocol

b39f06f

Adds STCurrency.h to libxrpl for Clio Fixes PR XRPLF#4885 botched by force push

legleux requested review from thejohnfreeman and ximinez March 6, 2024 03:43

thejohnfreeman requested changes Mar 6, 2024

View reviewed changes

conanfile.py Outdated Show resolved Hide resolved

legleux added 2 commits March 6, 2024 14:07

Merge branch 'develop' into relocate-RPCerr

bea1215

Conan libraries without "lib" prefix and "a" suffix

626dc61

legleux requested a review from thejohnfreeman March 6, 2024 22:15

legleux marked this pull request as ready for review March 6, 2024 22:15

legleux mentioned this pull request Mar 6, 2024

Add STCurrency.h to xrpl_core to fix clio build #4939

Merged

9 tasks

thejohnfreeman requested changes Mar 7, 2024

View reviewed changes

Builds/CMake/RippledCore.cmake Outdated Show resolved Hide resolved

Builds/CMake/RippledCore.cmake Outdated Show resolved Hide resolved

Builds/CMake/RippledCore.cmake Outdated Show resolved Hide resolved

rm .cpp files

ca8f0a0

thejohnfreeman approved these changes Mar 7, 2024

View reviewed changes

thejohnfreeman requested changes Mar 7, 2024

View reviewed changes

Don't allow windows to fail

ee774a8

ximinez requested changes Mar 7, 2024

View reviewed changes

.github/workflows/windows.yml Outdated Show resolved Hide resolved

src/ripple/protocol/impl/BuildInfo.cpp Outdated Show resolved Hide resolved

Revert version bump

fc179cc

ximinez approved these changes Mar 8, 2024

View reviewed changes

thejohnfreeman requested changes Mar 12, 2024

View reviewed changes

legleux added 2 commits March 12, 2024 10:55

Remove redundant installs

6dc5f45

rm const_container.hpp and comment

b8f55c8

thejohnfreeman approved these changes Mar 13, 2024

View reviewed changes

legleux added 4 commits March 13, 2024 15:25

Merge branch 'develop' into relocate_RPCerr

5634a38

rm const_container.hpp

d985810

Merge branch 'develop' into relocate_RPCerr

76f5716

un rm const_container.hpp

85d1c3a

thejohnfreeman approved these changes Mar 13, 2024

View reviewed changes

thejohnfreeman merged commit af9cabe into XRPLF:develop Mar 14, 2024
17 checks passed

sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024

Install more public headers (XRPLF#4940)

be48484

Fixes some mistakes in XRPLF#4885

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relocate RPCErr to ripple/protocol - fix previous PR #4940

Relocate RPCErr to ripple/protocol - fix previous PR #4940

legleux commented Mar 6, 2024

codecov-commenter commented Mar 6, 2024 •

edited

Loading

thejohnfreeman left a comment

ximinez left a comment

legleux commented Mar 7, 2024 •

edited

Loading

ximinez commented Mar 8, 2024

ximinez left a comment

thejohnfreeman Mar 12, 2024

thejohnfreeman Mar 12, 2024

thejohnfreeman commented Mar 12, 2024

ximinez commented Mar 12, 2024

		src/ripple/beast/utility/WrappedSink.h
		src/ripple/beast/utility/WrappedSink.h

Relocate RPCErr to ripple/protocol - fix previous PR #4940

Relocate RPCErr to ripple/protocol - fix previous PR #4940

Conversation

legleux commented Mar 6, 2024

High Level Overview of Change

Context of Change

API Impact

codecov-commenter commented Mar 6, 2024 • edited Loading

Codecov Report

thejohnfreeman left a comment

Choose a reason for hiding this comment

ximinez left a comment

Choose a reason for hiding this comment

legleux commented Mar 7, 2024 • edited Loading

ximinez commented Mar 8, 2024

ximinez left a comment

Choose a reason for hiding this comment

thejohnfreeman Mar 12, 2024

Choose a reason for hiding this comment

thejohnfreeman Mar 12, 2024

Choose a reason for hiding this comment

thejohnfreeman commented Mar 12, 2024

ximinez commented Mar 12, 2024

codecov-commenter commented Mar 6, 2024 •

edited

Loading

legleux commented Mar 7, 2024 •

edited

Loading