[ETCM-213] Reload bloom after restart #742

KonradStaniec · 2020-10-16T08:04:01Z

Description

Reload fast sync bloom filter after node restart

Proposed Solution

Expose rockdb iterator over the node storage column and code column and consume it by putting all known keys in bloom filter

Important Changes Introduced

Add LoadabeBloomFilter class which can load data from provided source
Fix bug with NodeStorage. Up until now all nodes were put in Defualt rocks db column by accident, this pr fixes it and node storage now save mpt nodes in correct namespace - this fix break state and requires db cleanup to start

…om-after-restart

lemastero · 2020-10-17T19:29:07Z

After margin [ETCM-141] scalafmt . This PR can be updated by git merge etcm-213/relad-bloom-after-restart-fmt

+629 −202
+528 -138

…om-after-restart

KonradStaniec · 2020-10-20T09:06:58Z

Just fyi: Iterating over 10milion keys and values saved in db, on my machine (ssd drive) take around 8s. Now there is around 150M mpt nodes to save. So taking worst case into account i.e restart neat the finish and ~10s for 10M values, the worst time user will wait to re-lad bloom filter will be 150s which imo it is reasonable time to wait.

Take in mind that in case of loading failure (i.e not loading all nodes from db) will not make state sync unusable but will just degrade its performance due to possible large number of false positives i.e large number of unnecessary db calls. That why error handling is done is in best effort manner. We report the loading failure and error, but we proceed to start state sync without any loading restarts and recovers. We could in theory save the last loaded key before failure, and try to restart iterator from this key but imo for it is not worth it.

and bloom filter of 150m keys, with 3% false positive setting should weigh less 200mb

…om-after-restart

mmrozek

Nice job. One minor comment only

mmrozek · 2020-10-20T14:57:58Z

src/main/scala/io/iohk/ethereum/db/dataSource/RocksDbDataSource.scala

+  private def moveIterator(it: RocksIterator): Observable[Either[IterationError, (Array[Byte], Array[Byte])]] = {
+    Observable
+      .fromTask(Task(it.seekToFirst()))
+      .flatMap(_ => Observable.fromTask(Task(it.isValid)))
+      .flatMap { valid =>
+        if (!valid) {
+          Observable.empty
+        } else {
+          Observable.fromTask(Task(Right(it.key(), it.value()))) ++ Observable
+            .repeatEvalF {
+              Task {
+                it.next()
+              }.flatMap { _ =>
+                if (it.isValid) {
+                  Task(Right(it.key(), it.value()))
+                } else {
+                  Task.raiseError(IterationFinished)
+                }
+              }
+            }
+            .onErrorHandleWith {
+              case IterationFinished => Observable.empty
+              case ex => Observable(Left(IterationError(ex)))
+            }
+        }
+      }
+  }


Maybe sth like that?

private def moveIterator(it: RocksIterator): Observable[Either[IterationError, (Array[Byte], Array[Byte])]] = { Observable .fromTask(Task(it.seekToFirst())) .flatMap(_ => Observable.fromTask(Task(it.isValid))) .filter(identity) .flatMap { _ => Observable.fromTask(Task(Right(it.key(), it.value()))) ++ Observable .repeatEvalF { Task { it.next() }.flatMap { _ => Task(it.isValid).flatMap { case true => Task(Right(it.key(), it.value())) case false => Task.raiseError(IterationFinished) } } } .onErrorHandleWith { case IterationFinished => Observable.empty case ex => Observable(Left(IterationError(ex))) } } }

Yep, second it.isValid shold probably be in Task. I will refactor whole bit in repeatEvalF to be in for-comp

…om-after-restart

src/it/scala/io/iohk/ethereum/db/RockDbIteratorSpec.scala

src/it/scala/io/iohk/ethereum/sync/FastSyncItSpecUtils.scala

kapke · 2020-10-22T08:44:23Z

src/main/scala/io/iohk/ethereum/db/storage/FastSyncStateStorage.scala

@@ -35,6 +35,8 @@ class FastSyncStateStorage(val dataSource: DataSource)

  override def keySerializer: String => IndexedSeq[Byte] = _.getBytes

+  override def keyDeserializer: IndexedSeq[Byte] => String = b => new String(b.toArray)


same as in appstate storage - I'd add charset here

kapke · 2020-10-22T08:45:08Z

src/main/scala/io/iohk/ethereum/db/storage/KnownNodesStorage.scala

@@ -13,6 +13,7 @@ class KnownNodesStorage(val dataSource: DataSource) extends TransactionalKeyValu

  val namespace: IndexedSeq[Byte] = Namespaces.KnownNodesNamespace
  def keySerializer: String => IndexedSeq[Byte] = _.getBytes
+  def keyDeserializer: IndexedSeq[Byte] => String = k => new String(k.toArray)


src/test/scala/io/iohk/ethereum/blockchain/sync/LoadableBloomFilterSpec.scala

kapke · 2020-10-22T08:47:54Z

src/test/scala/io/iohk/ethereum/blockchain/sync/LoadableBloomFilterSpec.scala

