bugs related to retrying an aborted transfer

[zoedberg]

While using the RGB sandbox @nicbus discovered some bugs related to retrying an aborted transfer.

To simplify and minimize the scope of the debug we tried to reproduce the same bugs in rgb-tests: RGB-WG/rgb-tests#23

The PR introduces 2 new tests that recreate a very similar scenario, where the only relevant change is the call to update_witnesses(), and they show different bugs. In detail:

• same_transfer_twice_no_update_witnesses:
  • the 2nd consignment that the sender creates has duplicated bundles spending the same input
  • the receiver considers the 2nd consignment as valid
• same_transfer_twice_update_witnesses:
  • the sender fails to recreate an identical transfer twice

For context, the bugs happen only:

• on branches ready for beta 9 (on beta 8 the sender constructs a valid consignement)
• if the receiver asks to receive on a blinded UTXO
• (only for same_transfer_twice_no_update_witnesses) if we call resolver.add_terminals

[dr-orlovsky]

• same_transfer_twice_no_update_witnesses:
  • the 2nd consignment that the sender creates has duplicated bundles spending the same input
  • the receiver considers the 2nd consignment as valid

Should we be considering this to be a bug? The idea I always had is that all alternative bundles should always be accepted such that if a re-org happens lately, the funds are not lost.

So, what makes a valid consignment is the fact that it contains at least one valid and mined bundle - but it can contain more and that should be ok.

• same_transfer_twice_update_witnesses:
  • the sender fails to recreate an identical transfer twice

This seems to be an issue and caused by the fact that we consider seals assigned to Tentative (i.e. mempool) witness transactions as spent... Still, this makes unclear how RBF than works...

[zoedberg]

Should we be considering this to be a bug? The idea I always had is that all alternative bundles should always be accepted such that if a re-org happens lately, the funds are not lost.

So, what makes a valid consignment is the fact that it contains at least one valid and mined bundle - but it can contain more and that should be ok.

It could be confusing but I agree in general this makes sense.
But I think we agree that only one of those duplicated allocations should be spendable, right?
If so, please check how I've changed the test (RGB-WG/rgb-tests@0c859f5): I've commented the allocation check and added a send, from the wallet that sees the duplicated allocations (i.e. wlt_2), of 200 (i.e. 2x100) assets, and the send works. This seems an important bug to me. I've also added a log of what wlt_1 sees and it reports 1900 + 200 assets as owned, which is more than the issued amount (i.e. 2000).

This seems to be an issue and caused by the fact that we consider seals assigned to Tentative (i.e. mempool) witness transactions as spent... Still, this makes unclear how RBF than works...

If you mean how the rbf_transfer test works, the only relevant difference I've found is that in rbf_transfer both the TXs are broadcasted (but not mined) while in the other two new tests we broadcast only the second TX.

@dr-orlovsky

[dr-orlovsky]

Intermediary report.

My investigation shows that with pay-to-witness a two distinct transitions and bundles are created, since a different blindings are generated for the assignments. This results in a situation when instead of 1 bundle and 2 witness transactions (which work well) we have 2 contradicting bundles and witness transactions.

This doesn't happen with pay-to-utxo, since a blinding is fixed and is provided by the beneficiary in a secret seal. In this case we have just 1 bundle (with 2 different witness transactions) and everything works well.

Investigating further why 2 alternative bundles doesn't work as expected

[dr-orlovsky]

Ok, now the real bug is the fact that in v0.11.0-beta.9 (as well as in all other pre-v0.12 versions) there is no state filtering basing on the witness transaction status (tentative or not); and all alternative states are seen as valid. This filtering is absent since ContractIface has no access to the witness status (it just knows the witness transaction ids, not their mining information).

I will try to add additional parameter to the ContractIface::extract_state* methods to filter state basing on the witness status and will see whether it is possible without big re-writes of the business logic flow.

[dr-orlovsky]

I will try to add additional parameter to the ContractIface::extract_state* methods to filter state basing on the witness status and will see whether it is possible without big re-writes of the business logic flow.

This was possible and quite simple. The fix is in #288