Questions about roles_logic_sv2 crate

[plebhash]

this discussion should guide the documentation of roles_logic_sv2, as described on #845

[plebhash]

Here's the current docs for roles_logic_sv2::utils::merkle_root_from_path:

stratum/protocols/v2/roles-logic-sv2/src/utils.rs

Lines 123 to 145 in 0f0ee1e

	/// It takes a coinbase transaction, a list of transactions, and a list of indices, and returns the
	/// merkle root of the transactions at the given indices
	///
	/// Arguments:
	///
	/// * `coinbase_tx_prefix`: the first part of the coinbase transaction, before the extranonce.
	/// This should be converted from [`binary_sv2::B064K`]
	/// * `coinbase_tx_suffix`: the coinbase transaction suffix, which is the part of the coinbase
	/// transaction after the extranonce. This should be converted from [`binary_sv2::B064K`]
	/// * `extranonce`: the extranonce that the miner is using this value should be converted from
	/// This should be converted from [`binary_sv2::B032`] and padded with zeros if not 32 bytes long
	/// * `path`: a list of transaction hashes that are used to calculate the merkle root.
	/// This should be converted from [`binary_sv2::U256`]
	///
	/// Returns:
	///
	/// A 32 byte merkle root as a vector if successful and None if the arguments are invalid.
	pub fn merkle_root_from_path<T: AsRef<[u8]>>(
	coinbase_tx_prefix: &[u8],
	coinbase_tx_suffix: &[u8],
	extranonce: &[u8],
	path: &[T],
	) -> Option<Vec<u8>> {

I'm particularly confused about the description for the path argument, because it says:

a list of transaction hashes that are used to calculate the merkle root.

When extended_to_standard_job calls it, we give an actual merkle_path, and not a list of transaction hashes.

stratum/protocols/v2/roles-logic-sv2/src/channel_logic/mod.rs

Lines 8 to 19 in 0f0ee1e

	pub fn extended_to_standard_job<'a>(
	extended: &NewExtendedMiningJob,
	coinbase_script: &[u8],
	channel_id: u32,
	job_id: Option<u32>,
	) -> Option<NewMiningJob<'a>> {
	let merkle_root = crate::utils::merkle_root_from_path(
	extended.coinbase_tx_prefix.inner_as_ref(),
	extended.coinbase_tx_suffix.inner_as_ref(),
	coinbase_script,
	&extended.merkle_path.inner_as_ref(),
	);

Also, I don't understand why it says:

This should be converted from [binary_sv2::U256]

Since we are actually converting from a Seq0255<'decoder, U256<'decoder>> and not just one single U256.

I feel this comment should be fixed. Do you agree @Fi3?

[plebhash]

hmm yeah by looking at merkle_root_from_path_ and reduce_path implementations I'm about 99% sure the comment for the path argument of merkle_root_from_path should be fixed

just want confirmation from @Fi3

[plebhash]

also about these comments:

stratum/protocols/v2/roles-logic-sv2/src/utils.rs

Line 166 in 0f0ee1e

// TODO remove when we have https://github.com/rust-bitcoin/rust-bitcoin/issues/1319

stratum/protocols/v2/roles-logic-sv2/src/utils.rs

Line 173 in 0f0ee1e

// TODO remove when we have https://github.com/rust-bitcoin/rust-bitcoin/issues/1319

I see that rust-bitcoin/rust-bitcoin#1319 ended up not getting any resolution and rust-bitcoin/rust-bitcoin#1321 ended up being closed

is it meaningful to try to aim for the MerkleBlock approach mentioned here rust-bitcoin/rust-bitcoin#1321 (comment)? Or should we just stick with the current solution and remove these TODO comments?

[Fi3]

yep we should fix the doc. About 1321 we agreed that this code do not belong in rust bitcoin but in sri so I think that we should leave it here.

[plebhash]

The spec definition of Channel says:

