Best-offer debugging #2944

rokopt · 2021-03-04T03:55:34Z

Best-offer debugging

Updates and integrates some formerly-private debugging for getBestOffer() which cross-checks its return values against a much slower, simpler algorithm.

Because the debug algorithm is so slow, it is compiled in by default only when tests are compiled in (or when explicitly requested with a configure option), and if is compiled in, it runs by default only under the test and fuzz commands (or when explicitly requested with a command-line option; it can also be disabled for test and fuzz with a command-line option).

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

src/test/FuzzerImpl.cpp

src/test/test.cpp

src/ledger/LedgerTxn.cpp

src/main/CommandLine.cpp

rokopt · 2021-03-04T22:02:14Z

To try to get some idea of the impact of this on test speed, I ran ./src/stellar-core test --all-versions --ll info -w NoTests -a -r simple "[tx]" (I'm guessing the transaction tests would include the most impacted) with and without best-offer debugging compiled in:

--enable-extrachecks --enable-asan --disable-best-offer-debugging: 7m55s
--enable-extrachecks --enable-asan --enable-best-offer-debugging: 7m57s

I'm guessing that's well within the variation (and even if it weren't, it's probably an irrelevant difference for a unit test), so I think we can indeed comfortably afford not to provide a --disable-best-offer-debugging for stellar-core test.

sisuresh

I reviewed the last commit and the logic looks good to me. Just included a few minor comments.

src/ledger/LedgerTxn.cpp

src/ledger/LedgerTxnImpl.h

src/ledger/LedgerTxn.cpp

rokopt · 2021-03-05T00:47:09Z

To try to get some idea of the impact of this on test speed, I ran ./src/stellar-core test --all-versions --ll info -w NoTests -a -r simple "[tx]" (I'm guessing the transaction tests would include the most impacted) with and without best-offer debugging compiled in:

--enable-extrachecks --enable-asan --disable-best-offer-debugging: 7m55s
--enable-extrachecks --enable-asan --enable-best-offer-debugging: 7m57s

I'm guessing that's well within the variation (and even if it weren't, it's probably an irrelevant difference for a unit test), so I think we can indeed comfortably afford not to provide a --disable-best-offer-debugging for stellar-core test.

I also tried it on a catchup. The range I chose took ~5.5 minutes without best-offer debugging, and ~45 minutes with it.

MonsieurNicolas · 2021-03-05T06:04:44Z

Is the configure option needed now that you have the config? Why not just enable it if people enable extra-checks?

rokopt · 2021-03-05T14:48:39Z

Is the configure option needed now that you have the config? Why not just enable it if people enable extra-checks?

Extrachecks are off unless --enable-extrachecks is explicitly specified, even if tests are being built. I think we were intending that tests would build and run with best-offer debugging unless someone explicitly requested otherwise. (Just keying best-offer debugging off BUILD_TESTS is an option.)

rokopt · 2021-03-06T02:21:52Z

Nicolas requested the removal of the separate configure option, --enable-best-offer-debugging, in favor of using --enable-extrachecks to determine whether to set -DBEST_OFFER_DEBUGGING.

He also requested removing the --enable-best-offer-debugging option from run and catchup, as if anyone needs such detailed debugging for those commands, they'll probably be modifying code and rebuilding anyway.

I've pushed commits to do each of those two things.

MonsieurNicolas

cool. you should squash those last 2 commits into the previous commits as they're (mostly) cancelling things that this PR added

MonsieurNicolas · 2021-03-06T20:36:01Z

src/ledger/InMemoryLedgerTxnRoot.cpp

@@ -10,8 +10,15 @@
 namespace stellar
 {

-InMemoryLedgerTxnRoot::InMemoryLedgerTxnRoot()
+InMemoryLedgerTxnRoot::InMemoryLedgerTxnRoot(
+#if BEST_OFFER_DEBUGGING


do not mix #if BEST_OFFER_DEBUGGING and #ifdef BEST_OFFER_DEBUGGING
What you want everywhere is #ifdef BEST_OFFER_DEBUGGING given how you set it in the configure script

Yikes, thanks for catching that. Fixed.

MonsieurNicolas · 2021-03-06T20:38:11Z

src/test/FuzzerImpl.cpp

@@ -983,7 +983,11 @@ void
 TransactionFuzzer::initialize()
 {
    resetRandomSeed(1);
-    mApp = createTestApplication(mClock, getFuzzConfig(0));
+    auto cfg = getFuzzConfig(0);
+#ifdef BEST_OFFER_DEBUGGING


you don't need this: as you're using a test configuration, it's already set

Good point. Removed.

rokopt · 2021-03-07T02:02:22Z

cool. you should squash those last 2 commits into the previous commits as they're (mostly) cancelling things that this PR added

Squashed. (The previous two were also squashable.)

src/ledger/LedgerTxn.cpp

rokopt · 2021-03-11T15:18:55Z

Fuzz metrics from current master without this PR:

And with this PR:

So the impact of this on exec speed is very substantial, although the benefits in terms of noticing bugs that fuzzing might find could also be large. (That all applies to #2959 as well.)

Compile in best-offer debugging if extrachecks are enabled.

The Config option is initialized to false, so that best-offer debugging will be used only if something explicitly turns it on. (For example, a test might set it in a Config option before calling createTestApplication(), or the "test" command might enable it by default if not explicitly disabled on the command line, or the "run" command might enable it if explicitly enabled on the command line.)

Always use best-offer debugging in `stellar-core test`.

Add parameters to the constructors of LedgerTxnRoot and InMemoryLedgerTxnRoot to pass in the state of the best-offer debug Config option (or "false" if best-offer debugging is not compiled in). If best-offer debugging is compiled in, also give those classes new boolean members to store the value of the option.

If best-offer debugging is compiled in and the run-time Config option is set, check the results of getBestOffer() against a new getBestOfferSlow().

MonsieurNicolas · 2021-03-12T01:07:39Z

r+ ae3dfa0

jonjove reviewed Mar 4, 2021

View reviewed changes

src/test/FuzzerImpl.cpp Outdated Show resolved Hide resolved

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

src/ledger/LedgerTxn.cpp Outdated Show resolved Hide resolved

src/main/CommandLine.cpp Outdated Show resolved Hide resolved

rokopt force-pushed the best-offer-debugging branch 2 times, most recently from da17ff3 to dae0070 Compare March 4, 2021 18:15

sisuresh reviewed Mar 4, 2021

View reviewed changes

src/ledger/LedgerTxn.cpp Outdated Show resolved Hide resolved

src/ledger/LedgerTxnImpl.h Show resolved Hide resolved

src/ledger/LedgerTxn.cpp Show resolved Hide resolved

MonsieurNicolas added this to In progress in v15.4.0 via automation Mar 5, 2021

MonsieurNicolas requested changes Mar 6, 2021

View reviewed changes

rokopt force-pushed the best-offer-debugging branch from 1feeee3 to 83a95fd Compare March 7, 2021 02:01

rokopt mentioned this pull request Mar 8, 2021

cleanup and merge orderbook not crossed invariant #2417

Closed

jonjove reviewed Mar 10, 2021

View reviewed changes

src/ledger/LedgerTxn.cpp Outdated Show resolved Hide resolved

src/ledger/LedgerTxn.cpp Outdated Show resolved Hide resolved

rokopt added 5 commits March 11, 2021 14:09

Compile-time flag for best-offer debugging

bf30549

Compile in best-offer debugging if extrachecks are enabled.

Set best-offer debugging Config option in tests

13be970

Always use best-offer debugging in `stellar-core test`.

Implement best-offer debugging

ae3dfa0

If best-offer debugging is compiled in and the run-time Config option is set, check the results of getBestOffer() against a new getBestOfferSlow().

rokopt force-pushed the best-offer-debugging branch from 6714930 to ae3dfa0 Compare March 11, 2021 22:09

latobarita merged commit d825052 into stellar:master Mar 12, 2021

v15.4.0 automation moved this from In progress to Done Mar 12, 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

Best-offer debugging #2944

Best-offer debugging #2944

rokopt commented Mar 4, 2021

rokopt commented Mar 4, 2021

sisuresh left a comment

rokopt commented Mar 5, 2021

MonsieurNicolas commented Mar 5, 2021

rokopt commented Mar 5, 2021

rokopt commented Mar 6, 2021

MonsieurNicolas left a comment

MonsieurNicolas Mar 6, 2021

rokopt Mar 7, 2021

MonsieurNicolas Mar 6, 2021

rokopt Mar 7, 2021

rokopt commented Mar 7, 2021

rokopt commented Mar 11, 2021

MonsieurNicolas commented Mar 12, 2021

Best-offer debugging #2944

Best-offer debugging #2944

Conversation

rokopt commented Mar 4, 2021

Best-offer debugging

Checklist

rokopt commented Mar 4, 2021

sisuresh left a comment

Choose a reason for hiding this comment

rokopt commented Mar 5, 2021

MonsieurNicolas commented Mar 5, 2021

rokopt commented Mar 5, 2021

rokopt commented Mar 6, 2021

MonsieurNicolas left a comment

Choose a reason for hiding this comment

MonsieurNicolas Mar 6, 2021

Choose a reason for hiding this comment

rokopt Mar 7, 2021

Choose a reason for hiding this comment

MonsieurNicolas Mar 6, 2021

Choose a reason for hiding this comment

rokopt Mar 7, 2021

Choose a reason for hiding this comment

rokopt commented Mar 7, 2021

rokopt commented Mar 11, 2021

MonsieurNicolas commented Mar 12, 2021