New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect value from getTotalTokens when fees are cached #439

Merged
merged 1 commit into from Dec 12, 2016

Conversation

Projects
None yet
2 participants
@zathras-crypto
Copy link

zathras-crypto commented Dec 10, 2016

When a property was not created via a fixed issuance, we use a method of iterating over the tally map to calculate the total number of tokens.

However after fees are activated, some tokens may not be in the tally, instead being held in the fee cache.

getTotalTokens() must be made aware of the fee cache and needs to include cached fees in the total count of tokens.

This change is consensus breaking.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 10, 2016

I didn't quite know what to do about this bud being that it's consensus affecting, so I've put it in as a separate PR.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 11, 2016

Hmm.. do we need to count cached amounts at all?

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 11, 2016

Hmm.. do we need to count cached amounts at all?

If we want to provide accurate data :)

Let's take a couple examples:

  1. We trade some Tether and Tether gets moved to the fee cache. When we now call something like getProperty on the RPC layer the information we provide about the number of tokens would be incorrect.
  2. I create a property via crowdsale. While the crowdsale is active we trade some of the property meaning some of the tokens are moved to the fee cache. Since getTotalTokens() is now inaccurate the crowdsale helpers can make a mess of the math, including creating more than MAX_INT64 tokens.

So to answer your question, yes I really do believe we need to count cached amounts - getTotalTokens() is a fairly critical function and I don't really consider it feasible to leave it returning incorrect data.

TL:DR; if we activate fees as is there are scenarios where we could break things and I'm a bit worried about that.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 11, 2016

Note - I know this was my mistake when I wrote the fees code, and I know no-one is going to like needing a fix before we can activate fees, but I just didn't see the problem before now :(

@dexX7 dexX7 added the consensus label Dec 12, 2016

@dexX7 dexX7 added this to the Next release milestone Dec 12, 2016

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 12, 2016

TL:DR; if we activate fees as is there are scenarios where we could break things and I'm a bit worried about that.

Thanks for laying out examples. I agree, we really shouldn't activate before this is fixed. :)

Hmm.. so this makes 0.2 consensus breaking. Do you think we should also publish an updated 0.10 based version? I think we might, if we release them simultaneously.

@dexX7

dexX7 approved these changes Dec 12, 2016

@dexX7 dexX7 added the status: ready label Dec 12, 2016

@dexX7 dexX7 merged commit 11c007e into OmniLayer:develop Dec 12, 2016

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

dexX7 added a commit that referenced this pull request Dec 12, 2016

Merge #439: Fix incorrect value from getTotalTokens when fees are cached
11c007e Fix incorrect value from getTotalTokens when fees are cached (zathras-crypto)
@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Dec 13, 2016

Hmm.. so this makes 0.2 consensus breaking. Do you think we should also publish an updated 0.10 based version? I think we might, if we release them simultaneously.

Yeah, that's what I really didn't like about the change. I really liked the idea of having 0.2 be a release that didn't change any rules.

Perhaps we could do a 0.0.11.3-rel with the fix and that becomes the stable version for now. Then when 0.2 comes it wouldn't contain any new rules. If we did another 0.10 base release there is the option to backport the perf improvements too. I'm not really sure though.

I'm also going to take another look at fees to see if there is anything else I can find.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Dec 13, 2016

Perhaps we could do a 0.0.11.3-rel with the fix and that becomes the stable version for now.

Sounds fine to me. We could then release it with 0.2, and provide a choice for users.

If we did another 0.10 base release there is the option to backport the perf improvements too. I'm not really sure though.

Hmm.. we should encourage people to use 0.2 anyway, so I don't think it's worth porting, but I wouldn't mind either. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment