Skip to content

fix: acc weight regression#1193

Merged
glevco merged 1 commit intomasterfrom
fix/update-acc-weight
Dec 11, 2024
Merged

fix: acc weight regression#1193
glevco merged 1 commit intomasterfrom
fix/update-acc-weight

Conversation

@glevco
Copy link
Contributor

@glevco glevco commented Dec 11, 2024

Motivation

The recent typing change in the meta.accumulated_weight introduced a regression in the BaseTransaction.update_accumulated_weight() method. Its implementation was not updated to consider the change. Also, the /transaction_acc_weight API was changed to incorrectly return work instead of weight, making it a breaking change for clients.

Acceptance Criteria

  • Fix BaseTransaction.update_accumulated_weight() so it correctly compares work to the provided stop_value.
  • Fix update_accumulated_weight() call in the consensus which was passing the incorrect stop_value.
  • Fix /transaction_acc_weight API so it returns the correct values (weight instead of work).

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco added the bug Something isn't working label Dec 11, 2024
@glevco glevco self-assigned this Dec 11, 2024
@github-actions
Copy link

github-actions bot commented Dec 11, 2024

🐰 Bencher Report

Branchfix/update-acc-weight
Testbedubuntu-22.04

🚨 1 Alert

BenchmarkMeasure
Units
ViewBenchmark Result
(Result Δ%)
Lower Boundary
(Limit %)
sync-v2 (up to 20000 blocks)Latency
nanoseconds (ns)
📈 plot
🚨 alert (🔔)
🚷 threshold
88,938,177,557.60
(-10.79%)
89,720,785,945.72
(100.88%)
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
nanoseconds (ns)
(Result Δ%)
Lower Boundary
nanoseconds (ns)
(Limit %)
Upper Boundary
nanoseconds (ns)
(Limit %)
sync-v2 (up to 20000 blocks)📈 view plot
🚨 view alert (🔔)
🚷 view threshold
88,938,177,557.60
(-10.79%)
89,720,785,945.72
(100.88%)
109,658,738,378.10
(81.10%)
🐰 View full continuous benchmarking report in Bencher

@glevco glevco marked this pull request as ready for review December 11, 2024 06:32
@codecov
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.89%. Comparing base (191dac6) to head (4089a3d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1193      +/-   ##
==========================================
- Coverage   84.95%   84.89%   -0.07%     
==========================================
  Files         323      323              
  Lines       25115    25124       +9     
  Branches     3859     3859              
==========================================
- Hits        21337    21328       -9     
- Misses       3036     3050      +14     
- Partials      742      746       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jansegre
jansegre previously approved these changes Dec 11, 2024
msbrogli
msbrogli previously approved these changes Dec 11, 2024
@glevco glevco dismissed stale reviews from msbrogli and jansegre via f42c4a7 December 11, 2024 20:59
@glevco glevco force-pushed the fix/update-acc-weight branch from f42c4a7 to 4089a3d Compare December 11, 2024 22:28
@glevco glevco merged commit 692a74c into master Dec 11, 2024
@glevco glevco deleted the fix/update-acc-weight branch December 11, 2024 23:38
jansegre added a commit that referenced this pull request Dec 17, 2024
@jansegre jansegre mentioned this pull request Dec 19, 2024
2 tasks
@jansegre jansegre mentioned this pull request Jan 9, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants