Tendermint tmlibs and common dependencies

[silasdavis]

tmlibs and common are core dependencies of Tendermint containing a lot of somewhat independent code. Aggregating this code does make a lot of sense for versioning Tendermint, but for other projects like this one creates an undesirable level of coupling that could be avoided. I would like to propose that we drop the dependency on both. This would make it possible to upgrade Tendermint and iavl independently without forcing an upgrade of one or the other, which is the issue I am running into. I would argue that the code dependencies that iavl has on tmlibs and common are trivial, and particularly in the latter case is not pulling its weight.

tmlibs is currently imported for a subset of the DB interface and NewMemDB. This is convenient but means we pull in an entire dependency of tmlibs which makes it hard to use independently with Tendermint. Instead we could replicate the portion of the DB interface we need (Get, Set, NewBatch, and Iterator). MemDB is pretty trivial and the implementation against this subset interface would be even easier. This also adds a significant advantage for extensibility and mocking int that NodeDB would only be asking for what it actually needs - which is not the entire DB interface (e.g. https://blog.chewxy.com/2018/03/18/golang-interfaces/). I think this combined with decoupling from Tendermint/tmlibs is probably a good enough reason to accept the small amount of duplication. Admittedly iavl does depend on the semantics of the DB interface, but I think we could make clear that you may want use tmlibs in your iavl-based application - you would also have the flexibility to use a previous version with this subset interface (I think - and if not it would easier to make an adapter).

common is used for HexBytes, RandStr, Fmt, and PanicSanity. I think this is one of those cases where being DRY is the wrong thing to do. These are all relatively low value methods and don't seem to me to justify the dependency. They could either be replicated or removed. I feel like HexBytes is a convention that should belong to parent projects, RandStr introduces non-determinism into the unit tests which is not ideal, and Fmt and PanicSanity add very little.

[ebuchman]

I'm in strong agreement.

Would you be open to make a PR to remove the dependencies?

Only thing is that it should be made very clear that the expect dbs come from tmlibs/db, but it shouldn't be a problem to replicate the parts of the interface we need and the MemDB itself within this repo.

[jaekwon]

<open debate>

The DB interfaces are stable and useful. We should also consider using more of it and decoupling it from tmlibs. The pain is partially due to us packaging many dependencies together. As packages (like db) mature I think we should move the out, and db is ripe. (it benefited by being developed in tandem with the SDK and Tendermint, so I'm not knocking tmlibs in any way).

I'd extend Dave's blog post with useful entangled interface sets like our tmlibs/db or https://golang.org/pkg/crypto/ . They are most useful/ergonomic when entangled (for as long as they are well designed and consistent), and they don't suffer from the smells that he's talking about. He mentioned recursive interfaces but I think entangled interfaces are the more general exceptional case.

And this is also due to the design constraints of Golang where isometric interfaces aren't identical. If they could be used interchangeably then this wouldn't be an issue. https://play.golang.org/p/mUBkkbQPz8_s

I'm more concerned about us making this proposed change without fully considering the reasons/heuristics, than the proposed change itself. It's easy enough to pull the DB.Iterator method out and define a new func Iterator2 ... in iavl where we can just pass in the db.Iterator method as a function. I just don't think it's necessary in this case, (and arguably it's more awkward to separate an iterator like this from the context of a db connection) and we ought to be spinning db out of tmlibs anyways, which would solve the problem.

Well designed interfaces like the ones in DB are hard earned... Once they've matured IMO we benefit by using them more, not less. I'm sure we'll find better examples too in the future.

[jaekwon]

I'm also on a mission to replace all instances of pkg/errors in all of our codebases, starting with IAVL, to this system: tendermint/tmlibs#220 , so eventually that will have to spin out of tmlibs as well. That can wait though, we can use pkg/errors for now.

[silasdavis]

The DB interfaces are stable and useful. We should also consider using more of it and decoupling it from tmlibs.

I agree with this. Extracting from tmlilb substantially solves the issue.

In terms of enhanced (de)composition of the db interface. As you say it's an annoyance of Go's type system that we can't do this as it. It seems like we could pull out NewBatch NewReverseIterator and NewIterator out of the main interface and provide them as standalone functions/interfaces. There is no coupling between dependent interfaces. We get the same hard-won interface and there is nothing to stop you from forming a superset interface.

I have regularly thought it would have been useful to implement an interception layer for IAVL just for the subset of DB it actually uses but implementing the parts it doesn't use felt onerous. Examples I recall are:

• Implementing a content-addressed layer against existing implementation
• Adding a sketch-based cache layer
• Providing a 'pseudo batch' to IAVL so I can coordinate other state updates with IAVL's commit

I think asking for a minimal interface is a good heuristic in go anyway, but to give some context.

I can see how defining the minimal interface at the consumer (e.g. IAVL) could lead to fragmentation though - and there is an advantage to centralising the component interfaces (in a db lib). There's less going on but here is the kind of approach I have taken: https://github.com/hyperledger/burrow/blob/develop/account/state/state.go

We could have the same interface as currently exists as a composition, but have get-set, get-set-sync, batch, iterator, etc as separate component interfaces. IAVL could compose the ones it needs. Could try and make this more concrete with a PR in IAVL as a prototype if it would help.

[silasdavis]

To add to this now IAVL has a dependency on Tendermint - for tendermint/libs/common and tendermint/crypto/tmhash. This is quite painful for upgrades since I dep now wants me to have compatible tendermint and IAVL versions.

It looks like this dependency is mostly for errors.Wrap and some straight-forward hash conventions that I feel could easily be copied. Is it really worth the dependency?

[silasdavis]

Having had a play with this - I agree with @jaekwon that there isn't really a nice way to extract the Iterator methods so that we could benefit from isomorphic interfaces. Is anyone from Tendermint in a position to extract db lib (let's not call it go-db :) )? We also do make use of the goleveldb backend for benchmarking which is not something we would want to duplicate (a version of memdb - maybe...) so I think a separate lib is the right answer here. I think the db lib will be a useful integration point for building other things (similar to the way IAVL is) so I'd love to see it spun out as 'sqlx' style layer for key-value stores in Go.

I'm up for making a PR that removes the other more trivial Tendermint dependencies and we could see how that looks in the meantime?

[ValarDragon]

Spinning of this db stuff is a fairly large design choice, we have chosen to adopt the monorepo style for better utilization of developer time. This would mean breaking off from it. We were actually discussing moving IAVL to the Cosmos-SDK as well.

I'm not opposed to this iff the DB stuff is very stable.

[silasdavis]

Personally I would like to see IAVL remain a separate library, but it is the worst of all worlds to have it in a separate repo but have dependencies on something has a heavy as Tendermint, so I suppose if that is the way it goes it would make sense to roll it into the cosmos-sdk.

It was a nightmare having half a dozen dependency libraries I agree, and particularly since in reality only Tendermint depended on them - they weren't mature and moved too quickly for that to make sense. So stability is one factor, but another one is if you genuinely have multiple independent things depending on a library. For the db libs you currently have at least: Tendermint, Burrow, IAVL, and the cosmos-sdk.

As @jaekwon says above:

The pain is partially due to us packaging many dependencies together. As packages (like db) mature I think we should move the out, and db is ripe.

I'm inclined to agree. I don't think the DB is necessarily 'very stable' - everything that depends on it is in principle alpha software! However I do think it is relatively more stable and it has real dependencies (and could attract more as a key-value db abstraction layer if maintained separately from Tendermint which I think would be for the good).

[ebuchman]

Probably would be good to see db move into its own repo eventually, but I don't think it's ready yet. Some upcoming changes:

• possible issues with ReverseIterator (prefixStore does not handle iteration correctly for nil endpoints cosmos-sdk#2243)
• experimenting with backends to improve performance (BlockIntervalSeconds metric & DB tests cleanup & benchmark improv. tendermint/tendermint#2337, [WIP] Fix goleveldb perf degradation tendermint/tendermint#2370).

Meanwhile, it looks like IAVL is taking on some more dependence on Tendermint for the general merkle proof stuff - not quite sure how to resolve that, and where the general merkle proof stuff should live, but if it's going to be in Tendermint and the IAVL needs it, then maybe IAVL should move to Tendermint too.

Ideally, nothing needs to move, and the IAVL just needs to implement some interfaces and maybe duplicate a bit of code, but otherwise doesn't have dependencies on Tendermint at all. So hopefully we can work towards that.

[tac0turtle]

Update: the only thing that is being used from tendermint/tendermint now is crypto, hopefully in the near future we can remove this as well.

[tac0turtle]

for the time being, we will keep tendermint as a dep because of tendermint/crypto, if you want it to be entirely removed, please reopen this issue