Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed 2.2.0-b2 #4979

Merged
merged 27 commits into from
Apr 5, 2024
Merged

Proposed 2.2.0-b2 #4979

merged 27 commits into from
Apr 5, 2024

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Apr 4, 2024

High Level Overview of Change

This is a beta for the 2.2.0 release.

Highlights:

The base branch is release. All releases (including betas) go in release. This PR will be merged with --ff-only (not squashed or rebased, and not using the GitHub UI) to both release and develop.

Context of Change

This introduces

  • the fast base58 codec, which speeds up base58 encoding and decoding significantly
  • a user version of the feature RPC command, allowing lookups without changing voting settings
  • the fixEmptyDID amendment to prevent creation of empty DID objects
  • the fixXChainRewardRounding amendment to improve the calculation of reward shares
  • consensus improvements to reduce de-syncs

It also fixes up some technical debt and build issues, and fixes the 'install' target so that downstream projects like clio can build properly.

Type of Change

  • Release

API Impact

No API impact.

ckeshava and others added 24 commits March 5, 2024 12:02
* 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
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).
* uses same formatting as admin RPC
* hides potentially sensitive data
This is likely the result of a typo when the code was simplified.
The problem was `CONAN_USERNAME` environment variable, which Conan 1.x uses as the default user in package references.
…RPLF#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.
Downgrade {upload,download}-artifact action to v3 because of unreliability with v4.
* 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.
- 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.
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.
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.
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 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
)

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.
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.
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.
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.
Merging changes for 2.1.1 from master into develop
* master is the default branch for that project. There's no point in
  using develop.
@Bronek
Copy link
Collaborator

Bronek commented Apr 4, 2024

Important caveat - because of #4893 , we have added a conan dependency on
xxHash::xxhash xxhash::xxhash, which is transitive for users of libxrpl . We could hide this dependency if we think it's a problem - i.e. move implementation of class xxhasher from header to .cpp so the dependency #include <xxhash.h> does not show up in header file xxhasher.hpp (which is seen in the library interface)

@ximinez
Copy link
Collaborator Author

ximinez commented Apr 4, 2024

@Bronek What is the worst case for users of libxrpl once they bump their dependency to this version or later? I assume they'd have to re-run conan install. Maybe wipe out their build directory?

@Bronek
Copy link
Collaborator

Bronek commented Apr 4, 2024

@Bronek What is the worst case for users of libxrpl once they bump their dependency to this version or later? I assume they'd have to re-run conan install. Maybe wipe out their build directory?

re-running conan install will be definitely needed; luckily it does not rebuild anything that's already in conan cache, so it should be quick, no need to wipe anytihing. The bigger problem is for projects which depend in libxrpl, as they will also (transitively) gain this dependency, without necessarily knowing about it. So they will also have to re-run conan install

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 85.93172% with 342 lines in your changes are missing coverage. Please review.

Project coverage is 76.96%. Comparing base (d7d15a9) to head (c88166e).
Report is 3 commits behind head on release.

Files Patch % Lines
src/test/basics/base58_test.cpp 80.20% 29 Missing and 10 partials ⚠️
src/ripple/rpc/handlers/GetAggregatePrice.cpp 82.71% 13 Missing and 15 partials ⚠️
src/ripple/app/tx/impl/SetOracle.cpp 85.00% 9 Missing and 15 partials ⚠️
src/ripple/protocol/impl/token_errors.h 17.24% 24 Missing ⚠️
src/ripple/protocol/impl/tokens.cpp 89.32% 9 Missing and 10 partials ⚠️
src/ripple/app/tx/impl/DeleteOracle.cpp 55.00% 11 Missing and 7 partials ⚠️
src/test/jtx/impl/Oracle.cpp 88.38% 12 Missing and 6 partials ⚠️
src/ripple/app/paths/impl/AMMOffer.cpp 40.74% 15 Missing and 1 partial ⚠️
src/ripple/protocol/impl/b58_utils.h 78.78% 6 Missing and 8 partials ⚠️
src/ripple/rpc/handlers/LedgerEntry.cpp 58.62% 8 Missing and 4 partials ⚠️
... and 32 more
Additional details and impacted files
@@             Coverage Diff              @@
##           release    #4979       +/-   ##
============================================
+ Coverage    61.53%   76.96%   +15.43%     
============================================
  Files          797     1129      +332     
  Lines        70158   131913    +61755     
  Branches     36261    39628     +3367     
============================================
+ Hits         43170   101527    +58357     
- Misses       19753    24464     +4711     
+ Partials      7235     5922     -1313     

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

@scottschurr scottschurr self-requested a review April 4, 2024 22:25
@intelliot intelliot merged commit c88166e into XRPLF:release Apr 5, 2024
33 of 34 checks passed
@ximinez ximinez deleted the develop-next branch April 5, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.