replace providers:Table with providers:SQLiteDatastore

[michaelsbradleyjr]

In a previous team call we discussed #40: the importance of keeping the size of the in-memory providers store bounded as we work to test the correctness and resilience of nim-codex. At the end of the discussion it was realized that, while limiting the in-memory size is important, we need to persist SPRs else the effectiveness of the DHT could be degraded with respect to the overall network.

Such persistence should not be without limits in terms of time on disk and total amount of disk space used, but the implication is that the size of the persisted set should be larger than the amount of memory committed to caching SPRs.

I started work on a PR for #40 by looking at the possibility of an LRU cache that could be used as a layer in a TieredDatastore (of nim-datastore), with SQLiteDatastore as the persistence layer. I also considered a more ad hoc combination of an LruCache (either the type that's included in this repo or the one from status-im/lrucache.nim) and SQLiteDatastore.

Either approach involves difficulties, which I will describe in comments in #40. Quick summary: I think we should attempt to leverage SQLite's Page Cache by adjusting its size, and possibly the page size, to achieve a performance boost (fewer reads from disk) without putting our own cache (written in Nim) in front of the database. Let's have a discussion about that in #40.

The most important changes in this PR are in libp2pdht/private/eth/p2p/discoveryv5/protocol.nim: modifying type Protocol so that providers: Table[NodeId, seq[SignedPeerRecord]] becomes providers: SQLiteDatastore.

An important aspect is that nim-datastore provides a key-value store, while the table being replaced involves a one-to-many relationship.

If customizing usage of the SQLite wrapper provided by (and used by) nim-datastore is an option, there are several possibilities for dealing with one-to-many, including:

• using multiple tables and joins.
• enabling SQLite's JSON support and for each key (CID) storing an array of SPRs that is updated over time.

