rippled
rippled copied to clipboard
Add Antithesis intrumentation
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
- Define and document instrumentation macros in
include/xrpl/beast/utility/instrumentation.h - Add external dependency
antithesis-sdk-cppinexternal/antithesis-sdk - Add support for
antithesis-sdk-cppinCMakeLists.txtandcmake/RippledInstall.cmake - Add new CMake option
voidstartocmake/RippledCompiler.cmake,cmake/RippledCore.cmakeandcmake/RippledSettings.cmake - Set compilation option
--build-idunconditionally incmake/RippledCompiler.cmake - Update
CONTRIBUTING.mdto document contracts and instrumentation - Add
UNREACHABLEinLogicErrorinsrc/libxrpl/basics/contract.cpp - All the remaining code changes (several hundred lines) are replacing
assertwith eitherASSERTorUNREACHABLE(where suitable), with added name - 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:
constexprfunctions (ASSERTwould fail compilation)- unit tests i.e. files under
src/test(no need for instrumentation) - unit tests-related modules (files under
beast/testandbeast/unit_test; no need for instrumentation)
Also, as explained in include/xrpl/beast/utility/instrumentation.h, there are minor differences between ASSERT and assert:
ASSERTmust 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 optionvoidstarrequires 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
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.
Additional details and impacted files
@@ 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 |
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.
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)
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
Am I correct in guessing that all of the edits under
include/andsrc/are just changing assertions to use the new macros?
Yes.
Am I correct in guessing that all of the edits under
include/andsrc/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 Before I merge this, can you write a commit message with a consise summary?
@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
voidstarto 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
Merged manually as d7e949193fe3a42c67f36c021daba9fd0d52780c.
$ git diff --exit-code Bronek/feature/antithesis-instrumentation && echo Identical
Identical