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

crypto: fix base58 encoding for BlsSignature #56

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Conversation

emturner
Copy link
Collaborator

@emturner emturner commented Dec 15, 2023

The current base58 module incorrectly had a 128 char limit set on decoding hashes - too small for bls sigs. We raise this limit to the length of BlsSigs hashes, which are the longest hashes we support.

fixes #49

@emturner emturner force-pushed the emturner@better-b58 branch 5 times, most recently from 6581ee5 to f625864 Compare December 15, 2023 16:41
@lierdakil
Copy link

lierdakil commented Dec 18, 2023

The current base58 dep has a 128 byte limit

This is true, but 128 bytes is not the same as 128 base58 characters.

which is not long enough for bls sigs

This is false, base58 will happily accept about 175-176 base58 characters, which corresponds to around 128 bytes. Bls signatures are mere 96 bytes, which corresponds to 142 characters (with prefix) in base58.

So this was never a problem with dependencies. I thought I explained it in the issue.

@emturner
Copy link
Collaborator Author

The current base58 dep has a 128 byte limit

This is true, but 128 bytes is not the same as 128 base58 characters.

which is not long enough for bls sigs

This is false, base58 will happily accept about 175-176 base58 characters, which corresponds to around 128 bytes. Bls signatures are mere 96 bytes, which corresponds to 142 characters (with prefix) in base58.

So this was never a problem with dependencies. I thought I explained it in the issue.

Ah thanks, apologies for the misunderstanding. Have updated

@lierdakil
Copy link

Hmm. I did some testing, and apparently I was under a few misconceptions. Apologies.

So while this works fine for bona fide Tezos hashes, there's an unfortunate landmine in the base58 library related to handling zero data: it will panic if there are more than 128 leading characters 1 in the input (which translate to zero bytes). So that explains why the check in from_base58check was so strict.

Hence, it turns out, the library switch was a better overall fix.

Can you revert back to f625864? Again, sorry for this.

Copy link

@lierdakil lierdakil left a comment

Choose a reason for hiding this comment

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

LGTM with a couple suggestions.

crypto/src/base58.rs Outdated Show resolved Hide resolved
crypto/src/base58.rs Outdated Show resolved Hide resolved
The current base58 dep has a 128 byte limit, which is not long enough for bls sigs
@emturner emturner merged commit 20b9b00 into master Dec 18, 2023
7 checks passed
@emturner emturner deleted the emturner@better-b58 branch December 18, 2023 12:58
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.

BlsSignature::from_base58_check on valid inputs returns error "data too long"
2 participants