Revert "Lazy crypto [alt design #1369] (#1371)" (#1435)

This reverts commit 023f7f4518.
2020-08-04 19:15:13 +02:00 · 2020-08-04 19:15:13 +02:00 · cf8cd8321b
parent c6674de5d2
commit cf8cd8321b
7 changed files with 72 additions and 112 deletions
--- a/beacon_chain/eth2_json_rpc_serialization.nim
+++ b/beacon_chain/eth2_json_rpc_serialization.nim
@ -14,7 +14,6 @@ proc fromJson*(n: JsonNode, argName: string, result: var ValidatorPubKey) =
  result = ValidatorPubKey.fromHex(n.getStr()).tryGet()

 proc `%`*(pubkey: ValidatorPubKey): JsonNode =
-  unsafePromote(pubkey.unsafeAddr)
  result = newJString($pubkey)

 proc fromJson*(n: JsonNode, argName: string, result: var List) =
@ -32,7 +31,6 @@ proc fromJson*(n: JsonNode, argName: string, result: var ValidatorSig) =
  result = ValidatorSig.fromHex(n.getStr()).tryGet()

 proc `%`*(value: ValidatorSig): JsonNode =
-  unsafePromote(value.unsafeAddr)
  result = newJString($value)

 proc fromJson*(n: JsonNode, argName: string, result: var Version) =
--- a/beacon_chain/spec/beaconstate.nim
+++ b/beacon_chain/spec/beaconstate.nim
@ -237,10 +237,12 @@ proc initialize_beacon_state_from_eth1*(
      Eth1Data(block_hash: eth1_block_hash, deposit_count: uint64(len(deposits))),
    latest_block_header:
      BeaconBlockHeader(
-        # This differs from the spec intentionally.
-        # We must specify the default value for `ValidatorSig`/`BeaconBlockBody`
-        # in order to get a correct `hash_tree_root`.
-        body_root: hash_tree_root(BeaconBlockBody())
+        body_root: hash_tree_root(BeaconBlockBody(
+          # This differs from the spec intentionally.
+          # We must specify the default value for `ValidatorSig`
+          # in order to get a correct `hash_tree_root`.
+          randao_reveal: ValidatorSig(kind: OpaqueBlob)
+        ))
      )
  )

@ -300,7 +302,10 @@ func is_valid_genesis_state*(preset: RuntimePreset,
 func get_initial_beacon_block*(state: BeaconState): SignedBeaconBlock =
  let message = BeaconBlock(
      slot: GENESIS_SLOT,
-      state_root: hash_tree_root(state))
+      state_root: hash_tree_root(state),
+      body: BeaconBlockBody(
+        # TODO: This shouldn't be necessary if OpaqueBlob is the default
+        randao_reveal: ValidatorSig(kind: OpaqueBlob)))
      # parent_root, randao_reveal, eth1_data, signature, and body automatically
      # initialized to default values.
  SignedBeaconBlock(message: message, root: hash_tree_root(message))
--- a/beacon_chain/spec/crypto.nim
+++ b/beacon_chain/spec/crypto.nim
@ -45,30 +45,18 @@ const
  RawPrivKeySize* = 48

 type
-  BlsValueKind* = enum
-    ToBeChecked
+  BlsValueType* = enum
    Real
-    InvalidBLS
-    OpaqueBlob # For SSZ testing only
+    OpaqueBlob

  BlsValue*[N: static int, T: blscurve.PublicKey or blscurve.Signature] = object
-    ## This is a lazily initiated wrapper for the underlying cryptographic type
-    ##
-    ## Fields intentionally private to avoid displaying/logging the raw data
-    ## or accessing fields without promoting them
-    ## or trying to iterate on a case object even though the case is wrong.
-    ## Is there a way to prevent macro from doing that? (SSZ/Chronicles)
-    #
-    # Note, since 0.20 case object transition are very restrictive
-    # and do not allow to preserve content (https://github.com/nim-lang/RFCs/issues/56)
-    # Fortunately, the content is transformed anyway if the object is valid
-    # but we might want to keep the invalid content at least for logging before discarding it.
-    # Our usage requires "-d:nimOldCaseObjects"
-    case kind: BlsValueKind
+    # TODO This is a temporary type needed until we sort out the
+    # issues with invalid BLS values appearing in the SSZ test suites.
+    case kind*: BlsValueType
    of Real:
-      blsValue: T
-    of ToBeChecked, InvalidBLS, OpaqueBlob:
-      blob: array[N, byte]
+      blsValue*: T
+    of OpaqueBlob:
+      blob*: array[N, byte]

  ValidatorPubKey* = BlsValue[RawPubKeySize, blscurve.PublicKey]

@ -87,58 +75,7 @@ type

  SomeSig* = TrustedSig | ValidatorSig

-# Lazy parsing
-# ----------------------------------------------------------------------
-
-func unsafePromote*[N, T](a: ptr BlsValue[N, T]) =
-  ## Try promoting an opaque blob to its corresponding
-  ## BLS value.
-  ##
-  ## ⚠️ Warning - unsafe.
-  ## At a low-level we mutate the input but all API like
-  ## bls_sign, bls_verify assume that their inputs are immutable
-  if a.kind != ToBeChecked:
-    return
-
-  # Try if valid BLS value
-  var buffer: T
-  let success = buffer.fromBytes(a.blob)
-
-  # Unsafe hidden mutation of the input
-  if true:
-    a.kind = Real # Requires "-d:nimOldCaseObjects"
-    a.blsValue = buffer
-  else:
-    a.kind = InvalidBLS
-
-# Accessors
-# ----------------------------------------------------------------------
-
-func setBlob*[N, T](a: var BlsValue[N, T], data: array[N, byte]) {.inline.} =
-  ## Set a BLS Value lazily
-  a.blob = data
-
-func keyGen*(ikm: openArray[byte]): BlsResult[tuple[pub: ValidatorPubKey, priv: ValidatorPrivKey]] {.inline.} =
-  ## Key generation using the BLS Signature draft 2 (https://tools.ietf.org/html/draft-irtf-cfrg-bls-signature-02)
-  ## Note: As of July-2020, the only use-case is for testing
-  ##
-  ## Validator key generation should use Lamport Signatures (EIP-2333)
-  ## (https://eips.ethereum.org/EIPS/eip-2333)
-  ## and be done in a dedicated hardened module/process.
-  var
-    sk: SecretKey
-    pk: PublicKey
-  if keyGen(ikm, pk, sk):
-    ok((ValidatorPubKey(kind: Real, blsValue: pk), ValidatorPrivKey(sk)))
-  else:
-    err "bls: cannot generate keypair"
-
-# Comparison
-# ----------------------------------------------------------------------
-
 func `==`*(a, b: BlsValue): bool =
-  unsafePromote(a.unsafeAddr)
-  unsafePromote(b.unsafeAddr)
  if a.kind != b.kind: return false
  if a.kind == Real:
    return a.blsValue == b.blsValue
@ -159,14 +96,16 @@ func toPubKey*(privkey: ValidatorPrivKey): ValidatorPubKey =
  ## Create a private key from a public key
  # Un-specced in either hash-to-curve or Eth2
  # TODO: Test suite should use `keyGen` instead
-  ValidatorPubKey(kind: Real, blsValue: SecretKey(privkey).privToPub())
+  when ValidatorPubKey is BlsValue:
+    ValidatorPubKey(kind: Real, blsValue: SecretKey(privkey).privToPub())
+  elif ValidatorPubKey is array:
+    privkey.getKey.getBytes
+  else:
+    privkey.getKey

 func aggregate*(x: var ValidatorSig, other: ValidatorSig) =
  ## Aggregate 2 Validator Signatures
  ## This assumes that they are real signatures
-  ## and will crash if they are not
-  unsafePromote(x.addr)
-  unsafePromote(other.unsafeAddr)
  x.blsValue.aggregate(other.blsValue)

 # https://github.com/ethereum/eth2.0-specs/blob/v0.12.2/specs/phase0/beacon-chain.md#bls-signatures
@ -180,11 +119,8 @@ func blsVerify*(
  ## The proof-of-possession MUST be verified before calling this function.
  ## It is recommended to use the overload that accepts a proof-of-possession
  ## to enforce correct usage.
-  unsafePromote(pubkey.unsafeAddr)
-  unsafePromote(signature.unsafeAddr)
-
  if signature.kind != Real:
-    # InvalidBLS signatures are possible in deposits (discussed with Danny)
+    # Invalid signatures are possible in deposits (discussed with Danny)
    return false
  if pubkey.kind != Real:
    # TODO: chronicles warning
@ -228,15 +164,13 @@ func blsFastAggregateVerify*(
  #           in blscurve which already exists internally
  #         - or at network/databases/serialization boundaries we do not
  #           allow invalid BLS objects to pollute consensus routines
-  unsafePromote(signature.unsafeAddr)
  if signature.kind != Real:
    return false
  var unwrapped: seq[PublicKey]
-  for i in 0 ..< publicKeys.len:
-    unsafePromote(publicKeys[i].unsafeAddr)
-    if publicKeys[i].kind != Real:
+  for pubkey in publicKeys:
+    if pubkey.kind != Real:
      return false
-    unwrapped.add publicKeys[i].blsValue
+    unwrapped.add pubkey.blsValue

  fastAggregateVerify(unwrapped, message, signature.blsValue)

@ -290,9 +224,12 @@ func fromRaw*[N, T](BT: type BlsValue[N, T], bytes: openArray[byte]): BlsResult[
    # Only for SSZ parsing tests, everything is an opaque blob
    ok BT(kind: OpaqueBlob, blob: toArray(N, bytes))
  else:
-    # Lazily instantiate the value, it will be checked only on use
-    # TODO BlsResult is now unnecessary
-    ok BT(kind: ToBeChecked, blob: toArray(N, bytes))
+    # Try if valid BLS value
+    var val: T
+    if fromBytes(val, bytes):
+      ok BT(kind: Real, blsValue: val)
+    else:
+      ok BT(kind: OpaqueBlob, blob: toArray(N, bytes))

 func fromHex*(T: type BlsCurveType, hexStr: string): BlsResult[T] {.inline.} =
  ## Initialize a BLSValue from its hex representation
@ -380,10 +317,6 @@ func shortLog*(x: ValidatorPrivKey): string =
 func shortLog*(x: TrustedSig): string =
  x.data[0..3].toHex()

-chronicles.formatIt BlsValue: it.shortLog
-chronicles.formatIt ValidatorPrivKey: it.shortLog
-chronicles.formatIt TrustedSig: it.shortLog
-
 # Initialization
 # ----------------------------------------------------------------------

@ -413,3 +346,4 @@ func init*(T: typedesc[ValidatorSig], data: array[RawSigSize, byte]): T {.noInit

 proc burnMem*(key: var ValidatorPrivKey) =
  key = default(ValidatorPrivKey)
+
--- a/beacon_chain/validator_api.nim
+++ b/beacon_chain/validator_api.nim
@ -265,7 +265,7 @@ proc installValidatorApiHandlers*(rpcServer: RpcServer, node: BeaconNode) =
      blockId: string) -> tuple[canonical: bool, header: SignedBeaconBlockHeader]:
    let bd = node.getBlockDataFromBlockId(blockId)
    let tsbb = bd.data
-    result.header.signature.setBlob(tsbb.signature.data)
+    result.header.signature.blob = tsbb.signature.data

    result.header.message.slot = tsbb.message.slot
    result.header.message.proposer_index = tsbb.message.proposer_index
--- a/tests/helpers/debug_state.nim
+++ b/tests/helpers/debug_state.nim
@ -7,10 +7,29 @@

 import
  macros,
+  nimcrypto/utils,
  ../../beacon_chain/spec/[datatypes, crypto, digest], ../../beacon_chain/ssz/types
  # digest is necessary for them to be printed as hex

-export crypto.`==`
+# Define comparison of object variants for BLSValue
+# https://github.com/nim-lang/Nim/issues/6676
+# (fully generic available - see also https://github.com/status-im/nim-beacon-chain/commit/993789bad684721bd7c74ea14b35c2d24dbb6e51)
+# ----------------------------------------------------------------
+
+proc `==`*(a, b: BlsValue): bool =
+  ## We sometimes need to compare real BlsValue
+  ## from parsed opaque blobs that are not really on the BLS curve
+  ## and full of zeros
+  if a.kind == Real:
+    if b.kind == Real:
+      a.blsvalue == b.blsValue
+    else:
+      $a.blsvalue == toHex(b.blob, true)
+  else:
+    if b.kind == Real:
+      toHex(a.blob, true) == $b.blsValue
+    else:
+      a.blob == b.blob

 # ---------------------------------------------------------------------

@ -29,10 +48,9 @@ proc compareStmt(xSubField, ySubField: NimNode, stmts: var NimNode) =
  let xStr = $xSubField.toStrLit
  let yStr = $ySubField.toStrLit

-  let isEqual = bindSym("==") # Bind all expose equality, in particular for BlsValue
  stmts.add quote do:
    doAssert(
-      `isEqual`(`xSubField`, `ySubField`),
+      `xSubField` == `ySubField`,
      "\nDiff: " & `xStr` & " = " & $`xSubField` & "\n" &
      "and   " & `yStr` & " = " & $`ySubField` & "\n"
    )
@ -41,16 +59,16 @@ proc compareContainerStmt(xSubField, ySubField: NimNode, stmts: var NimNode) =
  let xStr = $xSubField.toStrLit
  let yStr = $ySubField.toStrLit

-  let isEqual = bindSym("==") # Bind all expose equality, in particular for BlsValue
+
  stmts.add quote do:
    doAssert(
-      `isEqual`(`xSubField`.len, `ySubField`.len),
+      `xSubField`.len == `ySubField`.len,
        "\nDiff: " & `xStr` & ".len = " & $`xSubField`.len & "\n" &
        "and   " & `yStr` & ".len = " & $`ySubField`.len & "\n"
    )
    for idx in `xSubField`.low .. `xSubField`.high:
      doAssert(
-        `isEqual`(`xSubField`[idx], `ySubField`[idx]),
+        `xSubField`[idx] == `ySubField`[idx],
        "\nDiff: " & `xStr` & "[" & $idx & "] = " & $`xSubField`[idx] & "\n" &
        "and   " & `yStr` & "[" & $idx & "] = " & $`ySubField`[idx] & "\n"
      )
--- a/tests/mocking/mock_validator_keys.nim
+++ b/tests/mocking/mock_validator_keys.nim
@ -10,6 +10,7 @@

 import
  bearssl, eth/keys,
+  blscurve/bls_signature_scheme,
  ../../beacon_chain/spec/[datatypes, crypto, presets]

 proc newKeyPair(rng: var BrHmacDrbgContext): BlsResult[tuple[pub: ValidatorPubKey, priv: ValidatorPrivKey]] =
@ -21,7 +22,14 @@ proc newKeyPair(rng: var BrHmacDrbgContext): BlsResult[tuple[pub: ValidatorPubKe

  var ikm: array[32, byte]
  brHmacDrbgGenerate(rng, ikm)
-  return ikm.keygen()
+
+  var
+    sk: SecretKey
+    pk: bls_signature_scheme.PublicKey
+  if keyGen(ikm, pk, sk):
+    ok((ValidatorPubKey(kind: Real, blsValue: pk), ValidatorPrivKey(sk)))
+  else:
+    err "bls: cannot generate keypair"

 # this is being indexed inside "mock_deposits.nim" by a value up to `validatorCount`
 # which is `num_validators` which is `MIN_GENESIS_ACTIVE_VALIDATOR_COUNT`
--- a/tests/test_zero_signature.nim
+++ b/tests/test_zero_signature.nim
@ -39,17 +39,14 @@ suiteReport "Zero signature sanity checks":
  timedTest "SSZ serialization roundtrip of SignedBeaconBlockHeader":

    let defaultBlockHeader = SignedBeaconBlockHeader(
-      signature: ValidatorSig()
+      signature: ValidatorSig(kind: OpaqueBlob)
    )

    check:
      block:
-        let sigBytes = cast[ptr array[ValidatorSig.sizeof(), byte]](
-          defaultBlockHeader.signature.unsafeAddr)
-
        var allZeros = true
-        for i in 0 ..< ValidatorSig.sizeof():
-          allZeros = allZeros and sigBytes[i] == byte 0
+        for val in defaultBlockHeader.signature.blob:
+          allZeros = allZeros and val == 0
        allZeros

    let sszDefaultBlockHeader = SSZ.encode(defaultBlockHeader)