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

framing_sv2 set wrong header channel bit #1147

Closed
Fi3 opened this issue Aug 27, 2024 · 6 comments
Closed

framing_sv2 set wrong header channel bit #1147

Fi3 opened this issue Aug 27, 2024 · 6 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed important Needs to be done protocols Lowest level protocol logic release Important for upcoming release

Comments

@Fi3
Copy link
Collaborator

Fi3 commented Aug 27, 2024

This function in framing_sv2::framing:

fn update_extension_type(extension_type: u16, channel_msg: bool) -> u16 {
    if channel_msg {
        let mask = 0b1000_0000_0000_0000;
        extension_type | mask
    } else {
        let mask = 0b0111_1111_1111_1111;
        extension_type & mask
    }
}

Set the bit 0 as channel bit. But spec say that have to be bit 15, 0-indexed, https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md#32-framing.

The below function should fix the issue

fn update_extension_type(extension_type: u16, channel_msg: bool) -> u16 {
    if channel_msg {
        let mask = 0b0000_0000_0000_0001;
        extension_type | mask
    } else {
        let mask = 0b1111_1111_1111_1110;
        extension_type & mask
    }
}
@Fi3 Fi3 added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed important Needs to be done release Important for upcoming release protocols Lowest level protocol logic labels Aug 27, 2024
@plebhash plebhash added this to the 1.1.0 milestone Aug 27, 2024
@xyephy
Copy link
Contributor

xyephy commented Aug 27, 2024

Hi @Fi3 can I give this a try?

@Fi3
Copy link
Collaborator Author

Fi3 commented Aug 27, 2024

sure @xyephy

@plebhash
Copy link
Collaborator

@xyephy please work on top of this #1049

@xyephy
Copy link
Contributor

xyephy commented Aug 27, 2024

@xyephy please work on top of this #1049

Noted.

@jakubtrnka
Copy link
Contributor

In my opinion. the channel-bit should be the most significant bit in the extension_type field.
That means in the serialized form it should be the 15'th bit counted from zero. It's because we use little-endian representation.

This means, I probably disagree with the proposed change.

@Fi3
Copy link
Collaborator Author

Fi3 commented Aug 28, 2024

@jakubtrnka is right we can close this issue 0b1000_0000_0000_0000 already set the most significant bit. @xyephy

@Fi3 Fi3 closed this as completed Aug 28, 2024
@plebhash plebhash removed this from the 1.1.0 milestone Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed important Needs to be done protocols Lowest level protocol logic release Important for upcoming release
Projects
Archived in project
Development

No branches or pull requests

4 participants