Introduce "order book not crossed" invariant (for fuzzing only) #2959

rokopt · 2021-03-11T04:55:31Z

Introduce "order book not crossed" invariant

Resolves #2417

This PR introduces a new invariant which keeps track of the state of the order book as operations are applied, and asserts that it is not crossed.

There is currently no mechanism by which an invariant can be informed that an operation application for which its checkOnOperationApply() method had been called is being rolled back. Hence, we can not in general support an invariant such as this which maintains state across method calls. But we can when fuzzing, where setup operations are never rolled back and where we can explicitly tell invariants when to snapshot state (that is, after setup completes) and when to reset it to that state (after each fuzz test).

Therefore, this invariant is compiled in when fuzzing is configured, and not otherwise.

This PR updates #2210 to current code, adapting to some changes such as the introduction of InternalLedgerEntry and the rearranging of the fuzzing code, and adds support for passive offers.

Checklist

Reviewed the contributing document
Rebased on top of master (no merge commits)
Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
Compiles
Ran all tests
If change impacts performance, include supporting evidence per the performance document

rokopt · 2021-03-11T04:57:30Z

This invariant does appear to have a major effect on fuzzer execution speed. Metrics after 20 minutes in current code without this PR:

And with this PR:

src/invariant/OrderBookIsNotCrossed.cpp

src/invariant/test/InvariantTestUtils.cpp

src/test/FuzzerImpl.cpp

MonsieurNicolas · 2021-03-12T01:31:33Z

src/invariant/OrderBookIsNotCrossed.cpp

