Skip to content

[EXPERIMENTAL] Fix compilation errors with external/protobuf #5606

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

Draft
wants to merge 15 commits into
base: bthomee/proto
Choose a base branch
from

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Jul 24, 2025

High Level Overview of Change

This should fix compilation error in #5589

We probably do not want this to be merged; instead we should rely on conan-io/conan-center-index#27982

Context of Change

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Performance (increase or change in throughput and/or latency)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)
  • Documentation update
  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
  • Release

lmaisons and others added 11 commits July 22, 2025 11:42
The current implementation of rngfill is prone to false warnings from GCC about array bounds violations. Looking at the code, the implementation naively manipulates both the bytes count and the buffer pointer directly to ensure the trailing memcpy doesn't overrun the buffer. As expressed, there is a data dependency on both fields between loop iterations.

Now, ideally, an optimizing compiler would realize that these dependencies were unnecessary and end up restructuring its intermediate representation into a functionally equivalent form with them absent. However, the point at which this occurs may be disjoint from when warning analyses are performed, potentially rendering them more difficult to
determine precisely.

In addition, it may also consume a portion of the budget the optimizer has allocated to attempting to improve a translation unit's performance. Given this is a function template which requires context-sensitive instantiation, this code would be more prone than most to being inlined, with a decrease in optimization budget corresponding to the effort the optimizer has already expended, having already optimized one or more calling functions. Thus, the scope for impacting the the ultimate quality of the code generated is elevated.

For this change, we rearrange things so that the location and contents of each memcpy can be computed independently, relying on a simple loop iteration counter as the only changing input between iterations.
For jobs running in containers, $GITHUB_WORKSPACE and ${{ github.workspace }} might not be the same directory. The actions/checkout step is supposed to checkout into `$GITHUB_WORKSPACE` and then add it to safe.directory (see instructions at https://github.com/actions/checkout), but that's apparently not happening for some container images. We can't be sure what is actually happening, so we preemptively add both directories to `safe.directory`. See also the GitHub issue opened in 2022 that still has not been resolved actions/runner#2058.
This change addresses the issue #5336: Refactor HashRouter flags to be more type-safe.

* Switched numeric flags to enum type.
* Updated unit tests
If a feature was never voted on then it is safe to remove.
After the `FlowCross` amendment was retired (#5562), there was still some unused code left. This change removes the remaining remnants.
This change adds support for `DomainID` to existing transactions `MPTokenIssuanceCreate` and `MPTokenIssuanceSet`.

In #5224 `DomainID` was added as an access control mechanism for `SingleAssetVault`. The actual implementation of this feature lies in `MPToken` and `MPTokenIssuance`, hence it makes sense to enable the use of `DomainID` also in `MPTokenIssuanceCreate` and `MPTokenIssuanceSet`, following same rules as in Vault:

* `MPTokenIssuanceCreate` and `MPTokenIssuanceSet` can only set `DomainID` if flag `MPTRequireAuth` is set.
* `MPTokenIssuanceCreate` requires that `DomainID` be a non-zero, uint256 number.
* `MPTokenIssuanceSet` allows `DomainID` to be zero (or empty) in which case it will remove `DomainID` from the `MPTokenIssuance` object.

The change is amendment-gated by `SingleAssetVault`. This is a non-breaking change because `SingleAssetVault` amendment is `Supported::no`, i.e. at this moment considered a work in progress, which cannot be enabled on the network.
…#5579)

This change includes `network_id` data in the validations and ledger subscription stream responses, as well as unit tests to validate the response fields. Fixes #4783
This change renames the `libxrpl` profile to `default` to make it more usable.
Only needed until this PR is merged
conan-io/conan-center-index#27982
@Bronek Bronek added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Jul 24, 2025
Copy link

codecov bot commented Jul 24, 2025

Codecov Report

Attention: Patch coverage is 86.48649% with 15 lines in your changes missing coverage. Please review.

Please upload report for BASE (bthomee/proto@95a8d84). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/xrpld/overlay/detail/PeerImp.cpp 0.0% 11 Missing ⚠️
src/xrpld/app/misc/NetworkOPs.cpp 66.7% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff               @@
##             bthomee/proto   #5606   +/-   ##
===============================================
  Coverage                 ?   78.8%           
===============================================
  Files                    ?     814           
  Lines                    ?   71204           
  Branches                 ?    8337           
===============================================
  Hits                     ?   56109           
  Misses                   ?   15095           
  Partials                 ?       0           
Files with missing lines Coverage Δ
include/xrpl/beast/utility/rngfill.h 100.0% <100.0%> (ø)
include/xrpl/protocol/Feature.h 100.0% <ø> (ø)
include/xrpl/protocol/detail/ledger_entries.macro 100.0% <ø> (ø)
include/xrpl/protocol/detail/transactions.macro 100.0% <ø> (ø)
src/libxrpl/protocol/Feature.cpp 94.8% <100.0%> (ø)
src/xrpld/app/ledger/Ledger.cpp 82.9% <100.0%> (ø)
src/xrpld/app/misc/HashRouter.cpp 100.0% <100.0%> (ø)
src/xrpld/app/misc/HashRouter.h 100.0% <100.0%> (ø)
src/xrpld/app/tx/detail/CreateOffer.cpp 92.0% <100.0%> (ø)
src/xrpld/app/tx/detail/CreateOffer.h 100.0% <100.0%> (ø)
... and 7 more

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Before `XRPLF/ci` images, we did not have a `dependencies:` job for clang-16, so `instrumentation:` had to build its own dependencies. Now we have clang-16 Conan dependencies built in a separate job that can be used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DraftRunCI Normally CI does not run on draft PRs. This opts in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants