When starting a test, we are always in IBD because the timestamps on cached blocks are in the past. Usually, we solve that by generating a block at the beginning of the test.
That is clumsy and might even lead to other problems such as #15360 and #14446 (comment)
So fix that by getting rid of mocktime and always refreshing the last block of the cache when starting the test framework.
Should fix #14446
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
499- #
500- # blocks are created with timestamps 10 minutes apart
501- # starting from 2010 minutes in the past
502- block_time = self.mocktime - (201 * 10 * 60)
503+ # The 4th node gets only 24 immature blocks.
504 for i in range(2):
274@@ -275,6 +275,17 @@ def setup_nodes(self):
275 self.add_nodes(self.num_nodes, extra_args)
276 self.start_nodes()
277 self.import_deterministic_coinbase_privkeys()
278+ if not self.setup_clean_chain:
279+ for n in self.nodes:
280+ wait_until(lambda: n.getblockchaininfo()["blocks"] == 199)
Ah, so the wait_until would be needed if we wanted to -reindex on the first start (e.g. mempool_accept)
However, with txindex as a background thread, this is no longer required in mempool_accept.
503+ # The 4th node gets only 24 immature blocks.
504 for i in range(2):
505 for peer in range(4):
506 for j in range(25):
507- set_node_times(self.nodes, block_time)
508+ if i == 1 and peer == 3 and j == 24:
Maybe something more straightforward?
0def gen_blocks(peer, blocks):
1 node = self.nodes[peer]
2 address = node.get_deterministic_priv_key().address
3 return node.generatetoaddress(blocks, address)
4
5gen_blocks(0, 25)
6gen_blocks(1, 25)
7gen_blocks(2, 25)
8gen_blocks(3, 24)
No, that would generate 99 blocks, we need 199.
Not sure if this looks simpler:
0gen_blocks(0, 25)
1gen_blocks(1, 25)
2gen_blocks(2, 25)
3gen_blocks(3, 25)
4gen_blocks(0, 25)
5gen_blocks(1, 25)
6gen_blocks(2, 25)
7gen_blocks(3, 24)
In that case
0gen_blocks(0, 50)
1gen_blocks(1, 50)
2gen_blocks(2, 50)
3gen_blocks(3, 49)
Or is there a reason against?
See the comment two lines up:
0 # Create a 199-block-long chain; each of the 4 first nodes
1 # gets 25 mature blocks and 25 immature.
511- block_time += 10 * 60
512 # Must sync before next peer starts generating blocks
513 sync_blocks(self.nodes)
514
515+ for n in self.nodes:
516+ assert_equal(n.getblockchaininfo()["blocks"], 199)
Concept ACK. Please remove unrelated code style fixes from your commits.
I think ‘rot’ is the wrong terminology, and the comment here: https://github.com/bitcoin/bitcoin/pull/15419/commits/faeb65a8cdeb4495e5b0cd8b515a19911ba0a6ad#diff-64721c5ee64d44f7114d6d0d2226db4dR495 isn’t clear.
rotten as in from yesterday or from last week. If the cache was edible, it would be rotten, no?
There’s nothing wrong with the block, it’s just old. I think this would be much clearer to people reading the tests if you removed the rotten terminology and just said that the latest block needs to be within the last 24 hours for the nodes to consider themselves sync’ed to the tip, possibly with reference to IsInitialBlockDownload() and https://github.com/bitcoin/bitcoin/blob/904308dca3ff50afb704597c0a65d226dd40f635/src/validation.h#L113.
the latest block needs to be within the last 24 hours for the nodes to consider themselves sync’ed to the tip
That’s definitely useful to remind the reader of (including yours truly).
stale is still wrong:
Neither of those is the case here. The old block is not stale, it’s just old.
I suggest not creating new (confusing) terminology here and just describe what’s actually being implemented.
494 for i in range(2):
495 for peer in range(4):
496 for j in range(25):
497+ if i == 1 and peer == 3 and j == 24:
498+ # Don't store the very last block in the cache to avoid having it go stale
499+ # See IsInitialBlockDownload()
This needs more explanation, since iiuc it’s very relevant to the bug you’re trying to squash. What cache is going stale and what is the consequence of that? And in what we should one see IsInitialBlockDownload? What about the cache causes nodes to think they’re in or out of IBD? The absolute age of the block in the cache?
It’s explained a bit in the next commit, but that’s in a very different place.
The following tests failed on macOS 10.14.3 on the first run. logs:
0mining_basic.py | ✖ Failed | 15 s
1p2p_invalid_messages.py | ✖ Failed | 8 s
2tool_wallet.py | ✖ Failed | 2 s
3wallet_txn_clone.py | ✖ Failed | 11 s
4wallet_txn_clone.py --mineblock | ✖ Failed | 9 s
5wallet_txn_clone.py --segwit | ✖ Failed | 11 s
Some of that seems to be usage of modern Python, which might be unrelated. @jnewbery agreed. The problem seems to be that cache is not properly invalidated. Our test suite cache items should probably have an expiration timestamp, but that’s perhaps a bit overkill to engineer.
Can you remove unrelated style changes?
I don’t care much about additional new lines between classes (https://github.com/bitcoin/bitcoin/pull/15419/files#diff-a12894108450a68b27252d39d7b6e704R44) or terminating lists with commas (https://github.com/bitcoin/bitcoin/pull/15419/files#diff-86294e5ae5283eebdd9f98d79007a0e1R93).
If you think that should be the project code style, the right thing to do would be update the developer notes and add linters.
so that review can focus on the actual changes and is not wasted on counting how many spaces or newlines are between non-whitepace characters.
My suggestion is to not change code style in PRs which are supposed to be changing behaviour. Here for example: https://github.com/bitcoin/bitcoin/pull/15419/files#diff-64721c5ee64d44f7114d6d0d2226db4dR36 you add a newline that has nothing to do with the changes in this PR.
If you want review to focus on the actual changes, just remove unnecessary noise.
273@@ -275,6 +274,17 @@ def setup_nodes(self):
274 self.add_nodes(self.num_nodes, extra_args)
275 self.start_nodes()
276 self.import_deterministic_coinbase_privkeys()
277+ if not self.setup_clean_chain:
278+ for n in self.nodes:
279+ assert_equal(n.getblockchaininfo()["blocks"], 199)
280+ self.log.debug('Generate a block with current time to finalize the cache and assert we are out of IBD')
I don’t think “finalize the cache” would mean anything to people without more context. I’d suggest changing this to:
0 # To ensure that all nodes are out of IBD, the most recent block must
1 # be within the last 24 hours (see IsInitialBlockDownload()). Generate it here.
2 self.log.debug('Generate a block with current time')
280+ self.log.debug('Generate a block with current time to finalize the cache and assert we are out of IBD')
281+ block_hash = self.nodes[0].generate(1)[0]
282+ block = self.nodes[0].getblock(blockhash=block_hash, verbosity=0)
283+ for n in self.nodes:
284+ n.submitblock(block)
285+ chain_info = n.getblockchaininfo()
There’s no race here since submitblock() will only return after ActivateBestChain(), but if we ever make ProcessNewBlock() multithreaded, then there could be a race if submitblock() returns before the best chain has been activated.
I don’t think you need to do anything here, but you could add a comment or a wait_until() if you wanted to make this clear to readers.
491- # versions of the cache, set mocktime to Jan 1,
492- # 2014 + (201 * 10 * 60)"""
493- self.mocktime = 1388534400 + (201 * 10 * 60)
494-
495- # Create a 200-block-long chain; each of the 4 first nodes
496+ # Create a 199-block-long chain; each of the 4 first nodes
A couple of suggestions that could make this more efficient/clear:
sync_blocks() calls.0self.nodes[0].generatetoaddress(25, <node0_address>)
1self.nodes[0].generatetoaddress(25, <node1_address>)
2self.nodes[0].generatetoaddress(25, <node2_address>)
3self.nodes[0].generatetoaddress(25, <node3_address>)
4self.nodes[0].generatetoaddress(99, <dummy_address>)
or:
0for i in range(4):
1 self.nodes[0].generatetoaddress(25, self.nodes[i].get_deterministic_priv_key().address())
2self.nodes[0].generatetoaddress(99, <dummy_address>)
33@@ -34,6 +34,14 @@ def setup_network(self):
34 def run_test(self):
35 # All nodes should start with 1,250 BTC:
36 starting_balance = 1250
37+
38+ # All nodes should be out of IBD.
39+ # If the nodes are not all out of IBD, that can interfere with
40+ # blockchain sync later in the test when nodes are connected, due to
41+ # timing issues.
42+ for n in self.nodes:
Looks good. A few suggestions inline.
I still don’t like the fact that this PR adds random newlines to various files but if you insist, then I don’t care enough to block merging.