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

some fixes #1721

Merged
merged 4 commits into from
Jun 5, 2023
Merged

some fixes #1721

merged 4 commits into from
Jun 5, 2023

Conversation

A-23187
Copy link
Contributor

@A-23187 A-23187 commented Jan 5, 2023

  • add cli option --enable-mutation-pruner to control if to enable MutationPrunerBuilder
  • let cli option --enable-coverage-strategy to control if to load CoveragePluginBuilder
  • let cli option --enable-iprof to control if to load InstructionProfilerBuilder
  • remove the redundant print in MythrilDisassembler.hash_for_function_signature
  • fix the bug of non-hexadecimal numbers found when converting hex(func_hash)[2:] to bytes
  • other fixes

- add cli option --enable-mutation-pruner to control if to enable MutationPrunerBuilder
- let cli option --enable-coverage-strategy to control if to load CoveragePluginBuilder
- let cli option --enable-iprof to control if to load InstructionProfilerBuilder
- remove the redundant print in MythrilDisassembler.hash_for_function_signature
- fix the bug of non-hexadecimal numbers found when converting hex(func_hash)[2:] to bytes
- other fixes
@CLAassistant
Copy link

CLAassistant commented Jan 5, 2023

CLA assistant check
All committers have signed the CLA.

@norhh
Copy link
Collaborator

norhh commented Jan 7, 2023

It's better if these functions are enabled by default, For example, using Mutation Pruning gives the best performance. Users can directly use -v4 to directly get InstructionProfiling results and coverage results without explicitly using the --enable flags. Adding an option to disable them is more useful.

@@ -233,7 +233,6 @@ def hash_for_function_signature(func: str) -> str:
:param func: function name
:return: Its hash signature
"""
print(sha3(func))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice Catch!

@@ -259,7 +259,7 @@ def _execute_transactions(self, address):
)
if func_hashes:
func_hashes = [
bytes.fromhex(hex(func_hash)[2:]) for func_hash in func_hashes
bytes.fromhex(func_hash[2:]) for func_hash in func_hashes
Copy link
Collaborator

Choose a reason for hiding this comment

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

literal_eval in line 619 in cli.py changes the hex to integer. Hence we will have to re-hex it. Hence this change will not work with the current cli.py file.

Copy link
Contributor Author

@A-23187 A-23187 Jan 8, 2023

Choose a reason for hiding this comment

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

literal_eval in line 619 in cli.py changes the hex to integer. Hence we will have to re-hex it. Hence this change will not work with the current cli.py file.

The problem is that re-hex an integer to a string will miss the leading zeros which causes the error of non-hexadecimal number found in bytes.fromhex().
For example:

original_hash = '0x0b95d228'
hash_integer = int(original_hash, 16)
rehexed_hash = hex(hash_integer) # 0xb95d228, missing the leading zeros compared to original_hash
bytes.fromhex(rehexed_hash[2:]) # ValueError: non-hexadecimal number found in fromhex() arg at position 7

Also, I noticed that the type hint of args.transaction_sequences in line 19 of support_args.py is List[List[str]]. And I tried to run the myth cli with something like --transaction-sequences '[["0x0b95d228"]]' (quoting the function hash), which causes the error of 'str' object cannot be interpreted as an integer due to the re-hex operation. So I removed the re-hex here.
I think we should perform different operations here to convert function hash to bytes depending on the type of function hash.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You do not need to use '' when using --transaction-sequence. Something like --transaction-sequences [["0x89b8ae9b"],["0xaaaaaaaa"],[]] works

Copy link
Contributor Author

@A-23187 A-23187 Jan 9, 2023

Choose a reason for hiding this comment

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

Yep, this works, because the shell (bash I use) expand [["0x89b8ae9b"],["0xaaaaaaaa"],[]] to [[0x89b8ae9b],[0xaaaaaaaa],[]] before passing to myth. I add a statement print(args.transaction_sequences) before do literal_eval to check the value of args.transaction_sequences, and it prints [[0x89b8ae9b],[0xaaaaaaaa],[]].

Something like --transaction-sequences '[["0x0b95d228"]]' (put the hash in " and put the whole tx sequences in ') can avoid the expansion of the shell. It seems that no one would specify --transaction-sequences so weirdly like this. But when calling mythril in a script, I hope to specify the args.transaction_sequences directly using the return value of MythrilDisassembler.hash_for_function_signature without converting to integer:

args.transaction_sequences = [[MythrilDisassembler.hash_for_function_signature('fun_name()')]] # without converting to int

# args.transaction_sequences = [[int(MythrilDisassembler.hash_for_function_signature('fun_name()'), 16)]]

So I still think it is needed to consider different types when converting function hashes to bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this issue has already been sloved in the newest commit of the develop branch, so I remove my fix code

@A-23187 A-23187 requested a review from norhh May 31, 2023 15:09
@A-23187
Copy link
Contributor Author

A-23187 commented May 31, 2023

It's better if these functions are enabled by default, For example, using Mutation Pruning gives the best performance. Users can directly use -v4 to directly get InstructionProfiling results and coverage results without explicitly using the --enable flags. Adding an option to disable them is more useful.

Yay, I now change to disable options.

- add cli option --disable-mutation-pruner to disable MutationPrunerBuilder
- let cli option --disable-coverage-strategy to disable CoveragePluginBuilder
- let cli option --disable-iprof to disable InstructionProfilerBuilder
- remove the redundant print in MythrilDisassembler.hash_for_function_signature
- other fixes
Copy link
Collaborator

@norhh norhh left a comment

Choose a reason for hiding this comment

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

All Good! Thanks for the PR

@norhh norhh merged commit 499bce1 into Consensys:develop Jun 5, 2023
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.

3 participants