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

Add Antithesis intrumentation #5042

Closed
wants to merge 42 commits into from

Conversation

Bronek
Copy link
Collaborator

@Bronek Bronek commented Jun 10, 2024

High Level Overview of Change

Add Antithesis instrumentation.

Context of Change

This change adds the support for continuous testing (fuzzing) of rippled on Antithesis platform. A copy of Antithesis C++ SDK is added to external/antithesis-sdk (with clang-format disabled, to make it easier to keep up).

The instrumentation macros ASSERT and UNREACHABLE are defined inside include/xrpl/beast/utility/instrumentation.h. In a normal build, these macros are just wrappers (with added name) for regular assert macro. In such build, there is no dependency on external/antithesis-sdk. The documentation of macros is also in include/xrpl/beast/utility/instrumentation.h, and the naming convention for instrumentation macros has been added to CONTRIBUTING.md

If voidstar CMake option is enabled (which is only supported with clang compiler, version 16 or later, running on Linux, and requires a Debug build), these macros will register and forward instrumentation calls to external/antithesis-sdk; this in turn informs the Antithesis platform of all the contracts. In case if a contract is violated during fuzzing, a report will be created for rippled developers for troubleshooting. The purpose of name in instrumentation macros is to provide instrumentation calls with stable identity, which does not rely on line numbers.

The voidstar CMake option is not exposed in conanfile.py because it is not useful for programs with a dependency on this project; it is only useful when building rippled binary with the intent of running it on the Antithesis platform. This means that, for ordinary users, macros ASSERT or UNREACHABLE are just like named assert.

Both ASSERT or UNREACHABLE are so-called "always assertions" - that is, they are test properties, which must be at all times true. Antithesis also supports a "sometimes assertions" which this PR does not define; we will add them later, on a case-by-case basis.

Also, this PR does not use "rich assertions" which are added in Antithesis SDK 0.4; this will be also used in a separate PR, in order to keep this PR as close as possible to regular assert model (even though this makes fuzzing slower to discover bugs, than it would have been otherwise).

Summary of changes

  1. Define and document instrumentation macros in include/xrpl/beast/utility/instrumentation.h
  2. Add external dependency antithesis-sdk-cpp in external/antithesis-sdk
  3. Add support for antithesis-sdk-cpp in CMakeLists.txt and cmake/RippledInstall.cmake
  4. Add new CMake option voidstar to cmake/RippledCompiler.cmake, cmake/RippledCore.cmake and cmake/RippledSettings.cmake
  5. Set compilation option --build-id unconditionally in cmake/RippledCompiler.cmake
  6. Update CONTRIBUTING.md to document contracts and instrumentation
  7. Add UNREACHABLE in LogicError in src/libxrpl/basics/contract.cpp
  8. All the remaining code changes (several hundred lines) are replacing assert with either ASSERT or UNREACHABLE (where suitable), with added name
  9. The naming guidance is explained inside CONTRIBUTING.md

I strived to make the naming of contracts consistent everywhere, however that was difficult given the size of change.

Macro ASSERT is ALWAYS_OR_UNREACHABLE rather than ALWAYS

Instrumentation macros are registered in the Antithesis platform and they can be also used for the purpose of coverage reporting (that is, whether or not a given contract has been reached is also a test property on the Antithesis platform). The semantics of ALWAYS macro (as defined in Antithesis C++ SDK) is not only to check that a condition is always true, it is also that the given line of code is always executed during fuzzing. Since we do not expect all asserts to be always executed by a program during testing, we need a different macro, with the semantics that the condition is true if the given line is executed, otherwise the test platform should ignore the fact that a given line was not executed. This is the semantics of ALWAYS_OR_UNREACHABLE macro.

Macro ASSERT is not the same as assert

There are also locations where we do not use ASSERT or UNREACHABLE because the old style assert or assert(false) make more sense:

  • constexpr functions (ASSERT would fail compilation)
  • unit tests i.e. files under src/test (no need for instrumentation)
  • unit tests-related modules (files under beast/test and beast/unit_test; no need for instrumentation)

Also, as explained in include/xrpl/beast/utility/instrumentation.h, there are minor differences between ASSERT and assert:

  • ASSERT must have an unique name (naming convention in CONTRIBUTING.md)
  • the condition (which comes first) must be implicitly convertible to bool
  • during fuzzing, the program will continue execution past a failed ASSERT (like Release build) however the parameters will be fully evaluated (which is why CMake option voidstar requires Debug build)

