[Execution] Randomize collection fetch order

[SaveTheRbtz]

Map iteration order is not random, but instead undefined (as per go spec):

The iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next.

This does not mean random and especially not uniformly random. Therefore pre-randomize the order.

[bluesign]

Why do we need uniform random here?

[SaveTheRbtz]

Why do we need uniform random here?

The main reason for this randomness is to prevent the case scenario when we constantly request the same items over and over again because of the max batch size limit.

[bluesign]

Why do we need uniform random here?

The main reason for this randomness is to prevent the case scenario when we constantly request the same items over and over again because of the max batch size limit.

I thought about that but doesn't

flow-go/engine/common/requester/engine.go

Line 306 in bfa8295

cutoff := item.LastRequested.Add(item.RetryAfter)

prevent that ?

Sorry maybe asking too much (trying to grasp flow-go lately) but also why aborting here?

flow-go/engine/common/requester/engine.go

Line 339 in bfa8295

return false, fmt.Errorf("no valid providers available")

Instead of moving to the next item ?

[SaveTheRbtz]

Why do we need uniform random here?

The main reason for this randomness is to prevent the case scenario when we constantly request the same items over and over again because of the max batch size limit.

I thought about that but doesn't

flow-go/engine/common/requester/engine.go

Line 306 in bfa8295

cutoff := item.LastRequested.Add(item.RetryAfter)

prevent that ?

it would help sometimes depending on the phase of moon (timeouts, error rates, frequency of calling this function, etc.). It is better to explicitly randomize the order though.

Sorry maybe asking too much (trying to grasp flow-go lately) but also why aborting here?

flow-go/engine/common/requester/engine.go

Line 339 in bfa8295

return false, fmt.Errorf("no valid providers available")

Instead of moving to the next item ?

It's a good question. The answer to it is: it was simpler: the complexity of this function is already too high. In reality we just need to refactor that piece of code to reduce complexity.

[SaveTheRbtz]

bors merge

[bors]

Build succeeded:

• Integration Tests (make -C integration access-tests)
• Integration Tests (make -C integration bft-tests)
• Integration Tests (make -C integration collection-tests)
• Integration Tests (make -C integration consensus-tests)
• Integration Tests (make -C integration epochs-tests)
• Integration Tests (make -C integration execution-tests)
• Integration Tests (make -C integration ghost-tests)
• Integration Tests (make -C integration mvp-tests)
• Integration Tests (make -C integration network-tests)
• Integration Tests (make -C integration verification-tests)
• Lint (./)
• Lint (./crypto/)
• Lint (./integration/)
• Unit Tests (access)
• Unit Tests (admin)
• Unit Tests (cmd)
• Unit Tests (consensus)
• Unit Tests (engine)
• Unit Tests (fvm)
• Unit Tests (ledger)
• Unit Tests (module)
• Unit Tests (network)
• Unit Tests (others)
• Unit Tests (utils)