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

drop deprecated authentication_public_key from pool config #17959

Merged
merged 2 commits into from
May 4, 2024

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented May 3, 2024

Purpose:

As the TODO-comment explains:

TODO: remove this a few versions after 1.3, since authentication_public_key is deprecated. This is here to support
downgrading to versions older than 1.3.

I think it's safe to remove this. I discovered this as the current implementation of this backwards-compatibility also is not working right. It causes the _periodically_update_pool_state_task to run continuously. The reason is that calling add_auth_key() loads and re-saves the config file, thus bumping its "mtime". We use the mtime to determine whether ewe need to run this task early, rather than waiting the 60 secons we are supposed to.

Running the task calls add_auth_key() resulting in continuously running the task, wasting CPU and disk I/O.

This is especially apparent when you have multiple plotnfts.

Current Behavior:

When having many plotnfts, the farmer process pegs the CPU every 2 seconds. See the profile attached below.

New Behavior:

Having many plotnfts only causes the farmer process to perform the expensive work every 60 seconds. (There's still room for improvements in this regard, but this patch covers 95% of the problems).

Testing Notes:

I profiled the farmer process, depending on #17953

profile:

chia-hotspot-2

@arvidn arvidn added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label May 3, 2024
@arvidn arvidn changed the title Pool config list drop deprecated "authentication_public_key" from pool config May 3, 2024
@arvidn arvidn changed the title drop deprecated "authentication_public_key" from pool config drop deprecated authentication_public_key from pool config May 3, 2024
@arvidn arvidn requested a review from altendky May 3, 2024 06:47
Copy link
Contributor

github-actions bot commented May 3, 2024

File Coverage Missing Lines
chia/pools/pool_config.py 76.9% lines 91, 98-99
Total Missing Coverage
14 lines 3 lines 78%

Copy link

Pull Request Test Coverage Report for Build 8935153853

Details

  • 11 of 14 (78.57%) changed or added relevant lines in 2 files are covered.
  • 42 unchanged lines in 10 files lost coverage.
  • Overall coverage decreased (-0.006%) to 90.68%

Changes Missing Coverage Covered Lines Changed/Added Lines %
chia/pools/pool_config.py 10 13 76.92%
Files with Coverage Reduction New Missed Lines %
chia/harvester/harvester_api.py 1 73.02%
chia/full_node/full_node_api.py 1 80.92%
chia/rpc/rpc_server.py 1 89.0%
chia/wallet/util/wallet_sync_utils.py 1 86.54%
chia/data_layer/data_layer.py 2 85.26%
chia/data_layer/data_layer_wallet.py 2 91.26%
chia/wallet/wallet_node.py 4 88.46%
chia/server/node_discovery.py 4 79.96%
chia/full_node/full_node.py 4 85.19%
chia/_tests/core/util/test_lockfile.py 22 77.73%
Totals Coverage Status
Change from base Build 8931619722: -0.006%
Covered Lines: 97989
Relevant Lines: 108007

💛 - Coveralls

@arvidn arvidn marked this pull request as ready for review May 3, 2024 08:27
@arvidn arvidn requested a review from a team as a code owner May 3, 2024 08:27
@arvidn arvidn requested a review from fchirica May 3, 2024 13:19
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

sounds like this might fix the config load spam i've seen and reported a few times... but never made time to dig into. that'd be awesome. :] thanks

Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

"remove this a few versions after 1.3" - yea I guess we can do that :-)

@pmaslana pmaslana merged commit fdb87e6 into main May 4, 2024
357 of 359 checks passed
@pmaslana pmaslana deleted the pool-config-list branch May 4, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants