Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parallel vps' gas accounting rework #1835

Merged
merged 3 commits into from
Sep 25, 2023
Merged

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented Aug 18, 2023

Describe your changes

Currently, during the parallel execution of vps (more specifically in the try_fold function we use the set method of VpsGas to set the gas cost of the current Vp. This function only updates the max field as it assumes the VpsGas instance to have been just initialized and, therefore, be empty. From the rayon documentation though (https://docs.rs/rayon/latest/rayon/iter/trait.ParallelIterator.html#method.fold), it is stated that the sequence might be subdivided before being folded. In this case, more than on VP would be aggregated in one of these chunks and all of the VP following the first one would see a non-default VpsGas instance, i.e. an instance with the max field containing the gas cost of the previous vp. Given that the current implementation of set only overwrites the max field, we would end up under assessing the gas cost of the transaction. The correct logic, in this case, would be to update the maximum (if needed) and push the cheaper vp cost to the rest field.

I have tested this thing and it actually looks like rayon never produces subsequences with more than one element, i.e. all of the vps are allocated to their own subsequence and therefore do not incur in this problem. From some tests I saw that to trigger the misbehavior the collection needs to contain at least 8 elements, but this number is variable (depends from run to run) and also depends on the number of logical cpus allocated to rayon (which for Namada is non-constant, since we use half of the available logical cores). IMPORTANT: We might never come to such number of vps for a single transaction.

In the set method we also have a couple of debug_asserts verifying that the VpsGas instance is indeed empty (and therefore the current logic is correct).

My concern is that if the internals of rayon change (which might even go unnoticed by SemVer), or if we start triggering more vps per transaction than we currently do, we might start to experience this issue on some machines. If we don't catch this with the debug_asserts and the code gets shipped we might end up with a non-consistent gas accounting which would eventually lead to a break in consensus.

So this PR carries a small rework that ensures that the identity argument of try_fold is indeed an identity function and would guarantee the correct execution logic regardless of the rayon internals/execution context.

Indicate on which release or other PRs this topic is based on

v0.22.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@grarco grarco force-pushed the grarco/fix-parallel-vps branch from 189594d to d47058f Compare August 18, 2023 18:17
@grarco grarco changed the title Parallel vps' gas fix Parallel vps' gas accounting rework Aug 25, 2023
@grarco grarco force-pushed the grarco/fix-parallel-vps branch from 993ac99 to e146907 Compare September 7, 2023 12:42
@grarco grarco requested a review from tzemanovic September 8, 2023 15:15
tzemanovic
tzemanovic previously approved these changes Sep 12, 2023
@grarco grarco mentioned this pull request Sep 12, 2023
@grarco
Copy link
Collaborator Author

grarco commented Sep 12, 2023

pls update wasm

@grarco grarco marked this pull request as ready for review September 12, 2023 12:37
grarco added a commit that referenced this pull request Sep 21, 2023
Fraccaman added a commit that referenced this pull request Sep 25, 2023
* origin/grarco/fix-parallel-vps:
  changelog: add #1835
  Makes `max` field of `VpsGas` non-optional
  Fixes parallel vps' gas accounting
@brentstone brentstone merged commit 3291ada into main Sep 25, 2023
10 of 12 checks passed
@brentstone brentstone deleted the grarco/fix-parallel-vps branch September 25, 2023 17:19
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.

3 participants