Skip to content

refactor: Replace m_params with chainman.GetParams() #25830

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2022
Merged

refactor: Replace m_params with chainman.GetParams() #25830

merged 1 commit into from
Oct 19, 2022

Conversation

aureleoules
Copy link
Contributor

@aureleoules aureleoules commented Aug 12, 2022 •
edited
Loading

Fixes a TODO introduced in #24595.
Removes m_params from CChainState class and replaces it with m_chainman.GetParams().

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 13, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25172 (refactor: use std:: prefix for std lib funcs by fanquake)
  • #20827 (During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr)
  • #9245 (Drop IO priority to idle while reading blocks for peer requests and startup verification by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Aug 13, 2022
Closed
Merged
Closed
Merged
Merged
@aureleoules aureleoules force-pushed the 2022-08-replace-m_params branch from 83efe63 to 672fdf3 Compare September 16, 2022 12:18
@DrahtBot DrahtBot mentioned this pull request Sep 19, 2022
Merged
@aureleoules aureleoules force-pushed the 2022-08-replace-m_params branch from 672fdf3 to 5d3f98d Compare October 10, 2022 15:44
@maflcko
Copy link
Member

maflcko commented Oct 13, 2022

Why the close?

@aureleoules
Copy link
Contributor Author

@MarcoFalke I was asked by @adamjonas to close this during coredev.

@maflcko
Copy link
Member

maflcko commented Oct 13, 2022

It is fixing a TODO, so if that is no longer applicable, the TODO should be removed

@adamjonas
Copy link
Member

@MarcoFalke the request to close was based on lack of review and, therefore, the implied lack of support. If you'd be willing to review, @aureleoules can reopen.

@maflcko
Copy link
Member

maflcko commented Oct 13, 2022

sure

@aureleoules aureleoules reopened this Oct 13, 2022
@aureleoules
Copy link
Contributor Author

Note that I could have reduced the code diff by renaming params to m_params but since params is not a class member anymore I think its more confusing.

@DrahtBot DrahtBot mentioned this pull request Oct 18, 2022
Closed
maflcko
maflcko reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

review ACK 5d3f98d 🌎

Show signature

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 5d3f98d27879cd6d84b8590e947336e8d09613ed 🌎
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjJ8Qv/ct7XpfKidLyXHj8qvtiMkjeM7lH4rtiwWVOi23PmFTl3UXN+CJZwuXlt
P7onWWziczOZIJzJVVwFKgbOegIfG2QNWlVpSWsgQRYXYdCi8wRyqmsChxKUNdQ/
+Yl2ny0CMpoEORl1rD8/wFdvBhpKbkVDl/dbCKLmz1KUxLfFDWPfcZ2VdyCrtoKr
+jXmBUtYVGciQaD/MK638lK/AgBEy1GzbBnnBZKg8oKi729FsEzFY9C7mGFT7OMc
RMn2hRKMdl9Vjz5ffBVp7DXC5tHTG6RJMZAIilI6xdw77BX5qFK5+HqGsOh/27sx
sn6mA0kP+tqPxV4/lD6PgtoYpbmQN9afxVJ8zFLCOM/TU4gIbQzlApQ1iM4/x+jp
m5E3lE0OAmWgns8YQ1ui2VkbcQhPU+0OfzoZC/3VPH5fGF5L2fW0aFsdgy2jT4Ph
pIFZjGbXvI8m/WEok1Rps4ZEKcIqkFjK0q3l29gJILPHZNtagEf4s4zqWc7czmtd
DiVFo5K1
=Udt+
-----END PGP SIGNATURE-----

@maflcko
Copy link
Member

maflcko commented Oct 19, 2022

In the future it might be best not to add refactoring TODOs?

@maflcko maflcko merged commit a97791d into bitcoin:master Oct 19, 2022
@aureleoules aureleoules deleted the 2022-08-replace-m_params branch November 2, 2022 10:52
@bitcoin bitcoin locked and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Refactoring Validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aureleoules @DrahtBot @maflcko @adamjonas @fanquake