fix: remove unnecessary range proof verify and fix test temp disk usage #3334
Conversation
There was a problem hiding this comment.
Nice - maybe @philipr-za can confirm that this is sound and then we can remove the commented out code.
| "Range proof could not be verified".into(), | ||
| )); | ||
| } | ||
| // // A range proof can be constructed for an invalid value so we should confirm that the proof can be verified. |
There was a problem hiding this comment.
// range proof can be constructed for an invalid value so we should confirm that the proof can be verified.
Assuming the invalid value talked about here is a negative value? Something that is already an invariant because Microtari is backed by a u64.
There was a problem hiding this comment.
It does seem a bit paranoid to check the proof right after its been constructed with the same data. It came from the fact that you build the proof using the unblinded data and verify it with the commitment and the factory that produces the commitment here could be different to the commitment factory used within the tari_crypto crate during proof construction.
stringhandler
dismissed stale reviews from hansieodendaal and philipr-za
via
99e864f
stringhandler
force-pushed
the
mb-remove-extra-verify
branch
from
6870e8c
to
99e864f
Compare
|
Fixed tests by adding a simple check instead of running the full rangeproof validation. @philipr-za @hansieodendaal please have a look again |
| @@ -281,6 +282,12 @@ impl UnblindedOutput { | |||
| } | |||
|
|
|||
| pub fn as_transaction_output(&self, factories: &CryptoFactories) -> Result<TransactionOutput, TransactionError> { | |||
| if factories.range_proof.range() < 64 && self.value > MicroTari::from(1u64.shl(&factories.range_proof.range())) | |||
There was a problem hiding this comment.
I think this is correct as opposed to shl(....range() - 1)
There was a problem hiding this comment.
factories.range_proof.range() < 64 will never be true in our case since we set the range to 64.
1u64 << 64 will overflow so should be range() - 1.
There was a problem hiding this comment.
it's actually true for 90% of the tests, that's why it's there. My reasoning is that if it is 64, then a u64 automatically fits in.
The reason that this is here is because there are some tests that use a smaller range of 32. I'll have to check and see if range() - 1 is correct in this case
There was a problem hiding this comment.
so just trying 1u64.shl(64) panics with thread 'main' panicked at 'attempt to shift left with overflow'.
1u64(63) will give you half the 64-bit range.
If you are looking for the max value of an arbitrary bit depth you will need to shift up that many ones.
There was a problem hiding this comment.
This is correct now that I've added the = to the >=. The case where the range is 64 is covered simply by the fact that you cannot include a higher value inside a u64.
There was a problem hiding this comment.
I guess the short circuit protects us from the overflow
stringhandler
changed the title
stringhandler
force-pushed
the
mb-remove-extra-verify
branch
from
29459b5
to
064768f
Compare
Description
Remove an unnecessary rangeproof verify after we just created it
Note: Also reduced the size impact of LMDB temp databases that
cargo testhas when running on Windows machines.Motivation and Context
There is no benefit in verifying a rangeproof immediately after creating it
How Has This Been Tested?