From b35584632b45f6706c45d95bcf439f44f9083e4b Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Mon, 6 Jun 2022 15:56:59 +0200 Subject: [PATCH] sync: remove `step` from sync client implementation (#3678) * sync: remove `step` from sync client implementation Deprecated in the spec: https://github.com/ethereum/consensus-specs/pull/2856 - future PR:s will deprecate server support as well. --- beacon_chain/sync/sync_manager.nim | 4 +- beacon_chain/sync/sync_protocol.nim | 4 ++ beacon_chain/sync/sync_queue.nim | 31 +++++------ tests/test_sync_manager.nim | 79 +++++------------------------ 4 files changed, 31 insertions(+), 87 deletions(-) diff --git a/beacon_chain/sync/sync_manager.nim b/beacon_chain/sync/sync_manager.nim index 06ce8e7f4..64b0fe8b7 100644 --- a/beacon_chain/sync/sync_manager.nim +++ b/beacon_chain/sync/sync_manager.nim @@ -167,9 +167,9 @@ proc getBlocks*[A, B](man: SyncManager[A, B], peer: A, try: let res = if peer.useSyncV2(): - await beaconBlocksByRange_v2(peer, req.slot, req.count, req.step) + await beaconBlocksByRange_v2(peer, req.slot, req.count, 1) else: - (await beaconBlocksByRange(peer, req.slot, req.count, req.step)).map( + (await beaconBlocksByRange(peer, req.slot, req.count, 1)).map( proc(blcks: seq[phase0.SignedBeaconBlock]): auto = blcks.mapIt(newClone(ForkedSignedBeaconBlock.init(it)))) diff --git a/beacon_chain/sync/sync_protocol.nim b/beacon_chain/sync/sync_protocol.nim index f3157c532..2b1c34807 100644 --- a/beacon_chain/sync/sync_protocol.nim +++ b/beacon_chain/sync/sync_protocol.nim @@ -286,6 +286,8 @@ p2pProtocol BeaconSync(version = 1, # client call that returns `seq[ref SignedBeaconBlock]` will # will be generated by the libp2p macro - we guarantee that seq items # are `not-nil` in the implementation + # TODO reqStep is deprecated - future versions can remove support for + # values != 1: https://github.com/ethereum/consensus-specs/pull/2856 trace "got range request", peer, startSlot, count = reqCount, step = reqStep if reqCount == 0'u64 or reqStep == 0'u64: @@ -424,6 +426,8 @@ p2pProtocol BeaconSync(version = 1, # client call that returns `seq[ref ForkedSignedBeaconBlock]` will # will be generated by the libp2p macro - we guarantee that seq items # are `not-nil` in the implementation + # TODO reqStep is deprecated - future versions can remove support for + # values != 1: https://github.com/ethereum/consensus-specs/pull/2856 trace "got range request", peer, startSlot, count = reqCount, step = reqStep diff --git a/beacon_chain/sync/sync_queue.nim b/beacon_chain/sync/sync_queue.nim index eee74412d..108eebd56 100644 --- a/beacon_chain/sync/sync_queue.nim +++ b/beacon_chain/sync/sync_queue.nim @@ -37,7 +37,6 @@ type index*: uint64 slot*: Slot count*: uint64 - step*: uint64 item*: T SyncResult*[T] = object @@ -104,12 +103,11 @@ proc getShortMap*[T](req: SyncRequest[T], break else: res.add('.') - slider = slider + req.step + slider = slider + 1 res proc contains*[T](req: SyncRequest[T], slot: Slot): bool {.inline.} = - slot >= req.slot and slot < req.slot + req.count * req.step and - ((slot - req.slot) mod req.step == 0) + slot >= req.slot and slot < req.slot + req.count proc cmp*[T](a, b: SyncRequest[T]): int = cmp(uint64(a.slot), uint64(b.slot)) @@ -136,7 +134,7 @@ proc checkResponse*[T](req: SyncRequest[T], inc(dindex) else: return false - slot = slot + req.step + slot = slot + 1 rindex = rindex + 1'u64 if dindex == len(data): @@ -152,26 +150,26 @@ proc getFullMap*[T](req: SyncRequest[T], proc init[T](t1: typedesc[SyncRequest], kind: SyncQueueKind, start: Slot, finish: Slot, t2: typedesc[T]): SyncRequest[T] = let count = finish - start + 1'u64 - SyncRequest[T](kind: kind, slot: start, count: count, step: 1'u64) + SyncRequest[T](kind: kind, slot: start, count: count) proc init[T](t1: typedesc[SyncRequest], kind: SyncQueueKind, slot: Slot, count: uint64, item: T): SyncRequest[T] = - SyncRequest[T](kind: kind, slot: slot, count: count, item: item, step: 1'u64) + SyncRequest[T](kind: kind, slot: slot, count: count, item: item) proc init[T](t1: typedesc[SyncRequest], kind: SyncQueueKind, start: Slot, finish: Slot, item: T): SyncRequest[T] = let count = finish - start + 1'u64 - SyncRequest[T](kind: kind, slot: start, count: count, step: 1'u64, item: item) + SyncRequest[T](kind: kind, slot: start, count: count, item: item) proc empty*[T](t: typedesc[SyncRequest], kind: SyncQueueKind, t2: typedesc[T]): SyncRequest[T] {.inline.} = - SyncRequest[T](kind: kind, step: 0'u64, count: 0'u64) + SyncRequest[T](kind: kind, count: 0'u64) proc setItem*[T](sr: var SyncRequest[T], item: T) = sr.item = item proc isEmpty*[T](sr: SyncRequest[T]): bool {.inline.} = - (sr.step == 0'u64) and (sr.count == 0'u64) + (sr.count == 0'u64) proc init*[T](t1: typedesc[SyncQueue], t2: typedesc[T], queueKind: SyncQueueKind, @@ -262,8 +260,7 @@ proc `<`*[T](a, b: SyncResult[T]): bool = a.request.slot > b.request.slot proc `==`*[T](a, b: SyncRequest[T]): bool = - (a.kind == b.kind) and (a.slot == b.slot) and (a.count == b.count) and - (a.step == b.step) + (a.kind == b.kind) and (a.slot == b.slot) and (a.count == b.count) proc lastSlot*[T](req: SyncRequest[T]): Slot = ## Returns last slot for request ``req``. @@ -809,12 +806,10 @@ func updateRequestForNewSafeSlot[T](sq: SyncQueue[T], sr: var SyncRequest[T]) = # Request is only partially relevant. let numSlotsDone = outSlot - lowSlot - numStepsDone = (numSlotsDone + sr.step - 1) div sr.step - sr.slot += numStepsDone * sr.step - sr.count -= numStepsDone + sr.slot += numSlotsDone + sr.count -= numSlotsDone else: # Entire request is no longer relevant. - sr.step = 0 sr.count = 0 of SyncQueueKind.Backward: if outSlot >= highSlot: @@ -824,11 +819,9 @@ func updateRequestForNewSafeSlot[T](sq: SyncQueue[T], sr: var SyncRequest[T]) = # Request is only partially relevant. let numSlotsDone = highSlot - outSlot - numStepsDone = (numSlotsDone + sr.step - 1) div sr.step - sr.count -= numStepsDone + sr.count -= numSlotsDone else: # Entire request is no longer relevant. - sr.step = 0 sr.count = 0 proc pop*[T](sq: SyncQueue[T], maxslot: Slot, item: T): SyncRequest[T] = diff --git a/tests/test_sync_manager.nim b/tests/test_sync_manager.nim index 99d09b5da..14826a706 100644 --- a/tests/test_sync_manager.nim +++ b/tests/test_sync_manager.nim @@ -100,7 +100,7 @@ suite "SyncManager test suite": r11e == r11 r11.item == p1 r11e.item == r11.item - r11.slot == Slot(0) and r11.count == 1'u64 and r11.step == 1'u64 + r11.slot == Slot(0) and r11.count == 1'u64 template passThroughLimitsTest(kind: SyncQueueKind) = let @@ -187,7 +187,6 @@ suite "SyncManager test suite": req1.isEmpty() == false req1.slot == item[3][0] req1.count == item[3][1] - req1.step == 1'u64 req2.isEmpty() == true template twoFullRequests(kkind: SyncQueueKind) = @@ -248,12 +247,12 @@ suite "SyncManager test suite": case kkind of SyncQueueKind.Forward: check: - r21.slot == Slot(0) and r21.count == 1'u64 and r21.step == 1'u64 - r22.slot == Slot(1) and r22.count == 1'u64 and r22.step == 1'u64 + r21.slot == Slot(0) and r21.count == 1'u64 + r22.slot == Slot(1) and r22.count == 1'u64 of SyncQueueKind.Backward: check: - r21.slot == Slot(1) and r21.count == 1'u64 and r21.step == 1'u64 - r22.slot == Slot(0) and r22.count == 1'u64 and r22.step == 1'u64 + r21.slot == Slot(1) and r21.count == 1'u64 + r22.slot == Slot(0) and r22.count == 1'u64 template done(b: BlockEntry) = b.resfut.complete(Result[void, BlockError].ok()) @@ -957,12 +956,11 @@ suite "SyncManager test suite": let chain2 = newSeq[ref ForkedSignedBeaconBlock]() for counter in countdown(32'u64, 2'u64): - let req = SyncRequest[SomeTPeer](slot: Slot(1), count: counter, - step: 1'u64) + let req = SyncRequest[SomeTPeer](slot: Slot(1), count: counter) let sr = SyncResult[SomeTPeer](request: req, data: chain1) check sr.hasEndGap() == true - let req = SyncRequest[SomeTPeer](slot: Slot(1), count: 1'u64, step: 1'u64) + let req = SyncRequest[SomeTPeer](slot: Slot(1), count: 1'u64) let sr1 = SyncResult[SomeTPeer](request: req, data: chain1) let sr2 = SyncResult[SomeTPeer](request: req, data: chain2) check: @@ -974,12 +972,11 @@ suite "SyncManager test suite": let chain2 = newSeq[ref ForkedSignedBeaconBlock]() for counter in countdown(32'u64, 2'u64): - let req = SyncRequest[SomeTPeer](slot: Slot(10), count: counter, - step: 1'u64) + let req = SyncRequest[SomeTPeer](slot: Slot(10), count: counter) let sr = SyncResult[SomeTPeer](request: req, data: chain1) check sr.getLastNonEmptySlot() == Slot(10) - let req = SyncRequest[SomeTPeer](slot: Slot(100), count: 1'u64, step: 1'u64) + let req = SyncRequest[SomeTPeer](slot: Slot(100), count: 1'u64) let sr = SyncResult[SomeTPeer](request: req, data: chain2) check sr.getLastNonEmptySlot() == Slot(100) @@ -990,59 +987,22 @@ suite "SyncManager test suite": while counter < req.count: if not(req.contains(slot)): return false - slot = slot + req.step + slot = slot + 1 counter = counter + 1'u64 return true - var req1 = SyncRequest[SomeTPeer](slot: Slot(5), count: 10'u64, step: 1'u64) - var req2 = SyncRequest[SomeTPeer](slot: Slot(1), count: 10'u64, step: 2'u64) - var req3 = SyncRequest[SomeTPeer](slot: Slot(2), count: 10'u64, step: 3'u64) - var req4 = SyncRequest[SomeTPeer](slot: Slot(3), count: 10'u64, step: 4'u64) - var req5 = SyncRequest[SomeTPeer](slot: Slot(4), count: 10'u64, step: 5'u64) + var req1 = SyncRequest[SomeTPeer](slot: Slot(5), count: 10'u64) check: req1.checkRange() == true - req2.checkRange() == true - req3.checkRange() == true - req4.checkRange() == true - req5.checkRange() == true req1.contains(Slot(4)) == false req1.contains(Slot(15)) == false - req2.contains(Slot(0)) == false - req2.contains(Slot(21)) == false - req2.contains(Slot(20)) == false - - req3.contains(Slot(0)) == false - req3.contains(Slot(1)) == false - req3.contains(Slot(32)) == false - req3.contains(Slot(31)) == false - req3.contains(Slot(30)) == false - - req4.contains(Slot(0)) == false - req4.contains(Slot(1)) == false - req4.contains(Slot(2)) == false - req4.contains(Slot(43)) == false - req4.contains(Slot(42)) == false - req4.contains(Slot(41)) == false - req4.contains(Slot(40)) == false - - req5.contains(Slot(0)) == false - req5.contains(Slot(1)) == false - req5.contains(Slot(2)) == false - req5.contains(Slot(3)) == false - req5.contains(Slot(54)) == false - req5.contains(Slot(53)) == false - req5.contains(Slot(52)) == false - req5.contains(Slot(51)) == false - req5.contains(Slot(50)) == false - test "[SyncQueue] checkResponse() test": let chain = createChain(Slot(10), Slot(20)) - let r1 = SyncRequest[SomeTPeer](slot: Slot(11), count: 1'u64, step: 1'u64) - let r21 = SyncRequest[SomeTPeer](slot: Slot(11), count: 2'u64, step: 1'u64) - let r22 = SyncRequest[SomeTPeer](slot: Slot(11), count: 2'u64, step: 2'u64) + let r1 = SyncRequest[SomeTPeer](slot: Slot(11), count: 1'u64) + let r21 = SyncRequest[SomeTPeer](slot: Slot(11), count: 2'u64) check: checkResponse(r1, @[chain[1]]) == true @@ -1063,19 +1023,6 @@ suite "SyncManager test suite": checkResponse(r21, @[chain[2], chain[3]]) == false checkResponse(r21, @[chain[3]]) == false - checkResponse(r22, @[chain[1]]) == true - checkResponse(r22, @[]) == true - checkResponse(r22, @[chain[1], chain[3]]) == true - checkResponse(r22, @[chain[3]]) == true - checkResponse(r22, @[chain[1], chain[3], chain[5]]) == false - checkResponse(r22, @[chain[0], chain[1]]) == false - checkResponse(r22, @[chain[1], chain[2]]) == false - checkResponse(r22, @[chain[2], chain[3]]) == false - checkResponse(r22, @[chain[3], chain[4]]) == false - checkResponse(r22, @[chain[4], chain[5]]) == false - checkResponse(r22, @[chain[4]]) == false - checkResponse(r22, @[chain[3], chain[1]]) == false - test "[SyncQueue#Forward] getRewindPoint() test": let aq = newAsyncQueue[BlockEntry]() block: