Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use exhaustive match statement in peer/connection.rs #1027

Closed
yaahc opened this issue Sep 8, 2020 · 1 comment · Fixed by #1531
Closed

use exhaustive match statement in peer/connection.rs #1027

yaahc opened this issue Sep 8, 2020 · 1 comment · Fixed by #1531
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement

Comments

@yaahc
Copy link
Contributor

yaahc commented Sep 8, 2020

based on a comment in code review


do we want to try to find a way to get exhaustive matching here?

Originally posted by @yaahc in #1016 (comment)

@hdevalence
Copy link
Contributor

hdevalence commented Sep 8, 2020

The relevant part of connection.rs is in handle_message_as_request, which is responsible for interpreting a Bitcoin message as a Request from a remote peer, then passing it to our inbound network service.

Ideally, this function should be handled as a single match statement, where each match arm is independent of the others and handles exactly one pattern. This can make the code superficially uglier, e.g.,

            Message::GetData(items)
                if items
                    .iter()
                    .all(|item| matches!(item, InventoryHash::Tx(_))) => { /* ... */ }

but makes the code easier to analyze and change (because the match arms are more independent of each other, and each one does one thing.

Currently there are three match statements in a row, with _ wildcard cases. Ideally they would be combined into one match statement, with no wildcard cases. Initially, this will probably require adding a bunch of patterns whose bodies are like

Message::Foo => {
    tracing::debug!("unhandled message type Foo");
    None
}

but it will be useful to enumerate all of the messages we need to be able to handle, rather than having a single catch-all "unhandled" arm.

@yaahc yaahc added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement E-med labels Sep 10, 2020
@yaahc yaahc added this to the Sync and validate zcash mainnet milestone Sep 10, 2020
@mpguerra mpguerra removed this from the Sync and Network milestone Jan 5, 2021
@yaahc yaahc closed this as completed Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants