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

Refactor: const-sv2 crate #1135

Open
rrybarczyk opened this issue Aug 21, 2024 · 7 comments
Open

Refactor: const-sv2 crate #1135

rrybarczyk opened this issue Aug 21, 2024 · 7 comments
Labels
const-sv2 protocols Lowest level protocol logic refactor Implies refactoring code

Comments

@rrybarczyk
Copy link
Collaborator

rrybarczyk commented Aug 21, 2024

Background

This task is an outcome of the protocols Rust docs issues tracked in #845.

While documenting protocols::v2::const-sv2 in #1016, areas of potential code debt were identified. This issue servers as a place to list out these items to then be addressed in an organized manner. The initial Rust documentation effort was an immediate action, while a refactoring (which implies breaking API changes) is not so urgent priority, so for now we should leave this in the backlog for an appropriate moment in the future.

Identified Potential Code Debt

  • SV2_FRAME_HEADER_LEN_OFFSET, SV2_FRAME_HEADER_LEN_END, and NOISE_FRAME_MAX_SIZE are not used anywhere --> we should just remove them
  • ENCRYPTED_SV2_FRAME_HEADER_SIZE and NOISE_FRAME_HEADER_SIZE are declared in sv2-ffi, and imported in framing_sv2/src/header.rs, which is then imported into codec_sv2 just for this constant --> we should import them directly into codec_sv2 (cc @rrybarczyk)
  • NOISE_FRAME_HEADER_LEN_OFFSET is imported in framing_sv2 in the same way here, but then its alias is never used anywhere else --> we should remove it
  • RESPONDER_EXPECTED_HANDSHAKE_MESSAGE_SIZE is an alias of ELLSWIFT_ENCODING_SIZE --> we should create the alias where it's used, not here
  • MAC it's the same as AEAD_MAC_LEN --> we should remove it
  • NOISE_SUPPORTED_CIPHERS_MESSAGE is used to signal AESG support but we're dropping support for AESG --> we should remove it when we will officially remove support from our implementation
  • SV2_TEMPLATE_DISTR_PROTOCOL_DISCRIMINANT --> I would rename it in SV2_TEMPLATE_DISTRIBUTION_PROTOCOL_DISCRIMINANT
  • MESSAGE_TYPE_OPEN_EXTENDED_MINING_CHANNEL_SUCCES --> fix typo with MESSAGE_TYPE_OPEN_EXTENDED_MINING_CHANNEL_SUCCESS
  • MESSAGE_TYPE_RECONNECT is defined as 0x25 but according to specs it's 0x04 --> we need to move it to 0x04 and shift MESSAGE_TYPE_SET_GROUP_CHANNEL to 0x25 (we are not specs compliant now)

A final consideration could be to group constants in different modules (Mining Protocol, Job Distribution Protocol, Job Declaration Protocol, Common, Encryption, etc) so that it will be more clear also on https://docs.rs/. Here's an example:

/// Constants related to the SV2 frame structure.
pub mod frame {
    pub const EXTENSION_TYPE_NO_EXTENSION: u16 = 0;
    pub const SV2_FRAME_HEADER_SIZE: usize = 6;
    pub const SV2_FRAME_HEADER_LEN_OFFSET: usize = 3;
    pub const SV2_FRAME_HEADER_LEN_END: usize = 3;
    pub const SV2_FRAME_CHUNK_SIZE: usize = 65535;
    pub const AEAD_MAC_LEN: usize = 16;
    pub const ENCRYPTED_SV2_FRAME_HEADER_SIZE: usize = SV2_FRAME_HEADER_SIZE + AEAD_MAC_LEN;
}

/// Discriminants for distinct Stratum V2 (sub)protocols. 
pub mod protocol {
    pub const SV2_MINING_PROTOCOL_DISCRIMINANT: u8 = 0;
    pub const SV2_JOB_DECLARATION_PROTOCOL_DISCRIMINANT: u8 = 1;
    pub const SV2_TEMPLATE_DISTR_PROTOCOL_DISCRIMINANT: u8 = 2;
    pub const MESSAGE_TYPE_SETUP_CONNECTION: u8 = 0x0;
    pub const MESSAGE_TYPE_SETUP_CONNECTION_SUCCESS: u8 = 0x1;
    pub const MESSAGE_TYPE_SETUP_CONNECTION_ERROR: u8 = 0x2;
    pub const MESSAGE_TYPE_CHANNEL_ENDPOINT_CHANGED: u8 = 0x3;
}

