64 bits unsigned int #64

pmconrad · 2018-07-18T16:17:30Z

Test case and initial implementation for bitshares/bitshares-core#1088

jmjatlanta · 2018-08-10T12:30:19Z

src/io/varint.cpp

@@ -6,5 +6,5 @@ namespace fc
 void to_variant( const signed_int& var, variant& vo, uint32_t max_depth ) { vo = var.value; }
 void from_variant( const variant& var, signed_int& vo, uint32_t max_depth ) { vo.value = static_cast<int32_t>(var.as_int64()); }


All looks good so far. But this opens the can of worms... signed_int is 32 bit and unsigned_int is 64. Perhaps a separate ticket.

abitmore · 2018-08-11T19:06:41Z

Steem team is trying this: steemit/steem#2780.

jmjatlanta

bitshares-fc/include/fc/io/raw.hpp

Lines 192 to 200 in 7ac533b

    
           template<typename Stream> inline void unpack( Stream& s, unsigned_int& vi, uint32_t _max_depth ) { 
        
             uint64_t v = 0; char b = 0; uint8_t by = 0; 
        
             do { 
        
                 s.get(b); 
        
                 v |= uint32_t(uint8_t(b) & 0x7f) << by; 
        
                 by += 7; 
        
             } while( uint8_t(b) & 0x80 ); 
        
             vi.value = static_cast<uint32_t>(v); 
        
           }

I believe the static cast needs to be changed.

