Fix STO: require fee only for actual sends

[dexX7]

A fee should only be paid for sends to owners who actually receive something.

Scenario:

1) A1 starts with 1.00000000 MSC A1 starts with 10 MIndiv

A2 starts with 2 MIndiv A3 starts with 3 MIndiv A4 starts with 1 MIndiv A5 starts with 1 MIndiv A6 starts with 1 MIndiv

2) A1 sends 2 MIndiv to owners of MIndiv

3) A1 should have 0.99999998 MSC (-0.00000002 MSC) A1 should have 8 MIndiv (-2 MIndiv)

A2 should have 3 MIndiv (+1 MIndiv) A3 should have 4 MIndiv (+1 MIndiv) A4 should have 1 MIndiv A5 should have 1 MIndiv A6 should have 1 MIndiv

Currently A1 would pay 0.00000005 MSC, which is an unjustfied overpayment of 0.00000003 MSC.

[dexX7]

As a side note, I created some tests for STO and it looks like this is the only part where I see a failure. Everything else is good to go.

[zathras-crypto]

Supported, the whole reason @m21 rewrote this was to address this issue (fee charged for num owners instead of num owners receiving tokens)

[dexX7]

the whole reason @m21 rewrote this was to address this issue

Sorry, I can't follow?

[zathras-crypto]

https://github.com/mastercoin-MSC/mastercore/commit/2abfd2849db8ba7a83957c64eb976b406713c123

I identified this a couple of weeks back and advised @m21 that we were counting the number of addresses with tokens rather than the number of addresses that actually got paid. This commit was supposed to resolve that (and from my testing it did).

Will take another look :)

[dexX7]

I identified this a couple of weeks back and advised @m21 that we were counting the number of addresses with tokens rather than the number of addresses that actually got paid.

The payment receivers are a subset of owners (addresses with tokens). To continue with the example in the first post:

There are 6 owners of MIndiv, 5 owners qualify as recipient, because the other one is the sender, and 2 receive something, because only 2 MIndiv were distributed.

[zathras-crypto]

Not sure if this will help with context, but I grabbed convo from Skype for you below to give some background on the commit I mentioned above (you're always welcome in our Skype group mate!) - we need to get the fee stuff right as we want to tag 0.0.9 in the coming days. I'm sure I'd tested the patch as working but looking at the code path for your PR it does seem that your change is also needed so I'm a little confused lol. :P

Thanks Z

zathrasc: nothing to do with broadcast dude, it's the parsing - so with your fix in, I can see during parsing we no longer apply the fee to sender zathrasc: but I am still seeing an extra 1 willet diff which looking into now zathrasc: was 2 willets diff without your fix, now 1 willet diff with fix Michael: ah I didn't reparse zathrasc: so I'm looking at this (after your fix, after reparse) zathrasc:

zathras@coredev01:~/github/build/mastercore$ src/mastercored getsto_MP "b8aa7efb381f8c6450fe70f2e96a5cf2c9e60900124544e882fe546dfaf7c958" | grep stofee
    "totalstofee" : "0.00000093",
zathras@coredev01:~/github/build/mastercore$ cat ~/.bitcoin/testnet3/mastercore.log | grep -a8 b8aa7efb381f8c6450fe70f2e96a5cf2c9e60900124544e882fe546dfaf7c958 | grep Transfer\ fee
2014-12-22 21:00:41         Transfer fee: 0.00000094 MSC

zathrasc: before your fix it was Transfer fee: 0.00000095 MSC zathrasc: so your fix is def working for removing sender zathrasc: looking for the extra diff now Adam Chamely: i’m rebuilding now as well zathrasc: ok everything I can find from both logs and levelDB shows there are 93 recipients in that sto (b8aa) zathrasc: so fee should have been 93 not 94 zathrasc: looking further Adam Chamely: sending address and exodus address (is it not excluding exodus in calc)? zathrasc: exodus is included, there are no exclusions against crediting exodus Adam Chamely: ok zathrasc: Michael added a patch to drop by one the total addresses to remove sender zathrasc: seemed to be ok for last one, but the one I sent was 2 willets out without his fix, 1 willet out with fix zathrasc: trying to debug into something I can point @Michael too Adam Chamely: gotchya zathrasc: right so there is something else at play zathrasc: firstly, @Michael your patch zathrasc: shouldn't be required zathrasc:

// exclude the sender
      --n_owners;

is redundant (and actually subtracts one extra) since you already exclude the sender from adding to the OwnerAddrSet map:

// do not count the sender
      if (address == sender) continue;

so when you loop through the map and ++n_owners, you're not adding 1 to n_owners for the sender anyway because it's not in the map zathrasc: so tl:dr; the fix shouldn't have been required, something else is in play Michael: hm, oh yeah Michael: I should just do .size() weird zathrasc: yeah size() would be perfect I would think, assuming the map is correct (that's my assumption right now, still poking around) zathrasc: Ahh shit zathrasc: dude... zathrasc: your fee calculation is fundamentally flawed mate - you calculate it based on the number of addresses that have tokens in the STO property, and not on the number of addresses that will receive tokens (since some addresses may not get paid remember) Michael: sure sure Michael: let's see that spec again... zathrasc: spec is tl:dr each address that receives a token means a fee zathrasc: have a nose around bud zathrasc: but yeah gonna complicate things a little Marv Schneider: "The sending address must be charged a transfer fee for each address that receives coins as a result of this transaction." zathrasc: key point being receives zathrasc: sometimes in an STO due to divisibility or small numbers of tokens, sometimes the addresses with the lowest balances don't get anything zathrasc: thus a model where fee = count(addresses_with_tokens_of_said_property) isn't going to work Marv Schneider: "Be aware that some owners of the specified currency might receive zero coins due to rounding in calculating the number of coins for each owner. " zathrasc: we have to use a model where fee = count(addresses_with_tokens_that_actually_got_some_STO) zathrasc: so there' zathrasc: oops zathrasc: so there's the discrepency - 95 addresses have MSC, only 93 received a share of the MSC STO. Fee on RPC shows 93, fee in log and tallys is 95 (without the extra --n_owners patch) zathrasc: sorry @Michael to be a PITA but looks like we gotta expand loop 1 to cover a bit more of the math work from loop 2 zathrasc: brb guys :) Michael: I think that was the original reason for 2 loops; weird that I went away from it Marv Schneider: yes Michael: gotta step out for a bit; will continue working the fix tonight; bbl

[zathras-crypto]

There are 6 owners of MIndiv, 5 owners qualify as recipient, because the other one is the sender, and 2 receive something, because only 2 MIndiv were distributed.

Yep, no ambiguity here mate the fee should be 2 willets full stop - if you get a different result anywhere in your testing (which it sounds like you did) we need to fix it. Honestly I'm just surprised (and questioning my own testing ability haha) since I'm almost certain I tested Michael's changes as doing the right thing :(

[dexX7]

Honestly I'm just surprised (and questioning my own testing ability haha) since I'm almost certain I tested Michael's changes as doing the right thing :(

I coded almost all regtests to use balances to check the results passively, and you probably got fooled by the RPC output of "getsto_MP" which actually reports the correct fee. Here is the output of the unpatched and patched version created by test_sto.py#L239-L287 (I replaced and formatted some parts and added the RPC calls at the bottom):

Unpatched output: http://pastebin.com/QNBDYakx Patched output: http://pastebin.com/DaCMhtad

It's also notable, as a side note, that "recipients" of "getsto_MP" always seems to be empty.

[zathras-crypto]

Right right, thanks - actually it was the discrepancy between what we showed on RPC (which is derived from levelDB) versus what we were logging (which is printf'd directly in the function) that put me onto this in the first place.

Your results look pretty clear :)

FYI on getsto_MP (need to find some time to update API doc) - by default with no parameters will only display recipients in the wallet (so end users by default when running that call will only see their own payments not all of them which could be thousands). Chuck a "*" on the end mate, should give you what you need :)

[dexX7]

Chuck a "*" on the end mate, should give you what you need :)

Ahh yeah, that works. :)

[zathras-crypto]

Epic fail on my part guys, not sure what happened there was sure I'd tested - sorry :(

Thanks @dexX7 for the good eye & testing :)

Just confirmed the findings (note fee differs between log and RPC):

zathras@coredev01:~/github/build/mastercore$ rm ~/.bitcoin/testnet3/mastercore.log
zathras@coredev01:~/github/build/mastercore$ src/mastercored --testnet --daemon --server --checkblocks=1 --startclean
Master Core server starting
mastercore_init()TESTNET, line 2412, file: mastercore.cpp
CMPTradeList(): OK, line 376, file: mastercore.h
CMPSTOList(): OK, line 333, file: mastercore.h
CMPTxList(): OK, line 433, file: mastercore.h
[Snapshot] Exodus balance: 0.00000000
starting block= 263000, max_block= 315895
   {balance list removed}
starting block= 263000, max_block= 315895
n_total= 1183862, n_found= 1109
[Initialized] Exodus balance: 30019.93736224
zathras@coredev01:~/github/build/mastercore$ src/mastercored getsto_MP "b8aa7efb381f8c6450fe70f2e96a5cf2c9e60900124544e882fe546dfaf7c958" | grep stofee
    "totalstofee" : "0.00000093",
zathras@coredev01:~/github/build/mastercore$ cat ~/.bitcoin/testnet3/mastercore.log | grep -a200 \ txid:\ b8aa7efb381f8c6450fe70f2e96a5cf2c9e60900124544e882fe546dfaf7c958 | grep Transfer\ fee
    Transfer fee: 0.00000095 MSC

And after patch is correct:

zathras@coredev01:~/github/build/mastercore$ rm ~/.bitcoin/testnet3/mastercore.log
zathras@coredev01:~/github/build/mastercore$ src/mastercored --testnet --daemon --server --checkblocks=1 --startclean
Master Core server starting
mastercore_init()TESTNET, line 2412, file: mastercore.cpp
CMPTradeList(): OK, line 376, file: mastercore.h
CMPSTOList(): OK, line 333, file: mastercore.h
CMPTxList(): OK, line 433, file: mastercore.h
[Snapshot] Exodus balance: 0.00000000
starting block= 263000, max_block= 315895
   {balance list removed}
starting block= 263000, max_block= 315895
n_total= 1183862, n_found= 1109
[Initialized] Exodus balance: 30019.93736224
zathras@coredev01:~/github/build/mastercore$ src/mastercored getsto_MP "b8aa7efb381f8c6450fe70f2e96a5cf2c9e60900124544e882fe546dfaf7c958" | grep stofee
    "totalstofee" : "0.00000093",
zathras@coredev01:~/github/build/mastercore$ cat ~/.bitcoin/testnet3/mastercore.log | grep -a200 \ txid:\ b8aa7efb381f8c6450fe70f2e96a5cf2c9e60900124544e882fe546dfaf7c958 | grep Transfer\ fee
    Transfer fee: 0.00000093 MSC

@m21 this should be good to merge mate :)

Thanks Z

EDIT: Tested wrong version initially (without Michael's commit) retested with his commit and updated results - conclusion still the same, dexx's change is required.

[m21]

Thanks guys, a bit distracted at the moment, but I see my omission. Merged.