Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Good catch, but I'm wondering why the subtest cleanup did not take care of the standby nodes?
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.
During a subtest the standby nodes are just reset, meaning their cached numbers (channels, UTXOs and so on) are reset, but the nodes aren't actually terminated. I think this has been this way for a while now, we just never noticed (or we did and didn't investigate).
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.
actually I just realized we should only shutdown standby nodes here - the non-standbys have already been shut down during the cleanup in
subtest
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.
Yeah, but there is no
shutdownStandbyNodes()
, sinceshutdownNodes
just takes a booleanskipStandby
.And if there are no non-standby nodes, shutting down all of them results in the same thing: just shutting down the standby ones.
Or would you like me to add a
shutdownStandbyNodes()
function?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.
Okay double checked, indeed the subtest cleanup missed shutdown of standby nodes if the test succeeded.
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.
think I recalled why I wrongfully removed it in the first place - when the test fails, we'll shut down the node twice, resulting in some error msgs being printed.
Nah I think you are right - there's map that tracks the active node, and we remove it from the map whenever we shut it down, so no need for this func.
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.
Hmm, I just tried this out by causing a test to fail and I didn't see duplicate or weird error messages... Maybe that was fixed in another way in the meantime?
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.
yeah likely