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

cleanup and merge orderbook not crossed invariant #2417

Closed
MonsieurNicolas opened this issue Feb 4, 2020 · 3 comments · Fixed by #2959
Closed

cleanup and merge orderbook not crossed invariant #2417

MonsieurNicolas opened this issue Feb 4, 2020 · 3 comments · Fixed by #2959

Comments

@MonsieurNicolas
Copy link
Contributor

#2210

@MonsieurNicolas MonsieurNicolas added this to To do in v15.3.0 via automation Jan 21, 2021
@MonsieurNicolas
Copy link
Contributor Author

this implements one of #2884

@MonsieurNicolas MonsieurNicolas added this to To do in v15.4.0 via automation Feb 15, 2021
@MonsieurNicolas MonsieurNicolas removed this from To do in v15.3.0 Feb 15, 2021
@rokopt
Copy link
Contributor

rokopt commented Mar 8, 2021

A couple notes/questions:

  • In the current OrderBookIsNotCrossed invariant #2210 , the new invariant is compiled in and enabled when and only when we compile for fuzzing (FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION). Should we instead do this as we did the best-offer debugging (Best-offer debugging #2944 ) and compile it in when and only when extrachecks are enabled, and use a new Config parameter to make sure it's only used during stellar-core fuzz and stellar-core test (not, for example, stellar-core run or stellar-core catchup)?
  • The invariant as is can fire incorrectly because it doesn't know about passive offers. I think that the right fix is to test if (lowestAsk == highestBid) in checkCrossed() and in that case to fire the assertion only if there is at least one non-passive ask at that price and at least one non-passive bid at that price. But please let me know if that doesn't sound right!

@rokopt
Copy link
Contributor

rokopt commented Mar 9, 2021

  • In the current OrderBookIsNotCrossed invariant #2210 , the new invariant is compiled in and enabled when and only when we compile for fuzzing (FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION). Should we instead do this as we did the best-offer debugging (Best-offer debugging #2944 ) and compile it in when and only when extrachecks are enabled, and use a new Config parameter to make sure it's only used during stellar-core fuzz and stellar-core test (not, for example, stellar-core run or stellar-core catchup)?

After further thought, I realize that we can't do that with the order-book-not-crossed invariant: unlike other invariants, it maintains state (mOrderBook) from one call to checkOnOperationApply() to another. There's no provision for that in InvariantManager, and in particular it won't work because the order-book invariant won't know if the transaction whose operations had been applied were later rolled back. Consequently, it might think (in its mOrderBook) that there's an offer present which isn't, so it might spuriously believe that the order book is crossed when some new offer appears that would have crossed with that offer if it really still existed.

@MonsieurNicolas MonsieurNicolas moved this from To do to In progress in v15.4.0 Mar 17, 2021
@MonsieurNicolas MonsieurNicolas removed this from In progress in v15.4.0 Mar 27, 2021
@MonsieurNicolas MonsieurNicolas added this to To do in v17.0.0 via automation Mar 27, 2021
v17.0.0 automation moved this from To do to Done Mar 30, 2021
@MonsieurNicolas MonsieurNicolas added this to To do in v15.5.0 via automation Apr 6, 2021
@MonsieurNicolas MonsieurNicolas removed this from Done in v17.0.0 Apr 6, 2021
@MonsieurNicolas MonsieurNicolas moved this from To do to Done in v15.5.0 Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants