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

Combine near-duplicate Utxo creation functions #2467

Merged
merged 2 commits into from
Jul 9, 2021
Merged

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Jul 8, 2021

Motivation

We need to change how Utxos are created as part of #2231.

The deduplicated function will be used in a future PR to check for double-spends.

Priority and Risk

It is easier and less risky to make the changes required for double-spend validation in a single function.

The alternative is making the same changes at 3 separate places in the code, and trying to keep them all in sync.

Solution

  • Combine 3 similar functions into a common new_outputs function

As a side-effect, this change avoids re-calculating some transaction hashes.

This is part of ticket #2231, but it does not close that ticket.

Review

Anyone can review this PR. It's not urgent.

Reviewer Checklist

  • Code works as documented
  • Existing tests pass

This function will also be tested as part of the double-spend tests in a future PR.

Follow Up Work

Actually check for double-spends

@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Medium labels Jul 8, 2021
@teor2345 teor2345 requested a review from jvff July 8, 2021 05:54
@teor2345 teor2345 self-assigned this Jul 8, 2021
@teor2345 teor2345 force-pushed the dedup-utxo-creation branch from c7e1a0f to 1cb101a Compare July 8, 2021 05:57
@teor2345
Copy link
Contributor Author

teor2345 commented Jul 8, 2021

We need to merge PR #2463 before we merge this PR, so we have non-finalized state and transaction verifier test coverage.

Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

Looks good. I did add one suggestion that's optional and/or low-priority.

zebra-state/src/utxo.rs Show resolved Hide resolved
@teor2345 teor2345 merged commit d4cc867 into main Jul 9, 2021
@teor2345 teor2345 deleted the dedup-utxo-creation branch July 9, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants