Skip to content

Jonathan/fix map get active validator indices#46

Merged
GregTheGreek merged 5 commits intoChainSafe:masterfrom
JonathanLorimer:jonathan/fix-map-getActiveValidatorIndices
Dec 4, 2018
Merged

Jonathan/fix map get active validator indices#46
GregTheGreek merged 5 commits intoChainSafe:masterfrom
JonathanLorimer:jonathan/fix-map-getActiveValidatorIndices

Conversation

@JonathanLorimer
Copy link
Copy Markdown
Contributor

Small change:

Mutation with map isn't an issue, as it creates a new array with the mapped values. However, if you don't return a value within the lambda provided to map then it will, by default, insert undefined into the array. To avoid this, and do the filtering in a single iteration, I implemented reduce.

@GregTheGreek
Copy link
Copy Markdown
Member

@JonathanLorimer Awesome thanks for pointing that out! I made a super simple js file to test out the functionality and you're in-fact correct, looks like using reduce definitely solves the issue!
Could you make a few tests for this and then I I'll be more than happy to PR this through :)

Copy link
Copy Markdown
Member

@GregTheGreek GregTheGreek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make a few tests so we can have those pass :)
Maybe 1 with an empty validator list, with all validators that have active set and an array with no active?

@GregTheGreek GregTheGreek modified the milestones: Helper function test suite, Typescript Conversion Dec 4, 2018
@JonathanLorimer
Copy link
Copy Markdown
Contributor Author

JonathanLorimer commented Dec 4, 2018

@GregTheGreek added the test cases, just two things.

  • First, I had to change around the order of a couple of imports so that they were alphabetical (consider using ordered-imports: false in tslint if you don't want this), and I had to run tslint fix on the test file so the diff will be large

  • Second this code in stateTransitionHelpers.ts (line 148) is preventing TS from compiling, and I am not sure what it is supposed to accomplish. The issue is that we are trying to access an indexed value on participationBitfield, which is a number (I am not too familiar with bitwise operators, and how you can access them but TS is not liking the square brackets).
    const bit: int = (participationBitfield[Math.floor(index / 8)] >> (7 - (index % 8))) % 2;

@GregTheGreek
Copy link
Copy Markdown
Member

@JonathanLorimer
re test: Awesome thank you!

1 - Good point, I'll make that fix in a pr.

2 - I'm going to look into this, not sure why it's causing an issue. Previous pr it didn't seem to have that. Checking it out on my machine right now.

@GregTheGreek
Copy link
Copy Markdown
Member

@JonathanLorimer Ok i found out why its failing, line 24 of tsconfig.json is set to strict: true. since we don't have our integer and byte types finalized yet, this will naturally fail. So if you set it to false you'll be able to run the tests !

After running npm test the second test you wrote is failing. Expected: 0 Actual: 10.

Otherwise it's good to merge :)

@GregTheGreek
Copy link
Copy Markdown
Member

@JonathanLorimer Great work, thanks for contributing! :)

@GregTheGreek GregTheGreek merged commit 62ffdd6 into ChainSafe:master Dec 4, 2018
wemeetagain pushed a commit that referenced this pull request Aug 2, 2019
…elements

[ETHDenver] Merkleize ssz container elements
lodekeeper added a commit to lodekeeper/lodestar that referenced this pull request Mar 30, 2026
Key fixes in 2.0.x:
- Benchmark failures are now logged and cause CI to exit non-zero
  instead of being silently dropped (PR ChainSafe#45)
- CLI uses parseAsync to properly propagate async errors (PR ChainSafe#46)

Resolves ChainSafe#7484
nflaig pushed a commit that referenced this pull request Mar 30, 2026
## Motivation

Bumps `@chainsafe/benchmark` from `1.2.3` to `2.0.2`, which includes
fixes for error handling that were silently swallowing benchmark
failures.

### Fixes included in 2.0.x

- **[PR #45](ChainSafe/benchmark#45 —
Benchmark failures are now logged to stderr and cause CI to exit
non-zero instead of being silently dropped
- **[PR #46](ChainSafe/benchmark#46 — CLI
uses `parseAsync()` to properly propagate async handler errors to exit
code

### Verification

Ran fork-choice benchmarks locally to confirm they pass with the new
version:
- `computeDeltas` — 8/8 passing
- `forkChoice/updateHead` — 9/9 passing
- `forkChoice/onAttestation` — 1/1 passing
- `utils/bytes` — 20/20 passing

Resolves #7484

Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants