Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Fixes the calculation of the compute_meter_consumption #21944

Merged

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Dec 16, 2021

Problem

The calculation of the compute_meter_consumption across process_instruction() and process_message() was broken in #21671. The pre value was measured before InvokeContext::push(), then the compute meter was reset to a higher value and later the post value could be above the pre value, resulting in a negative consumption (overflow).

This was not caught by the tests as the bug only happens before the feature tx_wide_compute_cap is activated and it is always activated in the tests.

Summary of Changes

Moves from process_message() to process_instruction() so that it does not include modification of the compute_meter in InvokeContext::push().

Fixes #

@Lichtso Lichtso force-pushed the fix/compute_meter_consumption_measurement branch from e551410 to d29453a Compare December 16, 2021 11:14
@Lichtso Lichtso added the v1.9 label Dec 16, 2021
@codecov
Copy link

codecov bot commented Dec 16, 2021

Codecov Report

Merging #21944 (d29453a) into master (46e5350) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #21944   +/-   ##
=======================================
  Coverage    81.2%    81.2%           
=======================================
  Files         516      516           
  Lines      144353   144353           
=======================================
+ Hits       117315   117350   +35     
+ Misses      27038    27003   -35     

@Lichtso Lichtso merged commit 49cb161 into solana-labs:master Dec 16, 2021
@Lichtso Lichtso deleted the fix/compute_meter_consumption_measurement branch December 16, 2021 14:16
mergify bot pushed a commit that referenced this pull request Dec 16, 2021
…ss_instruction() and process_message(). (#21944)

(cherry picked from commit 49cb161)
mergify bot added a commit that referenced this pull request Dec 16, 2021
…ss_instruction() and process_message(). (#21944) (#21945)

(cherry picked from commit 49cb161)

Co-authored-by: Alexander Meißner <[email protected]>
@jackcmay
Copy link
Contributor

@taozhu-chicago This should also go into v1.8 correct?

@Lichtso
Copy link
Contributor Author

Lichtso commented Dec 16, 2021

I don't know it it was already broken prior to #21671, but that was only backported to 1.9 so I did the same for this PR.

@tao-stones
Copy link
Contributor

@taozhu-chicago This should also go into v1.8 correct?

The aggregated CUs from 1.8.10 looks reasonable. If impact of the bug fixed by this change is infrequently, I think it is OK to leave it out of 1.8. I am only considering the effort of release.

@brooksprumo brooksprumo mentioned this pull request Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants