mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Send to owners: double not sufficient for calculation #273

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago

See: https://github.com/msgilligan/bitcoin-spock/issues/27

While one could say: "fine, it's not super accurate, but it works more or less", there are more critical scenarios:

1) A starts with 9007199254740993 SPT 2) B starts with 9007199254740993 SPT 3) A sends 9007199254740993 SPT to all SPT owners

The result: 4) A ends up with 1 0 SPT 5) B ends up with 18014398509481985 SPT

1 SPT was not transferred.. simply.. vanished.. ;)

The following works with all test cases and should work with the full range:

using namespace boost::multiprecision;

int128_t temp = int128_t(owns) * int128_t(nValue);
int128_t piece = 1 + (temp - 1) / int128_t(totalTokens);

int64_t should_receive = piece.convert_to<int64_t>();
zathras-crypto commented 9 years ago

Vanishing satoshi? Great catch - no way we can allow funds to disappear due to rounding. In fact, has got me thinking about whether we could add in some kind of auditing module that runs at the start & end of block handler, caching total token counts & tallying any crowds/grants/revokes - audit module could confirm token counts correct at end of each block or similar and flag if something has gone wrong? Since very few transactions actually modify total counts most blocks would be a simple compare before/after block to make sure no sends/STOs/trades etc have changed total token counts.

Just thinking out loud here - but 1) very happy we have people like DexX with a sharp eye for stuff like this and 2) at what point does the code auditing itself eg as suggested above (in addition to external testing like spock) become appropriate?

Thanks :) Z

dexX7 commented 9 years ago

audit module

This would really be great, and sort of related to the whole "ledger"/"audit trail"-tally-idea.

disappear due to rounding

Actually this is not due to rounding, but because double was used. There is a gap and the number 9007199254740993 as double basically doesn't exist, so it's "rounded" to 9007199254740992.

sharp eye ... great catch

Hehe, after Marv's test revealed the existence of an issue, I started to look for edge cases and odd behavior. This catch was designed, so to speak.. ;)

I think the more safe guards there are, the better. Confirming after each block that token amounts haven't unexpectedly changed is definitely a very good idea.

zathras-crypto commented 9 years ago

Re auditing, pretty straight forward I think - limited effort for something that's probably going to be pretty valuable to us - I knocked up this very simplistic code and ran on testnet https://github.com/zathras-crypto/mastercore/commit/ea3760d17504d2bf3282afc931884d7d76eb5d8d (doesn't factor crowds/grants/revokes etc, just simple identification of changes but kinda highlights how easily we could put some checks in internally):


