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

PublicKey constructor will quietly accept invalid inputs #403

Merged
merged 4 commits into from
Jul 1, 2022

Conversation

liquizard
Copy link
Contributor

Status Type ⚠️ Core Change
Ready Bugfix Yes

Problem

Encountered the error invalid transaction: Transaction failed to sanitize accounts offsets correctly when submiting a Serum SettleFunds command. Managed to pinpoint the root cause of the problem being a rouge space on the end of a public key string constant. Removing this space resolved the issue - this must cause some internal side-effect near the TransactionBuilder.

Further investigation found that PublicKey constructor would silently accept all manner of shenanigans.
Also found that PublicKey.IsValid implementation relies on Base58Encoder validation logic that would accept whitespace.

Solution

Added a FastCheck internal method to PublicKey and added calls to it from constructor and IsValid.
Added and modified a few tests.

Deploy Notes

This is potentially a breaking change to any client apps that accept public keys that include whitespace before or after and pass them directly to the constructor. On balance, I believe this enhancement will prevent more problems than it will cause.

Copy link
Member

@tiago18c tiago18c left a comment

Choose a reason for hiding this comment

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

Discuss comments. Otherwise looks good!

PS: I traced the actual problem to the method 'FindAccountIndex' in the 'MessageBuilder` class. I'll issue a fix later to this one! This fix should also address performance!
(root problem is that decoder was correctly trimming whitespace, but the message builder is inefficiently converting between binary and b58 multiples times and comparing the original pk string with a converted one that loses the trailing space. )

src/Solnet.Wallet/PublicKey.cs Show resolved Hide resolved
src/Solnet.Wallet/Utilities/Base58Encoder.cs Show resolved Hide resolved
@tiago18c
Copy link
Member

Thanks for the change. Don't merge yet, let me merge the other change and we can do 2PRs in 1 version

@tiago18c tiago18c merged commit e61b309 into bmresearch:master Jul 1, 2022
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.

2 participants