VelocityHint removal

[nicbus]

Commit e6dbafe refactors the velocity class concept but I recall from the 2023-04-27 dev call that we reasoned differently.

Specifically, I recall we reasoned that:

• using derivation indexes 0 or 1, which are also used by standard bitcoin wallets, leads to the risk of burning assets
• using many derivation indexes can significantly slow down wallet sync process, which would equally impact all allocations, fast or slow
• using the derivation index requires committing to a velocity class at UTXO creation time, while the velocity class is still unknown
• wallets might want to change the velocity class of an RGB allocations

So, we decided to:

• not to use derivation indexes 0 or 1
• use 9 as the only derivation index for RGB allocations

The current situation instead:

• uses derivation index 0 for Unspecified VelocityHint
• uses derivation indexes 10/25/50/75/100 for increasing velocities

Also, if I'm not mistaken, we said that wallets would still benefit from having a way to assign a velocity class to RGB allocations, to be used e.g. to decide how to aggregate multiple allocations to UTXOs, and that this could be done via the supplement (although I'm not sure about the details on this).

[dr-orlovsky]

Well, the point is NOT to use VelocityClass for any sort of derivations AT ALL, but rather as a hint for how to do coinselection/coinallocation (aggregation of different state on UTXOs) by a wallet.

The derivation path MUST always use 9 for non-tapret-tweaked outputs and 10 for tapret-tweaked

[nicbus]

Not sure I understand your message.

In particular:

The derivation path MUST always use 9 for non-tapret-tweaked outputs and 10 for tapret-tweaked

At the moment using change index 9 doesn't work for opret commitments.
Using our rgb-sandbox I get:

Error: not enough PSBT output found to put all required state (can't add assignment 2000 for unspecified-velocity state).

Can you please give a more detailed explanation?

[dr-orlovsky]

Hmm, now I am not sure I understand you. Let me try to rephrase my answer once again:

The VelocityClass in the current refactored form MUST NOT be used in any form for the derivation. Derivation and velocity classes are now unrelated. Integer numbers of VelocityClass has no specific meaning and are arbitrary.

Now, about derivation indexes for RGB. We have decided to hold all assets of all classes under the same single derivation index - 9. I also suggest using another derivation index for any tapret-modified outputs - but this is offtopic to this issue.

[nicbus]

Seems like we already agree on how it's supposed to work.

Where the difference seems to lie is in how a transfer currently works.

To clarify what I mean, let me give some examples of what I get with rgb-sandbox if I use a derivation path with change index 9:

1. transferring all assets (no change) to a blinded UTXO works
2. transferring some assets (WitnessUTXO change) to a blinded UTXO fails
3. transferring all assets (no change) to an address (WitnessUTXO) works (except for the problem reported in witness UTXO produces no terminals nor bundles #71)
4. transferring some assets (WitnessUTXO change) to an address (WitnessUTXO) fails

Notes:

• I'm talking about opret, haven't tried tapret
• the PSBT always contains one output for the change (address derived with change index 9)

When it fails, I get the error:

Error: not enough PSBT output found to put all required state (can't add assignment 4000 for unspecified-velocity state).

Seems liike the issue is with the change output and that, using current versions of rgb-contracts/rgb-wallet, what we're discussing is not supported (yet).

[dr-orlovsky]

Yes, that's my point: we do agree and the merged change doesn't introduce anything against what we agreed upon 😀

Now, if I am getting you right, your point is that even though it did break something. I think if it is the case, the break is not caused by the change to VelocityClass since I never used (and do not use it) in any derivations in RGB-WG code. Thus, it is some other bugs which were revealed or caused by some other changes unrelated to VelocityClass.

[nicbus]

I guess the break has been caused by the re-numbering of the velocity classes.

If I'm correct, the bug was there already but was not visible in rgb-sandbox as it derived addresses with change index 1, which corresponded to the Any velocity class. In fact, to have it working again, I changed the index to 0, which now corresponds to velocity class Unspecified.

In pay(), PSBT outputs are iterated over and a VelocityHint is derived from their derivation to build the out_classes HashMap, which in my case comes up empty.
Then, in output_for_assignment, the velocity is extracted from the contract supplement (in my case it is always Unspecified) and out_classes is used to look up that velocity and retrieve the vout, which fails as out_classes is empty, therefore it returns the NoBlankOrChange error.

The beneficiary output is skipped, which is why it doesn't trigger the same outcome. As I see it, the change output currently needs to have been derived with a change index corresponding to a VelocityHint (as it previously was with AppDeriveIndex) or the transfer will fail.

Do you confirm my analysis?

[dr-orlovsky]

In pay(), PSBT outputs are iterated over and a VelocityHint is derived from their derivation to build the out_classes HashMap, which in my case comes up empty.

Shit, that's the part I have missed. Working on that.

[dr-orlovsky]

Now should be fixed with #77

[nicbus]

Fix confirmed on updated sandbox (commit 0b801f9).