RGB-WG / rgb-core

RGB Core Library: consensus validation for private & scalable client-validated smart contracts on Bitcoin & Lightning
https://spec.rgb.tech
Apache License 2.0
207 stars 52 forks source link

bugs related to retrying an aborted transfer #283

Open zoedberg opened 1 week ago

zoedberg commented 1 week ago

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: https://github.com/RGB-WG/rgb-tests/pull/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:

For context, the bugs happen only:

dr-orlovsky commented 4 days ago
  • 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 commented 4 days ago

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 (https://github.com/RGB-WG/rgb-tests/pull/23/commits/0c859f5b11c000e6041eed1725c37ab3cee3c7cd): 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