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

web3.eth.account signature hypothesis tests #107

Open
carver opened this issue Nov 30, 2017 · 5 comments
Open

web3.eth.account signature hypothesis tests #107

carver opened this issue Nov 30, 2017 · 5 comments
Labels
p3 normal

Comments

@carver
Copy link
Contributor

carver commented Nov 30, 2017

What was wrong?

Want to catch/prevent more issues like ethereum/web3.py#466 earlier.

How can it be fixed?

More hypothesis tests. Piper's suggestions (on ethereum/web3.py#477):

  • A signed transaction's signature recovers to the expected sender address
  • The raw transaction can be rlp decoded into a transaction object
  • An rlp decoded transaction object's v,r,s match the v,r,s from the signature.
  • The signature bytes are 65 bytes in length

The downside is that they can be pretty time-intensive. Just a test for the length of signature bytes added 7 seconds to a 53-second test suite.

Example:

--- a/tests/core/eth-module/test_accounts.py
+++ b/tests/core/eth-module/test_accounts.py
@@ -2,10 +2,19 @@
 
 import pytest
 
+from eth_keys.constants import (
+    SECPK1_N,
+)
 from eth_utils import (
     is_checksum_address,
 )
 
+from hypothesis import (
+    example,
+    given,
+    strategies as st,
+)
+
 from web3 import (
     Account,
     Web3,
@@ -268,6 +277,16 @@ def test_eth_account_sign(acct, message, key, expected_bytes, expected_hash, v,
     assert account.sign(message_text=message) == signed
 
 
+@given(st.binary(), st.integers(
+    min_value=Web3.toInt(hexstr='0x1' + '00' * 31),
+    max_value=SECPK1_N - 1
+    )
+)
+def test_message_signature_length(acct, message, key):
+    signed = acct.sign(message, key)
+    assert len(signed.signature) == 65
+
+
 @pytest.mark.parametrize(
     'txn, private_key, expected_raw_tx, tx_hash, r, s, v',
     (
@pipermerriam
Copy link
Member

We can use the same strategy that we used to speed up the test suite in the Py-EVM codebase for slow tests like this. We can setup a daily cron run on travis that runs and only run the slow tests during the cron run.

See implementation here: https://github.com/ethereum/py-evm/blob/master/tests/json-fixtures/test_state.py#L116-L119

@wolovim
Copy link
Member

wolovim commented Sep 18, 2020

[old issue cleanup]

I believe this hypothesis testing belongs (and already exists) in eth-account. Correct me if I'm wrong.

@wolovim wolovim closed this as completed Sep 18, 2020
@carver
Copy link
Contributor Author

carver commented Sep 18, 2020

Hm, I don't think this particular test exists yet in eth-account. Maybe copy/move the issue over and reopen?

@wolovim
Copy link
Member

wolovim commented Sep 18, 2020

Re: transfer, looks like I've got insufficient privileges in eth-account @carver. Would you mind moving it over? (The -> Transfer issue button lives in the sidebar.) Thanks!

@kclowes kclowes transferred this issue from ethereum/web3.py Sep 21, 2020
@kclowes
Copy link
Collaborator

kclowes commented Sep 21, 2020

@marcgarreau Transferred! TIL ✨

@wolovim wolovim reopened this Sep 21, 2020
@pacrob pacrob added the p3 normal label Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3 normal
Projects
None yet
Development

No branches or pull requests

5 participants