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

Change object_id to more than 32 bit #1088

Closed
abitmore opened this Issue Jun 23, 2018 · 15 comments

Comments

Projects
5 participants
@abitmore
Member

abitmore commented Jun 23, 2018

According to the code, object_id::instance is unsigned_int:

However, unsigned_int is actually implemented with uint32_t in FC (code is here):

struct unsigned_int {
   //...    
    uint32_t value;

That means the maximum value is 4,294,967,296.

As of writing, the object with biggest instance value in BitShares main net is 2.9.239985810, which is 1/18 of the maximum value.

The chain is now running at around 1M operations per day, which is around 365,000,000 operations per year, so around 10 years to hit the maximum. If average TPS reached 100, the limitation will be hit in a little more than a year.

This is an important issue, but we still have some time to fix it.

Things to be done:

  • implement a 64-bit integer wrapper, with variable-length serialization which is compatible with current unsigned_int, change object_id to use that one
  • check code elsewhere which is using the 32-bit unsigned_int and change to the 64-bit one as well
  • remove the 32-bit data type

Since it affects serialization, after fixed in core, client libraries will need to adapt, so we need to at least notify them. Old client libs should be compatible before we reach the 32bit limit. Known client libraries include:

By the way, total_ops and removed_ops in account_statistics_object are 32 bit. Perhaps worth a new issue though.

@abitmore abitmore added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jun 24, 2018

@ryanRfox ryanRfox moved this from New -Awaiting Core Team Evaluation to Unassigned - Protocol Upgrades in Project Backlog Jun 25, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 5, 2018

By the way, total_ops and removed_ops in account_statistics_object are 32 bit. Perhaps worth a new issue though.

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Jul 17, 2018

@ryanRfox I want to claim this issue if possible please

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 17, 2018

@cogutvalera thanks for your passion. However, to be honest, this issue is very complicated, I don't think you're ready.

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Jul 17, 2018

@abitmore let's see ? I don't think it is impossible for me or too complicated ;-) can I try to fix this issue or not ?

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 17, 2018

Of course you can try. But I still think it's better (for you) to get familiar with the code base with smaller issues first. Just my own opinion though.

By the way, this issue is now labeled "discussion needed". We need to evaluate it, figure out the scope, the direction, before start coding.

@cogutvalera

This comment has been minimized.

Member

cogutvalera commented Jul 17, 2018

@abitmore Thanks a lot ! Let's discuss it more before coding :-) 👍 Just want to teach myself faster about core code base by trying to fix more interesting and complicated issues. Let's discuss first of all this issue and we'll see later ...

@pmconrad

This comment has been minimized.

pmconrad commented Jul 18, 2018

@abitmore what is your concern regarding serialization?
AFAICS modifying unsigned_int to be based on uint64_t instead of uint32_t will be serialization-compatible with current code (until the value exceeds 32 bits obviously).

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 18, 2018

I mean we need to make sure all components will work after the value exceeded 32 bits; IIRC there was an issue about it, from EOS.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Jul 23, 2018

I am just thinking about the serialization issue. To test:

  1. Old code serialized an unsigned_int
  2. New code reads it in correctly
  3. New code serializes it, and passes it to old code
  4. Old code reads it.

My question is where are the places that use serialization? Persisting to disk and p2p are what come to mind. Any others? I may be able to create a test if everyone thinks it is worthwhile.

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 23, 2018

@jmjatlanta the test case in bitshares/bitshares-fc#64 explains. We can make 2 test cases, one for numbers less than 32 bit and the other for numbers greater than 32 bit, both old and new code should pass the former, but only new code will pass the latter.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Jul 24, 2018

I'm just thinking of the un-serialization of a 32-bit value by a peer running old code, where the stream was serialized by a machine running new code. So during the unpack of this value, will the old code do the right thing?

I'm not saying it is broken. I just want to provide a scenario that may not have been tested for. I think the only way to test that is to pack() with the new code and unpack() with the old code (where unsigned_int has a different definition).

As I have not gone through the particulars of pack and unpack, I may be thinking of a problem that simply does not exist.

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 25, 2018

Technically it's correct, but practically we won't have both the old code and the new code in the code base. So the test cases should be some inputs and expected outputs. Sure we need to test both pack and unpack.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Jul 25, 2018

Okay. Thank you for the clarification @abitmore . I assumed we used this serialization for p2p as well.

@abitmore abitmore added this to To Do in Feature Release (201808) via automation Jul 30, 2018

@abitmore abitmore removed this from Unassigned - Protocol Upgrades in Project Backlog Jul 30, 2018

@abitmore abitmore moved this from To Do to In Development in Feature Release (201808) Jul 30, 2018

@abitmore abitmore removed this from In Development in Feature Release (201808) Jul 30, 2018

@abitmore abitmore added this to To do in Feature release (201810) via automation Jul 30, 2018

@abitmore abitmore moved this from To do to In progress in Feature release (201810) Jul 30, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 30, 2018

Technically this is a consensus-changing issue, but practically we won't hit the edge in the soon future, so I moved it to a feature release.

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 23, 2018

Fixed by #1267.

@abitmore abitmore closed this Aug 23, 2018

Feature release (201810) automation moved this from In progress to Done Aug 23, 2018

@pmconrad pmconrad referenced this issue Aug 23, 2018

Merged

unsigned_int unpacking #1267

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