Fix serde

[ghostant-1017]

Motivation

Fix the bug in Block serialize and deserialize

[vicsn]

Nice find. Why did test_serde_json below not catch any issues? Can any of the underlying test logic be adjusted so it would have failed earlier? And as a result, would we need to re-evaluate any other test_serde_json functions in snarkVM?

[ghostant-1017]

@vicsn
In current test cases, we sample a couple of genesis blocks which's cumulative_weight is 0 to make tests on serialize and deserialize.

I don't think any adjustments are necessary, but we need to check if the types used in snarkVM are unsupported by JSON

[d0cd]

@ghostant-1017 great find!
Would it be possible to add some of the failing test cases to this PR?
Also curious what are the implications in rolling out the fix? Would it break any existing code/infra/services?

[ghostant-1017]

@d0cd
In JSON, the largest numeric type is f64, so any values in the u128 range that exceed f64 will cause test case failures. However, existing test cases do not cover this scenario.

To my knowledge, only the RPC interfaces related to /block/ have been affected by serialization.
Affect:
The json block returned after height 1203484 on mainnet can no longer be deserialized by the Block provided by snarkVM.

[vicsn]

In current test cases, we sample a couple of genesis blocks which's cumulative_weight is 0 to make tests on serialize and deserialize.

I think a clean approach to testing could be to add a function similar to sample_genesis_block(, which uses Block::from_unchecked to construct a random invalid block to test serialization/deserialization on.

If you're not interested to add the test, just let us know and I'll close this PR, adding the test in a new one.

[iamalwaysuncomfortable]

Nice find. Why did test_serde_json below not catch any issues? Can any of the underlying test logic be adjusted so it would have failed earlier? And as a result, would we need to re-evaluate any other test_serde_json functions in snarkVM?

@vicsn

serde-json by default deserializes directly to an f64 if it finds an integer that can only be represented by the u128 type.

The Number enum in serde-json without the arbitraryprecision flag can hold a u64, i64, or f64. Thus without the arbitraryprecision flag, Number isn't capable of holding a u128 directly and defaults to storing it as an f64. The arbitraryprecision flag creates an alternative Number enum that represents higher precision with two u64s, i64s, or f64s. I don't think it would be wise to rely on that flag because it would change how every single type is represented during the json Deserialization process and we're unaware the side effects that would have. It would also make everyone else using serde-json in Rust projects surrounding Aleo to use it.

The reason we didn't likely see it in testing, is because we didn't end up testing with a number above u64::MAX. If we had we might've caught the error, so test cases that use integers that require more than 8 bits should be added.

[ghostant-1017]

@vicsn I will make some changes according to @iamalwaysuncomfortable , and add the test case sir.

[vicsn]

Amazing!

[zkxuerb]

LGTM

[ljedrz]

LGTM 👌

[zosorock]

check-clippy finishes fine, in 34s. It is just CircleCI being its usual garbage.