Note that the fix @abitmore mentioned (steemit/steem#2780) included some added protection for malformed varint unpacking. I am not sure if we want to add that here too. That may warrant a separate issue.

abitmore · 2018-08-17T19:37:53Z

I tend to do the protection in this PR.

Line 196 should use 64 bit as well:

bitshares-fc/include/fc/io/raw.hpp

Lines 192 to 200 in 7ac533b

    
           template<typename Stream> inline void unpack( Stream& s, unsigned_int& vi, uint32_t _max_depth ) { 
        
             uint64_t v = 0; char b = 0; uint8_t by = 0; 
        
             do { 
        
                 s.get(b); 
        
                 v |= uint32_t(uint8_t(b) & 0x7f) << by; 
        
                 by += 7; 
        
             } while( uint8_t(b) & 0x80 ); 
        
             vi.value = static_cast<uint32_t>(v); 
        
           }

pmconrad · 2018-08-19T08:30:12Z

Latest commit shows that fc::signed_int serialization is broken for values in the range 0x40000000..0x7fffffff.
Will remove it from the codebase since it isn't used by the core (there are a few references though, so bumping fc will require core changes).

abitmore

How about throwing an exception when the input is not valid, e.g. too large?

abitmore · 2018-08-19T10:02:10Z

include/fc/io/raw.hpp

@@ -193,10 +193,10 @@ namespace fc {
      uint64_t v = 0; char b = 0; uint8_t by = 0;
      do {
          s.get(b);
-          v |= uint32_t(uint8_t(b) & 0x7f) << by;
+          v |= uint64_t(uint8_t(b) & 0x7f) << by;


How about checking by before the bit shift? For example, if b is 0x7f and by is 7*9=63, it will overflow, so result of the bit shift will be 1.

BTW I suspected that bitshares/bitshares-core#1256 was caused by something like this, but I guess it won't be the case due to limit of maximum message size.

https://en.cppreference.com/w/cpp/language/operator_arithmetic

For unsigned and positive a, the value of a << b is the value of a * 2^b, reduced modulo maximum value of the return type plus 1 (that is, bitwise left shift is performed and the bits that get shifted out of the destination type are discarded).

So for uint64_t, (0x7f << 63) == (1 << 63).

Hm, that means this change still doesn't enforce a canonical representation. Not sure if that was the intention.

So (0x7e << 63) == 0 which doesn't seem right to me (talking about our code but not the << operator itself).

Still, how about throwing an exception when the input is not valid, e.g. too large?

abitmore · 2018-08-19T10:32:26Z

include/fc/io/raw.hpp

          by += 7;
-      } while( uint8_t(b) & 0x80 );
-      vi.value = static_cast<uint32_t>(v);
+      } while( (uint8_t(b) & 0x80) && by < 64 );


Just a note: according to the discussion in steemit/steem#2780, at a glance this change will break consensus, but our serialization code won't produce data to trigger it, so it's safe to sanitize inputs like this.

jmjatlanta · 2018-08-19T13:21:21Z

I don't want to throw a wrench in the machine, but was just thinking "out of the box" a bit.

This implementation is very similar to the varint implementation that Google Protobuf uses. Perhaps looking at their implementation could give some insight as to how to code it. I would guess their implementation has even been tested for when Tuesday falls on a weekend.

pmconrad · 2018-08-19T16:53:44Z

Rebased on master for testing with bitshares/bitshares-core#1267 branch

abitmore · 2018-08-19T18:05:17Z

include/fc/io/raw.hpp

@@ -172,9 +172,11 @@ namespace fc {
      uint64_t v = 0; char b = 0; uint8_t by = 0;
      do {
          s.get(b);
+          if( by >= 64 || (by == 63 && b > 1) )


b > 1: should convert b to unsigned.

Actually, this check can be changed to if( by == 63 && b != 1 ), since

we can reject the data if b==0 (it doesn't make sense to add one more byte which is zero, although it's not an overflow);

we will either end the loop or throw an exception when b is 63, so by >= 64 will always be false thus can be removed.

If by == 63 && b < 0 then uint8_t(b) & 0x80 != 0, so the while loop will continue, and in the next round by >= 64 will trigger the condition and throw.

by >= 64 cannot be left out, because I removed the check from the while loop (want to throw, not just exit the loop).

We could disallow the case by == 63 && b == 0, but it will be handled by the enclosing logic (deserialize, then re-serialize and check the signature) anyway. Even if we disallow trailing 0 or 0x80 bytes completely, we do not have a completely canonical representation, because we do not know the original type that is being serialized as an unsigned_int here (could be uint8_t, uint16_t, uint32_t, uint64_t).

I think the original intent of the EOS fix was only to avoid the undefined behaviour associated with by growing bigger than the bit size of value.

Makes sense, thanks.
I still think it's better to check whether v < 0 is true here to make the code/logic clearer.

Added cast to uint8_t, it's cheaper than separately checking for <0.

pmconrad · 2018-08-20T13:39:18Z

This implementation is very similar to the varint implementation that Google Protobuf uses.

In principle this is not a bad suggestion. I'm all for not re-inventing wheels.

For blockchain code though, it is important to be internally consistent. It is always dangerous to rely on external libraries, because an internal change in that library could lead to breaking consensus.

abitmore

Changed code looks fine. Not sure if there is something missing.

pmconrad · 2018-08-22T12:40:20Z

@jmjatlanta please re-review.

abitmore mentioned this pull request Jul 23, 2018

Change object_id to more than 32 bit bitshares/bitshares-core#1088

Closed

6 tasks

jmjatlanta reviewed Aug 10, 2018

View reviewed changes

jmjatlanta suggested changes Aug 17, 2018

View reviewed changes

abitmore mentioned this pull request Aug 17, 2018

Review and backport EOS patch about unsigned_int unpacking bitshares/bitshares-core#993

Closed

pmconrad added 7 commits August 19, 2018 11:01

Added unit test for serialization/deserialization of unsigned_int

f8940a6

Support 64 bit values in unsigned_int object

9483935

Changed some casts to uint64_t

58ac6ae

Fix #993 - limit unpacking length of signed_int and unsigned_int

72bcc8a

Expanded tests for unsigned_int to 64 bits

a39e0d1

#993 - unit test

0c22469

Fixed alleged c&p bug

4b61f3c

abitmore reviewed Aug 19, 2018

View reviewed changes

This was referenced Aug 19, 2018

unsigned int unpacking cryptonomex/graphene#690

Closed

unsigned_int unpacking bitshares/bitshares-core#1267

Merged

abitmore mentioned this pull request Aug 19, 2018

Implement varuint64 and replace varint32 bitshares/bitsharesjs#29

Open

pmconrad added 2 commits August 19, 2018 18:26

Removed signed_int

1dcacba

Throw overflow_exception instead of silently cutting off data

79ff754

pmconrad force-pushed the 1088_unsigned_int branch from c50b539 to 79ff754 Compare August 19, 2018 16:53

abitmore reviewed Aug 19, 2018

View reviewed changes

Handle b<0

02a4516

abitmore approved these changes Aug 21, 2018

View reviewed changes

jmjatlanta approved these changes Aug 22, 2018

View reviewed changes

pmconrad merged commit 2405081 into master Aug 22, 2018

pmconrad deleted the 1088_unsigned_int branch August 22, 2018 15:12

This was referenced Aug 22, 2018

64 bit unsigned_int bitshares/python-bitshares#133

Open

unsigned_int unpacking #53

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

64 bits unsigned int #64

64 bits unsigned int #64

pmconrad commented Jul 18, 2018

jmjatlanta Aug 10, 2018

abitmore commented Aug 11, 2018

jmjatlanta left a comment •

edited

Loading

abitmore commented Aug 17, 2018

pmconrad commented Aug 19, 2018

abitmore left a comment

abitmore Aug 19, 2018

pmconrad Aug 19, 2018 •

edited

Loading

pmconrad Aug 19, 2018

abitmore Aug 19, 2018

abitmore Aug 19, 2018

abitmore Aug 19, 2018

jmjatlanta commented Aug 19, 2018

pmconrad commented Aug 19, 2018 •

edited

Loading

abitmore Aug 19, 2018

abitmore Aug 19, 2018 •

edited

Loading

pmconrad Aug 20, 2018 •

edited

Loading

abitmore Aug 20, 2018

pmconrad Aug 21, 2018

pmconrad commented Aug 20, 2018

abitmore left a comment

pmconrad commented Aug 22, 2018

		@@ -6,5 +6,5 @@ namespace fc
		void to_variant( const signed_int& var, variant& vo, uint32_t max_depth ) { vo = var.value; }
		void from_variant( const variant& var, signed_int& vo, uint32_t max_depth ) { vo.value = static_cast<int32_t>(var.as_int64()); }

	template<typename Stream> inline void unpack( Stream& s, unsigned_int& vi, uint32_t _max_depth ) {
	uint64_t v = 0; char b = 0; uint8_t by = 0;
	do {
	s.get(b);
	v \|= uint32_t(uint8_t(b) & 0x7f) << by;
	by += 7;
	} while( uint8_t(b) & 0x80 );
	vi.value = static_cast<uint32_t>(v);
	}

64 bits unsigned int #64

64 bits unsigned int #64

Conversation

pmconrad commented Jul 18, 2018

Choose a reason for hiding this comment

abitmore commented Aug 11, 2018

jmjatlanta left a comment • edited Loading

Choose a reason for hiding this comment

abitmore commented Aug 17, 2018

pmconrad commented Aug 19, 2018

abitmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmconrad Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmjatlanta commented Aug 19, 2018

pmconrad commented Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

abitmore Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

pmconrad Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmconrad commented Aug 20, 2018

abitmore left a comment

Choose a reason for hiding this comment

pmconrad commented Aug 22, 2018

jmjatlanta left a comment •

edited

Loading

pmconrad Aug 19, 2018 •

edited

Loading

pmconrad commented Aug 19, 2018 •

edited

Loading

abitmore Aug 19, 2018 •

edited

Loading

pmconrad Aug 20, 2018 •

edited

Loading