Coredb fix kvt save only fringe condition (#2592)

* Cosmetics, spelling, etc.

* Aristo: make sure that a save cycle always commits even when empty

why:
  If `Kvt` is tied to the `Aristo` DB save cycle, then this save cycle
  must also be committed if there is no data to save for `Aristo`.

  Otherwise this will lead to excessive core memory use with some fringe
  condition where Eth headers (or blocks) are downloaded while syncing
  and not really stored on disk.

* CoreDb: Correct persistent save mode

why:
  Saving `Kvt` first is seen as a harbinger (or canary) for `Aristo` as
  both run in sync. If `Kvt` succeeds saving first, so must be `Aristo`
  next. Other than this is a defect.

nimbus/db/aristo/aristo_delta.nim

@@ -13,16 +13,12 @@
 ##
 
 import
-  std/[strutils, tables],
+  std/tables,
-  chronicles,
   eth/common,
   results,
   ./aristo_delta/[delta_merge, delta_reverse],
   ./aristo_desc/desc_backend,
-  "."/[aristo_desc, aristo_get, aristo_layers, aristo_utils]
+  "."/[aristo_desc, aristo_layers]
-
-logScope:
-  topics = "aristo-delta"
 
 # ------------------------------------------------------------------------------
 # Public functions, save to backend
@@ -55,6 +51,15 @@ proc deltaPersistent*(
 
   # Blind or missing filter
   if db.balancer.isNil:
+    # Add a blind storage frame. This will do no harm if `Aristo` runs
+    # standalone. Yet it is needed if a `Kvt` is tied to `Aristo` and has
+    # triggered a save cyle already which is to be completed here.
+    #
+    # There is no need to add a blind frame on any error return. If there
+    # is a `Kvt` tied to `Aristo`, then it must somehow run in sync and an
+    # error occuring here must have been detected earlier when (implicitely)
+    # registering `Kvt`. So that error should be considered a defect.
+    ? be.putEndFn(? be.putBegFn())
     return ok()
 
   # Make sure that the argument `db` is at the centre so the backend is in

nimbus/db/aristo/aristo_init/rocks_db.nim

@@ -80,7 +80,7 @@ proc getVtxFn(db: RdbBackendRef): GetVtxFn =
       # Fetch serialised data record
       let vtx = db.rdb.getVtx(rvid).valueOr:
         when extraTraceMessages:
-          trace logTxt "getVtxFn() failed", vid, error=error[0], info=error[1]
+          trace logTxt "getVtxFn() failed", rvid, error=error[0], info=error[1]
         return err(error[0])
 
       if vtx.isValid:
@@ -95,7 +95,7 @@ proc getKeyFn(db: RdbBackendRef): GetKeyFn =
       # Fetch serialised data record
       let key = db.rdb.getKey(rvid).valueOr:
         when extraTraceMessages:
-          trace logTxt "getKeyFn: failed", vid, error=error[0], info=error[1]
+          trace logTxt "getKeyFn: failed", rvid, error=error[0], info=error[1]
         return err(error[0])
 
       if key.isValid:
@@ -207,8 +207,6 @@ proc putEndFn(db: RdbBackendRef): PutEndFn =
           case hdl.error.pfx:
           of VtxPfx, KeyPfx: trace logTxt "putEndFn: vtx/key failed",
             pfx=hdl.error.pfx, vid=hdl.error.vid, error=hdl.error.code
-          of FilPfx: trace logTxt "putEndFn: filter failed",
-            pfx=FilPfx, qid=hdl.error.qid, error=hdl.error.code
           of AdmPfx: trace logTxt "putEndFn: admin failed",
             pfx=AdmPfx, aid=hdl.error.aid.uint64, error=hdl.error.code
           of Oops: trace logTxt "putEndFn: oops",

nimbus/db/core_db/base.nim

@@ -166,8 +166,7 @@ proc `$$`*(e: CoreDbError): string =
 proc persistent*(
     db: CoreDbRef;
     blockNumber: BlockNumber;
-      ): CoreDbRc[void]
+      ): CoreDbRc[void] =
-      {.discardable.} =
   ## This function stored cached data from the default context (see `ctx()`
   ## below) to the persistent database.
   ##
@@ -187,16 +186,11 @@ proc persistent*(
       else:
         result = err(rc.error.toError $api)
         break body
-    block:
+    # Having reached here `Aristo` must not fail as both `Kvt` and `Aristo`
-      let rc = CoreDbAccRef(db.ctx).call(persist, db.ctx.mpt, blockNumber)
+    # are kept in sync. So if there is a legit fail condition it mist be
-      if rc.isOk:
+    # caught in the previous clause.
-        discard
+    CoreDbAccRef(db.ctx).call(persist, db.ctx.mpt, blockNumber).isOkOr:
-      elif CoreDbAccRef(db.ctx).call(level, db.ctx.mpt) != 0:
+      raiseAssert $api & ": " & $error
-        result = err(rc.error.toError($api, TxPending))
-        break body
-      else:
-        result = err(rc.error.toError $api)
-        break body
     result = ok()
   db.ifTrackNewApi: debug logTxt, api, elapsed, blockNumber, result
 

nimbus/db/kvt/kvt_init/rocks_db/rdb_desc.nim

@@ -24,7 +24,7 @@ type
     session*: WriteBatchRef          ## For batched `put()`
 
     basePath*: string                ## Database directory
-    delayedPersist*: KvtDbRef        ## Enable next proggyback write session
+    delayedPersist*: KvtDbRef        ## Enable next piggyback write session
 
   KvtCFs* = enum
     ## Column family symbols/handles and names used on the database