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

[Merged by Bors] - Don't rewrite poet config #4782

Closed
wants to merge 3 commits into from
Closed

Conversation

poszu
Copy link
Contributor

@poszu poszu commented Aug 7, 2023

Motivation

The poet config is unnecessarily and wrongly rewritten in node setup code.

Changes

Don't rewrite poet config at all.

activation/nipost.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #4782 (772c0fc) into develop (2c512aa) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           develop   #4782   +/-   ##
=======================================
  Coverage     76.7%   76.8%           
=======================================
  Files          260     260           
  Lines        29524   29522    -2     
=======================================
+ Hits         22672   22680    +8     
+ Misses        5407    5399    -8     
+ Partials      1445    1443    -2     
Files Changed Coverage Δ
activation/poet.go 75.6% <100.0%> (+0.4%) ⬆️
node/node.go 65.3% <100.0%> (-0.2%) ⬇️

... and 4 files with indirect coverage changes

@dshulyak
Copy link
Contributor

dshulyak commented Aug 7, 2023

would it be nice to have a test that correct config is being used? also true for other components obviously

@poszu
Copy link
Contributor Author

poszu commented Aug 7, 2023

would it be nice to have a test that correct config is being used? also true for other components obviously

Yes, but engineering a systest to check it would be really painful. Would you happen to have an idea of how to test it best?

@dshulyak
Copy link
Contributor

dshulyak commented Aug 7, 2023

maybe just a unit test to check that correct config is being used in poet client.
or systest, that ensures that we always select best poet proof even if it becomes available later, thinking:

  • throttle one of the poets with lower cpu limits - slow poet
  • make poet round end later (by 10s for example) on the fast poet
  • assert that all atxs are using fast poet

but it is indeed time consuming to setup all that

@poszu
Copy link
Contributor Author

poszu commented Aug 7, 2023

* throttle one of the poets with lower cpu limits - slow poet

* make poet round end later (by 10s for example) on the fast poet

Actually there is no need to throttle if one of the poets runs for less time, because then it will have less ticks.

Copy link
Contributor

@dshulyak dshulyak left a comment

Choose a reason for hiding this comment

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

Actually there is no need to throttle if one of the poets runs for less time, because then it will have less ticks.

true, but i assumed that 10s will not necessarily guarantee more ticks

@poszu
Copy link
Contributor Author

poszu commented Aug 7, 2023

bors merge

bors bot pushed a commit that referenced this pull request Aug 7, 2023
## Motivation
The poet config is unnecessarily and wrongly rewritten in node setup code.

## Changes
Don't rewrite poet config at all.
@bors
Copy link

bors bot commented Aug 7, 2023

Pull request successfully merged into develop.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Don't rewrite poet config [Merged by Bors] - Don't rewrite poet config Aug 7, 2023
@bors bors bot closed this Aug 7, 2023
@bors bors bot deleted the fix-poetcfg branch August 7, 2023 11:41
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