Skip to content

Create sign/ecdsa package - Second step of crypto refactoring#29

Closed
devin-ai-integration[bot] wants to merge 13 commits into
feature/packages-refactorfrom
devin/1751904772-create-sign-ecdsa-package
Closed

Create sign/ecdsa package - Second step of crypto refactoring#29
devin-ai-integration[bot] wants to merge 13 commits into
feature/packages-refactorfrom
devin/1751904772-create-sign-ecdsa-package

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

Update sign/ecdsa package to use common.Overwrite function

Summary

This PR addresses a GitHub comment from @tarakby requesting consolidation of the overwrite function usage across the codebase. The changes update the sign/ecdsa package to use the centralized common.Overwrite function instead of maintaining a local overwrite function.

Changes made:

  • Added import for github.com/onflow/crypto/common in sign/ecdsa/ecdsa.go
  • Removed local overwrite function definition (4 lines)
  • Updated single usage from defer overwrite(okm) to defer common.Overwrite(okm) in ECDSA key generation

This follows the same pattern already established in bls.go and consolidates memory clearing functionality into a single, more secure implementation.

Review & Testing Checklist for Human

  • Verify ECDSA key generation and signing still works correctly - Test both P-256 and secp256k1 curves to ensure no regression in cryptographic functionality
  • Confirm memory security properties are maintained - The new common.Overwrite uses crypto/rand.Read() before zero-fill, which is more secure than the old simple zero-fill loop
  • Test end-to-end ECDSA workflows - Generate keys, sign messages, verify signatures to ensure no performance or correctness regressions

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    subgraph Legend
        L1["Major Edit"]:::major-edit
        L2["Minor Edit"]:::minor-edit  
        L3["Context/No Edit"]:::context
    end
    
    subgraph "Crypto Package Structure"
        A["sign/ecdsa/ecdsa.go"]:::major-edit
        B["common/common.go"]:::context
        C["bls.go"]:::context
    end
    
    A -->|"imports & uses"| B
    C -->|"already uses"| B
    
    B -->|"provides Overwrite()"| D["Secure Memory Clearing"]
    A -->|"key generation"| E["ECDSA Private Keys"]
    
    classDef major-edit fill:#90EE90
    classDef minor-edit fill:#87CEEB  
    classDef context fill:#FFFFFF
Loading

Notes

  • This change is part of the larger crypto package refactoring effort to create modular, well-structured packages
  • The common.Overwrite function is actually more secure than the old local version as it uses cryptographically secure randomization before zero-fill
  • All CI checks passed (Unit Tests, Go code checks, C code checks)
  • Link to Devin run: https://app.devin.ai/sessions/5cc7e1e8f7f0401999ebefe0c7610ca5
  • Requested by: @tarakby

devin-ai-integration Bot and others added 2 commits July 7, 2025 16:02
- Move ecdsa.go to sign/ecdsa/ecdsa.go with ECDSA implementation
- Move ecdsa_test.go to sign/ecdsa/ecdsa_test.go with ECDSA tests
- Create sign/internal/testutils.go with shared test utilities
- Add init() function in sign/ecdsa to register ECDSA signers
- Create init.go to handle BLS registration and ECDSA import
- Update sign_test_utils.go to use internal testutils
- Remove original ecdsa.go and ecdsa_test.go files
- Maintain backward compatibility for public API

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
- Move ECDSA implementation from root ecdsa.go to sign/ecdsa/ecdsa.go
- Move ECDSA tests from root ecdsa_test.go to sign/ecdsa/ecdsa_test.go
- Create shared test utilities in sign/testutils/testutils.go
- Update sign package registration to work with ECDSA signers
- Add proper imports and package declarations
- Preserve ECDSA constants and algorithm registration

Note: Some test failures exist in encoding/decoding process that need debugging

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

- Remove ineffectual assignment to halg variable in TestECDSAHasher
- Resolves golangci-lint error: ineffectual assignment to halg (ineffassign)
- All linting checks now pass with 0 issues

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
Comment thread init.go

import (
"github.com/onflow/crypto/sign"
_ "github.com/onflow/crypto/sign/ecdsa"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

you can delete this line

devin-ai-integration Bot and others added 4 commits July 8, 2025 07:29
…calls

- Remove unused wrapper functions from sign_test_utils.go (getPRG, testGenSignVerify, etc.)
- Update test files to call testutils functions directly instead of removed wrappers
- Clean up unused imports in sign_test_utils.go

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
- Remove '_ "github.com/onflow/crypto/sign/ecdsa"' import from init.go
- This import was not needed since ECDSA registration happens in the sign/ecdsa package init()

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
- Update testKeysAlgorithm, testKeySize, benchSign, benchVerify to use testutils prefix in bls_test.go
- Update getPRG calls to use testutils.GetPRG in bls12381_utils_test.go
- Add missing testutils import to bls_thresholdsign_test.go
- All changes verified locally with go build and golangci-lint

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
…verwrite calls

- Add missing ECDSA import to init.go to trigger ECDSA registration
- Remove unused isG2Compressed function from no_cgo.go
- Remove unused overwrite function from common.go
- Replace overwrite() calls in bls.go with inline anonymous functions
- Remove unused crypto/rand import from common.go

Fixes 'signature scheme ECDSA_P256 is not supported' errors in SPOCK tests

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
Comment thread bls.go
devin-ai-integration Bot and others added 6 commits July 8, 2025 17:35
- Create new crypto/common package with Overwrite function for secure memory cleanup
- Update bls.go to use common.Overwrite instead of inline anonymous functions
- Add import for crypto/common package in bls.go
- Maintains same security functionality while improving code organization

Addresses GitHub comment from @tarakby requesting common package for overwrite function

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
- Add import for github.com/onflow/crypto/common
- Remove local overwrite function definition
- Replace overwrite(okm) call with common.Overwrite(okm)
- Addresses GitHub comment from @tarakby requesting consolidation of overwrite function usage

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
- Clean up fmt.Printf debug statements from rawDecodePrivateKey method
- Clean up debug statements from Equals method
- Code is now ready for review without debugging output

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
…d comprehensive validation

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
… import cycle

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
- Add IsNilHasherError and IsInvalidInputsError functions to test file to avoid import cycle
- Remove duplicate functions from ecdsa.go to prevent redeclaration errors
- Fix nil hasher check in isValidHasher function
- Update error checking logic to match actual error message format

Co-Authored-By: Tarak Ben Youssef <benyoussef.tarak@gmail.com>
@tarakby tarakby closed this Jul 11, 2025
@tarakby
tarakby deleted the devin/1751904772-create-sign-ecdsa-package branch July 17, 2025 09:55
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.

1 participant