Skip to content

Include PoS NetUpgrades in RegTest when specified from the command line #1021

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
merged 3 commits into from
Jul 4, 2023

Conversation

alfiedotwtf
Copy link
Contributor

This will be needed for #1004

Copy link
Contributor

@iljakuklic iljakuklic left a comment

Choose a reason for hiding this comment

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

I am fine with this in principle. But agree with @azarovh that some of the code duplication should be addressed and it should also be relatively easy to replicate the behaviour of the command line switch using a simple method call (on a builder or something like that) so we can get the same setting in unit tests too.

@alfiedotwtf
Copy link
Contributor Author

But agree with @azarovh that some of the code duplication should be addressed

Yep, agreed. Fixed.

it should also be relatively easy to replicate the behaviour of the command line switch using a simple method call (on a builder or something like that) so we can get the same setting in unit tests too.

To make pos-netupgrades an accessor to Builder I would need to add it to ChainConfigBuilder, which would then be seen to all chain types - something that should probably be avoided given we only want this flag in RegTest. As for getting the same setting under unit tests, ChainConfigOptions has all fields as pub so if need be it can be accessed there.

@alfiedotwtf alfiedotwtf force-pushed the alfie/pos-netupgrades branch from 3c58238 to 1f5f1ac Compare July 4, 2023 06:47
Copy link
Contributor

@TheQuantumPhysicist TheQuantumPhysicist left a comment

Choose a reason for hiding this comment

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

Unify the impl blocks and rename to regtest. Otherwise this looks good.

@alfiedotwtf alfiedotwtf merged commit d686c42 into master Jul 4, 2023
@alfiedotwtf alfiedotwtf deleted the alfie/pos-netupgrades branch July 4, 2023 09:47
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.

4 participants