+  "LoadableBloomFilter" should "load all correct elements " in testCaseM[Task] {
+    for {
+      source <- Task(Observable.fromIterable(Seq(Right(1L), Right(2L), Right(3L))))
+      filter <- Task.now(LoadableBloomFilter[Long](1000, source))


why not filter = LoadableBloomFilter(...) then? (and below too)

to be honest constant changes between = and <- just makes code unreadable for me, but sure i will change it 👍

kapke · 2020-10-22T08:51:25Z

src/test/scala/io/iohk/ethereum/blockchain/sync/StateSyncSpec.scala

+    override def buildBlockChain(): BlockchainImpl = {
+      val storages = getNewStorages
+      //iterating 1M key and values should force scheduler actor o enqueue last received command i.e StartSyncing
+      (0 until 1000000).foreach { i =>


what about overriding storages in a way that will make source for bloom filter return observable which doesn't emit elements until some signal?

Refactor source iterator Add docs to iterate methods Add explicit charset to storages

…om-after-restart

kapke · 2020-10-26T15:21:56Z

src/main/scala/io/iohk/ethereum/db/storage/StorageStringCharset.scala

+import java.nio.charset.Charset
+
+object StorageStringCharset {
+  val UTF8Charset = Charset.forName("UTF-8")


Minor: there is java.nio.StandardCharsets.UTF8 too

kapke · 2020-10-26T15:28:26Z

src/test/scala/io/iohk/ethereum/blockchain/sync/StateSyncSpec.scala

+        stateStorage = storages.stateStorage
+      ) {
+        override def mptStateSavedKeys(): Observable[Either[IterationError, ByteString]] = {
+          Observable.repeatEvalF(Task(Right(ByteString(1)))).takeWhile(_ => !loadingFinished)


minor: Observable.repeat?

kapke

LGTM!

KonradStaniec added 2 commits October 15, 2020 14:23

[ETCM-213] Expose db iterator over key-vals

a4217e5

[ETCM-213] Update SyncStateActor to load bloom filter at start

a6258da

KonradStaniec added the BREAKS STATE Affects self state retro-compatibility, needs data wiping label Oct 16, 2020

KonradStaniec changed the base branch from etcm-103/restartable-state-sync to develop October 16, 2020 12:24

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

1bdc6b9

…om-after-restart

KonradStaniec added 2 commits October 19, 2020 09:47

[ETCM-213] Properly handle restart while loading bloom filter

116782f

[ETCM-213] Extract bloom loading to sparate class. More tests.

524a463

KonradStaniec marked this pull request as ready for review October 19, 2020 10:59

KonradStaniec requested review from kapke and mmrozek October 19, 2020 10:59

KonradStaniec added 2 commits October 20, 2020 08:11

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

519f4d6

…om-after-restart

[ETCM-213] Fix scalafmt

54e75c1

rtkaczyk mentioned this pull request Oct 20, 2020

[ETCM-77] checkpoint syncing #728

Merged

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

e9833d1

…om-after-restart

KonradStaniec force-pushed the etcm-213/relad-bloom-after-restart branch from a1de42a to e9833d1 Compare October 20, 2020 12:55

mmrozek approved these changes Oct 20, 2020

View reviewed changes

KonradStaniec added 2 commits October 21, 2020 10:10

[ETCM-213] Refactor iterator impl

e4029e3

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

cc0cd99

…om-after-restart

kapke reviewed Oct 22, 2020

View reviewed changes

KonradStaniec added 7 commits October 22, 2020 13:18

[ETCM-213] Pr comments

6642568

Refactor source iterator Add docs to iterate methods Add explicit charset to storages

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

5d37321

…om-after-restart

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

6e7bb45

…om-after-restart

[ETCM-213] Fix merge conflicts

29ca388

[ETCM-213] Add one todo regarding async processing

ce8a83f

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

f46fd50

…om-after-restart

Merge remote-tracking branch 'origin/develop' into etcm-213/relad-blo…

1667f33

…om-after-restart

kapke reviewed Oct 26, 2020

View reviewed changes

kapke approved these changes Oct 26, 2020

View reviewed changes

KonradStaniec merged commit 1667f33 into develop Oct 26, 2020

KonradStaniec deleted the etcm-213/relad-bloom-after-restart branch October 26, 2020 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-213] Reload bloom after restart #742

[ETCM-213] Reload bloom after restart #742

KonradStaniec commented Oct 16, 2020 •

edited

Loading

lemastero commented Oct 17, 2020

KonradStaniec commented Oct 20, 2020 •

edited

Loading

mmrozek left a comment

mmrozek Oct 20, 2020

KonradStaniec Oct 21, 2020

kapke Oct 22, 2020

kapke Oct 22, 2020

kapke Oct 22, 2020

KonradStaniec Oct 22, 2020

kapke Oct 22, 2020

kapke Oct 26, 2020

kapke Oct 26, 2020

kapke left a comment

		@@ -35,6 +35,8 @@ class FastSyncStateStorage(val dataSource: DataSource)

		override def keySerializer: String => IndexedSeq[Byte] = _.getBytes

		override def keyDeserializer: IndexedSeq[Byte] => String = b => new String(b.toArray)

[ETCM-213] Reload bloom after restart #742

[ETCM-213] Reload bloom after restart #742

Conversation

KonradStaniec commented Oct 16, 2020 • edited Loading

Description

Proposed Solution

Important Changes Introduced

lemastero commented Oct 17, 2020

KonradStaniec commented Oct 20, 2020 • edited Loading

mmrozek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kapke left a comment

Choose a reason for hiding this comment

KonradStaniec commented Oct 16, 2020 •

edited

Loading

KonradStaniec commented Oct 20, 2020 •

edited

Loading