Sending same asset to multiple recipients with a single TX

[zoedberg]

This commit bc2ec1c has broken the possibility of sending the same asset to multiple recipients with a single TX.

In specific here:

-    /// Set of seals which are history terminals.
-    pub terminals: SmallOrdMap<BundleId, Terminal>,
+    /// Set of secret seals which are history terminals.
+    pub terminals: SmallOrdMap<BundleId, XChain<SecretSeal>>,

terminals lost support for multiple XChain<SecretSeal>. In the parent commit the Terminal struct was:

pub struct Terminal {
    pub seals: SmallOrdSet<XChain<TerminalSeal>>,
}

which means that multiple seals under the same bundle where supported.

This was working in 0.10, then it was not working in 0.11.0-beta.5 (not sure from which commit it stopped working), it has been fixed by @eauxxs in #200 (and therefore it was working in 0.11.0-beta.6) and then it has been broken again with bc2ec1c (and therefore is broken in 0.11.0-beta.7).

[dr-orlovsky]

Having multiple terminals per bundle adds a lot of programming complexity but makes a little sense:

• if we pay different beneficiaries, we should produce an independent consignment for each one (this will be especially important once we will have conceal procedure in v0.12)
• if we pay the same beneficiary, why we need to create multiple UTXOs for it?

[zoedberg]

adds a lot of programming complexity but makes a little sense

It doesn't add so much complexity in my opinion. And you may not see the use case but there is at least one, as an example we used this feature in our faucet and it allowed us to save a lot of bitcoins

if we pay different beneficiaries, we should produce an independent consignment for each one (this will be especially important once we will have conceal procedure in v0.12)

Only for privacy matters, but for now we can accept this compromise and keep things as they were before

if we pay the same beneficiary, why we need to create multiple UTXOs for it?

I've never talked about paying the same beneficiary, that wouldn't make sense

Anyway, bc2ec1c message says containers: refactor Consignment terminals to contain only secret seals, this doesn't seem like a refactor and in no PR or message you asked/told you were going to remove this feature that we're using since the beginning of RGB. This happened multiple times already and we agreed it was not the way to go, especially considering we are in the phase where we're trying to stabilize the protocol and test every possible scenario. I suggest reverting the title to the previous one (broken send of same asset to multiple recipients with a single TX) and fix it.

[dr-orlovsky]

It doesn't add so much complexity in my opinion. And you may not see the use case but there is at least one, as an example we used this feature in our faucet and it allowed us to save a lot of bitcoins

Well, you can write the code then – and I will review. I tried to do that today, and it takes > 100 lines + a lot of conditions which are unclear how to handle.

Trying to revert the PR introduces even more code back (which I was happy to remove since it was really unobvious one).

Only for privacy matters

"only" :) The privacy is the priority for RGB, always has been.

but for now we can accept this compromise and keep things as they were before

Before they were unoptimal, I see no reason of returning there.

Anyway, bc2ec1c message says containers: refactor Consignment terminals to contain only secret seals, this doesn't seem like a refactor and in no PR or message you asked/told you were going to remove this feature that we're using since the beginning of RGB.

The commit description provides all the details. The change was caused by the fact that contract introspection required the state of the contract to be evaluated from genesis upwards, and not from terminals backwards, as it was before. Due to that fact the terminals were removed from the ConsignmentApi, and there was no reason to keep any other information in them that the one which was required to resolve secret seals - a part of functionality which is purely standard library-related (and not consensus-related). That has removed the only valid case for having multiple terminals per bundle in a consignment (since the multiple beneficiaries must have independent consignments).

Thus, the change was just a consequence (as well as a part) of other approved changes, and it is a part of PR which you have reviewed and approved.

This happened multiple times already and we agreed it was not the way to go, especially considering we are in the phase where we're trying to stabilize the protocol and test every possible scenario.

@zoedberg please stop this tone. I am doing impossible, working 24/7 with no vacations, weekends and holidays, and you approve all the changes. Do not blame me for a removed useless stuff, which was removed as a part of the work on other matters, and PRs with removal of which you have approved yourself. Blame yourself as a reviewer missing that.

That kind of tone happened multiple times over the past months, and I won't tolerate that anymore

I suggest reverting the title to the previous one (broken send of same asset to multiple recipients with a single TX) and fix it.

I spent about ~2hr trying doing that and understood that it doesn't worth it, making any audit much more complex and not adding anything actually useful. Than, I wrote my comment above asking of rationale. Instead, I have got your valuable opinion of me being guilty in everything. Thank you, now you can propose your own PR reverting the changes and we will decide whether it worth merging.

[dr-orlovsky]

use case but there is at least one, as an example we used this feature in our faucet and it allowed us to save a lot of bitcoins

