fix!: don't use the ledger unless both keys are for ledger #6492
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test Results (CI) 3 files 126 suites 36m 20s ⏱️ For more details on these failures, see this check. Results for commit ed131a9. ♻️ This comment has been updated with latest results. |
ghpbot-tari-project
added
P-acks_required
Test Results (Integration tests)37 tests 37 ✅ 16m 9s ⏱️ Results for commit 2889e23. |
SWvheerden
previously approved these changes
Aug 21, 2024
SWvheerden
changed the title
ghpbot-tari-project
removed
the
P-reviews_required
brianp
force-pushed
the
bp-fix-one-sided-send
branch
from
ac6d44e to
cbe0617
Compare
SWvheerden
previously approved these changes
Aug 21, 2024
ghpbot-tari-project
added
the
P-conflicts
…t before combining it with the common parts
brianp
force-pushed
the
bp-fix-one-sided-send
branch
from
bfef129 to
ed131a9
Compare
ghpbot-tari-project
added
P-reviews_required
| @@ -46,6 +44,7 @@ mod test { | |||
| const NOP_IDENTIFIER: &str = "0173"; | |||
| const PUSH_ONE_IDENTIFIER: &str = "017c"; | |||
| const CHECK_SIG_VERIFY_IDENTIFIER: &str = "21ad"; | |||
| const PUSH_PUBKEY_IDENTIFIER: &str = "217e"; | |||
There was a problem hiding this comment.
@brianp, we should not re-declare this here, rather include minotari_ledger_wallet_common::PUSH_PUBKEY_IDENTIFIER
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Correct the key requests coming from the key manager in the
Motivation and Context
sign_with_nonce_and_challengecode path to properly request ledger keys only when both keys are to be derived from the ledger, otherwise use the software key managehash(hash(script) + hash(common))but this can be simplified tohash(script + hash(common). There's no need to hash the script by itself, before hashing it alongside the common hash.How Has This Been Tested?
One sided sends are borked from ledger. TX creation is requesting the wrong keys from ledger.
Metadata signature generation is wrong.
What process can a PR reviewer use to test or verify this change?
Breaking Changes
The change in hashing function causes the gen block, and faucets to no longer match. This hashing change is a breaking change for the purpose of simplification.