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

Empty Emoji String Bug Fix #1736

Merged
merged 1 commit into from
Apr 20, 2020
Merged

Empty Emoji String Bug Fix #1736

merged 1 commit into from
Apr 20, 2020

Conversation

StriderDM
Copy link
Contributor

@StriderDM StriderDM commented Apr 20, 2020

Description

Check if Emoji string is empty before trying to validate it.

Motivation and Context

Bug fix

How Has This Been Tested?

cargo test --all --all-features

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Feature refactor (No new feature or functional changes, but performance or technical debt improvements)
  • New Tests
  • Documentation

Checklist:

  • I'm merging against the development branch.
  • I ran cargo-fmt --all before pushing.
  • I have squashed my commits into a single commit.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Sorry, something went wrong.

@StriderDM StriderDM changed the title Bug Fix Empty Emoji String Bug Fix Apr 20, 2020
@StriderDM StriderDM force-pushed the emoji-empty_string-check branch from 081a08c to 22c542c Compare April 20, 2020 09:52
Copy link

@crypt0ginge crypt0ginge left a comment

Choose a reason for hiding this comment

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

Looks good to me.

philipr-za
philipr-za previously approved these changes Apr 20, 2020
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

Thanks - nice catch. Style nits though :)

base_layer/wallet/src/util/emoji.rs Outdated Show resolved Hide resolved
@CjS77
Copy link
Collaborator

CjS77 commented Apr 20, 2020

Had a closer look; I think the bug is actually in is_valid (which is a public function). So rather do

/// Checks whether the last digit in the array matches the checksum for the array minus the last digit.
pub fn is_valid(arr: &[usize], dict_len: usize) -> bool {
    if arr.len() < 2 { return false; }
    let cs = checksum(&arr[..arr.len() - 1], dict_len);
    cs == arr[arr.len() - 1]
}

str_to_pubkey can stay how it is

in case it's not clear, the current PR will still expose the same bug in is_valid, so this change must be made anyway, and there's no value in double-checking the array length.

Check if Emoji string is empty before trying to validate it.

cargo fmt

clippy lint

Style fix

Check for empty array in luhn.rs since function is public.
@StriderDM StriderDM force-pushed the emoji-empty_string-check branch from 8dfead5 to 24925e3 Compare April 20, 2020 10:43
Copy link
Collaborator

@CjS77 CjS77 left a comment

Choose a reason for hiding this comment

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

👍

@CjS77 CjS77 merged commit 46dd74c into development Apr 20, 2020
@CjS77 CjS77 deleted the emoji-empty_string-check branch April 22, 2020 09:59
CjS77 added a commit that referenced this pull request Apr 25, 2020
** Major Changes from 0.0.9

**** Store and forward

Peers will hold onto message for recipients that are not online and deliver the messages to them when they appear again.

**** OsX package installer

**** Many documentation improvements

**** Ephemeral keys for private messages

This is a big change that preserves privacy on the network but dramatically reduces the amount of traffic peers have to deal with.

**** Emoji Ids

*** Other changes

- The target difficulty for a specified PoW algorithm is included in the block header. This allows the target difficulty
  of any block height to be calculated by only processing the last set of target difficulty samples up to that height.
- Don't mark peers as offline if there are no existing connections (#1763)
- Add UTXO selection strategy for large txs
- Base node: Dynamically determine build version (#1760)
- Include random peers for liveness ping (#1753)
- RandomX - Version Update (#1754)
- Add generic debug log function to FFI (#1752)
- Lots of logging improvements
- Added list-transactions and cancel-transaction commands (#1746)
- ASCII table output for list-peers and list-connections (#1709)
- Improve Difficulty adjustment manager
- Modular configuration via ConfigLoader and ConfigPath traits
- Fix chain monitoring bug in Transaction Service (#1739)
- Empty Emoji String Bug Fix  (#1736)
- Coin-split base node cli command
- Complete the basic OSX pkg build
- Perform reorgs only on stronger tip accumulated difficulties
- Use filesystem storage for dht.db on libwallet (#1735)
- Fix duplicate message propagation (#1730)
- Introduced accumulated difficulty validators to allow different rules for testing and running running a base node. -
- Changes to peer offline handling (#1716)
- Update Transaction cancellation to work for Inbound and Outbound Txs
- Added oneshot reply to outbound messaging (#1703)
- Add transaction stress test command to CLI
- Implemented basic `make-it-rain` command
- Fix MmrCache rewind issue
- Use ephemeral key for private messages (e.g Discovery) (#1686)
- Limit orphan pool size
- Added a function to list UTXOs in the console (#1678)
- Prevent adding yourself as a peer (#1665)
- Update transaction weights (#1661)
- Fix block period calculation
- Validators will now check the weight of a block when doing validation (#1648)
- Cleaned up duplicate code from the Blockchain db
- The ban peer log will now supply n reason why the peer was banned (#1638)
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.

None yet

4 participants