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

Catch the "hooks" branch up with "develop" with a big ol' merge #4927

Open
wants to merge 494 commits into
base: hooks
Choose a base branch
from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Feb 23, 2024

High Level Overview of Change

Note that this is not fully functional code! It is being merged into the hooks branch, which is still "in-development".

This branch merges develop into hooks, and makes some fixes / updates to get it partially working.

Results by OS:

  1. It builds on Linux, but a unit test run doesn't finish. Some relatively simple unit tests fail. e.g. AccountSet.
  2. It doesn't build on Windows / MSVC, but it gets a lot further than the original PR did. Notably, it is able to download and set up the wasmedge dependency using Conan. All of the errors appear to be limited to applyHook.cpp, and all appear to be due to parsing / preprocessing errors with the massively complex macros in macros.h
  3. It builds with MacOS clang, but fails to start: dyld[6586]: Library not loaded: /opt/homebrew/opt/llvm/lib/libLLVM.dylib Referenced from: <68C35517-5F51-3521-A8EE-2CE9D9882E28> /Users/ed/.conan/data/wasmedge/0.9.0/_/_/package/72521513b9fee5d76bcfd97cf0256cfa56cb3897/lib/libwasmedge_c.dylib

I have made no significant effort to fix any remaining issues, and I don't intend to, though I will be happy to accept contributions. This is not intended to be working code, just to get the branch caught up so that someone else can get it working.

Context of Change

This is a follow up to #4225, which also only affected that branch.

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Tests (you added tests for code that already exists, or your new feature included in this PR)

Future Tasks

Significant work and debugging needs to be done to get this "ready for prime time".

Note that this is not fully functional code! It is being merged into the hooks branch, which is still "in-development".

intelliot and others added 30 commits September 15, 2023 08:55
Make the instructions a bit easier to follow. Users on different
platforms can look for their platform name to find relevant information.
Removed the unused variable `none` from `Writer.cpp` which was causing
build errors on clang version 14.
`LedgerHistory::fixIndex` returns `false` if a repair was performed.

Fix XRPLF#4572
Copy the new code to `src/secp256k1` without changes:
`src/secp256k1` is identical to bitcoin-core/secp256k1@acf5c55 (v0.3.2).

We could consider changing to a Git submodule, though that would require
changes to the build instructions because we are not using submodules
anywhere else.
* Reorganize some changelog entries
* Add note about portable binaries
* Dev blog: https://xrpl.org/blog
Add Boost::json to the list of linked Boost libraries.

This seems to be required for macOS.
gateway_balances
* When `account` does not exist in the ledger, return `actNotFound`
  * (Previously, a normal response was returned)
  * Fix XRPLF#4290
* When required field(s) are missing, return `invalidParams`
  * (Previously, `invalidHotWallet` was incorrectly returned)
  * Fix XRPLF#4548

channel_authorize
* When the specified `key_type` is invalid, return `badKeyType`
  * (Previously, `invalidParams` was returned)
  * Fix XRPLF#4289

Since these are breaking changes, they apply only to API version 2.

Supersedes XRPLF#4577
Co-authored-by: Chenna Keshava B S <[email protected]>
Clean up the peer-to-peer protocol start/close sequences by introducing
START_PROTOCOL and GRACEFUL_CLOSE messages, which sync inbound/outbound
peer send/receive. The GRACEFUL_CLOSE message differentiates application
and link layer failures.

* Introduce the `InboundHandoff` class to manage inbound peer
  instantiation and synchronize the send/receive protocol messages
  between peers.
* Update `OverlayImpl` to utilize the `InboundHandoff` class to manage
  inbound handshakes.
* Update `PeerImp` for improved handling of protocol messages.
* Modify the `Message` class for better maintainability.
* Introduce P2P protocol version `2.3`.
Currently, the `BUILD.md` instructions suggest using `.build` as the
build directory, so this change helps to reduce confusion.

An alternative would be to instruct developers to add `/.build/` to
`.git/info/exclude` or to user-level `.gitignore` (although the latter
is very intrusive). However, it is being added here because it is a good
practice to have a sensible default that's consistent with the build
instructions.
A few methods, including `book_offers`, take currency codes as
parameters. The XRPL doesn't care if the letters in those codes are
lowercase or uppercase, as long as they come from an alphabet defined
internally. rippled doesn't care either, when they are submitted in a
hex representation. When they are submitted in an ASCII string
representation, rippled, but not XRPL, is more restrictive, preventing
clients from interacting with some currencies already in the XRPL.

This change gets rippled out of the way and lets clients submit currency
codes in ASCII using the full alphabet.

Fixes XRPLF#4112
Fix the Windows build by using `unsigned int` (instead of `uint`).