/// Mining Protocol message types.
pub mod mining {
    pub const MESSAGE_TYPE_OPEN_STANDARD_MINING_CHANNEL: u8 = 0x10;
    pub const MESSAGE_TYPE_OPEN_STANDARD_MINING_CHANNEL_SUCCESS: u8 = 0x11;
    pub const MESSAGE_TYPE_OPEN_MINING_CHANNEL_ERROR: u8 = 0x12;
    pub const MESSAGE_TYPE_OPEN_EXTENDED_MINING_CHANNEL: u8 = 0x13;
    pub const MESSAGE_TYPE_OPEN_EXTENDED_MINING_CHANNEL_SUCCES: u8 = 0x14;
    pub const MESSAGE_TYPE_NEW_MINING_JOB: u8 = 0x15;
    pub const MESSAGE_TYPE_UPDATE_CHANNEL: u8 = 0x16;
    pub const MESSAGE_TYPE_UPDATE_CHANNEL_ERROR: u8 = 0x17;
    pub const MESSAGE_TYPE_CLOSE_CHANNEL: u8 = 0x18;
    pub const MESSAGE_TYPE_SET_EXTRANONCE_PREFIX: u8 = 0x19;
    pub const MESSAGE_TYPE_SUBMIT_SHARES_STANDARD: u8 = 0x1a;
    pub const MESSAGE_TYPE_SUBMIT_SHARES_EXTENDED: u8 = 0x1b;
    pub const MESSAGE_TYPE_SUBMIT_SHARES_SUCCESS: u8 = 0x1c;
    pub const MESSAGE_TYPE_SUBMIT_SHARES_ERROR: u8 = 0x1d;
    pub const MESSAGE_TYPE_NEW_EXTENDED_MINING_JOB: u8 = 0x1f;
    pub const MESSAGE_TYPE_MINING_SET_NEW_PREV_HASH: u8 = 0x20;
    pub const MESSAGE_TYPE_SET_TARGET: u8 = 0x21;
    pub const MESSAGE_TYPE_SET_CUSTOM_MINING_JOB: u8 = 0x22;
    pub const MESSAGE_TYPE_SET_CUSTOM_MINING_JOB_SUCCESS: u8 = 0x23;
    pub const MESSAGE_TYPE_SET_CUSTOM_MINING_JOB_ERROR: u8 = 0x24;
    pub const MESSAGE_TYPE_RECONNECT: u8 = 0x25;
    pub const MESSAGE_TYPE_SET_GROUP_CHANNEL: u8 = 0x26;
}

@rrybarczyk rrybarczyk added refactor Implies refactoring code protocols Lowest level protocol logic binary-sv2 const-sv2 labels Aug 21, 2024
@rrybarczyk rrybarczyk changed the title Refactor: binary-sv2::const-sv2 crate Refactor: const-sv2 crate Aug 21, 2024
@rrybarczyk
Copy link
Collaborator Author

Any crate-specifc constants should belong in that crate. For example, INITIATOR_EXPECTED_HANDSHAKE_MESSAGE_SIZE and RESPONDER_EXPECTED_HANDSHAKE_MESSAGE_SIZE should exist in the noise_sv2 crate.

@rrybarczyk
Copy link
Collaborator Author

Consider renaming SV2_FRAME_CHUNK_SIZE to NOISE_CHUNK_SIZE. It caused me confusion when documenting the framing_sv2::header::Header::encrypted_len() function. See this comment thread.

@GitGab19
Copy link
Collaborator

Consider renaming SV2_FRAME_CHUNK_SIZE to NOISE_CHUNK_SIZE. It caused me confusion when documenting the framing_sv2::header::Header::encrypted_len() function. See this comment thread.

I'm pretty sure this constant is also used in codec_sv2 and there it makes sense to use this name because it's used in the encryption phase (take a look here). It's good to take note of it anyway, but let me know what you think about it

@GitGab19
Copy link
Collaborator

While preparing the slideshow for const_sv2, I explored again constant by constant, and the only ones which are used in more than one crate are:

  • SV2_FRAME_HEADER_SIZE (framing_sv2 and codec_sv2)
  • SV2_FRAME_CHUNK_SIZE (framing_sv2 and codec_sv2)
  • AEAD_MAC_LEN (framing_sv2 and codec_sv2)
  • RESPONDER_EXPECTED_HANDSHAKE_MESSAGE_SIZE (roles/network_helpers_sv2 and codec_sv2)
  • INITIATOR_EXPECTED_HANDSHAKE_MESSAGE_SIZE (roles/network_helpers_sv2, noise_sv2, and codec_sv2)

@jbesraa
Copy link
Contributor

jbesraa commented Oct 18, 2024

While preparing the slideshow for const_sv2, I explored again constant by constant, and the only ones which are used in more than one crate are:

* `SV2_FRAME_HEADER_SIZE` (`framing_sv2` and `codec_sv2`)

* `SV2_FRAME_CHUNK_SIZE` (`framing_sv2` and `codec_sv2`)

* `AEAD_MAC_LEN` (`framing_sv2` and `codec_sv2`)

* `RESPONDER_EXPECTED_HANDSHAKE_MESSAGE_SIZE` (`roles/network_helpers_sv2` and `codec_sv2`)

* `INITIATOR_EXPECTED_HANDSHAKE_MESSAGE_SIZE` (`roles/network_helpers_sv2`, `noise_sv2`, and `codec_sv2`)

I wouldn't say RESPONDER_EXPECTED_HANDSHAKE_MESSAGE_SIZE is used in multiple crates as network_helpers_sv2 is not an internal crate. I think codec_sv2 should export the variables it is expecting users will need (like RESPONDER_EXPECTED_HANDSHAKE_MESSAGE_SIZE) and network_helpers_sv2 should import it from codec_sv2.

@Fi3
Copy link
Collaborator

Fi3 commented Oct 18, 2024

having all the sv2 const in one place IMO is better then having them around, the the interested crates can reexport them if needed

@Shourya742
Copy link
Contributor

Remove unused dependency: [dependencies] secp256k1 = { version = "0.28.2", default-features = false, features =["hashes", "alloc","rand","rand-std"] }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
const-sv2 protocols Lowest level protocol logic refactor Implies refactoring code
Projects
None yet
Development

No branches or pull requests

5 participants