Introduce Request Id for binary port communication #315
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
May 22, 2024 12:05
rafal-ch
changed the title
rafal-ch
commented
May 28, 2024
zajko
reviewed
May 30, 2024
Cargo.toml
Outdated
| @@ -31,3 +31,7 @@ toml = "0.5.8" | |||
| tracing = { version = "0", default-features = false } | |||
| tracing-subscriber = "0" | |||
| serde = { version = "1", default-features = false } | |||
|
|
|||
| [patch.'https://github.com/casper-network/casper-node.git'] | |||
| casper-binary-port = { path = "../casper-node/binary_port" } | |||
There was a problem hiding this comment.
This won't work in CI, or in any workspace that doesn't have ../casper-node, right?
Wouldn't it be more universal if you pushed your casper-node changes to a branch and refer casper-binary-port to a github branch?
zajko
reviewed
May 30, 2024
| @@ -116,6 +118,14 @@ fn resolve_address(address: &str) -> anyhow::Result<SocketAddr> { | |||
| .ok_or_else(|| anyhow::anyhow!("failed to resolve address")) | |||
| } | |||
|
|
|||
| fn encode_request(req: &BinaryRequest, id: u16) -> Result<Vec<u8>, bytesrepr::Error> { | |||
| let header = BinaryRequestHeader::new(SUPPORTED_PROTOCOL_VERSION, req.tag(), id); | |||
There was a problem hiding this comment.
So this is a header that has both SUPPORTED_PROTOCOL_VERSION and the request id? Shouldn't it be a wrapper over a vector of headers? A tuple (u8, bytes[]) where the u8 would be a header tag? That would give us flexibility of adding more headers in the future. If we don't do it like that - we're painting ourselves into a corner, aren't we?
There was a problem hiding this comment.
I dodged the issue by introducing the "header version" field which is a single u16. Upon handling the request, the binary port component will reject the requests that come with different header version. When we change the header again post 2.0 release, users will need to also upgrade their sidecar installations.
I decided not to support multiple headers because when we're adding a field it usually means that we actually need it for further processing. For example, if we added request id, but we get request w/o request id (from legacy sidecar) we can't properly handle it anyway.
added 9 commits
June 5, 2024 14:41
alsrdn
approved these changes
Jun 14, 2024
casperlabs-bors-ng bot
added a commit
to casper-network/casper-node
that referenced
this pull request
Jun 18, 2024
4726: Introduce Request Id for binary port communication r=rafal-ch a=rafal-ch Heads-up: There are several functional changes in this single PR just because each breaking change to the sidecar requires some coordinated merging effort between node and sidecar. Hence, it's more convenient to deliver such changes in one go. This PR introduces: 1. the concept of "request id" to the binary port. This is used to dodge an edge case where it may happen that a response which has not been consumed is incorrectly served as a request to the next request. The corresponding PR in casper-sidecar is [here](casper-network/casper-sidecar#315). 2. Binary request header version - this will prevent binary port from trying to parse binary message headers of incompatible types 3. Promote binary port error code from `u8` to `u16`, because we already have nearly 60 error codes 4. Additional unit tests verifying whether the binary message codec can properly handle large quantities of messages. 5. Additional tests to see if protocol version check is respected Fixes: casper-network/casper-sidecar#312 Co-authored-by: Rafał Chabowski <[email protected]>
casperlabs-bors-ng bot
added a commit
to casper-network/casper-node
that referenced
this pull request
Jun 18, 2024
4726: Introduce Request Id for binary port communication r=rafal-ch a=rafal-ch Heads-up: There are several functional changes in this single PR just because each breaking change to the sidecar requires some coordinated merging effort between node and sidecar. Hence, it's more convenient to deliver such changes in one go. This PR introduces: 1. the concept of "request id" to the binary port. This is used to dodge an edge case where it may happen that a response which has not been consumed is incorrectly served as a request to the next request. The corresponding PR in casper-sidecar is [here](casper-network/casper-sidecar#315). 2. Binary request header version - this will prevent binary port from trying to parse binary message headers of incompatible types 3. Promote binary port error code from `u8` to `u16`, because we already have nearly 60 error codes 4. Additional unit tests verifying whether the binary message codec can properly handle large quantities of messages. 5. Additional tests to see if protocol version check is respected Fixes: casper-network/casper-sidecar#312 Co-authored-by: Rafał Chabowski <[email protected]>
casperlabs-bors-ng bot
added a commit
to casper-network/casper-node
that referenced
this pull request
Jun 18, 2024
4726: Introduce Request Id for binary port communication r=rafal-ch a=rafal-ch Heads-up: There are several functional changes in this single PR just because each breaking change to the sidecar requires some coordinated merging effort between node and sidecar. Hence, it's more convenient to deliver such changes in one go. This PR introduces: 1. the concept of "request id" to the binary port. This is used to dodge an edge case where it may happen that a response which has not been consumed is incorrectly served as a request to the next request. The corresponding PR in casper-sidecar is [here](casper-network/casper-sidecar#315). 2. Binary request header version - this will prevent binary port from trying to parse binary message headers of incompatible types 3. Promote binary port error code from `u8` to `u16`, because we already have nearly 60 error codes 4. Additional unit tests verifying whether the binary message codec can properly handle large quantities of messages. 5. Additional tests to see if protocol version check is respected Fixes: casper-network/casper-sidecar#312 Co-authored-by: Rafał Chabowski <[email protected]>
zajko
approved these changes
Jun 18, 2024
casperlabs-bors-ng bot
added a commit
to casper-network/casper-node
that referenced
this pull request
Jun 18, 2024
4726: Introduce Request Id for binary port communication r=rafal-ch a=rafal-ch Heads-up: There are several functional changes in this single PR just because each breaking change to the sidecar requires some coordinated merging effort between node and sidecar. Hence, it's more convenient to deliver such changes in one go. This PR introduces: 1. the concept of "request id" to the binary port. This is used to dodge an edge case where it may happen that a response which has not been consumed is incorrectly served as a request to the next request. The corresponding PR in casper-sidecar is [here](casper-network/casper-sidecar#315). 2. Binary request header version - this will prevent binary port from trying to parse binary message headers of incompatible types 3. Promote binary port error code from `u8` to `u16`, because we already have nearly 60 error codes 4. Additional unit tests verifying whether the binary message codec can properly handle large quantities of messages. 5. Additional tests to see if protocol version check is respected Fixes: casper-network/casper-sidecar#312 Co-authored-by: Rafał Chabowski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR introduces the concept of "request id" to the binary port. This is used to dodge an edge case where it may happen that a response which has not been consumed is incorrectly served as a request to the next request.
The corresponding PR in casper-node is here.
Fixes: #312