zathras@coredev01:~/github/build/mastercore$ cut -b21- ~/.bitcoin/testnet3/mastercore.log | grep AUDIT
AUDIT: During block 263137 the number of tokens for property 2 changed from 0 to 1703400
AUDIT: During block 263140 the number of tokens for property 2 changed from 1703400 to 3974600
AUDIT: During block 263449 the number of tokens for property 2 changed from 3974600 to 4542400
AUDIT: During block 263537 the number of tokens for property 2 changed from 4542400 to 5110200
AUDIT: During block 263646 the number of tokens for property 2 changed from 5110200 to 60678000
AUDIT: During block 263688 the number of tokens for property 2 changed from 60678000 to 61813600
AUDIT: During block 263697 the number of tokens for property 2 changed from 61813600 to 62381400
AUDIT: During block 263705 the number of tokens for property 2 changed from 62381400 to 63517000
AUDIT: During block 264594 the number of tokens for property 2 changed from 63517000 to 64084800
AUDIT: During block 264604 the number of tokens for property 2 changed from 64084800 to 66356000
AUDIT: During block 264605 the number of tokens for property 2 changed from 66356000 to 67491600
AUDIT: During block 265089 the number of tokens for property 2 changed from 67491600 to 68059400
AUDIT: During block 265092 the number of tokens for property 2 changed from 68059400 to 69195000
AUDIT: During block 269664 the number of tokens for property 2 changed from 69195000 to 69249600
AUDIT: During block 269673 the number of tokens for property 2 changed from 69249600 to 69250200
AUDIT: During block 269677 the number of tokens for property 2 changed from 69250200 to 69250800
AUDIT: During block 269734 the number of tokens for property 2 changed from 69250800 to 69305400
AUDIT: During block 270494 the number of tokens for property 2 changed from 69305400 to 10069305400
AUDIT: During block 270496 the number of tokens for property 2 changed from 10069305400 to 21069305400
AUDIT: During block 270521 the number of tokens for property 2 changed from 21069305400 to 31069305400
AUDIT: During block 270522 the number of tokens for property 2 changed from 31069305400 to 61069305400
AUDIT: During block 270523 the number of tokens for property 2 changed from 61069305400 to 111069305400
AUDIT: During block 270524 the number of tokens for property 2 changed from 111069305400 to 161069305400
AUDIT: During block 270526 the number of tokens for property 2 changed from 161069305400 to 211070441000
AUDIT: During block 270527 the number of tokens for property 2 changed from 211070441000 to 211071008788
AUDIT: During block 270602 the number of tokens for property 2 changed from 211071008788 to 221071908788
AUDIT: During block 270606 the number of tokens for property 2 changed from 221071908788 to 221072484488
AUDIT: During block 270608 the number of tokens for property 2 changed from 221072484488 to 221074787288
AUDIT: During block 270611 the number of tokens for property 2 changed from 221074787288 to 221075362988
AUDIT: During block 270673 the number of tokens for property 2 changed from 221075362988 to 231075362988
AUDIT: During block 270679 the number of tokens for property 2 changed from 231075362988 to 231075930788
AUDIT: During block 270707 the number of tokens for property 2 changed from 231075930788 to 231078201988
AUDIT: During block 270709 the number of tokens for property 2 changed from 231078201988 to 231085583388
AUDIT: During block 270720 the number of tokens for property 2 changed from 231085583388 to 232085583388
AUDIT: During block 270721 the number of tokens for property 2 changed from 232085583388 to 232086734788
AUDIT: During block 270773 the number of tokens for property 2 changed from 232086734788 to 304151630588
AUDIT: During block 270776 the number of tokens for property 2 changed from 304151630588 to 309151630588
AUDIT: During block 270783 the number of tokens for property 2 changed from 309151630588 to 311151630588
AUDIT: During block 271182 the number of tokens for property 2 changed from 311151630588 to 331151630588
AUDIT: During block 271184 the number of tokens for property 2 changed from 331151630588 to 361151630588
AUDIT: During block 271193 the number of tokens for property 2 changed from 361151630588 to 362151630588
AUDIT: During block 273350 the number of tokens for property 2147483653 changed from 0 to 418908
AUDIT: During block 273353 the number of tokens for property 2 changed from 362151630588 to 362151630565
AUDIT: During block 274200 the number of tokens for property 2 changed from 362151630565 to 362152206265
AUDIT: During block 275753 the number of tokens for property 2 changed from 362152206265 to 364997750465
AUDIT: During block 275758 the number of tokens for property 2 changed from 364997750465 to 366107861565
AUDIT: During block 277863 the number of tokens for property 2 changed from 366107861565 to 375107861565
AUDIT: During block 278210 the number of tokens for property 2147483660 changed from 0 to 1000
AUDIT: During block 278213 the number of tokens for property 2147483660 changed from 1000 to 900
AUDIT: During block 278232 the number of tokens for property 2147483661 changed from 0 to 12345678900
AUDIT: During block 278233 the number of tokens for property 2147483661 changed from 12345678900 to 2469135690
AUDIT: During block 278769 the number of tokens for property 2 changed from 375107861565 to 380107861565
AUDIT: During block 278998 the number of tokens for property 2 changed from 380107861565 to 380607861565
AUDIT: During block 279005 the number of tokens for property 2 changed from 380607861565 to 381107861565
AUDIT: During block 279007 the number of tokens for property 2 changed from 381107861565 to 383107861565
AUDIT: During block 279009 the number of tokens for property 2147483664 changed from 0 to 10000
AUDIT: During block 279011 the number of tokens for property 2 changed from 383107861565 to 384337861565
AUDIT: During block 279033 the number of tokens for property 2 changed from 384337861565 to 386537861565
AUDIT: During block 279156 the number of tokens for property 3 changed from 0 to 5
AUDIT: During block 279242 the number of tokens for property 5 changed from 0 to 500
AUDIT: During block 279508 the number of tokens for property 2 changed from 386537861565 to 391536861565
AUDIT: During block 279509 the number of tokens for property 2 changed from 391536861565 to 411536861565
AUDIT: During block 279623 the number of tokens for property 2 changed from 411536861565 to 423881540465
AUDIT: During block 279903 the number of tokens for property 2 changed from 423881540465 to 424981540465
AUDIT: During block 280742 the number of tokens for property 2 changed from 424981540465 to 424981540424
AUDIT: During block 280755 the number of tokens for property 2 changed from 424981540424 to 424981595124
AUDIT: During block 280904 the number of tokens for property 2 changed from 424981595124 to 425281595124
AUDIT: During block 281094 the number of tokens for property 2 changed from 425281595124 to 426281595124
AUDIT: During block 281256 the number of tokens for property 3 changed from 5 to 20
AUDIT: During block 281997 the number of tokens for property 7 changed from 0 to 5000
AUDIT: During block 281998 the number of tokens for property 7 changed from 5000 to 2500
AUDIT: During block 282079 the number of tokens for property 2 changed from 426281595124 to 426281595079
AUDIT: During block 282125 the number of tokens for property 2 changed from 426281595079 to 427381595079
AUDIT: During block 290249 the number of tokens for property 3 changed from 20 to 17
AUDIT: During block 300321 the number of tokens for property 9 changed from 0 to 10000000000
AUDIT: During block 301458 the number of tokens for property 2 changed from 427381595079 to 432381595079
AUDIT: During block 301461 the number of tokens for property 2 changed from 432381595079 to 437381595079
AUDIT: During block 302004 the number of tokens for property 2 changed from 437381595079 to 437481595079
AUDIT: During block 302190 the number of tokens for property 2 changed from 437481595079 to 437481649679
AUDIT: During block 302193 the number of tokens for property 2 changed from 437481649679 to 437481704279
AUDIT: During block 306478 the number of tokens for property 2 changed from 437481704279 to 442481704279
AUDIT: During block 309030 the number of tokens for property 2 changed from 442481704279 to 444481704279
AUDIT: During block 317285 the number of tokens for property 2 changed from 444481704279 to 444581704279
AUDIT: During block 317309 the number of tokens for property 2 changed from 444581704279 to 444595368279
AUDIT: During block 318002 the number of tokens for property 9 changed from 10000000000 to 10123456700
AUDIT: During block 318004 the number of tokens for property 9 changed from 10123456700 to 10246913400
AUDIT: During block 318024 the number of tokens for property 36 changed from 0 to 100000000000
AUDIT: During block 318174 the number of tokens for property 36 changed from 100000000000 to 200000000000
AUDIT: During block 318376 the number of tokens for property 2 changed from 444595368279 to 444695368279
AUDIT: During block 319506 the number of tokens for property 2 changed from 444695368279 to 454695368279

Thanks Z

dexX7 commented 9 years ago

Interesting branch. And now I finally know from where you were pulling the raw transaction data! ;)

What you did is indeed straight forward. I guess the next step would be to register all changes that were intended, e.g. when properties are created. Pretty close to an audit ledger actually.


Given that there are a handful of PRs pending at the moment and not very much feedback: what's your take on this one?

zathras-crypto commented 9 years ago

Yeah, I'd really like to see us take a moment and refactor - especially for example update_tally_map into something that we can easily pull an audit trail from (each change should be registered in the trail with corresponding txid etc) but I know the push is going straight for MetaDEx once STO is out so not sure how much love that will get higher up short term - can always hope :)

Re this PR, would like to hear what @m21 thinks but long story short - I definitely support in principle and 100% say we can't tag until this issue resolved - I noticed your other comment re the change of approach to integer math so going to stick my nose in that - truth be told have not yet had an in-depth look yet...

Thanks Z

dexX7 commented 9 years ago

My bad, the token didn't vanish completely, but is still in A's possession, so only the distribution is faulty.

@zathras-crypto: it works so far. :)

dexX7 commented 9 years ago

Fixed by #275.