No change to the github workflow, yet

We need to upgrade clang compiler from version 14 to version no older than 16 in order to enable voidstar option, which would be needed to validate that the instrumentation macros do work with the the headers in antithesis-sdk-cpp (in particular that the ASSERT conditions are implicitly convertible to bool). I raised an issue #5153 for this upgrade. Once it is done, I intend to submit a separate PR which will define a new job to build with the voidstar option.

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

Copy link

codecov bot commented Jun 10, 2024

Codecov Report

Attention: Patch coverage is 79.72820% with 179 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (f64cf91) to head (91da172).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/xrpld/overlay/detail/PeerImp.cpp 0.0% 15 Missing ⚠️
src/libxrpl/json/json_value.cpp 40.0% 12 Missing ⚠️
src/xrpld/app/ledger/detail/LedgerMaster.cpp 38.5% 8 Missing ⚠️
src/xrpld/perflog/detail/PerfLogImp.cpp 0.0% 8 Missing ⚠️
src/xrpld/app/misc/detail/ValidatorList.cpp 80.0% 5 Missing ⚠️
src/xrpld/overlay/detail/OverlayImpl.cpp 37.5% 5 Missing ⚠️
src/xrpld/peerfinder/detail/Logic.h 54.5% 5 Missing ⚠️
src/libxrpl/basics/Log.cpp 0.0% 4 Missing ⚠️
src/xrpld/app/ledger/detail/InboundLedger.cpp 50.0% 4 Missing ⚠️
src/xrpld/app/main/Application.cpp 71.4% 4 Missing ⚠️
... and 73 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           develop   #5042     +/-   ##
=========================================
- Coverage     77.9%   77.9%   -0.0%     
=========================================
  Files          784     784             
  Lines        66681   66681             
  Branches      8162    8158      -4     
=========================================
- Hits         51950   51919     -31     
- Misses       14731   14762     +31     
Files with missing lines Coverage Δ
include/xrpl/basics/Buffer.h 100.0% <100.0%> (ø)
include/xrpl/basics/SlabAllocator.h 94.6% <100.0%> (ø)
include/xrpl/basics/Slice.h 96.8% <100.0%> (ø)
include/xrpl/basics/partitioned_unordered_map.h 99.1% <100.0%> (ø)
include/xrpl/basics/random.h 100.0% <100.0%> (ø)
include/xrpl/basics/scope.h 100.0% <100.0%> (ø)
include/xrpl/basics/spinlock.h 93.5% <100.0%> (ø)
include/xrpl/beast/asio/io_latency_probe.h 96.8% <100.0%> (ø)
include/xrpl/beast/clock/manual_clock.h 100.0% <100.0%> (ø)
.../beast/container/detail/aged_unordered_container.h 96.5% <100.0%> (ø)
... and 233 more

... and 2 files with indirect coverage changes

Impacted file tree graph

