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

Fix ScheduleSignHandler signature trim and verification building #9920

Merged

Conversation

jsync-swirlds
Copy link
Member

  • Fixed some general signature verification and record building issues previously identified
  • Changed the trim to walk the key structure for required keys, rather than attempting a direct match (which will often fail incorrectly) in trim
  • Added check in SignHandler to throw exception if a ScheduleSign transaction does not result in a change to signatories, or execution.
  • Enabled 17 HapiTest specs in ScheduleSignSpecs.

Fixes #9869

@jsync-swirlds jsync-swirlds self-assigned this Nov 15, 2023
Copy link

github-actions bot commented Nov 15, 2023

Node: Unit Test Results

    2 281 files  ±0      2 281 suites  ±0   1h 25m 48s ⏱️ - 3m 55s
118 349 tests ±0  118 315 ✔️ ±0  34 💤 ±0  0 ±0 
126 667 runs  ±0  126 633 ✔️ ±0  34 💤 ±0  0 ±0 

Results for commit 7c3f5f7. ± Comparison against base commit a394b31.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 15, 2023

Node: E2E Test Results

    1 files      1 suites   21m 9s ⏱️
312 tests 312 ✔️ 0 💤 0
334 runs  334 ✔️ 0 💤 0

Results for commit 7c3f5f7.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 15, 2023

Node: Integration Test Results

280 tests  ±0   280 ✔️ ±0   28m 25s ⏱️ -14s
    5 suites ±0       0 💤 ±0 
    5 files   ±0       0 ±0 

Results for commit 7c3f5f7. ± Comparison against base commit a394b31.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 16, 2023

Node: HAPI Test Results

1 247 tests  +1   903 ✔️ +12   1h 33m 10s ⏱️ + 7m 19s
   166 suites ±0   344 💤  - 10 
   166 files   ±0       0  -   1 

Results for commit 7c3f5f7. ± Comparison against base commit a394b31.

♻️ This comment has been updated with latest results.

@jsync-swirlds jsync-swirlds force-pushed the 09869-ScheduleSignHandler-signature-verification branch 2 times, most recently from 78175c9 to 086df14 Compare November 16, 2023 05:22
@jsync-swirlds jsync-swirlds marked this pull request as ready for review November 16, 2023 15:19
@jsync-swirlds jsync-swirlds requested review from a team as code owners November 16, 2023 15:19
@jsync-swirlds jsync-swirlds force-pushed the 09869-ScheduleSignHandler-signature-verification branch 2 times, most recently from a921742 to f099c77 Compare November 16, 2023 16:29
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

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

LGTM!

@jsync-swirlds jsync-swirlds force-pushed the 09869-ScheduleSignHandler-signature-verification branch from f099c77 to 28224b3 Compare November 16, 2023 17:17
* Fixed some general signature verification and record building issues previously identified
* Changed the trim to walk the key structure for required keys, rather than attempting a direct match (which will often fail incorrectly) in trim
* Added check in SignHandler to throw exception if a ScheduleSign transaction does not result in a change to signatories, or execution.
* Enabled 17 HapiTest specs in ScheduleSignSpecs.

Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
Copy link
Contributor

@iwsimon iwsimon left a comment

Choose a reason for hiding this comment

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

LGTM

@jsync-swirlds jsync-swirlds merged commit cd27dd7 into develop Nov 16, 2023
14 of 15 checks passed
@jsync-swirlds jsync-swirlds deleted the 09869-ScheduleSignHandler-signature-verification branch November 16, 2023 21:33
nickpoorman pushed a commit that referenced this pull request Nov 22, 2023
Signed-off-by: Joseph Sinclair <joseph.sinclair@swirldslabs.com>
Signed-off-by: Nick Poorman <nick@swirldslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScheduleSignHandler doesn't verify signature properly
3 participants