The error, introduced by XRPLF#4618, looks something like:
  rpc\impl\RPCHelpers.h(299,5): error C2061: syntax error: identifier
  'uint' (compiling source file app\ledger\Ledger.cpp)
…LF#4410)

Amendment "flapping" (an amendment repeatedly gaining and losing
majority) usually occurs when an amendment is on the verge of gaining
majority, and a validator not in favor of the amendment goes offline or
loses sync. This fix makes two changes:

1. The number of validators in the UNL determines the threshold required
   for an amendment to gain majority.
2. The AmendmentTable keeps a record of the most recent Amendment vote
   received from each trusted validator (and, with `trustChanged`, stays
   up-to-date when the set of trusted validators changes). If no
   validation arrives from a given validator, then the AmendmentTable
   assumes that the previously-received vote has not changed.

In other words, when missing an `STValidation` from a remote validator,
each server now uses the last vote seen. There is a 24 hour timeout for
recorded validator votes.

These changes do not require an amendment because they do not impact
transaction processing, but only the threshold at which each individual
validator decides to propose an EnableAmendment pseudo-transaction.

Fix XRPLF#4350
Modify the `XChainBridge` amendment.

Before this patch, two door accounts on the same chain could could own
the same bridge spec (of course, one would have to be the issuer and one
would have to be the locker). While this is silly, it does not violate
any bridge invariants. However, on further review, if we allow this then
the `claim` transactions would need to change. Since it's hard to see a
use case for two doors to own the same bridge, this patch disallows
it. (The transaction will return tecDUPLICATE).
Update minimum compiler requirement for building the codebase. The
feature "using enum" is required. This feature was introduced in C++20.

Updating the C++ compiler to version 11 or later fixes this error:

```
Building CXX object CMakeFiles/xrpl_core.dir/src/ripple/protocol/impl/STAmount.cpp.o
/build/ripple/binary/src/ripple/protocol/impl/STAmount.cpp: In lambda function:
/build/ripple/binary/src/ripple/protocol/impl/STAmount.cpp:1577:15: error: expected nested-name-specifier before 'enum'
 1577 |         using enum Number::rounding_mode;
      |               ^~~~
```

Fix XRPLF#4693
Address a stack-use-after-scope issue when using rvalues with
`soci::use`. Replace rvalues with lvalues to ensure the scope extends
beyond the end of the expression.

The issue arises from `soci` taking a reference to the rvalue without
copying its value or extending its lifetime. `soci` references rvalues
in `soci::use_container` and then the address in `soci_use_type`. For
types like `int`, memory access post-lifetime is unlikely to cause
issues. However, for `std::string`, the backing heap memory can be freed
and potentially reused, leading to a potential segmentation fault.

This was detected on x86_64 using clang-15 with asan. asan confirms
resolution of the issue.

Fix XRPLF#4675
Make transactions and pseudo-transactions share the same commonFields
again. This regularizes the code in a nice way.

While this technically allows pseudo-transactions to have a
TicketSequence field, pseudo-transactions are only ever constructed by
code paths that don't add such a field, so this is not a transaction
processing change. It may be possible to add a separate check to ensure
TicketSequence (and other fields that don't make sense on
pseudo-transactions) are never added to pseudo-transactions, but that
should not be necessary. (TicketSequence is not the only common field
that can not and does not appear in pseudo-transactions.) Note:
TicketSequence is already documented as a common field.

Related: XRPLF#4637

Fix XRPLF#4714
When a new transactor is added, there are several places in applySteps
that need to be modified. This patch refactors the code so only one
function needs to be modified.
…F#4721)

Context: The `DisallowIncoming` amendment provides an option to block
incoming trust lines from reaching your account. The
asfDisallowIncomingTrustline AccountSet Flag, when enabled, prevents any
incoming trust line from being created. However, it was too restrictive:
it would block an issuer from authorizing a trust line, even if the
trust line already exists. Consider:

1. Issuer sets asfRequireAuth on their account.
2. User sets asfDisallowIncomingTrustline on their account.
3. User submits tx to SetTrust to Issuer.

At this point, without `fixDisallowIncomingV1` active, the issuer would
not be able to authorize the trust line.

The `fixDisallowIncomingV1` amendment, once activated, allows an issuer
to authorize a trust line even after the user sets the
asfDisallowIncomingTrustline flag, as long as the trust line already
exists.
P2P link compression is a feature added in 1.6.0 by XRPLF#3287.

https://xrpl.org/enable-link-compression.html

If the default changes in the future - for example, as currently
proposed by XRPLF#4387 - the comment will be updated at that time.

Fix XRPLF#4656
* Add a new API Changelog section for release 1.10.
* Mark `jss::fee_ref` as deprecated.
* Fix a copy-paste error in one of the unit tests.
Artifactory support was added to the `nix` builds with XRPLF#4556. This
extends that support to the Windows build. Now the Windows build works;
CI will build and test a Windows release build. This only affects CI and
does not change any C++ code.

