mastercoin-MSC / mastercore

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

STO: with int128_t: (owns * value) / total #275

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago

See the related issue #273, this covers the full range without any loss of precision.

1) It does not overflow and fits into int128_t:

Let's assume a theoretical maximum:

  owns  = 9223372036854775807
  value = 9223372036854775807
  =>
  temp  = owns * value
        = 9223372036854775807^2
        = ~2^126

2) The result fits into int64_t:

At least two property owners are required for a "send to owners", and therefore the value to send or amount owned is guaranteed less than the total number of tokens:

  owns  < INT64_MAX
  value < INT64_MAX
  total = INT64_MAX
  =>
  piece < INT64_MAX' * INT64_MAX' / INT64_MAX
        < INT64_MAX
zathras-crypto commented 9 years ago

Hey DexX, On the bit where you're using the boost convert_to on piece to get the uint64 for should_receive can you ELI5 how rounding is applied? The previous usage of ceil was a clear roundup, but I'm unsure what's happening with rounding when you convert the int128_t piece (I don't see any explicit rounding). Thanks :) Z

dexX7 commented 9 years ago

int128_t is part of Boost's multiprecision library.

You don't see the following, because it was already included (for some crowdfunding calculations, IIRC):

#include <boost/multiprecision/cpp_int.hpp>

using boost::multiprecision::int128_t;

To convert an int128_t back to int64_t (or uint64_t), Boost has a convert_to<uint64_t>() method. This does not round, but simply changes the data type.

Integer divison discards the remainder and always rounds down:

9223372036854775807 / 9223372036854775806 = 1 (would be: 1.000000000000000000108420217249..)
9223372036854775806 / 9223372036854775807 = 0 (would be: 0.999999999999999999891579782751..)

ceil() can be used with double, but since we don't have any fractional number, 1 + (divident - 1) / divisor has a similar effect and is used as replacement:

1 + ((9223372036854775807 - 1) / 9223372036854775806) = 2
1 + ((9223372036854775806 - 1) / 9223372036854775807) = 1

In the PR it's here:

int128_t piece = 1 + ((temp - 1) / int128_t(totalTokens));
marv-engine commented 9 years ago

@dexX7 Does this PR pass all the STO tests? @m21 @zathras-crypto @dacoinminster @CraigSellars @msgilligan @achamely

dexX7 commented 9 years ago

Yup.

marv-engine commented 9 years ago

@m21 @zathras-crypto @dacoinminster @CraigSellars @msgilligan @achamely @dexX7 Is there a reason not to accept this PR now?

CraigSellars commented 9 years ago

If we pass all consensus tests and get better precision, I’m in favor of merging now.

marv-engine commented 9 years ago

@dexX7 Can you test with 5000 or 10000 owners, and also with different multiples of tokens to be distributed compared to what the owners have, e.g.:

  1. owners have 1, 2, 3, ..., N indivisible
    • STO 100,000,000 times what each owner has
    • STO 3 times what each owner has
  2. owners have 1.0, 2.0, 3.0, ..., N.0 divisible
    • STO .5 times what each owner has
    • STO 3 times what each owner has

whatever conditions & values you think will produce informative results.

Thanks!

dexX7 commented 9 years ago

I've run a several tests with 5000-8000 owners manually, but in those dimensions automated funding becomes an issue. This is totally unrelated to STO, and an issue with Bitcoin Core's "sendtoaddress" RPC call: when sending tokens to each owner, at some point the transaction creation fails with "transaction too large". I'm not sure why, and I haven't looked into it. It appears to be a known issue though. I'd probably need to do some cleanup from time to time and sweep dust which would otherwise be used to create a too large transaction.

Anyway, I started with a very basic dust collector at the end of the send to many owners tests and it allows to run the whole test suite in one go:

Debug output and detailed results are available under "Standard output".

The test data is here as table: MSCSendToManyOwnersSpec.groovy#L221

marv-engine commented 9 years ago

@dexX7 Great work!

Just to be clear, this PR passes all the other STO tests taken from the spreadsheet? Also, can you do a run with at least a thousand (or more) owners?

dexX7 commented 9 years ago

Yes, the test plan tests run fine as well. Here provided via RPC call "sendtoowners_MP" and here via raw transaction data to bypass the RPC interface (to some degree). The skipped tests are those which involve the meta DEx.

Also, can you do a run with at least a thousand (or more) owners?

Sure, I'll update, once I have more results.

dexX7 commented 9 years ago

Hehe.. two hours later: 25000 owners? Here we go:

Owners start with 25000000000, 50000000000, 75000000000 ...

The owners receive 1, 2, 3, ...

The last five digits of the resulting balance should matches the owner number:

Owner 1: 25000000001 Owner 2: 50000000002 ...

http://bitwatch.co/uploads/sto-25k-25000000000-1/

I'll flip the numbers and run it again and tomorrow I'll push the updated dust collector, so this can be reproduced on the CI server, if desired.

dexX7 commented 9 years ago

24796 owners

Owners start with 0.00000001, 0.00000002, 0.00000003, ...

The owners receive 300.00000000, 600.00000000, 900.00000000, ...

The last five digits of the resulting balance should matches the owner number:

Owner 1: 300.00000001 Owner 2: 600.00000002 ...

http://bitwatch.co/uploads/sto-25k-1-30000000000/

This went very well.

marv-engine commented 9 years ago

@dexX7 Great work!