Return better error messages from P2P requests #1311
Assignees
Comments
|
We can try using error codes and just make sure we have graceful handling of unknown error codes. |
11 tasks
11 tasks
acerone85
added a commit
that referenced
this issue
Oct 22, 2024
## Linked Issues/PRs <!-- List of related issues/PRs --> Related issue: #1311 ## Context The P2P service uses the [request_response](https://docs.rs/libp2p-request-response/latest/libp2p_request_response/) protocol to let peer request information (e.g. block headers, or transactions in the tx pool) from other peers. Currently, the P2P service provides a protocol `/fuel/req_res/0.0.1`, which defines responses to have an `Option` value. When a request from a peer is unsuccessful, we send a `None` value as a response, wrapped around the relevant response type. In practice, we want to be able to send more meaningful information in responses when requests are unsuccessful, in the form of error codes. This requires implementing a new version of the protocol `/fuel/req_res/0.0.2` where Response messages are changed to contain error codes. Because we want to be backward compatible with peers that will only use the version `0.0.1` of the protocol, we must account for requests sent by, and responses sent to, those peers. ## Description - Rename the `ResponseMessage` type in the P2P service to `LegacyResponseMessage` - Add a `ResponseMessageErrorCode` enum, which for now only contains a `LegacyNodeEmptyResponse` enum. - Implement a new `ResponseMessage` type that returns a `Result<_, ResponseMessageErrorCode>` wrapped around enum variants - Change P2P service to use the neq `ResponseMessage` type. Responses previously returned as `None` are now returned as `Err<LegacyNodeEmptyResponse>`. - Change the `PostcardCodec` to serialize and deserialize according to two different versions of the `/fuel/req_res` protocol: `/fuel/req_res/0.0.1` and `/fuel/req_res/0.0.2`. - Serialization and deserialization of responses using `/fuel/req_res/0.0.1` are performed by first converting a `ResponseMessage` into a `LegacyResponseMessage` and vicecersa, thus preserving backward-compatibility - Change the req_res behaviour used by the `P2P` service to use both version of the req_res protocol. ### TODO (here or in different PRs) - [ ] Investigate how protocols are exchanged during peer handshakes. We don't want two nodes that both understand `/fuel/req_res/0.0.2` to communicate using `/fuel/req_res/0.0.1`. - [ ] Add more meaningful error codes. ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [x] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [x] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else?
12 tasks
acerone85
added a commit
that referenced
this issue
Nov 14, 2024
## Linked Issues/PRs <!-- List of related issues/PRs --> Closes #1311 ## Description <!-- List of detailed changes --> This PR follows #2362, which introduces a new version of the protocol `/fuel/req_response/0.0.2` which allows to return Errors in the response. This PR makes the following changes: - [X] the different errors that can be returned in a response are implemented. - [X] The type of `ResponseSender` used by a peer to propagate received responses upstream is changed to allow the rrors to be propagated - [X] Error Responses received by a peer following a request are displayed with as log messages with `warning` level. ## Checklist - [ ] Breaking changes are clearly marked as such in the PR description and changelog - [ ] New behavior is reflected in tests - [ ] [The specification](https://github.com/FuelLabs/fuel-specs/) matches the implemented behavior (link update PR if changes are needed) ### Before requesting review - [ ] I have reviewed the code myself - [ ] I have created follow-up issues caused by this PR and linked them here ### After merging, notify other teams [Add or remove entries as needed] - [ ] [Rust SDK](https://github.com/FuelLabs/fuels-rs/) - [ ] [Sway compiler](https://github.com/FuelLabs/sway/) - [ ] [Platform documentation](https://github.com/FuelLabs/devrel-requests/issues/new?assignees=&labels=new+request&projects=&template=NEW-REQUEST.yml&title=%5BRequest%5D%3A+) (for out-of-organization contributors, the person merging the PR will do this) - [ ] Someone else?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Over P2P, when a party requests a value and the sender can't fulfill the request, we aren't providing adequate error reporting.
For example, if you request a
Blockby block height and that block doesn't exist, you will receive aNonewith no explanation. It would be better to receive an error explaining that the block for requested height does not exist.The text was updated successfully, but these errors were encountered: