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

[plebhash]

the motivation for this question is my work on #1437

I'm trying to understand the implementation of fn get_bip_34_bytes()

first we take script_prefix from NewTemplate.coinbase_prefix

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

Line 301 in 5496d8a

let script_prefix = &new_template.coinbase_prefix.to_vec()[..];

then we do some verification based on script_prefix[0]:

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

Lines 318 to 325 in 5496d8a

	// add 1 cause 0 is push 1 2 is 1 is push 2 ecc ecc
	// add 1 cause in the len there is also the op code itself
	let bip34_len = script_prefix[0] as usize + 2;
	if bip34_len == script_prefix.len() {
	Ok(script_prefix[0..bip34_len].to_vec())
	} else {
	Err(Error::InvalidBip34Bytes(script_prefix.to_vec()))
	}

it seems that this assumes that the first byte of NewTemplate.coinbase_prefix carries length?

this is confusing because the spec for NewTemplate clearly says that none of these bytes should be interpreted as length:

[GitGab19]

Yeah from my past research there, I remember that the first byte of this field carries the length.

At least according to BIP34:

Add height as the first item in the coinbase transaction's scriptSig, and increase block version to 2. The format of the height is "minimally encoded serialized CScript" -- first byte is number of bytes in the number (will be 0x03 on main net for the next 150 or so years with 223-1 blocks), following bytes are little-endian representation of the number (including a sign bit). Height is the height of the mined block in the block chain, where the genesis block is height zero (0).

Ref: https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki

My suggestion would be to change the name of this field into coinbase_bip34_bytes or similar to avoid any confusion.

Let's just keep in mind that even just renaming it would break current implementations..

[Sjors]

I'm wouldn't rename it. What if there's a future soft fork that adds more stuff to the scriptSig? Or what if the node wants to add something for fun?

If BIP34 was the only thing that matters, then the node should just provide the height and SRI can encode it. But I think the design should stay more generic.

[plebhash]

the motivation for renaming is to avoid confusion like this, which is very recurring.

so I'm still pro the idea of renaming this field.

but to @Sjors point, perhaps it's not wise to focus on the BIP34 aspect of it.

I guess scriptSig_prefix would be more appropriate?

[Sjors]

The coinbase scriptSig must start with the BIP34 stuff. My hypothetical future softfork would hopefully add more at the start of a scriptSig (after BIP34). And if the node wants to add "Made by Me" there, they can also just do that at the start.

So a name like coinbase_scriptsig_prefix indeed makes sense.

Also, a coinbase can have a witness in theory. Might be good to allocate an optional field for that, though it could be added later.

[Sjors]

There's ongoing discussion about what to name the scriptSig of a coinbase transaction: https://github.com/murchandamus/bips/pull/1/files#r1248345325

So coinbase_prefix might be acceptable, though maybe coinbase_field_prefix is less ambiguous.