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

feat!: update key parsing #5636

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Aug 13, 2023

Description

Updates key parsing to differentiate between canonical- and reduction-based use cases. Makes several updates to account for EpochTime changes. Updates genesis data.

Motivation and Context

An update to tari-crypto differentiates secret key parsing between cases that should use canonical byte arrays, and those that use random byte arrays requiring modular wide reduction.

This PR makes corresponding changes to account for this.

It also makes several updates required since the tari_utilities release used here also makes breaking EpochTime changes that can't be cleanly done separately.

How Has This Been Tested?

Existing tests pass. Uses of parsed secret keys have been manually inspected for correctness.

What process can a PR reviewer use to test or verify this change?

Assert that secret keys and scalars instantiated from byte arrays use the appropriate approach: parsing from canonical byte arrays, or modular wide reduction from uniform byte arrays.

Confirm that EpochTime changes reflect the intended logic, especially relating to timestamp and difficulty handling.

BREAKING CHANGE: This modifies how secret keys and scalars are constructed, and is therefore a breaking change.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Aug 13, 2023
@AaronFeickert AaronFeickert changed the title fix!: update key parsing feat!: update key parsing Aug 13, 2023
@github-actions
Copy link

github-actions bot commented Aug 13, 2023

Test Results (CI)

1 243 tests   1 243 ✔️  9m 15s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit 01fc2ba.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Aug 13, 2023

Test Results (Integration tests)

  2 files    1 errors  2 suites   3m 30s ⏱️
14 tests 13 ✔️ 0 💤 1
15 runs  14 ✔️ 0 💤 1

For more details on these parsing errors and failures, see this check.

Results for commit 01fc2ba.

♻️ This comment has been updated with latest results.

@AaronFeickert AaronFeickert force-pushed the scalar-byte-array branch 4 times, most recently from 2a44798 to e2b798c Compare September 18, 2023 22:01
stringhandler
stringhandler previously approved these changes Sep 19, 2023
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

happy with this so far

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Sep 19, 2023
@AaronFeickert AaronFeickert force-pushed the scalar-byte-array branch 2 times, most recently from 1ef94cc to 064f56d Compare September 22, 2023 13:55
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Sep 25, 2023
@AaronFeickert AaronFeickert marked this pull request as ready for review September 25, 2023 21:38
@AaronFeickert
Copy link
Collaborator Author

Special attention should also be paid to the opcode and script stack changes. It was not clear what the intended handling was for providing verification message data to the relevant opcodes, nor how array data should be parsed to scalars for computation of Ristretto points in scripts.

@AaronFeickert
Copy link
Collaborator Author

This PR now encompasses changes required from a tari_utilities PR that removes certain arithmetic operations from EpochTime. Because these changes are included in the same release as the changes to ByteArray, it's not possible to do separate PRs that will pass testing.

Extra care should be taken to review the EpochTime changes in this PR, since they are used in timestamp and difficulty calculations.

Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

Looking good, just 1 nit about an error type

base_layer/core/src/chain_storage/blockchain_database.rs Outdated Show resolved Hide resolved
infrastructure/tari_script/src/script.rs Outdated Show resolved Hide resolved
@ghpbot-tari-project ghpbot-tari-project added the CR-too_long Changes Requested - Your PR is too long label Sep 29, 2023
@AaronFeickert AaronFeickert force-pushed the scalar-byte-array branch 2 times, most recently from 52d931d to 4b3cdcb Compare September 29, 2023 20:24
@AaronFeickert
Copy link
Collaborator Author

AaronFeickert commented Oct 2, 2023

Marking as draft to make it clear that merging is blocked on dependency updates. Specifically, tari-crypto needs a new release version.

@AaronFeickert AaronFeickert marked this pull request as draft October 2, 2023 21:17
@AaronFeickert AaronFeickert force-pushed the scalar-byte-array branch 2 times, most recently from f8cc7a8 to 729c19b Compare October 13, 2023 20:57
@AaronFeickert AaronFeickert force-pushed the scalar-byte-array branch 2 times, most recently from a816ff7 to 5dd8903 Compare October 27, 2023 15:33
@SWvheerden
Copy link
Collaborator

Replacing with PR: #5900 so that it includes the tari_crypto release tag 0.19.0 instead of the github main branch

@SWvheerden SWvheerden closed this Nov 2, 2023
@AaronFeickert AaronFeickert deleted the scalar-byte-array branch November 4, 2023 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR-too_long Changes Requested - Your PR is too long P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants