Initial full sync impl #117

yglukhov · 2019-02-18T10:46:02Z

Some notes:

The idea of "separate requestor/receiver" appeared to have another "flaw", namely suboptimal bandwidth control (we want to request the next batch of data as soon as one of the previous requests is done). At this point I felt there are too many nuances in the approach that slowly drive its complexity towards or even above the "classical" one for no significant benefit. cc @arnetheduck
The simulation is pretty broken right now, so it's hard to debug the sync.

zah · 2019-02-18T11:23:16Z

beacon_chain/sync_protocol.nim

+        # TODO: drop this peer
+        assert(false)
+
+      let db = peer.network.protocolState(BeaconSync).db


This could be peer.networkState.db

zah · 2019-02-18T11:31:25Z

beacon_chain/sync_protocol.nim

+
+    let blks = await p.getBlocks(node.headBlockRoot, numBlocksToDownload.int)
+    if blks.isSome:
+      node.applyBlocks(blks.get.blocks)


It would be nice leave some placeholders for the required error handling logic here.

arnetheduck · 2019-02-18T13:47:23Z

beacon_chain/sync_protocol.nim

+        var slot = int64(blk.slot)
+        let targetSlot = slot + step * (num - 1)
+        while slot != targetSlot:
+          if slot < 0 or not db.getBlock(uint64(slot), blk):


this operation looks odd - how can the database know the block of a particular slot? that requires following the fork choice, and it might change over time..

arnetheduck · 2019-02-18T14:16:13Z

we can start without it, but what will soon happen is that we'll have a tightly coupled logic that will be hard to debug and improve, because the effect of pulling on one piece of code will be hard to analyze - ie if all the responsibilities are intertwined, with no API between them to regulate the flow of information, it's just a matter of time before the code becomes about as reliable as the nim compiler..

the idea with the design is not to remove all information flow between the layers - it's to start with the assumption that "no information should flow" and gradually weaken that to arrive at a minimal design where all information flows for a well defined reason and where the complexity price is motivated by actual benefits, rather than starting with "everything is tangled" and trying to untangle from there

yglukhov · 2019-03-01T15:02:13Z

The sync seems to work now, but it relies on the RLP serialization fix, which breaks nimble test in a mysterious way.

arnetheduck · 2019-03-01T18:33:41Z

beacon_chain/sync_protocol.nim

+    node*: BeaconNode
+    db*: BeaconChainDB
+
+func toHeader(b: BeaconBlock): BeaconBlockHeader =


any reason for not using construction syntax? ie toHeader(b): .. = BeaconBlockHeader(slot: b.slot, ...)?

No reason really. Somehow I keep forgetting about this option :)

yglukhov · 2019-03-01T20:10:16Z

Should be good to go

zah · 2019-03-02T08:46:37Z

beacon_chain/beacon_chain_db.nim

-    array[N + 1, byte] =
-  result[0] = byte ord(kind)
-  result[1 .. ^1] = key
+func subkey[T](kind: DbKeyKind, key: T): auto =


Isn't this function a bit dangerous? I can pass a string by accident and it will do the wrong thing. Taking openarray[byte] seems a bit safer.

Good point. Added an overload instead.

zah · 2019-03-02T08:50:09Z

beacon_chain/spec/crypto.nim


 proc append*(writer: var RlpWriter, value: ValidatorSig) =
  writer.append value.getBytes()

 proc read*(rlp: var Rlp, T: type ValidatorSig): T {.inline.} =
-  ValidatorSig.init rlp.toBytes.toOpenArray
+  let r = rlp.read(seq[byte])


The version using toBytes doesn't perform an allocation. Was it wrong somehow?

Yeah, it wasn't advancing the read cursor. Fixed it differently.

zah · 2019-03-02T08:52:16Z

beacon_chain/sync_protocol.nim

+      bestRoot: Eth2Digest # TODO
+      bestSlot: uint64 = node.state.data.slot
+
+    await peer.status(protocolVersion, networkId, latestFinalizedRoot, latestFinalizedEpoch,


This should use the new handshake proc that gets a number of tricky details right, but I'll fix it myself in a later commit.

arnetheduck · 2019-03-04T20:05:19Z

beacon_chain/beacon_chain_db.nim

+proc putBlock*(db: BeaconChainDB, key: Eth2Digest, value: BeaconBlock) =
+  let slotKey = subkey(kSlotToBlockRoots, value.slot)
+  var blockRootsBytes = db.backend.get(slotKey)
+  var blockRoots = blockRootsBytes.toSeq(Eth2Digest)


technically could just use ssz here no?

Sure. Just... why?

for simplicity so we just have one serialization format in db (and reuse the code for flattening a seq)

Well, I don't have a strong opinion, but a few reasons I like my approach more:

The serialization format is localized to the beacon_db, so not a lot to keep in mind

My format is pretty much memcpy which can easily be optimized to the least possible amount of allocations, thus more efficient.

I was more curious than anything.. I wouldn't have bothered with coming up with a special serialization for it (and the additional unsafe code that comes with). I get the feeling the real optimization here will be to drop rocksdb at some point.

arnetheduck · 2019-03-04T20:08:21Z

beacon_chain/sync_protocol.nim

+  )
+
+proc fromHeader(b: var BeaconBlock, h: BeaconBlockHeader) =
+  b.slot = h.slot


