Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util to convert legacy outputs to seraphis lib compatible enotes #22

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

j-berman
Copy link

This type conversion is useful to scan legacy txs (returned via the /getblocks.bin RPC endpoint) using the Seraphis wallet lib scanner. Can see it in action in this commit.

Copy link
Owner

@UkoeHB UkoeHB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not review the implementation in detail, but it looks generally correct.

Comment on lines 149 to 151
* outparam: enotes -
*/
void legacy_outputs_to_enotes(const cryptonote::transaction &tx, std::vector<LegacyEnoteVariant> &enotes);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* outparam: enotes -
*/
void legacy_outputs_to_enotes(const cryptonote::transaction &tx, std::vector<LegacyEnoteVariant> &enotes);
* outparam: enotes_out -
*/
void legacy_outputs_to_enotes(const cryptonote::transaction &tx, std::vector<LegacyEnoteVariant> &enotes_out);

Comment on lines 564 to 566
{
return tx.rct_signatures.type == rct::RCTTypeBulletproof2 || tx.rct_signatures.type == rct::RCTTypeCLSAG || tx.rct_signatures.type == rct::RCTTypeBulletproofPlus;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be multiple lines. The goal is to keep file width approximately equal to the //-- barriers.

@@ -554,4 +555,186 @@ void get_legacy_enote_record(const LegacyIntermediateEnoteRecord &intermediate_r
get_legacy_enote_record(intermediate_record, key_image, record_out);
}
//-------------------------------------------------------------------------------------------------------------------
bool is_encoded_amount_v1(const cryptonote::transaction &tx)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem to be missing header entries for all of these functions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're private helper functions for the exposed legacy_outputs_to_enotes

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case they should be static at the top of the file, with doubled-up //-- barriers.

is_encoded_amount_v2(tx);
}
//-------------------------------------------------------------------------------------------------------------------
bool try_out_to_legacy_enote_v1(const cryptonote::transaction &tx, const size_t output_index, sp::LegacyEnoteVariant &enote)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool try_out_to_legacy_enote_v1(const cryptonote::transaction &tx, const size_t output_index, sp::LegacyEnoteVariant &enote)
bool try_out_to_legacy_enote_v1(const cryptonote::transaction &tx, const size_t output_index, sp::LegacyEnoteVariant &enote_out)

etc.

@UkoeHB
Copy link
Owner

UkoeHB commented Oct 23, 2023

@jeffro256 can you review the details of this PR? Then I will merge it.

/// C
enote_v3.amount_commitment = tx.rct_signatures.outPk[output_index].mask;
/// enc(a)
const size_t byte_size = sizeof(enote_v3.encoded_amount);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const size_t byte_size = sizeof(enote_v3.encoded_amount);
constexpr size_t byte_size = sizeof(enote_v3.encoded_amount);

Should technically be constexpr to be used portably in a static_assert expression.

/// C
enote_v5.amount_commitment = tx.rct_signatures.outPk[output_index].mask;
/// enc(a)
const size_t byte_size = sizeof(enote_v5.encoded_amount);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Comment on lines +429 to +430
if (output_index >= tx.vout.size())
return false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a minor performance change, but if you change the function signature of try_out_to_legacy_enote_vx to have a reference const cryptonote::tx_out& (like is_legacy_enote_vx) instead of an output index, then you won't have to check this if branch 5 times for every single enote.

Copy link
Author

@j-berman j-berman Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I didn't do that is because the amount commitments and encrypted amounts aren't on the tx_out, they're in tx.rct_signatures.outPk[output_index] and tx.rct_signatures.ecdhInfo[output_index] respectively, and passing those in by reference and handling each try_out_to_legacy function signature differently felt clunky

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough... it doesn't matter much either way

@j-berman
Copy link
Author

Squashed

@UkoeHB UkoeHB merged commit b6c52f0 into UkoeHB:seraphis_lib Oct 24, 2023
DangerousFreedom1984 pushed a commit to DangerousFreedom1984/seraphis_lib that referenced this pull request Jan 19, 2024
DangerousFreedom1984 pushed a commit to DangerousFreedom1984/seraphis_lib that referenced this pull request Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants