rippled icon indicating copy to clipboard operation
rippled copied to clipboard

Add Antithesis intrumentation

Open Bronek opened this issue 1 year ago • 6 comments

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 https://github.com/XRPLF/rippled/issues/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)
  • [x] 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

Bronek avatar Jun 10 '24 20:06 Bronek

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 :warning:
src/libxrpl/json/json_value.cpp 40.0% 12 Missing :warning:
src/xrpld/app/ledger/detail/LedgerMaster.cpp 38.5% 8 Missing :warning:
src/xrpld/perflog/detail/PerfLogImp.cpp 0.0% 8 Missing :warning:
src/xrpld/app/misc/detail/ValidatorList.cpp 80.0% 5 Missing :warning:
src/xrpld/overlay/detail/OverlayImpl.cpp 37.5% 5 Missing :warning:
src/xrpld/peerfinder/detail/Logic.h 54.5% 5 Missing :warning:
src/libxrpl/basics/Log.cpp 0.0% 4 Missing :warning:
src/xrpld/app/ledger/detail/InboundLedger.cpp 50.0% 4 Missing :warning:
src/xrpld/app/main/Application.cpp 71.4% 4 Missing :warning:
... 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

codecov[bot] avatar Jun 10 '24 21:06 codecov[bot]

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 avatar Jun 14 '24 17:06 Bronek

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)

intelliot avatar Jun 26 '24 21:06 intelliot

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 avatar Jul 02 '24 14:07 Bronek

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 avatar Jul 02 '24 15:07 Bronek

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

Yes.

@thejohnfreeman not anymore. With the commit https://github.com/XRPLF/rippled/pull/5042/commits/8d7cd9c558aeea3eabe490342a926985b10c552b all the asserts have now gained names, which I tried to deduce by reading the surrounding code. This was needed to ensure that Antithesis reports do not lose the history of passing/failing asserts, as the location of asserts in code changes due to code churn.

Bronek avatar Jul 22 '24 11:07 Bronek

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

ximinez avatar Dec 03 '24 00:12 ximinez

@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

Bronek avatar Dec 03 '24 09:12 Bronek

Merged manually as d7e949193fe3a42c67f36c021daba9fd0d52780c.

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

ximinez avatar Dec 03 '24 20:12 ximinez