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

Fix pingpong teal test #2835

Merged
merged 8 commits into from
Sep 14, 2021
Merged

Conversation

egieseke
Copy link
Contributor

@egieseke egieseke commented Sep 3, 2021

Summary

Updated the netgoal utility to only create one wallet per non-participating node to better concentrate Algos for use by pingpong.

Test Plan

Ran netgoal to update the scenario2 recipe and tested the result with the CICD performance pipeline.

…lding per wallet.

Only create one wallet per NPN Host to consolidate value for pingpong testing.
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2021

Codecov Report

Merging #2835 (d3a27a5) into master (ac51e70) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2835      +/-   ##
==========================================
- Coverage   47.12%   47.09%   -0.03%     
==========================================
  Files         349      349              
  Lines       56434    56434              
==========================================
- Hits        26593    26580      -13     
- Misses      26859    26869      +10     
- Partials     2982     2985       +3     
Impacted Files Coverage Δ
network/wsPeer.go 72.14% <0.00%> (-3.07%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
catchup/service.go 69.35% <0.00%> (-0.78%) ⬇️
network/wsNetwork.go 61.09% <0.00%> (+0.18%) ⬆️
ledger/acctupdates.go 62.46% <0.00%> (+0.33%) ⬆️
network/requestTracker.go 71.12% <0.00%> (+0.86%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac51e70...d3a27a5. Read the comment docs.

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks good to me. Please make sure to regenerate all the scenarios with this change and include them in the PR.

@tsachiherman
Copy link
Contributor

Changing the total supply can trigger some failures that would not be possible on mainnet. Are we truely ran out of money?

@onetechnical
Copy link
Contributor

Changing the total supply can trigger some failures that would not be possible on mainnet. Are we truely ran out of money?

+1. I'm still not a fan of increasing the amount of supply, as it indicates an unrealistic test (we would never increase mainnet supply).

algobarb
algobarb previously approved these changes Sep 8, 2021
Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

Messaged Eric to learn more about the details. The results look correct on Datadog now so I think the last fix to rebalance stake fixed it! Changes look good to me.

Revert total non participating wallets to 400 as before.
@egieseke
Copy link
Contributor Author

egieseke commented Sep 8, 2021

looks good to me. Please make sure to regenerate all the scenarios with this change and include them in the PR.

Updated scenario1 and scenario3.

@egieseke
Copy link
Contributor Author

egieseke commented Sep 8, 2021

Changing the total supply can trigger some failures that would not be possible on mainnet. Are we truely ran out of money?

By rebalancing the stake between participating and non-participating nodes, increasing the total stake is not required. Reverted total stake back to 10 billion Algos.

Copy link
Contributor

@algobarb algobarb left a comment

Choose a reason for hiding this comment

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

Looks like Scenario1 results look ok with the rebalanced stake and removed wallets as well. Looks good to me.

@egieseke
Copy link
Contributor Author

Changing the total supply can trigger some failures that would not be possible on mainnet. Are we truely ran out of money?

+1. I'm still not a fan of increasing the amount of supply, as it indicates an unrealistic test (we would never increase mainnet supply).

@onetechnical Good point. Updated so that the total stake remains at 10 billion. Split stake evenly between participating and nonparticipating nodes, which provides a sufficient amount for the teal pingpong tests.

if npnHosts > 0 {
wallets = 2 * wallets
// split participating an non participating stake evenly
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we always been assuming an even split between participating and non participating nodes? I wonder if that should be configurable. It looks like that would be a new feature though so probably out of scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was evenly split before. I will create a ticket to make it configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created this ticket to allow configuration of stake ratio: https://github.com/algorand/go-algorand-internal/issues/1566

@algojohnlee algojohnlee merged commit 827ac62 into algorand:master Sep 14, 2021
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

7 participants