@sdaftuar - I never reviewed the dbcrash.py test in #10148 . This PR fixes up a few nits:
time indirectly, no need for try-except importing httplib since that's a python 2/3 compatibility trick, and our tests always run in Py3)low-priority, should be fairly uncontroversial, but let me know if you disagree with any of these.
Looks like this test is acting up: https://travis-ci.org/bitcoin/bitcoin/jobs/248398016 https://travis-ci.org/bitcoin/bitcoin/jobs/248398018 Might need a similar change to #10659.
39 | +from test_framework.util import * 40 | 41 | +class ChainstateWriteCrashTest(BitcoinTestFramework): 42 | def __init__(self): 43 | super().__init__() 44 | - self.num_nodes = 4
I don't feel strongly, but the reason I put this in was that I didn't think that changes to the default test framework should cause tests to break -- since this test relies on a particular configuration, I felt it was best to define that locally.
Also, can you set a custom timewait for the RPC connections in this test to resolve the travis issues that have come up so far: https://travis-ci.org/bitcoin/bitcoin/jobs/248398018#L2153
We might as well just get rid of the default value. There is nothing special about "4" that justifies to use it as default value.
ok, you've convinced me. I'll leave this in for now.
We can remove the default num_nodes in a future PR (and probably remove the default topology and premine while we're at it).
98 | + 99 | try: 100 | self.nodes[node_index].submitblock(block) 101 | return True 102 | - except (httplib.CannotSendRequest, httplib.RemoteDisconnected) as e: 103 | + except (http.client.CannotSendRequest, http.client.RemoteDisconnected) as e:
I think this RemoteDisconnected exception may not exist pre-3.5, which would explain this travis failure: https://travis-ci.org/bitcoin/bitcoin/jobs/248398016#L3321
I'm not sure what the best way to handle that is...
I think I've fixed this in my fixup commit b394a00f6c34e20e5ff555c8a07c6558e319a34a. Just catch different errors depending on which version of Python is being run.
I think later we should move this logic to a separate method named send_rpc_may_fail() or similar in TestNode to keep the test script code clean.
167 | @@ -175,9 +168,8 @@ def verify_utxo_hash(self): 168 | nodei_utxo_hash = self.restart_node(i, self.nodes[3].getbestblockhash()) 169 | assert_equal(nodei_utxo_hash, node3_utxo_hash) 170 | 171 | - 172 | def generate_small_transactions(self, node, count, utxo_list): 173 | - FEE = 1000 # TODO: replace this with node relay fee based calculation 174 | + FEE = 1000 # TODO: replace this with node relay fee based calculation
Feel free to address the TODO as well :) I think you just do something like relayfee = self.nodes[0].getnetworkinfo()['relayfee'], and then convert to the right units.
I haven't addressed this. What are the downsides to using a fixed fee?
Looks good, thanks for cleaning this up. Please take a look at the fixes needed to make travis happy as well.
I think I've fixed the Travis issues, although it's difficult to tell for sure since Travis only runs extended tests in master.
For the first, I split the generate call in create_confirmed_utxos() into many smaller generate calls. For the second, I catch different errors depending on the Python version.
Both of these solutions should be moved into TestNode once that's merged (ie a node_generate() method which automatically breaks the generate call into smaller pieces, and a send_rpc_may_fail() method for RPCs that may or may not time out).
I haven't addressed the fixed fee todo. I don't think that's too much of a problem, but let me know if you think it could cause issues.
One question: I'm confused about the randomness in re-org'ing. As far as I can see, the way this works:
That throws up events with very low probabilities. For example, we'll re-org back 40 blocks in 1/40 * 1/44 = 1/1760 tests. For me, such indeterminateness is troubling. If there was a problem that caused large re-orgs to fail, then they'd only be caught very intermittently by this test, and it'd be difficult to debug.
I prefer my tests to be more deterministic where possible. How would you feel about changing the re-orgs to a fixed schedule (ie make sure the test covers re-orgs of 1, 2, 4, 8, 16 and 32 blocks, or some similar schedule)?
utACK b394a00f6c34e20e5ff555c8a07c6558e319a34a. Feel free to squash.
squashed
Going to merge this now, since it might fix the test failures we see on master right now. Further cleanup can always be completed in later commits.
That throws up events with very low probabilities. For example, we'll re-org back 40 blocks in 1/40 * 1/44 = 1/1760 tests. For me, such indeterminateness is troubling. If there was a problem that caused large re-orgs to fail, then they'd only be caught very intermittently by this test, and it'd be difficult to debug.
I prefer my tests to be more deterministic where possible. How would you feel about changing the re-orgs to a fixed schedule (ie make sure the test covers re-orgs of 1, 2, 4, 8, 16 and 32 blocks, or some similar schedule)? @jnewbery I don't think the right way to think of this test is that we're testing reorgs. Really, we're testing a more complex state (utxo cache + disk state) and trying to get as much coverage of various edge cases as we can. Since it's hard for me to imagine all the possible states we could be in, I wanted to simulate as much normal looking bitcoin transaction volume as I reasonably could, and then randomly stress it with reorgs and crashes in the hopes that if an edge case had a bug, we'd eventually see it.
If I knew exactly how to enumerate all the edge cases, then I'd happily convert this to a deterministic set of tests, but lacking that I think the randomness here is an appropriate test strategy.
I don't think the right way to think of this test ... , but lacking that I think the randomness here is an appropriate test strategy.
ACK