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

[qa] smartfees: Properly use ordered dict #7980

Merged
merged 2 commits into from May 3, 2016

Conversation

Projects
None yet
3 participants
@MarcoFalke
Member

MarcoFalke commented Apr 30, 2016

Dictionaries are not guaranteed to be sorted; Use an OrderedDict to avoid random failures of the smartfees test.

@MarcoFalke MarcoFalke added the Tests label Apr 30, 2016

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 1, 2016

Contributor

@MarcoFalke Marko, do you have a sample of how it fails without this change? I did 100 runs of master to compare and ACK this and it was always OK...

Contributor

paveljanik commented May 1, 2016

@MarcoFalke Marko, do you have a sample of how it fails without this change? I did 100 runs of master to compare and ACK this and it was always OK...

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 2, 2016

Member

@paveljanik This depends on your environment. I will post exact steps to reproduce tonight, when I am home.

Member

MarcoFalke commented May 2, 2016

@paveljanik This depends on your environment. I will post exact steps to reproduce tonight, when I am home.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 2, 2016

Member

+1 for using OrderedDict. Normal dicts's order is randomized (they use a random seed for security reasons) and it's clear that the order of those outputs here matters.

Aside: as of Python 3.5 OrderedDict is implemented in C and hence almost as fast as normal dict. Not that it matters in this particular instance.

Member

laanwj commented May 2, 2016

+1 for using OrderedDict. Normal dicts's order is randomized (they use a random seed for security reasons) and it's clear that the order of those outputs here matters.

Aside: as of Python 3.5 OrderedDict is implemented in C and hence almost as fast as normal dict. Not that it matters in this particular instance.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 2, 2016

Member

To reproduce I'd suggest using an OrderedDict with the items reversed.

(can't do it myself right now, running the extended test suite using python 3)

This PR is also part of #7814.

Member

laanwj commented May 2, 2016

To reproduce I'd suggest using an OrderedDict with the items reversed.

(can't do it myself right now, running the extended test suite using python 3)

This PR is also part of #7814.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 2, 2016

Member

@paveljanik To test this you can use python -h to see how you enable random hash seeds, if it is not yet enabled.

E.g. python -R qa/rpc-tests/smartfees.py --srcdir=src fails on master, passes with this patch.

Member

MarcoFalke commented May 2, 2016

@paveljanik To test this you can use python -h to see how you enable random hash seeds, if it is not yet enabled.

E.g. python -R qa/rpc-tests/smartfees.py --srcdir=src fails on master, passes with this patch.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 2, 2016

Contributor

Still not able to reproduce here on master, OS X.

On the other hand, this command now failed for me on OS X, with this PR:

$ python -R qa/rpc-tests/smartfees.py  --srcdir=src
Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/test2CcA11
This test is time consuming, please be patient
Splitting inputs to small size so we can generate low priority tx's
Finished splitting
Will output estimates for 1/2/3/6/15/25 blocks
Creating transactions and mining them with a block size that can't keep up
JSONRPC error: 16: bad-txns-in-belowout
  File "/private/tmp/bitcoin-7980/qa/rpc-tests/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "qa/rpc-tests/smartfees.py", line 244, in run_test
    self.transact_and_mine(10, self.nodes[2])
  File "qa/rpc-tests/smartfees.py", line 220, in transact_and_mine
    self.memutxo, Decimal("0.005"), min_fee, min_fee)
  File "qa/rpc-tests/smartfees.py", line 67, in small_txpuzzle_randfee
    txid = from_node.sendrawtransaction(completetx, True)
  File "/private/tmp/bitcoin-7980/qa/rpc-tests/test_framework/coverage.py", line 50, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/private/tmp/bitcoin-7980/qa/rpc-tests/test_framework/authproxy.py", line 139, in __call__
    raise JSONRPCException(response['error'])
Stopping nodes
Cleaning up
Failed
$ 

Edit: Python is Python 2.7 on this test machine.

Contributor

paveljanik commented May 2, 2016

Still not able to reproduce here on master, OS X.

On the other hand, this command now failed for me on OS X, with this PR:

$ python -R qa/rpc-tests/smartfees.py  --srcdir=src
Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/test2CcA11
This test is time consuming, please be patient
Splitting inputs to small size so we can generate low priority tx's
Finished splitting
Will output estimates for 1/2/3/6/15/25 blocks
Creating transactions and mining them with a block size that can't keep up
JSONRPC error: 16: bad-txns-in-belowout
  File "/private/tmp/bitcoin-7980/qa/rpc-tests/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "qa/rpc-tests/smartfees.py", line 244, in run_test
    self.transact_and_mine(10, self.nodes[2])
  File "qa/rpc-tests/smartfees.py", line 220, in transact_and_mine
    self.memutxo, Decimal("0.005"), min_fee, min_fee)
  File "qa/rpc-tests/smartfees.py", line 67, in small_txpuzzle_randfee
    txid = from_node.sendrawtransaction(completetx, True)
  File "/private/tmp/bitcoin-7980/qa/rpc-tests/test_framework/coverage.py", line 50, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/private/tmp/bitcoin-7980/qa/rpc-tests/test_framework/authproxy.py", line 139, in __call__
    raise JSONRPCException(response['error'])
