transaction vout duplication bug fix

[rajarshimaitra]

Fixes RGB-WG/rgb-node#127

[dr-orlovsky]

Did you tried testing this against the case described in the original issue? (meaning that with this fix transaction output is not duplicated with transfer command)

[dr-orlovsky]

I am afraid this is wrong and does not match the solution from the original issue:

• adding fee to id in https://github.com/LNP-BP/rust-lnpbp/blob/e35da7d534250ac7c52ebfae45c8bd772400a7ab/src/rgb/stash/anchor.rs#L139-L154
• Using 0 instead of fee in https://github.com/LNP-BP/rust-lnpbp/blob/e35da7d534250ac7c52ebfae45c8bd772400a7ab/src/rgb/stash/anchor.rs#L197

The solution was to provide TxContainer with the correct vout. Since TxContainer takes vout as (fee+protocol_factor)%num outputs we must use this instead of either fee or protocol_factor (I propose the later) and set the other one to 0.

[rajarshimaitra]

I haven't been able to test it cause I couldn't get rgb-cli bin compiled on master.

Sorry I am not seeing how this wrong. We are passing on fee and protocol_factor=0 to TxContainer. So the vout calculation in TxContainer should produce the same result as vout of anchor https://github.com/rgb-org/rgb-core/blob/0deefa7998ab5401f084e46970eb624538165cfd/src/stash/anchor.rs#L157
Instead of 0 fee we are passing on the correct fee and 0 as protocol_factor, which made sense to me. So both the derivation now should match.

What am I missing?

I also didn't get why protocol_factor was set to vout in previous version.

Since TxContainer takes vout as (fee+protocol_factor)%num outputs we must use this instead of either fee or protocol_factor (I propose the later) and set the other one to 0.

I am not sure why we should trick TxContainer with vout as protocol_factor. TxContainer doesn't hold vout directly, it calculates vout from fee and protcol_factor, so by passing the correct values in these two variables it should be able to calculate correct vout

[claudiosdc]

I am afraid that this fix is not compatible with other parts of the RGB node code. So I am proposing some adjustments to it.

[claudiosdc]

I do not think that setting the protocol_factor to zero is the best approach here. Besides, this is not compatible with what is done in the verify associated function of the Anchor structure.

So I propose that protocol_factor be initialize the same way that it is done there.

Suggested change

	// TODO: use enum instead of hardcoded value
	let protocol_factor = 0;
	let protocol_factor = (
	Uint256::from_be_bytes(*contract_id.as_slice()) % Uint256::from_u64(num_outs).unwrap()
	).low_u64() as u32;

[claudiosdc]

According to my suggested change above, there would be no need to cast protocol_factor here. So, it should be written only as:

Suggested change

	protocol_factor: protocol_factor as u32,
	protocol_factor: protocol_factor,

[dr-orlovsky]

Am I right that this PR was superceded by #2 and can be close dnow?

[claudiosdc]

Am I right that this PR was superceded by #2 and can be close dnow?

Yes, that is correct.

[dr-orlovsky]

Ok, closing in favor of merged #2