humm, this looks a bit dangerous in that it lets BeaconBlock.data keep its old value.. b = BeaconBlock(...)?

or ideally take both a body and a header, so it's impossible to forget to set data..

Good point. Will do.

arnetheduck · 2019-03-04T20:11:35Z

beacon_chain/sync_protocol.nim

+    await peer.status(protocolVersion, networkId, latestFinalizedRoot, latestFinalizedEpoch,
+        bestRoot, bestSlot)
+
+    let m = await peer.nextMsg(BeaconSync.status)


what happens here if peer sends something different, or doesn't send anything at all?

Different dispatchers will be called (e.g. getBeaconBlockHeaders, getBeaconBlockBodies, etc), while this coroutine will just hang forever, until the peer is disconnected, and nextMsg eventually raises.

arnetheduck · 2019-03-04T20:13:02Z

beacon_chain/sync_protocol.nim

+
+  requestResponse:
+    proc getBeaconBlockHeaders(peer: Peer, blockRoot: Eth2Digest, slot: uint64, maxHeaders: int, skipSlots: int) =
+      # TODO: validate maxHeaders


TODO and implement skipSlots

…#117)

zah approved these changes Feb 18, 2019

View reviewed changes

arnetheduck reviewed Feb 18, 2019

View reviewed changes

yglukhov force-pushed the sync2 branch from 64f5a5b to 3bf3269 Compare February 19, 2019 12:42

mratsim mentioned this pull request Feb 19, 2019

Fix yet another epoch slot issue #123

Closed

yglukhov force-pushed the sync2 branch 4 times, most recently from e72a1a3 to 994c787 Compare February 25, 2019 13:14

yglukhov force-pushed the sync2 branch 4 times, most recently from 2b4365e to 8058079 Compare March 1, 2019 14:59

Initial full sync impl

bed6510

yglukhov force-pushed the sync2 branch 2 times, most recently from f82cab3 to bed6510 Compare March 1, 2019 18:01

arnetheduck reviewed Mar 1, 2019

View reviewed changes

Comments addressed

4c31f50

yglukhov changed the title ~~WIP: Initial full sync impl~~ Initial full sync impl Mar 1, 2019

zah reviewed Mar 4, 2019

View reviewed changes

Comments addressed

9ea0bf4

zah approved these changes Mar 4, 2019

View reviewed changes

yglukhov merged commit 21e4deb into master Mar 4, 2019

yglukhov deleted the sync2 branch March 4, 2019 19:05

arnetheduck reviewed Mar 4, 2019

View reviewed changes

arnetheduck mentioned this pull request Mar 4, 2019

Phase 0 Wire Protocol ethereum/consensus-specs#692

Closed

yglukhov added a commit that referenced this pull request Mar 5, 2019

Comments on #117 addressed

b1d2c05

arnetheduck pushed a commit that referenced this pull request Mar 5, 2019

Comments on #117 addressed (#154)

38f4875

etan-status pushed a commit that referenced this pull request May 12, 2022

Produce an error when the result variable is used in void async procs (…

0d4d000

…#117)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial full sync impl #117

Initial full sync impl #117

yglukhov commented Feb 18, 2019

zah Feb 18, 2019

zah Feb 18, 2019

arnetheduck Feb 18, 2019

arnetheduck commented Feb 18, 2019

yglukhov commented Mar 1, 2019

arnetheduck Mar 1, 2019

yglukhov Mar 1, 2019

yglukhov commented Mar 1, 2019

zah Mar 2, 2019

yglukhov Mar 4, 2019

zah Mar 2, 2019

yglukhov Mar 4, 2019

zah Mar 2, 2019

arnetheduck Mar 4, 2019

yglukhov Mar 5, 2019

arnetheduck Mar 5, 2019

yglukhov Mar 5, 2019

arnetheduck Mar 5, 2019

arnetheduck Mar 4, 2019

arnetheduck Mar 4, 2019

yglukhov Mar 5, 2019

arnetheduck Mar 4, 2019

yglukhov Mar 5, 2019

arnetheduck Mar 4, 2019

Initial full sync impl #117

Initial full sync impl #117

Conversation

yglukhov commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck commented Feb 18, 2019

yglukhov commented Mar 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yglukhov commented Mar 1, 2019

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

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