Stopping nodes
Cleaning up
Failed
$ 

Edit: Python is Python 2.7 on this test machine.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 2, 2016

Contributor

When I run the test as ./smartfee.py, it is always OK here though (both master and this PR) - this is what I did before.

Using your method (with or without -R), I have various failures here, e.g.:

bitcoin-master$ python -R qa/rpc-tests/smartfees.py  --srcdir=src
Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/test_u6yGY
This test is time consuming, please be patient
Splitting inputs to small size so we can generate low priority tx's
Unexpected exception caught during testing: IndexError('pop from empty list',)
  File "/private/tmp/bitcoin-master/qa/rpc-tests/test_framework/test_framework.py", line 133, in main
    self.setup_network()
  File "qa/rpc-tests/smartfees.py", line 165, in setup_network
    split_inputs(self.nodes[0], self.nodes[0].listunspent(0), self.txouts, True)
  File "qa/rpc-tests/smartfees.py", line 79, in split_inputs
    prevtxout = txins.pop()
Stopping nodes
Cleaning up
Failed
bitcoin-master$ 
Contributor

paveljanik commented May 2, 2016

When I run the test as ./smartfee.py, it is always OK here though (both master and this PR) - this is what I did before.

Using your method (with or without -R), I have various failures here, e.g.:

bitcoin-master$ python -R qa/rpc-tests/smartfees.py  --srcdir=src
Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/test_u6yGY
This test is time consuming, please be patient
Splitting inputs to small size so we can generate low priority tx's
Unexpected exception caught during testing: IndexError('pop from empty list',)
  File "/private/tmp/bitcoin-master/qa/rpc-tests/test_framework/test_framework.py", line 133, in main
    self.setup_network()
  File "qa/rpc-tests/smartfees.py", line 165, in setup_network
    split_inputs(self.nodes[0], self.nodes[0].listunspent(0), self.txouts, True)
  File "qa/rpc-tests/smartfees.py", line 79, in split_inputs
    prevtxout = txins.pop()
Stopping nodes
Cleaning up
Failed
bitcoin-master$ 
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 2, 2016

Member

@paveljanik Thanks for noticing! Of course I need to fix it in the other places as well...

Member

MarcoFalke commented May 2, 2016

@paveljanik Thanks for noticing! Of course I need to fix it in the other places as well...

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 2, 2016

Contributor

Tests are running.

@MarcoFalke Feel free to cherrypick typos from bcdd81c

Contributor

paveljanik commented May 2, 2016

Tests are running.

@MarcoFalke Feel free to cherrypick typos from bcdd81c

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke May 3, 2016

Member

@paveljanik Done, thx.

Member

MarcoFalke commented May 3, 2016

@paveljanik Done, thx.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik May 3, 2016

Contributor

20 runs of rm -rf cache/; python -R qa/rpc-tests/smartfees.py --srcdir=src in both master and this PR: master 12x Failed, this PR: all OK.

ACK 43bbcd0

Contributor

paveljanik commented May 3, 2016

20 runs of rm -rf cache/; python -R qa/rpc-tests/smartfees.py --srcdir=src in both master and this PR: master 12x Failed, this PR: all OK.

ACK 43bbcd0

@MarcoFalke MarcoFalke merged commit 43bbcd0 into bitcoin:master May 3, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request May 3, 2016

Merge #7980: [qa] smartfees: Properly use ordered dict
43bbcd0 [qa] Fix typos in doc and comments (Pavel Janík)
fa17f93 [qa] smartfees: Properly use ordered dict (MarcoFalke)

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1604-qaOrderedDict branch May 3, 2016

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jun 9, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 12, 2016

nomnombtc added a commit to nomnombtc/bitcoin that referenced this pull request Nov 13, 2016

@dagurval dagurval referenced this pull request Dec 28, 2017

Merged

Fixes to smartfees.py #290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment