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

Consensus block value calculation for produceBlockV3 API call. #5873

Merged
merged 22 commits into from
Mar 11, 2024

Conversation

cheatfate
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Feb 9, 2024

Unit Test Results

         9 files  ±0    1 113 suites  ±0   27m 41s ⏱️ + 4m 24s
  4 240 tests ±0    3 893 ✔️ ±0  347 💤 ±0  0 ±0 
16 915 runs  ±0  16 517 ✔️ ±0  398 💤 ±0  0 ±0 

Results for commit c53822f. ± Comparison against base commit 2d519ac.

♻️ This comment has been updated with latest results.

@cheatfate cheatfate force-pushed the consensus-block-value branch 2 times, most recently from aa77553 to b6081cd Compare February 27, 2024 11:57
@tersec
Copy link
Contributor

tersec commented Mar 4, 2024

#6022 doesn't replace the "front-end" parts of this, but does provide a new back-end calculation approach

# Returns value of `block-consensus-value` in Wei units.
(uint64(r.attestations) + uint64(r.sync_aggregate) +
uint64(r.proposer_slashings) +
uint64(r.attester_slashings)).u256 * 1000000000.u256
Copy link
Contributor

@tersec tersec Mar 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This 1000000000.u256 is unnecessarily runtime-constructed, complete with the customary usually pointless nimZeroMem():

N_LIB_PRIVATE N_NIMCALL(void,
                        blockConsensusValue__validatorsZbeacon95validators_112)(
    tyObject_BlockRewards__9c3PGRoMCH9aJ4gpUEVcHWJA *r,
    tyObject_StUint__ExULMdD1zVCTKy89cyuy4Dg *Result) {
  tyObject_StUint__ExULMdD1zVCTKy89cyuy4Dg T1_;
  tyObject_StUint__ExULMdD1zVCTKy89cyuy4Dg T2_;
  nimZeroMem((void *)(&T1_), sizeof(tyObject_StUint__ExULMdD1zVCTKy89cyuy4Dg));
  u256__specZvalidator_12417(
      (NU64)((NU64)((NU64)((NU64)((NU64)((NU64)((*r).attestations) +
                                         (NU64)((*r).sync_aggregate))) +
                           (NU64)((*r).proposer_slashings))) +
             (NU64)((*r).attester_slashings)),
      (&T1_));

  nimZeroMem((void *)(&T2_), sizeof(tyObject_StUint__ExULMdD1zVCTKy89cyuy4Dg));
  u256__OOZvendorZnim45ethZethZcommonZeth95types_308(((NI)1000000000), (&T2_));

  star___OOZvendorZnim45stintZstint_435((&T1_), (&T2_), Result);
}

Better to define it as a const to avoid the

  nimZeroMem((void *)(&T2_), sizeof(tyObject_StUint__ExULMdD1zVCTKy89cyuy4Dg));
  u256__OOZvendorZnim45ethZethZcommonZeth95types_308(((NI)1000000000), (&T2_));

part.

Copy link
Contributor Author

@cheatfate cheatfate Mar 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish not to change it, because its annoying, please re-read all the comments before and lets end it please. If you think its correct, lets use it as it is, because i wish not to check c-code on this specific thing, there is a lot more u256 conversions around which was not highlighted.
Also block-proposal is made once per 12 seconds on mainnet, so its CPU intensive operation.

@tersec tersec merged commit f088e5f into unstable Mar 11, 2024
13 checks passed
@tersec tersec deleted the consensus-block-value branch March 11, 2024 14:18
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.

None yet

4 participants