I chose to stick with vanilla datastore. For each row in the table, the key is a combination of the CID and a fast hash (murmur3) of the corresponding SPR: /[cid]/[spr-hash]. Lookup for all persisted SPRs for a particular CID is performed with query key /[cid]/*. There is a bit of overhead associated with this approach (more bytes stored/retrieved per SPR). We should discuss it in comments below.

With the switch from Table to SQLiteDatastore there is a need for additional error handling. At present (per this PR), those additional non-fatal errors are detected using without or isErr and then trace logged. That approach effectively swallows the errors, but the behavior of the DHT is not changed nor should its stability be reduced. However, we may want to consider switching from trace to error logging so those errors would surface when running production builds.

Limiting the size of the DHT store on disk is naturally related to codex-storage/nim-codex#159 and can be implemented in a future PR.

There is some de-duplication of code in protocol.nim that can be done re: querying the datastore, but I'm waiting on that until we've decided if the approach taken so far is the one we want.

[codecov]

Codecov Report

Base: 76.22% // Head: 76.86% // Increases project coverage by +0.63% 🎉

Coverage data is based on head (6b0057d) compared to base (f824c0a).
Patch coverage: 90.90% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   76.22%   76.86%   +0.63%     
==========================================
  Files          16       16              
  Lines        2385     2455      +70     
==========================================
+ Hits         1818     1887      +69     
- Misses        567      568       +1     

Flag	Coverage Δ
unittests	76.86% <90.90%> (+0.63%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
libp2pdht/private/eth/p2p/discoveryv5/protocol.nim	90.93% <90.32%> (+0.67%)	⬆️
...ibp2pdht/private/eth/p2p/discoveryv5/transport.nim	96.69% <100.00%> (+0.86%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[emizzle]

Overall, really nice direction, well done 👏

Given that SQLite has json support (as mentioned above), it appears we have two options for KVPs (k => v):

1. nodeid+spr.bytes.hash => spr.bytes
2. nodeid => json_object([spr.bytes])

What are the performance considerations for each of these regarding SPR updates (which are usually the most application _ sql transaction intensive). For both options, an SPR update would look like:

Option 1 (nodeid+spr.bytes.hash => spr.bytes)

1. Get record
2. Update record with new SPR
3. Delete old record
4. Insert new record

Option 2 (nodeid => json_object([spr.bytes])

1. GET/UPDATE json array in record with new spr

Note for option 2, we could likely handle the GET/UPDATE in one transaction via sub query:

UPDATE providers SET sprs =
  (SELECT json_replace(x, oldspr, newspr) FROM
    (SELECT sprs FROM providers WHERE nodeid=<nodeid> LIMIT 1) AS x
  )

Where json_replace is the correct SQLite json command for replacing a value in a json array.

[Bulat-Ziganshin]

I think that instead of JSON, we can use just simple sequence of SPRs delimited by any symbol, f.e. chr(0).

Whether record-per-SPR or record-per-CID is preferable depends on access stats. If we mainly read table, then record-per-CID is preferable. If we update often, record-per-SPR is more efficient. Another consideration is the average size of SPR and the average size of SPRs-per-CID list.

Does anyone know the answers to these questions, especially the first one?

Using record-per-CID also should simplify code, allowing us to reuse the existing machinery.

Finally, a cache will have the advantage of not requiring deserializing the found data.

[Bulat-Ziganshin]

Why do we need it at all? Maybe just disable that in the first version? Or use DHT only for CIDs of entire datasets??

[michaelsbradleyjr]

I've created #43 for discussion of a couple of aspects of the persistent providers store.

Mentioning here as well as in the new issue:

As discussed during the team call on Sep 1, changes related to codex-storage/nim-codex#227 will significantly reduce the amount of data kept in the providers store. The implication being that a simpler approach to cache + persistent datastore should be okay.

We should also consider #42 in relation to the persistent store.

[dryajov]

This should definitely be Datastore and it would be initialized with a TieredDatastore , which should in turn contain an LRUDatastore and the SQLiteDatastore. Afaik, we're missing the LRUDatastore, but it should be relatively straight forward to implement one based on the implementation we're using in codex proper. It might even just be a wrapper around it.

[Bulat-Ziganshin]

I would rather make a copy and adapt it to Nim-Datastore API. Eventually, it will be great to move all but Network stores from Codex to Nim-Datastore and drop Codex own blockstore API entirely.

[michaelsbradleyjr]

Good catch re: providers, I meant to change it earlier but forgot.

LRUDatastore is easy to implement by combining with e.g. status-im/lrucache.nim, but there are some things to consider, which is why I held off doing that. I wrote a lot in the PR description above, so will try to summarize:

(1)

Since the Datastore API is not generic, the data parameter/return will need to be seq[byte]. It's not a huge problem, but if the caller is concerned about how much memory the full cache will use, then they will need to have some idea about the length of the data blobs in order to specify an appropriate capacity. Seems clunky, and I thought maybe it's best to wait on LRUDatastore until we make the nim-datastore API generic using vtables approach or however we do it.

(2)

We are dealing with a one (NodeId) to many (SPR) relationship. So how are we going to represent that in a key-value store? In a TieredDatastore setup, the way we choose to do it will impact all of the tiers.

(a)

The way I did it in this PR is to build each key by combining the NodeId (Cid) with a fast hash of the SPR:

/[cid]/[spr-hash]

That converts the one-to-many relationship to one-to-one. But that means that SPR lookup becomes a query call for /[cid]/*.

In a TieredDatastore the only source of truth for query is the bottom tier; for us that would mean skipping the LRU cache and querying SQLite. I didn't come up with the idea of limiting queries to the bottom tier, it's in the doc comments for the original Python datastore. It does make sense, though, because it's hard to understand what it would look like to orchestrate a query through 2+ tiers.

Thinking about that problem is what led me to suggest SQLite's own page cache (configured to our preferred size, cf. codex-storage/nim-datastore#25) might do a good enough job such that we don't need to implement a caching layer in Nim.

(b)

Another way to deal with one-to-many is to store the values (SPRs) in a seq and serialize them as one big blob. That can work, but there are tradeoffs.

LRUDatastore might be too coarse: some NodeIds may have a lot of SPRs, others just a few, but cache eviction only considers the LRU aspect for NodeId.

If we switch to a two-level cache as proposed in #40, then we may have to depart from TieredDatastore. That would be okay, but see #43 or my long comment in #40 about the decision that forces on us.

[michaelsbradleyjr]

In codex-storage/nim-datastore#27 I made changes to support switching providers: SQLiteDatastore to providers: Datastore, and I've already updated this PR to make use of those changes.

A PR introducing LRUDatastore will be submitted shortly.