getCommitteeAssignments

[3xtr4t3rr3str14l]

Resolves #2716 by implementing getCommitteeAssignments, a replacement of assembleAttesterDuty.

current benchmark results:

get attester duties
================================================================
new way: getCommitteeAssignments                                      0.6062110 ops/s      1.649591  s/op      2 runs    3.299 s
new way (plus index2pubkey): getCommitteeAssignments                  0.7766266 ops/s      1.287620  s/op      2 runs    2.576 s
old way: assembleAttesterDuty batch                                   0.2985871 ops/s      3.349106  s/op      2 runs    6.698 s
    ✓ performance comparison (12678ms)

The benchmark test I used to get those numbers is in this PR. It also checks to make sure that getCommitteeAssignments gets the same values that batch assembleAttesterDuty previously did.

Removed assembleAttesterDuty as well.

[codeclimate]

Code Climate has analyzed commit e756863 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.

[3xtr4t3rr3str14l]

NOTE: noticed something may be off in my benchmark, so I'm going to double check it and put the results in the description once I'm done checking & benchmarking.

[3xtr4t3rr3str14l]

Had to fix a bug (which the fix for is now in this PR), but the latest performance metrics of this PR are in the description. I could maybe go further with some of the optimizations (not sure just yet), but as I'm heading out for the day, I'm leaving what I got here for now until I can take a second look.

[dapplion]

Benchmarks should be the same because the code is fundamentally the same. You've replaced a for loop for a map and another for loop for a find index. The time complexity of the algorithm is the same, but maybe less obvious.

It should even be slower because callback-based iterators are less optimized than for of loops (but I'm not sure). However a basic for loop will always be much faster.

To make this faster you have to remove one loop and iterate the committee once instead of one time per validator.

[dapplion]

Current algorytm loops through the entire shuffled indices array for each validator. Not only that but it returns the committee array so it is looped again afterwards.

for (const validatorIndex of validatorIndices) {
    const committeeAssignment = (function () {
      for (let slot = epochStartSlot; slot < epochStartSlot + SLOTS_PER_EPOCH; slot++) {
        const committeeCount = this.getCommitteeCountAtSlot(slot);
        for (let i = 0; i < committeeCount; i++) {
          const committee = this.getBeaconCommittee(slot, i);
          if (committee.includes(validatorIndex)) {
            return {
              validators: committee as List<number>,
              committeeIndex: i,
              slot,
            };
          }
        }
      }
    })();

    if (committeeAssignment) {
      let validatorCommitteeIndex = -1;
      let index = 0;
      for (const i of readonlyValues(committeeAssignment.validators)) {
        if (ssz.ValidatorIndex.equals(i, validator.index)) {
          validatorCommitteeIndex = index;
          break;
        }
        index++;
      }
      return {
        pubkey: validator.pubkey,
        validatorIndex: validator.index,
        committeeLength: committeeAssignment.validators.length,
        committeesAtSlot: epochCtx.getCommitteeCountAtSlot(committeeAssignment.slot),
        validatorCommitteeIndex,
        committeeIndex: committeeAssignment.committeeIndex,
        slot: committeeAssignment.slot,
      };
    }
  }

---

A more reasonable approach would be to iterate the shuffled array once for all validatorIndices and not loop twice through the committee array

[3xtr4t3rr3str14l]

@dapplion I followed your suggestion and got significant performance increases:

number of runs:  50
assembleAttesterDuty batch performance avg:  3332.42
getCommitteeAssignments performance avg:  4.22

Now the solution just uses getCommitteeAssignments (new epochCtx batch function) directly and does the work that running assembleAttesterDuty in batch previously did.

The benchmark test I used to get those numbers is in this PR. It also checks to make sure that getCommitteeAssignments gets the same values that batch assembleAttesterDuty previously did.

stale

[dapplion]

Mixup? This changes do not belong to this PR

[3xtr4t3rr3str14l]

I was getting some errors after changing the yarn run test:perf command to yarn run benchmark, so I made changes here accordingly.

[dapplion]

This seems to speed up getCommitteeAssignments 2x for small # of validators, 30x for large # of validators, so it may be "good enough"?

These are the current numbers in my local machine

    ✓ getCommitteeAssignments - req 1 vs - 200000 vc                      225.6465 ops/s    4.431711 ms/op        -       1024 runs   4.54 s
    ✓ getCommitteeAssignments - req 100 vs - 200000 vc                    141.3410 ops/s    7.075087 ms/op        -       1024 runs   7.25 s
    ✓ getCommitteeAssignments - req 1000 vs - 200000 vc                   124.7096 ops/s    8.018632 ms/op        -       1024 runs   8.25 s

Since getCommitteeAssignments() is something we should call twice per epoch I think it's good for now. The node will spend ~10ms every 6min doing executing this logic (0.003% rate).

Note that prior to this PR, for 1000 validators the node will do 4.4ms x 1000 = 4 sec total. AKA not acceptable

[twoeths]

Looks good to me, just a small note that if we use state.validators.persistent frequently, it's maybe better to write a method in CachedValidatorList so that we get through the proxy a single time

[dapplion]

Looks good to me, just a small note that if we use state.validators.persistent frequently, it's maybe better to write a method in CachedValidatorList so that we get through the proxy a single time

Good idea! Can you open an issue for it with more details?