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!: change signature construction to allow better HW support #5282

Merged

Conversation

SWvheerden
Copy link
Collaborator

@SWvheerden SWvheerden commented Mar 30, 2023

Description

Changes the challenges of all signatures to not be Hash(Key||Nonce||Feat_1||Feat_2||..||Feat_x) but rather Hash(Key||Nonce||Hash(Feat_1||Feat_2||..||Feat_x))

Adds the version to the kernel signature as this is currently not checked.

Motivation and Context

This is done because hardware devices need to sign all the data they need to be sent over to the hardware device to be signed. If we have to send all the features over this becomes quite significant in bytes. This is now changed to only send the message field as a single 32-byte array and this still comments to all the features.

The kernel version is technically malleable as the version is not committed to in the signature, this is now fixed.
How Has This Been Tested?

All tests pass

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

Make sure all chain signatures are changed.
Adds version to Kernel signature

Breaking Changes

Change all chain signature challenges: Kernel, Script, Metadata

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify
    Complete chain reset.

BREAKING CHANGE: Complete chain reset

@SWvheerden SWvheerden marked this pull request as ready for review March 30, 2023 13:42
@stringhandler stringhandler changed the title feat: change signature construction to allow better HW support feat!: change signature construction to allow better HW support Mar 30, 2023
@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 Mar 30, 2023
@SWvheerden SWvheerden added A-base_node Area - The Tari base node executable and libraries W-consensus_breaking Warn - A change requiring a hard fork to be activated P-high-risk Process - High risk S-high-severity Severity - High W-transaction-breaking Warning - Changes data that wallets use to send transactions A-wallet Area - related to the wallet labels Mar 30, 2023
hansieodendaal
hansieodendaal previously approved these changes Mar 31, 2023
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

utACK

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.

I'm surprised to see no tests being updated, which probably means we are not checking for maleablity in tests

@SWvheerden
Copy link
Collaborator Author

The reason we do not see tests updated is that the signatures are generated using the helper functions
So updating them, updates the tests as well.
We test comprehensibly for malleability here: https://github.com/tari-project/tari/blob/development/base_layer/core/tests/chain_storage_tests/chain_storage.rs#L1981

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.

utACK

@stringhandler stringhandler merged commit 82d2dcb into tari-project:development Mar 31, 2023
@SWvheerden SWvheerden mentioned this pull request Mar 31, 2023
4 tasks
CjS77 added a commit that referenced this pull request Apr 4, 2023
Description
---
Resets the dates for the following networks:
Esmeralda: Today
Nextnet: April 14th
Stagenet: Jun 14

Motivation and Context
---
For Nextnet and Stagenet these are the dates when these changes come
into effect.
Esmeralda had a new Gen block as of PR #5282 

How Has This Been Tested?
---
Unit tests

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


Breaking Changes
---

- [x] None
- [x] Requires data directory on base node to be deleted
- [x] Requires hard fork
- [x] Other - Please specify
New Gen block, requires network reset

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->

Co-authored-by: Cayle Sharrock <CjS77@users.noreply.github.com>
@SWvheerden SWvheerden deleted the sw_change_signatures branch April 5, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-base_node Area - The Tari base node executable and libraries A-wallet Area - related to the wallet P-acks_required Process - Requires more ACKs or utACKs P-high-risk Process - High risk P-reviews_required Process - Requires a review from a lead maintainer to be merged S-high-severity Severity - High W-consensus_breaking Warn - A change requiring a hard fork to be activated W-transaction-breaking Warning - Changes data that wallets use to send transactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants