Questions about roles_logic_sv2 crate

[GitGab19]

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

[plebhash]

ChannelFactory is private.

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

Line 198 in b900d0a

struct ChannelFactory {

it's used as inner in some heavily opinionated channel factories (which are public):

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

Lines 974 to 975 in b900d0a

	pub struct PoolChannelFactory {
	inner: ChannelFactory,

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

Lines 1294 to 1295 in b900d0a

	pub struct ProxyExtendedChannelFactory {
	inner: ChannelFactory,

---

but what if the user needs to implement some application logic that is different from PoolChannelFactory and ProxyExtendedChannelFactory?

couldn’t it be useful to make ChannelFactory public?

ofc roles_logic_sv2 could still provide opinionated factories like PoolChannelFactory and ProxyExtendedChannelFactory, but the point is that IMO we should allow the user to go in different directions depending on their needs, and that seems impossible now.

[plebhash]

with a trait-based pattern on channel factory, it would be much easier to leverage roles_logic_sv2 to create re-usable libs on the application layer (roles becoming libs, not only bin)

[Fi3]

could work

[plebhash]

note to @GitGab19

we should consider also bringing logic from job_creator and job_dispatcher modules into this trait-based pattern

[plebhash]

also common_properties module (which is already heavily based on traits)

[plebhash]

also some things from utils module

e.g.: Id, GroupId

[plebhash]

@Fi3 can you elaborate?

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

Lines 316 to 321 in b900d0a

	/// Called when we want to replicate a channel already opened by another actor.
	/// is used only in the jd client from the template provider module to mock a pool.
	/// Anything else should open channel with the new_extended_channel function
	pub fn replicate_upstream_extended_channel_only_jd(
	&mut self,
	target: binary_sv2::U256<'static>,

[Fi3]

is used by the JDC, the JDC is a pool so need a PoolChannelFactory but it also need to use the extranonce set by the pool so we use this method to instanciate a PoolChannelFactory with a specific and immutable extranonce prefix

[Fi3]

is a pool since it create jobs, is not a pool since it need to get the extranonce prefix from the pool

[plebhash]

this is a todo, right?

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

Lines 1261 to 1266 in b900d0a

	fn check_set_custom_mining_job(
	&self,
	_set_custom_mining_job: &SetCustomMiningJob<'static>,
	) -> bool {
	true
	}

[Fi3]

up to discussion maybe we can just remove it and check in another place

[plebhash]

oh ok I just realized that check_set_custom_mining_job is a helper function, which is only called inside on_new_set_custom_mining_job

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

Lines 1241 to 1259 in b900d0a

	/// Called when a new custom mining job arrives
	pub fn on_new_set_custom_mining_job(
	&mut self,
	set_custom_mining_job: SetCustomMiningJob<'static>,
	) -> SetCustomMiningJobSuccess {
	if self.check_set_custom_mining_job(&set_custom_mining_job) {
	self.negotiated_jobs.insert(
	set_custom_mining_job.channel_id,
	set_custom_mining_job.clone(),
	);
	SetCustomMiningJobSuccess {
	channel_id: set_custom_mining_job.channel_id,
	request_id: set_custom_mining_job.request_id,
	job_id: self.inner.job_ids.next(),
	}
	} else {
	todo!()
	}
	}

this confirms that this logic is essentially a "todo" and currently PoolChannelFactory blindly accepts any incoming Custom Jobs without checking anything

[Fi3]

I think that the pool check the signature but not 100% sure, but if do it it do it in another place

[plebhash]

everything defined under channel_logic::proxy_group_channel (GroupChannels, GroupChannel) module is never used anywhere in SRI

why?

[Fi3]

it can be useful for someone and dont' think it need to be maintained, but also removing it make sense since we don't use it. We need also to decide if we plan to support group channels or not, if we don't we can simplify also the channels factories.

[plebhash]

AFAIK Braiins FW opens Standard Channels, not Extended.

This is based on a conversation with Marin from support, and I have not been able to validate it in practice due to the current Noise incompatibility issues.

If we choose not to support Group/Standard channels on SRI, I believe this will become another point of incompatibility between Braiins FW and SRI.

cc @jakubtrnka

[plebhash]

actually @rrybarczyk pointed out that it is being used here:

stratum/roles/mining-proxy/src/lib/upstream_mining.rs

Line 17 in 7483090

proxy_group_channel::GroupChannels,

stratum/roles/mining-proxy/src/lib/upstream_mining.rs

Lines 47 to 50 in 7483090

	pub enum ChannelKind {
	Group(GroupChannels),
	Extended(Option<ProxyExtendedChannelFactory>),
	}

stratum/roles/mining-proxy/src/lib/upstream_mining.rs

Lines 98 to 108 in 7483090

	fn reset(&mut self) {
	match self {
	ChannelKind::Group(_) => {
	*self = ChannelKind::Group(GroupChannels::new());
	}
	ChannelKind::Extended(_) => {
	*self = ChannelKind::Extended(None);
	}
	}
	}
	}

stratum/roles/mining-proxy/src/lib/upstream_mining.rs

Lines 110 to 117 in 7483090

	impl From<super::ChannelKind> for ChannelKind {
	fn from(v: super::ChannelKind) -> Self {
	match v {
	super::ChannelKind::Group => Self::Group(GroupChannels::new()),
	super::ChannelKind::Extended => Self::Extended(None),
	}
	}
	}

[Fi3]

yep we use it in the mining-proxy but is very old and unmaintained code so I didn't mentioned it

[GitGab19]

I agree, I would say that we should not rely on the current mining-proxy to make evaluations about roles_logic_sv2. IMO best thing would be to clean and refactor the crate to make it do what we know roles need to do. If we are going to notice something missing which could be useful for a proper mining-proxy, we could expand roles_logic in the proper way in the future.

[plebhash]

description of job_creator module:

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

Lines 1 to 2 in b900d0a

	//! The job creator module provides logic to create extended mining jobs given a template from
	//! a template provider as well as logic to clean up old templates when new blocks are mined

why is there no support for Standard Jobs?

is it still a "todo", or is there some deeper reason?

[plebhash]

hmm I'm looking into job_dispatcher module:

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

Lines 1 to 5 in b900d0a

	//! The Job Dispatcher contains relevant logic to maintain group channels in proxy roles such as:
	//! - converting extended jobs to standard jobs
	//! - handling updates to jobs when new templates and prev hashes arrive, as well as cleaning up old
	//! jobs
	//! - determining if submitted shares correlate to valid jobs

is this the reason why job_creator only creates Extended Jobs? because we assume the Dispatcher will handle the conversion?

[Fi3]

yep conversion is done in another place. A job born as extended that is a more general concept. Then somewhere is transformed into a standard job. (or not if the we use an sv1 mining device)

[plebhash]

what is the purpose of job_creator::StrippedCoinbaseTx?

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

Lines 381 to 383 in b900d0a

	/// Helper type to strip a segwit data from the coinbase_tx_prefix and coinbase_tx_suffix
	/// to ensure miners are hashing with the correct coinbase
	struct StrippedCoinbaseTx {

if we remove segwit data from the coinbase and then create a job with this new coinbase, the miner will find a block without segwit support?

[Fi3]

no, we need to remove it cause some miners do not support segwit data in the coinbase

[plebhash]

hmm ok but I'm still confused

I would assume this would be optional, and a very rare use-case

but tProxy seems to be making it mandatory?

stratum/roles/translator/src/lib/proxy/next_mining_notify.rs

Lines 15 to 22 in 0520c58

	pub fn create_notify(
	new_prev_hash: SetNewPrevHash<'static>,
	new_job: NewExtendedMiningJob<'static>,
	clean_jobs: bool,
	) -> server_to_client::Notify<'static> {
	// TODO 32 must be changed!
	let new_job = extended_job_to_non_segwit(new_job, 32)
	.expect("failed to convert extended job to non segwit");

[Fi3]

no is better to remove it always do we are sure that will work, otherwise you should match the downstream against a list of firmware that support it and decide based on that would be very hard

[plebhash]

why does PoolMessages parser include JobDeclaration and TemplateDistribution?

that feels misleading, since a Pool can only handle Mining + Common messages

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

Lines 1022 to 1031 in e650561

	pub enum PoolMessages<'a> {
	#[cfg_attr(feature = "with_serde", serde(borrow))]
	Common(CommonMessages<'a>),
	#[cfg_attr(feature = "with_serde", serde(borrow))]
	Mining(Mining<'a>),
	#[cfg_attr(feature = "with_serde", serde(borrow))]
	JobDeclaration(JobDeclaration<'a>),
	#[cfg_attr(feature = "with_serde", serde(borrow))]
	TemplateDistribution(TemplateDistribution<'a>),
	}

[plebhash]

then we have AnyMessage as an alias, which actually feels like the correct name

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

Line 89 in e650561

pub type AnyMessage<'a> = PoolMessages<'a>;

[plebhash]

IMO we should replace PoolMessages with AnyMessage and not have a PoolMessages parser

[Fi3]

I completely agree.

[GitGab19]

why do we have only OpenStandardMiningChannel and OpenStandardMiningChannelSuccess implemented for the MiningRouter in routing_logic.rs?
I'm having some problems in understanding the real necessity of this module, since it seems implemented only for the Mining Proxy, could you give me a deeper overview on it @Fi3 ?

[Fi3]

I think that we can remove it

[GitGab19]

I assume that same goes for selectors.rs, right? Digging there as well, it's hard to understand the real reason behind its conception.

[GitGab19]

I guess the reason why both modules were born is related to supporting the Mining Proxy. But since the only messages which are somehow implemented are the OpenStandardMiningChannel and OpenStandardMiningChannelSuccess, I'm wondering if also for the current Mining Proxy these modules are really useful. If Mining Proxy is working, it probably means that all other mining messages are not using routing_logic.rs or selectors.rs as they were supposed to be used (?)

[plebhash]

what does TODO can be also 3 mean?

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

Lines 239 to 259 in 67a3f00

	fn coinbase_tx_prefix(
	coinbase: &Transaction,
	script_prefix_len: usize,
	) -> Result<B064K<'static>, Error> {
	let encoded = coinbase.serialize();
	// If script_prefix_len is not 0 we are not in a test environment and the coinbase will have the
	// 0 witness
	let segwit_bytes = match script_prefix_len {
	0 => 0,
	_ => 2,
	};
	let index = 4 // tx version
	+ segwit_bytes
	+ 1 // number of inputs TODO can be also 3
	+ 32 // prev OutPoint
	+ 4 // index
	+ 1 // bytes in script TODO can be also 3
	+ script_prefix_len; // bip34_bytes
	let r = encoded[0..index].to_vec();
	r.try_into().map_err(Error::BinarySv2Error)
	}

[Fi3]

it can't I removed it in my last PR

[plebhash]

you removed for coinbase_tx_prefix but coinbase_tx_suffix still has it:
https://github.com/Fi3/stratum-1/blob/627ad3956603ad9d745dcc0369d7e49178fc0e99/protocols/v2/roles-logic-sv2/src/job_creator.rs#L283

[Fi3]

https://github.com/Fi3/stratum-1/blob/ImproveCoinbase/protocols/v2/roles-logic-sv2/src/job_creator.rs#L257