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

add LegacyEnoteOriginContext #16

Conversation

SNeedlewoods
Copy link

@SNeedlewoods SNeedlewoods commented Dec 5, 2023

The changes quickly added up to get around compiler errors. Now it builds fine, but fails at unit_tests seraphis_enote_scanning.trivial_ledger. Before I go too deep in the complete wrong direction, some feedback would be appreciated.
I added comments with QUESTION and TODO (without a well defined distinction), for things I currently focus on.


Edit after squash:
this PR adds:

  • LegacyEnoteOriginContextV1 for pre-RingCT enotes which get indexed by enote_same_amount_ledger_index
  • LegacyEnoteOriginContextV2 for post-RingCT enotes which get indexed by rct_enote_ledger_index
  • LegacyEnoteOriginContextVariant which contains either V1 or V2 contexts
  • a bool is_pre_rct to LegacyEnoteV1 to help differentiate between V1 and V2 contexts
  • function to create pre-RingCT LegacyEnoteV1
  • some *_ref functions
  • constructors for LegacyContextual(Intermediate)EnoteRecords to set the LegacyEnoteOriginContextVariant to the correct type or to default type post-RingCT
  • functionality for the new indexes in the scanner and mock ledger
  • tests in seraphis_enote_scanning: legacy_pre_rct_1 & legacy_coinbase_1

Feel free to review.

Copy link

@j-berman j-berman left a comment

Choose a reason for hiding this comment

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

Quick first pass, it's looking like a good start. Thank you :)

src/seraphis_main/contextual_enote_record_types.h Outdated Show resolved Hide resolved
src/seraphis_main/contextual_enote_record_types.h Outdated Show resolved Hide resolved
src/seraphis_main/scan_balance_recovery_utils.cpp Outdated Show resolved Hide resolved
src/seraphis_main/contextual_enote_record_types.h Outdated Show resolved Hide resolved
src/seraphis_main/contextual_enote_record_types.cpp Outdated Show resolved Hide resolved
src/seraphis_main/contextual_enote_record_types.h Outdated Show resolved Hide resolved
@SNeedlewoods
Copy link
Author

Minor update, now failing 16 tests
seraphis_enote_scanning.legacy_pre_transition_*
seraphis_enote_scanning.legacy_sp_transition_*
seraphis_integration.txtype_squashed_v1
seraphis_multisig.txtype_squashed_v1

@SNeedlewoods
Copy link
Author

still not done, but the last one was a big baby step for me

not worth a review at this point, I think, I have an idea what to do next

@SNeedlewoods SNeedlewoods changed the title added LegacyEnoteOriginContext & EnoteOriginContextVariant add LegacyEnoteOriginContext Dec 30, 2023
@SNeedlewoods
Copy link
Author

At this stage I'd like to get some feedback.
Should I add something?
e.g. there are no tests for LegacyEnoteV4 in seraphis_enote_scanning.cpp
Are there too many or too few checks for ledger_context.get_legacy_amount_counts in seraphis_enote_scanning.cpp?
Have I missed something?

Copy link

@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.

Small review

src/seraphis_mocks/mock_ledger_context.cpp Outdated Show resolved Hide resolved
src/seraphis_mocks/mock_ledger_context.cpp Outdated Show resolved Hide resolved
src/seraphis_mocks/mock_ledger_context.cpp Outdated Show resolved Hide resolved
src/seraphis_core/legacy_enote_types.cpp Outdated Show resolved Hide resolved
@rbrunner7
Copy link
Member

Something looks very wrong currently with your PR, with 165 commits. I would guess either you did not yet rebase to the new state of our seraphis_wallet branch, or you did rebase but something went wrong.

@SNeedlewoods SNeedlewoods force-pushed the x_LegacyEnoteOriginContext branch 4 times, most recently from 01b39ef to caffdae Compare March 8, 2024 15:37
@rbrunner7
Copy link
Member

I am sorry to not have noticed, and asked this question much earlier already, as only now I finally had a look at the files that this PR modifies: They are all Seraphis library files, right? Thus seems to me this should be a PR to the Seraphis library, not to here.

I believe following a rule "Seraphis library modifications always to @UkoeHB's Seraphis library repo and branch" will make rebasing / updating our Seraphis wallet branch easier, and it be clearer where to find what.

@rbrunner7
Copy link
Member

I had a look at the PR nevertheless, to check whether I could review, and found out that it's too specific for me, I lack both the necessary detailed knowledge about the protocols involved with all these types of transactions and enotes, and knowledge about the workings of the Seraphis library ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants