From cf8cd8321b63fa835b6816d3053631f8b8644bc9 Mon Sep 17 00:00:00 2001 From: Jacek Sieka Date: Tue, 4 Aug 2020 19:15:13 +0200 Subject: [PATCH] Revert "Lazy crypto [alt design #1369] (#1371)" (#1435) This reverts commit 023f7f4518fb8dd9cab2af5faba76b6975893dea. --- beacon_chain/eth2_json_rpc_serialization.nim | 2 - beacon_chain/spec/beaconstate.nim | 15 ++- beacon_chain/spec/crypto.nim | 116 ++++--------------- beacon_chain/validator_api.nim | 2 +- tests/helpers/debug_state.nim | 30 ++++- tests/mocking/mock_validator_keys.nim | 10 +- tests/test_zero_signature.nim | 9 +- 7 files changed, 72 insertions(+), 112 deletions(-) diff --git a/beacon_chain/eth2_json_rpc_serialization.nim b/beacon_chain/eth2_json_rpc_serialization.nim index f3b47ada5..63212a7ee 100644 --- 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) = diff --git a/beacon_chain/spec/beaconstate.nim b/beacon_chain/spec/beaconstate.nim index be2a14f55..cf69a97bc 100644 --- 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)) diff --git a/beacon_chain/spec/crypto.nim b/beacon_chain/spec/crypto.nim index 23ddff594..333da1232 100644 --- 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) + diff --git a/beacon_chain/validator_api.nim b/beacon_chain/validator_api.nim index 4300a1bed..ec44c5aa5 100644 --- 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 diff --git a/tests/helpers/debug_state.nim b/tests/helpers/debug_state.nim index f4c6e05dc..f603571bf 100644 --- 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" ) diff --git a/tests/mocking/mock_validator_keys.nim b/tests/mocking/mock_validator_keys.nim index 536a259bd..c478af389 100644 --- 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` diff --git a/tests/test_zero_signature.nim b/tests/test_zero_signature.nim index 437820d9d..391a5615d 100644 --- 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)