* Copy the remote setup step outcome fix from XRPLF#4716 discussion
* Allow the Windows job to succeed if tests fail:
  * Currently the tests do not always pass, even on a single threaded
    run on the GitHub runners. So we are using parallel runs and mark
    the test step as allowed to fail (continue-on-error).
  * At this point, it's more important that the build succeeds than that
    the tests succeed, because:
  * We've got plenty of test coverage on the other jobs.
  * Test failures are much rarer than build failures because of
    cross-platform issues.
  * Having a test failure locally doesn't interrupt a workflow nearly as
    much as a build failure.

Note that Conan Center cannot hold the binaries we need. They do not
build the configurations we need, and they will not add them.

## Future Tasks

This introduces a new bottleneck since the build and test takes over an
hour. Speed up the job by:

* Making this job run on heavy Windows runners.
* Increasing the number of hardware threads.
…#4746)

Update the nix CI runner. This commit does not modify any source code
files. The unix builds were successful, but the binaries were not
uploaded to the internal artifactory. This PR borrows an idea from
@ximinez to attempt to fix this issue.

After successful authentication, the `outcome` variable contains a
string. In the upload step, we are checking if outcome == 'success' as a
prerequisite for uploading the binary.

This commit updates the contents of the `outcome` variable.
scottschurr and others added 10 commits July 30, 2024 12:47
* upstream/develop:
  Update gcovr EXCLUDE (5084)
  Fix crash inside `OverlayImpl` loops over `ids_` (5071)
  Set version to 2.3.0-b2
  docs: Document the process for merging pull requests (5010)
  Remove unused constants from resource/Fees.h (4856)
  fix: change error for invalid `feature` param in `feature` RPC (5063)
  Ensure levelization sorting is ASCII-order across platforms (5072)
  fix: Fix NuDB build error via Conan patch (5061)
  Disallow filtering account_objects by unsupported types (5056)
  Set version to 2.2.1
  Use error codes throughout fast Base58 implementation
  Improve error handling in some RPC commands
* upstream/develop:
  Factor out Transactor::trapTransaction (5087)
  Remove shards (5066)
scottschurr and others added 19 commits August 7, 2024 18:14
* Add fixNFTokenPageLinks amendment:

It was discovered that under rare circumstances the links between
NFTokenPages could be removed.  If this happens, then the
account_objects and account_nfts RPC commands under-report the
NFTokens owned by an account.

The fixNFTokenPageLinks amendment does the following to address
the problem:

- It fixes the underlying problem so no further broken links
  should be created.
- It adds Invariants so, if such damage were introduced in the
  future, an invariant would stop it.
- It adds a new FixLedgerState transaction that repairs
  directories that were damaged in this fashion.
- It adds unit tests for all of it.
* upstream/develop:
  Address rare corruption of NFTokenPage linked list (4945)
Implements a CI workflow that detects when a new version of libxrpl is
proposed, uploads it to artifactory under the `clio` channel and
notifies Clio's CI to check this newly proposed version.
* Add "doxygen" to list of supported branches to allow for testing and
  development.
* Add titles / H1 to some .md files that don't have them.
* upstream/develop:
  chore: Fix documentation generation job: (5091)
  chore: libxrpl verification on CI (5028)
* Log when duplicate concurrent inbound ledger are filtered.
* RAII for containers that track concurrent inbound ledger.
* Comment on when to asynchronously acquire inbound ledgers, which
   is possible to be always OK, but should have further review.
* Other small logging changes

Co-authored-by: Ed Hennis <[email protected]>
* refactor filtering of validations to specifically avoid
 concurrent checkAccept() calls for the same validation ledger hash.
* Log when duplicate concurrent validation requests are filtered.
* RAII for containers that track concurrent validation requests.
* upstream/master:
  Set version to 2.2.2
  Allow only 1 job queue slot for each validation ledger check
  Allow only 1 job queue slot for acquiring inbound ledger.
  Track latencies of certain code blocks, and log if they take too long
* upstream/develop:
  docs: Update options documentation (5083)
  refactor: Remove dead headers (5081)
  refactor: Remove reporting mode (5092)
* upstream/develop:
  Update Release Notes for 2.2.1 and 2.2.2
  Set version to 2.2.2
  Allow only 1 job queue slot for each validation ledger check
  Allow only 1 job queue slot for acquiring inbound ledger.
  Track latencies of certain code blocks, and log if they take too long
* Retry some failed RPC connections / commands in unit tests
* Remove orphaned `getAccounts` function

Co-authored-by: John Freeman <[email protected]>
* upstream/develop:
  test: Retry RPC commands to try to fix MacOS CI jobs (5120)
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.