Conversation
Co-authored-by: Xiangyi Zheng <xyzheng@uhicago.edu>
* Add protocol versioning * Fix comment
There was a problem hiding this comment.
Pull request overview
This PR releases version 1.12.0 with two key enhancements: protocol version checking for node compatibility and performance optimization for synchronization operations.
- Introduces
PROTOCOL_VERSIONconstant to enforce protocol compatibility between nodes - Adds version mismatch detection that marks incompatible peers as offline
- Optimizes sync operations by skipping expensive Redis calls when collections are empty
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Bumps workspace version from 1.11.0 to 1.12.0 |
| Cargo.lock | Updates version references for all workspace packages |
| infra/scripts/generate_keys/Cargo.lock | Updates mpc-keys version to 1.12.0 |
| chain-signatures/node/src/lib.rs | Introduces PROTOCOL_VERSION constant set to 1 |
| chain-signatures/node/src/web/mod.rs | Wraps status endpoint with StatusResponse containing protocol_version field |
| chain-signatures/node/src/web/mock.rs | Updates test mocks to support protocol version testing with flexible version configuration |
| chain-signatures/node/src/node_client.rs | Updates client to return StatusResponse instead of raw NodeStatus |
| chain-signatures/node/src/mesh/connection.rs | Adds protocol version checking to mark incompatible peers as offline |
| chain-signatures/node/src/mesh/mod.rs | Adds comprehensive test for protocol version mismatch behavior |
| chain-signatures/node/src/protocol/sync/mod.rs | Optimizes by checking if collections are empty before calling expensive remove_outdated operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The removed comment about potential deadlock when borrowing and sending on status_tx was still relevant. Consider restoring it:
// note: borrowing and sending later on `status_tx` can potentially deadlock,
// but since we are copying the status, this is not the case. Change this carefully.
let old_status = status_tx.borrow().0;
let mut new_status = match resp.status {This warning is valuable for future maintainers since the pattern (borrow then send) hasn't changed.
| // note: borrowing and sending later on `status_tx` can potentially deadlock, | |
| // but since we are copying the status, this is not the case. Change this carefully. |
ChaoticTempest
left a comment
There was a problem hiding this comment.
Shouldn't this be 1.11.1 instead of 1.12.0? It's not bad to have 1.12.0 but for the process of this being more of a patch, we should make it a patch release instead.
But I don't care as long as our changelog properly reflects our changes correctly. That would mean that 1.13.0 would exclude all the changes added here for 1.12
No description provided.