I understand the value of batching the payments in one transaction. My point is that for that you still have to produce independent consignment for each of the receivers, thus the feature is still there, it is just that API of RGB standard library now forces you to do the multiple batched transfer in the correct way

[fedsten]

Having multiple terminals per bundle adds a lot of programming complexity but makes a little sense:

Is there a thread somewhere where this was discussed? To my knowledge at least three separate teams are using/planning to use this features for airdrops in their apps, if there are trade-offs involved it should be discussed before applying the change

[dr-orlovsky]

Is there a thread somewhere where this was discussed?

The change was a part of epic fixing the way we process and filter out valid state basing on the witness transaction mining status. It was required in order to fix bugs related to RBFs, re-orgs etc. As a sub-epic, this has required full refactoring of how RGB Core validates the state, since some re-org of witness transaction changes the validation flow. Thus, we had to move from using consignment terminals into a completely new flow (which was originally planned for v0.12) and resulted in the new Consignment API.

All the epic was spawned across dozens of PRs in all the repos, each of which were heavily tested. There were more than 5000 lines of code affected; this specific change was a part of #241. The set of PRs was discussed and approved in our work group in Telegram (I have sent you all the links), with Zoe agreeing on the merge after running the tests.

To my knowledge at least three separate teams are using/planning to use this features for airdrops in their apps, if there are trade-offs involved it should be discussed before applying the change

Sending assets to many beneficiarties with just one bitcoin tx is still possible. Each sender just needs to receive his own consignment. The airdrop case should not be affected by that - or let me know the details how airdrops are specifically happening

Another important thing: the removed code with many terminals per consignment tx had a bug in it, which was not clear how to fix - it was possible to create a terminal with empty set of seals. Thus, it was another reason why I have removed it. Brigning it back potentially opens some attack vectors, so instead of just "reverting" I have to redesign the way terminals work anyway - I tried that yesterday, it took ~2hr and still I wasn't able to find the simple way of doing it. So the cost of "bringing that feature back" is about 5-6 hrs + ~500-1000 additional lines of quite convoluted code - thus, I propose the solution "just do a separate consignment for each of the recepients" as much more rational and safe one (also this is a proper way from the privacy perspective)

[zoedberg]

I'm sorry about the tone, I'm frustated as things keep changing and stabilization of code seems like a mirage. Anyway, I wrote on telegram that I wasn't able to run rgb-lib tests and only ran the integration tests (which don't have the feature we're discussing here), and I also clarified that I wasn't able to read every line of code of the PRs since the refactor was huge.

Coming back to the issue, did I understood correctly that the feature is still there and I just need to update the code where I construct the consignment in order to construct one per beneficiary? If you confirm this I will try ASAP and then let you know if it works or if I encounter issues.

[dr-orlovsky]

I understand your frustration! I am also frustrated by that... Sometimes I think this is a never-ending story...

But believe me, I do not do any changes unless they are related to a bug, and try to agree on everything with the community here. It is just that the issue with witness re-ordering issue had affected so much code this Aug that it was nearly impossible to agree each single change in those PRs...

did I understood correctly that the feature is still there and I just need to update the code where I construct the consignment in order to construct one per beneficiary? If you confirm this I will try ASAP and then let you know if it works or if I encounter issues.

Yes: you can do multiple transfers in one bitcoin tx for the same asset; it is just their beneficiaries must be distinct and each one will require there own consignment (such that no consignment contains multiple terminals for the same bitcoin tx).

[zoedberg]

Yes: you can do multiple transfers in one bitcoin tx for the same asset; it is just their beneficiaries must be distinct and each one will require there own consignment (such that no consignment contains multiple terminals for the same bitcoin tx).

I confirm that by creating a different consignment for each receiver I'm able to send the same asset to multiple recipients with a single TX.

I think we should change the Stock::transfer method (https://github.com/RGB-WG/rgb-std/blob/master/src/persistence/stock.rs#L715) though. Currently it gets an array of secret_seals:

pub fn transfer(
    &self,
    contract_id: ContractId,
    outputs: impl AsRef<[XOutputSeal]>,
    secret_seals: impl AsRef<[XChain<SecretSeal>]>,
) -> Result<Transfer, StockError<S, H, P, ConsignError>> {
    let consignment = self.consign(contract_id, outputs, secret_seals)?;
    Ok(consignment)
}

but since providing multiple secret_seals leads to a panic (here https://github.com/RGB-WG/rgb-std/blob/master/src/persistence/stock.rs#L781, assert_eq!(res, None);) I think we should change secret_seals to secret_seal and [XChain<SecretSeal>] to XChain<SecretSeal>.

[dr-orlovsky]

Done in #265 and RGB-WG/rgb#240