+void
+OrderBookIsNotCrossed::resetForFuzzer()
+{
+    mOrderBook = mRolledBackOrderBook;


resetForFuzzer is likely very slow as it's copying a large data structure, yet it's unlikely that offers got changed.
I think we should perform the actual copy as late as possible: probably in OrderBookIsNotCrossed::check
(reset is probably still here though)

Good idea, done.

I see that you reverted your change. From the looks of it, it was just not correct.
you need to always clear in resetForFuzzer (for stability) and CoW

src/invariant/OrderBookIsNotCrossed.cpp

src/invariant/test/OrderBookIsNotCrossedTests.cpp

src/invariant/OrderBookIsNotCrossed.h

rokopt · 2021-03-15T14:57:09Z

Alright, I've gone over the inherited files here and made a bunch of changes:

The part I found trickiest was just getting the code to build and run in as many situations as possible! We want it to build any time we have BUILD_TESTS configured, and to be able to run the invariant's own unit tests in that case, to minimize code rot. And we want to build and use it when fuzzing is configured. But we don't want the invariant to be active during any other unit tests, because it maintains state across calls to checkOnOperationApply() and the code base in general does not support invariants that do that. What I ended up doing was not registering the invariant in general because test configs enable .*, so if the invariant were registered, then it would be enabled during tests by default. Instead, I exported a registerAndEnableInvariant(), unlike most invariants which export a registerInvariant() and let the config manager decide which to enable. Then only those callers that explicitly know how to handle the order-book invariant's maintaining state can call registerAndEnableInvariant(). And two callers do so: the fuzzer and the invariant's own unit tests. The invariant's own unit tests also have to disable other invariants, because they modify ledger entries directly, which can break some other invariants. (That part is something that several other invariant tests do as well -- they set INVARIANT_CHECKS to just themselves.)
Rebased on master, which in particular includes the best-offer debugging code, with which there were a couple small conflicts.
Improved the passive-offer-crossing-detection algorithm to break early if possible.
Removed some weird C++ such as the references to temporaries and unnecessary at()s.
Removed using namespace and added conventional namespace use.
Added an assertion to makeUpdateList().
Deferred copying mRolledBackOrderBook as late as we can.
Made extractAssets() clearer (I hope).
Removed some changes which aren't necessary anymore after the above changes.

jonjove

I didn't review the tests yet because I think the implementation is going to change. You should add tests for some of the interesting floating-point edge cases I pointed out.

src/test/FuzzerImpl.cpp

src/invariant/OrderBookIsNotCrossed.cpp

src/invariant/OrderBookIsNotCrossed.h

rokopt · 2021-03-16T20:20:36Z

I've pushed changes to address all comments except on the floating-point reciprocal issue; these changes are small so I figure I may as well make them available for review while I think about the floating-point.

src/invariant/OrderBookIsNotCrossed.h

rokopt · 2021-03-18T19:41:45Z

After much interesting chat about arithmetic, I've pushed a commit to restore the ordering by double arithmetic within the invariant, as it's done in stellar-core: the salient change is now simply to do reciprocals in rational arithmetic before any conversion to double. In effect, once a value becomes a double, it becomes constant (it should not be used in any further computation, only in comparison).

jonjove · 2021-03-18T19:55:00Z

@rokopt can you squash+rebase at this point? We've gone through a lot of iterations here, with considerable back-and-forth on changes, and I need to get a clean look at it.

rokopt · 2021-03-18T20:10:00Z

@rokopt can you squash+rebase at this point? We've gone through a lot of iterations here, with considerable back-and-forth on changes, and I need to get a clean look at it.

Sure, done.

src/invariant/OrderBookIsNotCrossed.h

jonjove · 2021-03-18T20:49:39Z

src/invariant/OrderBookIsNotCrossed.cpp

+
+namespace stellar
+{
+double


Nitpick: you probably want this in the anonymous namespace or static.

I've removed uses of static in favor of either the anonymous namespace or of membership in OrderBookNotCrossed, depending on whether any members of the latter need the types/functions.

src/invariant/OrderBookIsNotCrossed.cpp

src/invariant/OrderBookIsNotCrossed.h

src/invariant/test/OrderBookIsNotCrossedTests.cpp

rokopt · 2021-03-19T17:53:15Z

Note: in addition to responding to the latest code review comments, I've also pushed a commit to remove the mRollbackBeforeNextCheck optimization, because I discovered that it causes occasional crashes during long fuzzing runs. I don't understand why yet. We can either delay this PR until I've found a fix, or merge it and file an issue to perform that optimization (correctly) afterwards, as you prefer.

src/invariant/test/OrderBookIsNotCrossedTests.cpp

rokopt · 2021-03-22T23:24:58Z

Note: in addition to responding to the latest code review comments, I've also pushed a commit to remove the mRollbackBeforeNextCheck optimization, because I discovered that it causes occasional crashes during long fuzzing runs. I don't understand why yet. We can either delay this PR until I've found a fix, or merge it and file an issue to perform that optimization (correctly) afterwards, as you prefer.

This optimization is restored now, in a corrected form.

MonsieurNicolas · 2021-03-25T22:04:22Z

Looks like it's basically ready to go? Needs to be rebased and we can merge. @jonjove what do you think?

As a sanity check I didn't see an update on the fuzzer stability front (only one I see here is the one from very early on). So maybe @rokopt can post this in parallel?

jonjove · 2021-03-25T22:11:59Z

@MonsieurNicolas I just went through and resolved all open comments from me. Should be ready for rebase/squash/merge. Agreed that I'd like to see the fuzzer statistics though.

This should reduce the chances of the code rotting.

This mechanism will allow for fuzzer-specific invariants that support a mechanism for notifying them when setup has completed and then when each new test starts.

Make the fuzzer call the new InvariantManager snapshot/reset mechanism, as well as calling the existing one for LedgerTxnRoot whenever BUILD_TESTS is configured.

A new invariant which keeps track of the state of the order book as operations are applied, and asserts that it is not crossed. There is currently no mechanism by which an invariant can be informed that an operation application for which its checkOnOperationApply() method had been called is being rolled back. Hence, we can not in general support an invariant such as this which maintains state across method calls. But we will be able to do so when fuzzing, where setup operations are never rolled back and where we can explicitly tell invariants when to snapshot state (that is, after setup completes) and when to reset it to that state (after each fuzz test).

rokopt · 2021-03-26T00:29:53Z

I've squashed and rebased. Stability didn't change noticeably in this PR; it's still at the ~92% where it's been since the ledger-key mapping work:

MonsieurNicolas · 2021-03-26T17:54:19Z

mmm, I do have a question though. Is the "exec speed" really that slow compared to both master and the previous iteration of that PR?

I would not expect such a difference as the vast majority of fuzzing should not involve this invariant (it is, after all, unlikely that the fuzzer explores branches with offers).

MonsieurNicolas · 2021-03-26T18:08:32Z

src/invariant/OrderBookIsNotCrossed.cpp

+{
+    updateOrderBook(ltxDelta);
+    auto assetPairs = extractAssetPairs(ltxDelta);
+    return assetPairs.size() > 0 ? check(assetPairs) : std::string{};


I think that the reason things are super slow is that assetPairs.size() > 0 is always true even when there are no changes to the order book from this operation. In fact we should not perform any work in extractAssetPairs if the operation didn't touch the order book.

Hmm, why must assetPairs.size() be > 0? It looks to me as though if there are no OFFERs in LedgerTxnDelta, then it will be 0, and I put in some logs and they say that it is often 0.

One thing I mentioned to Jon about performance which I should bring up is that exec speed of fuzzing seems highly variable on my laptop, even from run to run without code changes (unlike stability, for example, which is generally within a fraction of a percent across many runs). Fuzzing does make my laptop fan roar, and I have a suspicion (though no confirmation) that there might be highly variable amounts of CPU-throttling going on depending on I-don't-know-what thermal factors.

meh and I thought my head was straight in the morning :)

yeah this is extracting only from the current txn object so not it

MonsieurNicolas · 2021-03-26T18:09:04Z

I think I found the bug, let me know if this makes sense @rokopt

MonsieurNicolas · 2021-03-27T01:07:20Z

I ran the version with the invariant enabled and without based on the same initial corpus (so no variation because of that) and they performed the same way, so it was probably a fluke with your computer

MonsieurNicolas · 2021-03-27T01:07:49Z

we'll merge as soon as master reopens

jonjove · 2021-03-30T18:53:57Z

r+ dffc55f

rokopt mentioned this pull request Mar 11, 2021

Best-offer debugging #2944

Merged

6 tasks

MonsieurNicolas requested changes Mar 12, 2021

View reviewed changes

sisuresh reviewed Mar 12, 2021

View reviewed changes

src/invariant/OrderBookIsNotCrossed.h Outdated Show resolved Hide resolved

rokopt force-pushed the order-book-not-crossed-pr branch from 0491449 to ee37b83 Compare March 15, 2021 14:42

jonjove suggested changes Mar 16, 2021

View reviewed changes

src/invariant/OrderBookIsNotCrossed.h Outdated Show resolved Hide resolved

jonjove reviewed Mar 17, 2021

View reviewed changes

src/invariant/OrderBookIsNotCrossed.h Outdated Show resolved Hide resolved

rokopt force-pushed the order-book-not-crossed-pr branch from 702bb4b to e6181d2 Compare March 17, 2021 20:52

rokopt requested review from jonjove and MonsieurNicolas March 18, 2021 19:45

rokopt force-pushed the order-book-not-crossed-pr branch from 6ab6a3d to 639fb29 Compare March 18, 2021 20:07

jonjove reviewed Mar 19, 2021

View reviewed changes

rokopt requested a review from jonjove March 19, 2021 17:53

stellar deleted a comment from rokopt Mar 20, 2021

jonjove reviewed Mar 22, 2021

View reviewed changes

src/invariant/test/OrderBookIsNotCrossedTests.cpp Outdated Show resolved Hide resolved

rokopt added 2 commits March 25, 2021 16:10

Move generateOffer to InvariantTestUtils to export

bfad6ad

Build resetForFuzzer() under BUILD_TESTS

6034963

This should reduce the chances of the code rotting.

rokopt added 6 commits March 25, 2021 16:10

Add {snapshot,reset}forFuzzer to InvariantManager

5bf2c0b

This mechanism will allow for fuzzer-specific invariants that support a mechanism for notifying them when setup has completed and then when each new test starts.

Call new snapshot/reset interfaces from fuzzer

1ab7b56

Make the fuzzer call the new InvariantManager snapshot/reset mechanism, as well as calling the existing one for LedgerTxnRoot whenever BUILD_TESTS is configured.

Unit tests for order-book-not-crossed invariant

7996d9e

Windows build of order-book-not-crossed invariant

0d707e6

Use order-book-not-crossed invariant when fuzzing

dffc55f

rokopt force-pushed the order-book-not-crossed-pr branch from e74eb98 to dffc55f Compare March 26, 2021 00:28

MonsieurNicolas reviewed Mar 26, 2021

View reviewed changes

MonsieurNicolas approved these changes Mar 27, 2021

View reviewed changes

latobarita merged commit c64aa5a into stellar:master Mar 30, 2021

jonjove mentioned this pull request Mar 31, 2021

Add fuzzer only invariants #2884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce "order book not crossed" invariant (for fuzzing only) #2959

Introduce "order book not crossed" invariant (for fuzzing only) #2959

rokopt commented Mar 11, 2021

rokopt commented Mar 11, 2021

MonsieurNicolas Mar 12, 2021

rokopt Mar 15, 2021

MonsieurNicolas Mar 20, 2021

rokopt commented Mar 15, 2021

jonjove left a comment

rokopt commented Mar 16, 2021

rokopt commented Mar 18, 2021

jonjove commented Mar 18, 2021

rokopt commented Mar 18, 2021

jonjove Mar 18, 2021

rokopt Mar 19, 2021

rokopt commented Mar 19, 2021

rokopt commented Mar 22, 2021

MonsieurNicolas commented Mar 25, 2021

jonjove commented Mar 25, 2021

rokopt commented Mar 26, 2021

MonsieurNicolas commented Mar 26, 2021

MonsieurNicolas Mar 26, 2021

rokopt Mar 26, 2021

MonsieurNicolas Mar 27, 2021

MonsieurNicolas commented Mar 26, 2021

MonsieurNicolas commented Mar 27, 2021

MonsieurNicolas commented Mar 27, 2021

jonjove commented Mar 30, 2021

Introduce "order book not crossed" invariant (for fuzzing only) #2959

Introduce "order book not crossed" invariant (for fuzzing only) #2959

Conversation

rokopt commented Mar 11, 2021

Introduce "order book not crossed" invariant

Checklist

rokopt commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rokopt commented Mar 15, 2021

jonjove left a comment

Choose a reason for hiding this comment

rokopt commented Mar 16, 2021

rokopt commented Mar 18, 2021

jonjove commented Mar 18, 2021

rokopt commented Mar 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rokopt commented Mar 19, 2021

rokopt commented Mar 22, 2021

MonsieurNicolas commented Mar 25, 2021

jonjove commented Mar 25, 2021

rokopt commented Mar 26, 2021

MonsieurNicolas commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonsieurNicolas commented Mar 26, 2021

MonsieurNicolas commented Mar 27, 2021

MonsieurNicolas commented Mar 27, 2021

jonjove commented Mar 30, 2021