Refactor rct bulletproofs struct, per Jcape suggestion #6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
these are introduced at block version 3 the token type of every pseudo output and real output must be listed in `pseudo_output_token_ids` and `output_token_ids` in the `SignatureRctBulletproofs` object
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
this should happen at the time of adding RTH support to mobilecoind
Co-authored-by: Remoun Metyas <[email protected]>
this addresses review comments
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
Co-authored-by: Remoun Metyas <[email protected]>
this cleans up the constructor
It was suggested that instead of having a bunch of repeated fields, that all have to have the same length, we should just have one struct that is repeated, and deprecate all the older repeated fields. This is an experimental commit refactoring rct bulletproofs object in this direction. Right now, I think the change is a good idea in the long term, but in the short term it is high risk because it makes this code harder to review. It creates a bunch of additional churn around checking `RingMLSAG` signatures due the need to support backwards compatibility For example, in the `fn key_images(&self)` function, if we overlooked that `RingMLSAG` can appear in two different places now, then we can cause an infinite spending bug. It might be simpler to just create an entirely new SignatureRctBulletproofs object, than to keep nursing the old one along and branching everywhere on block version. But this will definitely increase the complexity of review. The only complexity being saved here is, avoiding the need to check that some lists have the same length, which while annoying, isn't that complicated.
cbeck88
force-pushed
the
feature/mixed-transactions
branch
from
0037f82
to
71cd422
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It was suggested that in the Mixed Transactions PR, in the
SignatureRctBulletproofsobject, instead of having a bunch of repeated fields,that all have to have the same length, we should just have one struct
that is repeated, and deprecate all the older repeated fields.
This is an experimental commit refactoring rct bulletproofs object
in this direction.
Right now, I think the change is a good idea in the long term, but in
the short term it is high risk because it makes this code harder to review.
It creates a bunch of additional churn around checking
RingMLSAGsignaturesdue the need to support backwards compatibility
For example, in the
fn key_images(&self)function, if we overlookedthat
RingMLSAGcan appear in two different places now, then we cancause an infinite spending bug.
It might be simpler to just create an entirely new SignatureRctBulletproofs
object, than to keep nursing the old one along and branching everywhere
on block version. But this will definitely increase the complexity of review.
The only complexity being saved here is, avoiding the need to check that some
lists have the same length, which while annoying, isn't that complicated.