Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2887 +/- ##
==========================================
- Coverage 57.17% 57.16% -0.02%
==========================================
Files 2091 2091
Lines 171543 171528 -15
==========================================
- Hits 98081 98051 -30
+ Misses 64713 64708 -5
- Partials 8749 8769 +20
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
masih
left a comment
There was a problem hiding this comment.
This is a bigger change than I was expecting: it fixes the bug discussed in Slack, and fundamentally changes the behaviour of the node. In that, after this change the process errors out instead of softly restarting the reactor in-process.
This is clearly simpler, but it means that without an external process supervisor there is no "restart" after self-remediation. The process simply dies.
I probably would have broken this into two PRs, just for the sake of release notes: one to fix the clear bug regarding restart trigger during cooldown, and one to remove it. At the very least this needs a mention in the next release notes for 6.3 patch release. Cc @philipsu522
No blockers.
0fb652c to
9a2738b
Compare
These cooldowns provide no value, by keeping the process alive while it does not make any sense. Also there was a semantic mismatch between blocksync sending at most 1 message to the restartCh, and WaitForQuitSignals which ignored messages until cooldown has passed. I think that cooldown in blocksync logic was exactly trying to avoid sending messages when WaitForQuitSignals could have ignored them, which is a very fragile way of doing things.