Skip to content

[qa] smartfees: Properly use ordered dict #7980

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 3, 2016
Merged

[qa] smartfees: Properly use ordered dict #7980

merged 2 commits into from
May 3, 2016

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Apr 30, 2016 •
edited
Loading

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

@maflcko maflcko added the Tests label Apr 30, 2016
@paveljanik
Copy link
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...

@maflcko
Copy link
Member Author

maflcko commented May 2, 2016

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

@laanwj
Copy link
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
Copy link
Member

laanwj commented May 2, 2016 •
edited
Loading

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.

@maflcko
Copy link
Member Author

maflcko 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
Copy link
Contributor

paveljanik commented May 2, 2016 •
edited
Loading

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
Copy link
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$

@maflcko maflcko force-pushed the Mf1604-qaOrderedDict branch from facc8fb to fa17f93 Compare May 2, 2016 17:36
@maflcko
Copy link
Member Author

maflcko commented May 2, 2016

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

@paveljanik
Copy link
Contributor

Tests are running.

@MarcoFalke Feel free to cherrypick typos from bcdd81c

@maflcko maflcko force-pushed the Mf1604-qaOrderedDict branch from 9289f49 to 43bbcd0 Compare May 2, 2016 20:31
@maflcko
Copy link
Member Author

maflcko commented May 3, 2016

@paveljanik Done, thx.

@paveljanik
Copy link
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

@maflcko maflcko merged commit 43bbcd0 into bitcoin:master May 3, 2016
maflcko pushed a commit that referenced this pull request May 3, 2016
Merge #7980: [qa] smartfees: Properly use ordered dict
88b77c7 
43bbcd0 [qa] Fix typos in doc and comments (Pavel Janík)
fa17f93 [qa] smartfees: Properly use ordered dict (MarcoFalke)
@maflcko maflcko deleted the Mf1604-qaOrderedDict branch May 3, 2016 11:13
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 9, 2016
[qa] smartfees: Properly use ordered dict
c0fe8b5 
Github-Pull: bitcoin#7980
Rebased-From: fa17f93 43bbcd0
@maflcko maflcko mentioned this pull request Dec 6, 2016
Merged
@dagurval dagurval mentioned this pull request Dec 28, 2017
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maflcko @paveljanik @laanwj