Packing

[rrtoledo]

This PR tackles issue #52 .

[AntoineRondelet]

In addition to the review please make sure to rebase this branch on top of develop @rrtoledo

[AntoineRondelet]

FYI: You'll need to rebase with develop for the build to pass @rrtoledo

[AntoineRondelet]

bits_input = extract_extra_bits(index, index+(size_extra_bits-1), primary_inputs);

Hum... here, you do:

uint size_extra_bits = digest_length % field_capacity;

Which leads to size_extra_bits = digest_length if digest_length < field_capacity.
Then if digest_length < field_capacity, you set size_extra_bits = 0, and then, you do

bits_input = extract_extra_bits(index, index+(size_extra_bits-1), primary_inputs);

Here the size_extra_bits-1 will be -1,, no? Why did you size_extra_bits instead of digest_length-field_capacity here? @rrtoledo

[AntoineRondelet]

Basically, we'll have:

bits_input = extract_extra_bits(index, index+(0-1), primary_inputs);

so the end will be smaller than begin in the case where size_extra_bits is set to 0

well, actually, it seems that we need a require to enforce that digest_length > field_capacity for the function to work as expected

[rrtoledo]

I used the modulo (uint size_extra_bits = digest_length % field_capacity;) as a generalisation (say we have digest_length = 2*field_capacity + a for some a). I did not use modulo consistently however which may lead to problems (if a simple substraction is not enough).

The -1 is not to extract the last position asked. I can see how this leads to a problem. A solution would be to change the function definitions so that the second argument is the number of bits we want to extract and not the position of the last bit we want to extract.

I wrote size_extra_bits = 0 to have an edge case where we do not need to extract any bits (when digest_length <= field_capacity) . The -1 however brings an error (we'll have the start position > end position, and potentially the end position equal to -1 resulting in an overflow). The previous solution should resolve it.

[AntoineRondelet]

I did not use modulo consistently however which may lead to problems (if a simple substraction is not enough).

Yes

A solution would be to change the function definitions so that the second argument is the number of bits we want to extract and not the position of the last bit we want to extract

I like this solution better yes

[rrtoledo]

Fixed.

[AntoineRondelet]

Since length is constrainted to be <16, (length-1)/field_capacity will always be 0, and so index_end_bytes32 = index_start_bytes32, right?

[AntoineRondelet]

You should do uint index_end_bytes32 = 1 + 1 + 2*jsIn + jsOut + (offset+start+length-1)/field_capacity; if I get your intention correctly. In fact, (offset+start)/field_capacity will be div_floored, and (length-1)/field_capacity = 0

[rrtoledo]

Yes you are right.

I tried to do an optimization and the tests passed only because of our particular case. I'll correct that right away.

[AntoineRondelet]

Let's have something working first, and we'll think about optimizations in a second time. For now, we should focus on having a documented and clear code that works.

[AntoineRondelet]

#104

[AntoineRondelet]

uint padded_start = start + offset + ((primary_inputs.length-1 == index_start_bytes32) ? potential_padding : length_extra_bits);

and now you do:

res = res * uint16(2**(padded_start % 8));

Is this supposed to be equivalent to what you did in your previous commit

uint index_start_bit = (offset + padding_start + start) % 8;

? Because it does not seem to be.

[rrtoledo]

It should be equivalent:
We have that
padding_start = digest_length - ( (offset + (digest_length - field_capacity)*(2*jsIn + jsOut)) % field_capacity if primary_inputs.length-1 == index_start_bytes32
else digest_length - field_capacity

On the other side we have:
uint padded_start = start + offset + ((primary_inputs.length-1 == index_start_bytes32) ? potential_padding : length_extra_bits);
So what was "padding start" should be: ((primary_inputs.length-1 == index_start_bytes32) ? potential_padding : length_extra_bits);
Thus we have
potential_padding = digest_length - ((offset + length_extra_bits*(2*jsIn + jsOut)) % field_capacity);, the same as before, for primary_inputs.length-1 == index_start_bytes32 and by definition length_extra_bits equals digest_length - field_capacity

The main difference is than now if we ask for a length of 0 we directly leave.

[AntoineRondelet]

Throughout this PR, and #104 we bumped into several edge cases that should be fixed now:

1. The end pointer (now removed) was potentially smaller than start (and some underflow on uint were possible)
2. The require on length was not properly set

Thus, to make sure that the flaws addressed in the PR have properly been patched please add test cases to hit the require, to pass a length == 0 (hit the return), and other potential scenarios you feel are relevant to test the fixes discussed in this PR

[rrtoledo]

I totally agree.
I intend to add a few more tests.

I want to cover the following cases:

• length of 0
• length within the require
• length above the require

And for length within the require:

• bits spread over two field elements
• bits spread over one field element but two bytes
• bits spread over one byte of a field element.

I also intend to add a require on the size of the function input primary_inputs (we might try to retrieve non existing elements) and add some tests on it.

[rrtoledo]

I forgot: we also need to constraint start and start + length - 1.

[AntoineRondelet]

Just realized that in https://github.com/clearmatics/zeth/blob/packing/src/test/note_test.cpp we do:

get_bits256_from_vector(hex_digest_to_binary_vector("FF0000000000000000000000000000000000000000000000000000000000000F"));

and

bits64 value_bits64 = get_bits64_from_vector(hex_to_binary_vector("2F0000000000000F"));

Let's just replace get_bits64_from_vector(hex_to_binary_vector( by hex_value_to_bits64
and get_bits256_from_vector(hex_digest_to_binary_vector( by hex_digest_to_bits256

[AntoineRondelet]

Note: This can be fixed in another PR as it is not strictly related to this one

[rrtoledo]

Fixed in PR #151