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
allow IPCVal to be instantiated from config #2866
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2866 +/- ##
===========================================
+ Coverage 62.1% 62.33% +0.23%
===========================================
Files 212 212
Lines 17288 17314 +26
===========================================
+ Hits 10736 10793 +57
+ Misses 5647 5620 -27
+ Partials 905 901 -4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! The PR looks great. I left a few nits. As a general note, I wonder if we shouldn't refactor out the logic that creates and starts the priv validators into a separate function. NewNode is already very long... Not blocking this PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍃 🌴 🌤
Thanks @joe-bowman
bcf97f3
to
2af6ac5
Compare
…v validator instantiations
d11d325
to
025df5e
Compare
If 0.26.4 is planned to be release in preparation for GoS, is there any chance to get this merged in too? It doesn't touch any existing code, so should be low risk but will enable some participants to run their planned production configurations for GoS. |
@@ -2,8 +2,6 @@ | |||
|
|||
## v0.26.4 | |||
|
|||
*TBD* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*TBD* | |
*TBD* |
|
||
n, err := DefaultNewNode(config, log.TestingLogger()) | ||
|
||
assert.NoError(t, err, "expected no err on DefaultNewNode") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NoError(t, err, "expected no err on DefaultNewNode") | |
assert.NoError(t, err) |
|
||
n, err := DefaultNewNode(config, log.TestingLogger()) | ||
|
||
assert.NoError(t, err, "expected no err on DefaultNewNode") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NoError(t, err, "expected no err on DefaultNewNode") | |
assert.NoError(t, err) |
config.BaseConfig.PrivValidatorListenAddr = addr | ||
n, err := DefaultNewNode(config, log.TestingLogger()) | ||
|
||
assert.NoError(t, err, "expected no err on DefaultNewNode") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NoError(t, err, "expected no err on DefaultNewNode") | |
assert.NoError(t, err) |
* update ConsensusState#OnStop comment * consensus: set logger for WAL in tests * refactor privValidator client code and tests follow-up on #2866
Ref #2827
(I have since seen #2847 which is a fix for the same issue; this PR has tests and docs too ;) )