Bls v0.10.1 #780
Merged
Bls v0.10.1 #780
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Jenkins has a different error instead of a stack smashing: https://ci.status.im/blue/organizations/jenkins/nimbus%2Fnim-beacon-chain%2Fprs/detail/PR-780/2/pipeline/68
|
tersec
reviewed
Mar 5, 2020
| if skipValidation notin flags and | ||
| not blsFastAggregateVerify( | ||
| pubkeys, signing_root.data, indexed_attestation.signature | ||
| ): |
There was a problem hiding this comment.
This was the possible apparent cause of a couple of EF test vector failures. Hopefully, it improves things.
tersec
reviewed
Mar 5, 2020
tests/mocking/mock_attestations.nim
Outdated
| ) | ||
| let signing_root = compute_signing_root(msg, domain) | ||
|
|
||
| return bls_sign(privkey, signing_root.data) |
There was a problem hiding this comment.
Could omit return to get better warnings from Nim, perhaps.
tersec
approved these changes
Mar 5, 2020
| y = init(ValidatorSig, x.getBytes()) | ||
|
|
||
| # Silly serialization check that fails without the right import | ||
| check: x == y |
There was a problem hiding this comment.
Absolutely no idea, but I removed getBytes (and init but quickly reintroduced it).
It may have been related to the generic sandwich or init template ordering: status-im/nim-stew#9
| result.point.inf() | ||
| # TODO: remove fully if the comment below is not true anymore and | ||
| # and we don't need this workaround | ||
| # # TODO bls_verify_multiple(...) used to have this workaround, and now it |
There was a problem hiding this comment.
Yeah, in theory the way this is called, we shouldn't -- when I last checked all this a couple weeks ago, the way the new BLS code is called is explicit in listing the key pairs, pre-aggregation, so doesn't need evidently fragile/high-assumption tricks like this.
| # https://github.com/ethereum/eth2.0-specs/blob/v0.9.4/specs/bls_signature.md#bls_aggregate_pubkeys | ||
| func bls_aggregate_pubkeys*(keys: openArray[ValidatorPubKey]): ValidatorPubKey = | ||
| keys.combine() | ||
| func shortLog*(x: BlsValue): string = |
There was a problem hiding this comment.
Is there value in adding the equivalent https://github.com/ethereum/eth2.0-specs/blob/v0.10.1/ or so spec references (or, IETF spec references, as appropriate), maybe? That's always a tradeoff, since when it's either otherwise adequately tested, or quite static once written, sometimes it's a net loss in code maintainability to have a URL that just needs to be updated with no real ROI. It can still be useful to catch stray outdated code easily.
There was a problem hiding this comment.
I'm probably going well beyond that in nim-blscurve:
| @@ -15,24 +15,19 @@ func get_eth1data_stub*(deposit_count: uint64, current_epoch: Epoch): Eth1Data = | |||
| block_hash: hash_tree_root(hash_tree_root(voting_period).data), | |||
| ) | |||
|
|
|||
| when ValidatorPrivKey is BlsValue: | |||
There was a problem hiding this comment.
If this is related to the local private key storage, this might be an instance of what @zah discussed as excessive specialization on a design which should be regardless decoupled. If so, maybe note as such.
There was a problem hiding this comment.
This was when we were hesitating between using the raw private keys and being able to handle fake blobs, especially due to testing faking the BLS stuff.
This was referenced Mar 8, 2020
|
So the stack smashing happens in GCC but not with Clang. backtrace:
This is when compiling mock_validator_keys even when forcing heap alloc via proc genMockPrivKeys(privkeys: var array[MIN_GENESIS_ACTIVE_VALIDATOR_COUNT, ValidatorPrivKey]) =
for pk in privkeys.mitems():
let pair = newKeyPair()
pk = pair.priv
proc genMockPubKeys(
pubkeys: var array[MIN_GENESIS_ACTIVE_VALIDATOR_COUNT, ValidatorPubKey],
privkeys: array[MIN_GENESIS_ACTIVE_VALIDATOR_COUNT, ValidatorPrivKey]
) =
for idx, privkey in privkeys:
pubkeys[idx] = pubkey(privkey)
# TODO: Ref array necessary to use a proc to avoid stack smashing in ECP_BLS381_mul (see gdb)
var MockPrivKeys*: ref array[MIN_GENESIS_ACTIVE_VALIDATOR_COUNT, ValidatorPrivKey]
new MockPrivKeys
genMockPrivKeys(MockPrivKeys[])
var MockPubKeys*: ref array[MIN_GENESIS_ACTIVE_VALIDATOR_COUNT, ValidatorPubKey]
new MockPubKeys
genMockPubKeys(MockPubKeys[], MockPrivKeys[]) |
|
Changing to indexing doesn't work either let MockPrivKeys* = block:
var privkeys: array[MIN_GENESIS_ACTIVE_VALIDATOR_COUNT, ValidatorPrivKey]
for i in 0 ..< privkeys.len:
let pair = newKeyPair()
privkeys[i] = pair.priv
privkeys
let MockPubKeys* = block:
var pubkeys: array[MIN_GENESIS_ACTIVE_VALIDATOR_COUNT, ValidatorPubKey]
for i in 0 ..< MockPrivKeys.len:
pubkeys[i] = pubkey(MockPrivKeys[i])
pubkeysSo it doesn't like an instance of: nim-lang/Nim#12747
|
This was referenced Mar 11, 2020
|
The Jenkins failure is due to a serialization problem, either import issue or a missing save/write/encode to make the BLS objects serialize as Milagro instead of as hex
|
- EF - attester slashings - 2 tests - EF - sanity blocks - 1 test - NBC - attestation - 1 test
|
Don't merge, yet another stack smashing issue. |
|
Repro: This only happens with GCC even with Clang ASAN.
|
|
Previous repro was missing some env variables from env.sh Here is one with Clang that should be more relevant, it's once again an issue in ECP_mul however the seckey here should be lesser than the curve order:
|
etan-status
added a commit
to etan-status/nimbus-eth2
that referenced
this pull request
Dec 9, 2021
In status-im#780 a test was disabled that verified that an attestation with empty `aggregation_bits` completes successfully. The test was never re-introduced, and as of the current consensus spec v1.1.6, such attestations are not considered valid, as they fail the check in `is_valid_indexed_attestation`. This patch fully removes that outdated test, and moves it to the list of pending invalid attestation tests.
arnetheduck
pushed a commit
that referenced
this pull request
Dec 9, 2021
In #780 a test was disabled that verified that an attestation with empty `aggregation_bits` completes successfully. The test was never re-introduced, and as of the current consensus spec v1.1.6, such attestations are not considered valid, as they fail the check in `is_valid_indexed_attestation`. This patch fully removes that outdated test, and moves it to the list of pending invalid attestation tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the BLS implementation to the Eth2 spec v0.10.1 and should unblock the remaining work on naive aggregation #497 and honest validator #758.
At the moment:
This was made mechanically for the block pool / validator pool and the mocking procedures in testing and checked against v0.10.1 for the spec parts (i.e. part of the validator_pool got specced out in the honest validator document, proc name to be updated)
Domainis now defined in nim-beacon-chain and not in blscurve repoTODO: