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.

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

Allow to override chainstate options for regtest #301

Merged
merged 14 commits into from
Jul 26, 2022
Merged

Allow to override chainstate options for regtest #301

merged 14 commits into from
Jul 26, 2022

Conversation

stanislav-tkach
Copy link
Contributor

No description provided.

iljakuklic
iljakuklic reviewed Jul 22, 2022
View reviewed changes
Copy link
Contributor

@iljakuklic iljakuklic left a comment •
edited

Choose a reason for hiding this comment

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

Adding some initial observations, will have a proper look next week

node/Cargo.toml Show resolved Hide resolved
test/functional/test_framework/test_node.py
@@ -107,7 +107,7 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, timeout_factor, bitc
self.args = [
self.binary,
"--datadir={}".format(datadir_path),
"run",
"regtest",
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool I kind of like the command-based interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad to hear! I was afraid that no one would like this approach.

node/src/runner.rs Show resolved Hide resolved
@stanislav-tkach stanislav-tkach marked this pull request as ready for review July 25, 2022 07:56
iljakuklic
iljakuklic reviewed Jul 25, 2022
View reviewed changes
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.

Adding some more comments. Approving now but I think the coin_decimals one should be addressed or at least discussed.

node/src/runner.rs Show resolved Hide resolved
node/src/runner.rs Show resolved Hide resolved
TheQuantumPhysicist
TheQuantumPhysicist reviewed Jul 25, 2022
View reviewed changes
Copy link
Collaborator

@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.

The minimal requirement to get this PR ready is to support testnet configuration too.

I also don't see tests for ChainConfig settings either, which I'm not sure about. Do we want to have tests for that?

@stanislav-tkach
Copy link
Contributor Author

stanislav-tkach commented Jul 25, 2022 •
edited

@TheQuantumPhysicist I'm not sure about the tests. Ideally I would prefer these changes to be tested in two ways:

@TheQuantumPhysicist
Copy link
Collaborator

@TheQuantumPhysicist I'm not sure about the tests. Ideally I would prefer these changes to be tested in two ways:

  • Simple assert_cmd tests. This currently isn't trivial because assert_cmd don't allows to pass signals to the binary and we need that to stop the node.
  • Indirectly via functional tests. As far as I can see, the functional tests need a lot of work, so this is outside the scope of this pull request.

Actually with functional tests should happen sooner or later. If you think you can do it (in this PR or another one), I'm all for that. Again, I think this is a good opportunity to practice a little with functional tests. After all, there has to be some form of testing for these command line arguments.

@stanislav-tkach
Copy link
Contributor Author

I suppose I can do that, but I would prefer to work on functional tests in a separate pull request.

@stanislav-tkach stanislav-tkach merged commit 2bf5b35 into master Jul 26, 2022
@stanislav-tkach stanislav-tkach deleted the node-chainstate-config-options branch July 26, 2022 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iljakuklic iljakuklic iljakuklic approved these changes

@TheQuantumPhysicist TheQuantumPhysicist TheQuantumPhysicist approved these changes

@altonen altonen altonen approved these changes

@azarovh azarovh Awaiting requested review from azarovh azarovh is a code owner

@muursh muursh Awaiting requested review from muursh

@erubboli erubboli Awaiting requested review from erubboli erubboli is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stanislav-tkach @TheQuantumPhysicist @iljakuklic @altonen