services/horizon: Update in memory orderbook graph using Horizon DB instead of ingestion

[tamirms]

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

What

Close #2250

• Add DB functions to query updated offers and removed offers
• The query to find removed offers searches for all "Offer Removed" effects. But, we currently neglect to ingest these effects (see Offer Created, Offer Updated, Offer Removed effects #166 ). So, I have updated the effects processor to create "Offer Removed" effects whenever an offer is consumed or canceled.
• Implement OrderBookStream which polls the horizon db for created, updated, and removed offers and applies those changes to an in memory order book graph.
• Verify that the in memory order book graph produced by OrderBookStream is consistent with the in memory order book graph populated by ingestion. Note that we may remove this verification code later.
• Remove IngestInMemoryOnly flag and use OrderBookStream to provide an order book graph which can be used by request serving horizon instances.
• Remove in memory ingestion code paths.

Why

See discussion in #2250 for motivation.

Known limitations

• OrderBookStream assumes that offer ids are globally unique. In other words, if an offer is created and later removed it is not possible for another offer to be created in the future with the same id. I believe that this assumption is consistent with Stellar Core behavior but I'm asking the Stellar Core team to be sure.
  [edit: verified with jon that "offerIDs are unique in the sense that if an offer is removed, that offerID will never be reused."]

• I still need to add tests for OrderBookStream and the EffectsProcessor. I will do that when I receive a 👍 on the overall approach / design.

[bartekn]

I finished the first pass and I think this approach looks good to me. I added a couple inline comments.

I think that order book graph dependency on effects is not ideal. We have Horizon plugins on the roadmap what means that in the future Horizon could start path finding plugin without ingesting history (and effects). Because we'll have to do decoupling eventually maybe it's worth doing it now? To propose an alternative: maybe we can just store ledger entry changes for offers in a table that gets flushed every couple minutes?

If we are going to stick with effects for any reason I think it's worth adding created and updated effects too.

[abuiles]

👍 - would it make sense to split the effects and order book changes into different PRs?

[tamirms]

I think that order book graph dependency on effects is not ideal. We have Horizon plugins on the roadmap what means that in the future Horizon could start path finding plugin without ingesting history (and effects). Because we'll have to do decoupling eventually maybe it's worth doing it now? To propose an alternative: maybe we can just store ledger entry changes for offers in a table that gets flushed every couple minutes?

@bartekn
Is there a reason besides plugins for why we shouldn't be using effects? Plugins have probably been in the Horizon roadmap for over a year. I wouldn't bet that we will start working on plugins any time soon.

I could store ledger entry changes in a new table but I thought that having Offer Removed effects would actually be useful for people using our API. We have received questions about how to find offers which have been removed from the order book.

If there is a performance reason to avoid effects I can create a new table just to store removed offer ids. However, if the hesitation to add new effects is based on plugins alone then I don't think that's a valid concern.

[bartekn]

Is there a reason besides plugins for why we shouldn't be using effects? Plugins have probably been in the Horizon roadmap for over a year. I wouldn't bet that we will start working on plugins any time soon.

I'm not against the new effects. I just feel we should decouple two components (history-effects and order book graph). Maybe you're right that the plugins won't be here for a long time. Even if we want to work on decoupling in the future it shouldn't be that hard. So at this point I think my only concerns are: a) adding just a single offer effect type (we can add more in a separate PR but would be great to have all offer effects within a single Horizon version), b) #2630 (comment).

[tamirms]

@bartekn I just thought of a way to record deleted offers without using effects which is much cleaner. I'll update the PR with the new approach

[tamirms]

@bartekn can you take another look at the PR? I have changed the approach so that we don't rely on effects. Let me know if you have concerns about the new design

[bartekn]

OK, this is great!

I wonder about one last thing. It seems that we actually maintain two graphs on ingesting instances: one updated using OrderbookProcessor and one using OrderBookStream. I understand that we do so to be able to verify graph on non-ingesting instance. I think we can actually remove OrderbookProcessor and update just one graph using OrderBookStream.

To verify it we can just periodically compare offers in graph to those in a DB (and offers in DB are verified using buckets so we can be sure are correct because equality is transitive).

This can be done in a separate PR if you agree with the idea.

[tamirms]

I wonder about one last thing. It seems that we actually maintain two graphs on ingesting instances: one updated using OrderbookProcessor and one using OrderBookStream. I understand that we do so to be able to verify graph on non-ingesting instance. I think we can actually remove OrderbookProcessor and update just one graph using OrderBookStream.

yes, I planned on making this change in the next horizon release. I wanted to run the current verification routine for a week or so in staging or production before replacing it with a comparison with the offers table.

[tamirms]

@bartekn the PR is now ready for a full review, can you take another look? I have cleaned up the code and added all the missing tests.