@Bronek Bronek force-pushed the feature/antithesis-instrumentation branch 11 times, most recently from b4e3cfb to 2b422d6 Compare June 14, 2024 13:02
@@ -120,7 +120,7 @@ else ()
target_link_libraries (common
INTERFACE
-rdynamic
$<$<BOOL:${is_linux}>:-Wl,-z,relro,-z,now>
$<$<BOOL:${is_linux}>:-Wl,-z,relro,-z,now,--build-id>
Copy link
Collaborator Author

@Bronek Bronek Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unconditional addition of --build-id is intentional, but could be made conditional (on voidstar), like other options are. I just think it is a useful option to have even in a regular build.

@Bronek
Copy link
Collaborator Author

Bronek commented Jun 14, 2024

The choice of cmake option name voidstar comes from the reference to libvoidstar.so in the implementation of the Antithesis instrumentation - which is inside external/antithesis-sdk/antithesis_instrumentation.h file. I am open to replacing it with something else, e.g. instrumentation.

@Bronek Bronek force-pushed the feature/antithesis-instrumentation branch 2 times, most recently from 602bbc7 to 4a6d7bd Compare June 14, 2024 18:34
@Bronek Bronek marked this pull request as ready for review June 17, 2024 09:00
@Bronek Bronek force-pushed the feature/antithesis-instrumentation branch 3 times, most recently from a676c1a to d315dde Compare June 24, 2024 18:14
@intelliot
Copy link
Collaborator

6 validators is a good start and should yield useful results.

But just curious, how difficult would it be to later scale up the test to 35 (or more) validators?

(This would make the testing even more realistic and predict what could happen in the future if more validators are added)

Copy link
Collaborator

@thejohnfreeman thejohnfreeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct in guessing that all of the edits under include/ and src/ are just changing assertions to use the new macros?

include/xrpl/basics/instrumentation.h Outdated Show resolved Hide resolved
include/xrpl/basics/instrumentation.h Outdated Show resolved Hide resolved
Builds/levelization/results/loops.txt Outdated Show resolved Hide resolved
@Bronek
Copy link
Collaborator Author

Bronek commented Jul 2, 2024

6 validators is a good start and should yield useful results.

But just curious, how difficult would it be to later scale up the test to 35 (or more) validators?

(This would make the testing even more realistic and predict what could happen in the future if more validators are added)

It's just a question of CPU utilisation on the Antithesis platform, that is balancing price/speed

@Bronek
Copy link
Collaborator Author

Bronek commented Jul 2, 2024

Am I correct in guessing that all of the edits under include/ and src/ are just changing assertions to use the new macros?

Yes.

@Bronek Bronek requested a review from ximinez July 2, 2024 20:30
@Bronek Bronek force-pushed the feature/antithesis-instrumentation branch from 6f634cd to 88050fb Compare July 11, 2024 21:11
@Bronek Bronek force-pushed the feature/antithesis-instrumentation branch from 1fbc1cb to 1db9890 Compare October 22, 2024 17:40
@bthomee bthomee self-requested a review October 22, 2024 19:01
@Bronek Bronek added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Oct 22, 2024
@Bronek Bronek force-pushed the feature/antithesis-instrumentation branch from ff250ce to 83808fa Compare October 23, 2024 14:26
@bthomee bthomee self-requested a review October 23, 2024 15:40
This was referenced Oct 23, 2024
@Bronek Bronek added the Blocked label Oct 23, 2024
@ximinez ximinez added this to the 2.4.0 (2025) milestone Nov 6, 2024
Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this will be merged after rippled 2.3.0

@ximinez
Copy link
Collaborator

ximinez commented Dec 3, 2024

@Bronek Before I merge this, can you write a commit message with a consise summary?

@Bronek
Copy link
Collaborator Author

Bronek commented Dec 3, 2024

@Bronek Before I merge this, can you write a commit message with a consise summary?

Add Antithesis instrumentation

  • Copy Antithesis SDK version 0.4.0 to directory external/
  • Add build option voidstar to enable instrumentation with Antithesis SDK
  • Define instrumentation macros ASSERT and UNREACHABLE in terms of regular C assert
  • Replace asserts with named ASSERT or UNREACHABLE
  • Add UNREACHABLE to LogicError
  • Document instrumentation macros in CONTRIBUTING.md

ximinez pushed a commit to ximinez/rippled that referenced this pull request Dec 3, 2024
* Copy Antithesis SDK version 0.4.0 to directory external/
* Add build option `voidstar` to enable instrumentation with Antithesis SDK
* Define instrumentation macros ASSERT and UNREACHABLE in terms of regular C assert
* Replace asserts with named ASSERT or UNREACHABLE
* Add UNREACHABLE to LogicError
* Document instrumentation macros in CONTRIBUTING.md
@ximinez
Copy link
Collaborator

ximinez commented Dec 3, 2024

Merged manually as d7e9491.

$ git diff --exit-code Bronek/feature/antithesis-instrumentation && echo Identical
Identical

@ximinez ximinez closed this Dec 3, 2024
ximinez added a commit to ximinez/rippled that referenced this pull request Dec 3, 2024
* upstream/develop:
  test: Check for some unlikely null dereferences in tests (XRPLF#5004)
  Add Antithesis intrumentation (XRPLF#5042)
@Bronek Bronek removed the Blocked label Dec 3, 2024
@ximinez ximinez mentioned this pull request Dec 3, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Status: 👀 Needs review
Development

Successfully merging this pull request may close these issues.

5 participants