I322 autoshutdown of ex validators#324
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements automatic shutdown functionality for nodes that transition from being validators to regular nodes, as defined in issue #322. The implementation includes a safeguard to prevent shutdown during major sync operations.
- Added logic to detect when a node transitions from validator to regular node status
- Implemented conditional shutdown that skips the operation if the node is currently syncing
- Increased the graceful shutdown timeout from 5 to 15 seconds to allow more time for cleanup
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/ethcore/src/engines/hbbft/hbbft_state.rs | Implements auto-shutdown logic for ex-validators with sync detection to prevent premature shutdown |
| bin/oe/run.rs | Increases soft shutdown timeout from 5 to 15 seconds to accommodate validator shutdown process |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | ||
| // for unit tests no problem, demand shutddown wont to anything if its a unit test. | ||
| // e2e tests needs adaption. |
There was a problem hiding this comment.
Spelling error: "adaption" should be "adaptation".
| // e2e tests needs adaption. | |
| // e2e tests needs adaptation. |
| if client | ||
| .as_full_client() | ||
| .expect("full client") | ||
| .is_major_syncing() | ||
| { | ||
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | ||
| } else { | ||
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | ||
| // for unit tests no problem, demand shutddown wont to anything if its a unit test. | ||
| // e2e tests needs adaption. | ||
| // this gracefully shuts down a node, if it was a validator before, but now it is not anymore. | ||
| client.demand_shutdown(); |
There was a problem hiding this comment.
The .expect("full client") will panic if as_full_client() returns None. Consider using proper error handling instead of panicking, especially in validator shutdown logic. For example:
if let Some(full_client) = client.as_full_client() {
if full_client.is_major_syncing() {
// handle syncing case
} else {
// handle shutdown case
}
}| if client | |
| .as_full_client() | |
| .expect("full client") | |
| .is_major_syncing() | |
| { | |
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | |
| } else { | |
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | |
| // for unit tests no problem, demand shutddown wont to anything if its a unit test. | |
| // e2e tests needs adaption. | |
| // this gracefully shuts down a node, if it was a validator before, but now it is not anymore. | |
| client.demand_shutdown(); | |
| if let Some(full_client) = client.as_full_client() { | |
| if full_client.is_major_syncing() { | |
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | |
| } else { | |
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | |
| // for unit tests no problem, demand shutddown wont to anything if its a unit test. | |
| // e2e tests needs adaption. | |
| // this gracefully shuts down a node, if it was a validator before, but now it is not anymore. | |
| client.demand_shutdown(); | |
| } | |
| } else { | |
| warn!(target: "engine", "Could not obtain full client for validator shutdown logic."); |
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | ||
| } else { | ||
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); |
There was a problem hiding this comment.
Multiple spelling and grammar errors in this debug message:
- "inhttps" should be "in https" (missing space)
- "Node" should be lowercase "node" for consistency (appears twice)
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | |
| } else { | |
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | |
| debug!(target: "engine", "node was a validator, and became regular node, but we are syncing, not shutting down node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | |
| } else { | |
| info!(target: "engine", "node was a validator, and became regular node. shutting down node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); |
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | ||
| } else { | ||
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | ||
| // for unit tests no problem, demand shutddown wont to anything if its a unit test. |
There was a problem hiding this comment.
Multiple spelling and grammar errors in this comment:
- "shutddown" should be "shutdown"
- "wont to anything" should be "won't do anything"
- Missing period at the end of the sentence
| // for unit tests no problem, demand shutddown wont to anything if its a unit test. | |
| // For unit tests, no problem; demand shutdown won't do anything if it's a unit test. |
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | ||
| } else { | ||
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); |
There was a problem hiding this comment.
Grammar improvements needed:
- "shutting down Node" should be "shutting down node" (lowercase 'node' for consistency)
- Consider capitalizing the first word after the period in the URL reference
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down Node as defined inhttps://github.com/DMDcoin/diamond-node/issues/322."); | |
| } else { | |
| info!(target: "engine", "Node was a validator, and became regular node. shutting down Node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | |
| debug!(target: "engine", "Node was a validator, and became regular node, but we are syncing, not shutting down node as defined in https://github.com/DMDcoin/diamond-node/issues/322. Shutting down node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); | |
| } else { | |
| info!(target: "engine", "Node was a validator, and became regular node. Shutting down node as defined in https://github.com/DMDcoin/diamond-node/issues/322."); |
No description provided.