TX20 cancel affecting TX21 orderbook reserved balances

[achamely]

Address: msJ2h47ZrxFJjksVvPy8ik4h2HFfa9W1zV PropertyID: 1

Discrepancy:
reserved balance shows 0 (should be 3.5) available shows 63.5 (should be 60)

Reasons: Address has 2 open orders on DEx.2 for 3.5 MSC 58db47011ed7db0941b50dded44bc738d52cffce11cdd0e6e5ca999e182ed3d0 a5fb5ce90d20a47197e12b09fa0fe775eb99ddb58e5709c92642e51c176aebd2

Potential Cause: Tx20 cancel (cdce647178bd47916e448e1c7ca3de35aec6e76154e66a771ba6ea1062b0ef59) was intended to cancel only the original tx20 sale (550bc22751eaee1e466da456f7f0e6a208d1f19f8fba12a3a3ab9e7da3614f32)

It appears it is also grabbing the 'reserved balance' from the two open orderbook orders referenced above. The actual orderbook order still shows open / normal as expected.

[dexX7]

Confirmed. It's indeed the cancel at the end of the sequence.

A1 starts with 50.0 BTC, 63.5 TMSC, 0.0 TDiv1, 0 TIndiv1 A1 offers 0.5 TMSC for 5000000000 TIndiv1 A1 offers 3.0 TMSC for 30.0 TDiv1 A1 offers 10.0 TMSC for 0.1 BTC

Balance should be: 50.0 TMSC available and 13.5 TMSC ... OK https://gist.githubusercontent.com/dexX7/857d08e92d086a2adec6/raw

A1 canceles 10.0 TMSC for 0.1 BTC Balance should be: 60.0 TMSCH available and 3.5 TMSC reserved ... FAIL Balance was actually 63.5 TMSC available and 0.0 TMSC reserved https://gist.githubusercontent.com/dexX7/7cb24e869d9ebddf4028/raw

[marv-engine]

Good catch! This is a good regression test.

Has anyone tested the complement - to make sure a tx21 Cancel doesn't touch the reserved amounts for other tx21's or tx20's?

[dexX7]

Even though my test framework works fine, I'm not yet 100 % satisfied with reporting the results and so, but while optimizing this test, I noticed something interesting:

1) A1 starts with 50.0 BTC, 63.5 MSC, 63.5 TMSC, 0.0 TDiv1, 0 TIndiv1 2) A1 offers 0.5 TMSC for 5000000000 TIndiv1 3) A1 offers 3.0 TMSC for 30.0 TDiv1 4) A1 offers 10.0 MSC for 0.1 BTC

Balance: 53.5 MSC (reserved: 10.0 MSC) Balance: 60.0 TMSC (reserved: 3.5 MSC) Balance: 0.0 TDiv1 (reserved: 0.0 TDiv1) Balance: 0 Indiv1 (reserved: 0 Indiv1)

5) A1 cancels 10.0 MSC for 0.1 BTC

Balance: 63.5 MSC (reserved: 0.0 MSC) Balance: 60.0 TMSC (reserved: 3.5 MSC) Balance: 0.0 TDiv1 (reserved: 0.0 TDiv1) Balance: 0 Indiv1 (reserved: 0 Indiv1)

So it appears there is only an interference, if trades are in the same ecosystem.

Will test the complement soon.

[marv-engine]

Here's my guess for why a tx20 Cancel affects the address' whole reserved amount within the same ecosystem (without looking at the code, so it is just a guess) - when tx20 was written, it was the only way to create a reserved amount and there could be only one active tx20 per address, so there was no need to note the amount reserved by the tx20. The tx20 Cancel would move the whole reserved balance back to the available balance.

[dexX7]

Here is an extreme example:

1) A1 starts with 50.0 MSC 2) A1 offers 10.0 MSC for 2.0 BTC (traditional)

Traditional DEx: 10.0 MSC for 2.0 BTC @ 0.2 BTC/MSC Meta DEx for MSC: empty Balance: 40.0 MSC (reserved: 10.0 MSC)

3) A1 offers 40.0 MSC for 700 MIndiv (or anything else)

Traditional DEx: 50.0 MSC for 10.0 BTC @ 0.2 BTC/MSC Meta DEx for MSC: 40.0 MSC for 700 MIndiv Balance: 0.0 MSC (reserved: 50.0 MSC)

4) A1 cancels 10.0 MSC for 2.0 BTC (traditional)

Traditional DEx: empty Meta DEx for MSC: 40.0 MSC for 700 MIndiv Balance: 50.0 MSC (reserved: 0.0 MSC)

5) A1 cancels 40.0 MSC for 700 MIndiv

Traditional DEx: empty Meta DEx for MSC: empty Balance: 90.0 MSC (reserved: 0.0 MSC)

---

@marv-engine: This sounds plausible, but I didn't look at the code yet, too.

[dexX7]

It looks like there are basically three areas that result in this behavior:

1) When cancelling a traditional DEx offer, the whole "resevered" amount for a given property is taken and added to the "actual balance". (mastercore_dex.cpp#L466). This seems to be what you already suspected, @marv-engine.

2) When cancelling a meta DEx offer, there are two seperate balance updating operations:

1. Subtract offer amount from "reserved balance" (mastercore_dex.cpp#L878)
2. Add offer amount to "actual balance" (mastercore_dex.cpp#L879)

A check exisists to ensure it can only be subtracted what's available, but this lets only step 1 fail (or rather: result in an operation without effect), so nevertheless step 2 still follows.

3) When it comes to the RPC call getactivedexsells_MP, something similar happens:

The reported unit price is calculated based on the original amounts (2.0 BTC/10.0 MSC = 0.2 BTC/MSC in the last example), but it's multiplied with the "available" amount of the seller - which is again the whole "reserved balance". (mastercore_rpc.cpp#L1420-L1429)

[zathras-crypto]

Great pickup guys - so the reuse of the same reserved funds for MetaDEx and regular DEx is causing some conflicts - let me talk with Michael on this one

[m21]

Thanks guys, what you are seeing makes sense, RESERVEs are common in the code. Will chat with the core team on best redesign ideas.

[dexX7]

I think there should be no direct manipulation of balances or global token amounts, but instead one or more interfaces to provide commands such as:

• Create an amount of new units of property, added to the balance of A
• Destroy an amount of property, subtracted from the balance of A
• Move an amount of property, subtracted from the balance of A and added to the balance of B
• ...

Usually any credit of tokens should come with a debit and vice versa, (...) and say for example a "transfer of tokens", initiated by a "simple send", encapsulated as "move", is just an abstraction of a composition of sub-operations such as "credit", "debit", "lock", "unlock", which affect balances and state.

Each step must satisfy certain criteria and should be bound to something identifiable like a transaction. There could further be strict transitions such as available -> reserved -> subtracted.

[zathras-crypto]

I actually really like the idea of an auditable trail of create/destroy/move - a swap in for updatetally and we can make sure locks are done in these functions etc - just thinking out loud but sounds like a good idea

[dexX7]

I actually really like the idea of an auditable trail of create/destroy/move

This would be awesome, in particular in the context of #143. I already tested logging tally updates, but one key issue with this is that updates are currently not associated with something identifiable, like a transaction hash. On the second+ startup it then appears to be the case that an aggregate of tally updates is appended to the output which can't be filtered out.

[m21]

The original issue was fixed long ago, @achamely -- agreed?

[dexX7]

@m21: this was fixed.