The main difference between standard and extended channels is that standard channels cannot manipulate the coinbase transaction / Merkle path, as they operate solely on provided Merkle roots. We call this header-only mining. Extended channels, on the other hand, are given extensive control over the search space so that they can implement various advanced use cases such as translation between v1 and v2 protocols, difficulty aggregation, custom search space splitting, etc.

This separation vastly simplifies the protocol implementation for clients that don’t support extended channels, as they only need to implement the subset of protocol messages related to standard channels (see Mining Protocol Messages for details).

This clearly states that Standard Channels are meant for HOM mining, where the search space is limited to a fixed merkle root. Up until now, my mental model was quite clear about this distinction.

But now as I study roles_logic_sv2, I see that ChannelFactory has:

stratum/protocols/v2/roles-logic-sv2/src/channel_logic/channel_factory.rs

Lines 197 to 201 in 0f0ee1e

	struct ChannelFactory {
	ids: Arc<Mutex<GroupId>>,
	standard_channels_for_non_hom_downstreams:
	HashMap<u64, StandardChannel, BuildNoHashHasher<u64>>,
	standard_channels_for_hom_downstreams: HashMap<u32, StandardChannel, BuildNoHashHasher<u32>>,

so seeing standard_channels_for_non_hom_downstreams there is quite confusing.

Also, add_standard_channel takes a is_header_only: bool parameter:

stratum/protocols/v2/roles-logic-sv2/src/channel_logic/channel_factory.rs

Lines 220 to 226 in 0f0ee1e

	pub fn add_standard_channel(
	&mut self,
	request_id: u32,
	downstream_hash_rate: f32,
	is_header_only: bool,
	id: u32,
	) -> Result<Vec<Mining>, Error> {

@Fi3 why do we have this contradiction between spec and implementation? Under which conditions would a Standard Channel be used for non-HOM mining?

[plebhash]

also, here's the definition of StandardChannel:

stratum/protocols/v2/roles-logic-sv2/src/common_properties.rs

Lines 65 to 76 in 0f0ee1e

	pub struct StandardChannel {
	/// Newly assigned identifier of the channel, stable for the whole lifetime of the connection.
	/// e.g. it is used for broadcasting new jobs by `NewExtendedMiningJob`
	pub channel_id: u32,
	/// Identifier of the group where the standard channel belongs
	pub group_id: u32,
	/// Initial target for the mining channel
	pub target: Target,
	/// Extranonce bytes which need to be added to the coinbase to form a fully valid submission:
	/// (full coinbase = coinbase_tx_prefix + extranonce_prefix + extranonce + coinbase_tx_suffix).
	pub extranonce: Extranonce,
	}

if the extranonce is already fixed inside the body of StandardChannel, how would it be rolled in non-HOM?

[plebhash]

also, the spec for OpenStandardMiningChannel message doesn't have any field where the client would signal if they are doing HOM or not... so how does the server decide if the StandardChannel will be HOM or not?

[Fi3]

bit 0 of setupconnection is the flag that set if downstream is HOM or not.
Look at OpenStandardMiningChannel.Success you have an extranonce part. That cause if a proxy non HOM relay standard channels opened by HOM devices the non HOM proxy aslo if is opening standard channels will receive extended job. Look at NewExtendedMiningJob definition the part about group channel explain how it shuld work in that case.

1. the hom downstream open a standard channel
2. proxy non hom open a standard channel with pool (that see that proxy i non hom)
3. pool set extranonce for that specific channel (so for the specific hom downstream)
4. proxy remember the extranonce
5. pool send extended job to proxy
6. for each hom downstream proxy replace the extranonce calculate m root and send the job

[plebhash]

if the proxy in this example is non HOM, why does it open a standard channel with the pool, instead of an extended channel?

it could still calculate the merkle_root to send to the HOM downstreams.

[Fi3]

it might want to group standard channels from downstream, maybe a pool want to go that way to have telemetry or for whatever other reasons. But you should ask the spec's authors.