Add test for syncing blocks generated after invalidateblock. #17335

TheBlueMatt commented at 11:09 PM on October 31, 2019: member

Suhas wrote this test to demonstrate a bug in invalidateblock back in 2015, and at some point between then and now it started passing, so best to just merge it so we don't regress.

Closes #5806 (the original issue in which this test was written)

DrahtBot added the label Tests on Oct 31, 2019

TheBlueMatt force-pushed on Oct 31, 2019

TheBlueMatt commented at 11:35 PM on October 31, 2019: member

Can we turn off these lints? Who cares at all if we have some unused imports in some python tests? This seems entirely useless.

TheBlueMatt force-pushed on Nov 1, 2019

DrahtBot commented at 2:42 AM on November 1, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16756 (test: Connection eviction logic tests by mzumsande)

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.

in test/functional/p2p_post_invalidate_sync.py:7 in 50190dfaaf outdated

   0 | @@ -0,0 +1,48 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2014 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +
   6 | +#
   7 | +# Test invalidateblock

instagibbs commented at 2:06 PM on November 1, 2019:

for p2p, since we already have RPC tests for it(I had to look)

instagibbs approved

instagibbs commented at 2:12 PM on November 1, 2019: member

utACK

in test/functional/p2p_post_invalidate_sync.py:16 in 50190dfaaf outdated

  11 | +
  12 | +class InvalidateBlockTest(BitcoinTestFramework):
  13 | +    def set_test_params(self):
  14 | +        self.num_nodes = 2
  15 | +        self.setup_clean_chain = True
  16 | +        self.extra_args = [['-debug', '-whitelist=127.0.0.1'],['-debug', '-whitelist=127.0.0.1']]

jnewbery commented at 6:32 PM on November 1, 2019:

why -debug? It's on by default.

Please comment why -whitelist is required

TheBlueMatt commented at 7:47 PM on November 1, 2019:

Because that's what was there before, its just a test, and it doesn't seem to hurt anything.

jnewbery commented at 8:11 PM on November 1, 2019:

How can it be there before if this is a new test?

TheBlueMatt commented at 8:17 PM on November 1, 2019:

The code was taken, almost verbatim from #5806, its not fresh.

jnewbery commented at 8:40 PM on November 1, 2019:

Adding '-whitelist=127.0.0.1' to tests seems to me to make them less realistic, more brittle and harder to maintain if different connection options are added. It also changes p2p behaviour so that it's less obvious that the test is testing what you want to test in a focused way. Does whitelisting interact with how blocks are sync'ed in the event of a block being invalidated? I don't know, and by adding this option you're requiring reviewers to figure that out (or just rubberstamp ACK this PR without understanding the code). So my request is to either remove this or comment on why you've chosen to add this option.

in test/functional/p2p_post_invalidate_sync.py:6 in 50190dfaaf outdated

   0 | @@ -0,0 +1,48 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2014 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +
   6 | +#

jnewbery commented at 6:33 PM on November 1, 2019:

Prefer to use file docstrings over code comments for the top-level comment

TheBlueMatt commented at 7:48 PM on November 1, 2019:

I don't know what a docstrng is.

jnewbery commented at 8:11 PM on November 1, 2019:

Here you go: https://www.python.org/dev/peps/pep-0257/

jnewbery commented at 8:55 PM on November 4, 2019:

Take a look at the other tests, particularly https://github.com/bitcoin/bitcoin/blob/master/test/functional/example_test.py#L5 and https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines. Documenting what the test is supposed to be testing and why is a courtesy to any other developer who tries to understand this in future. "Test invalidateblock" doesn't explain what this test is for.

in test/functional/p2p_post_invalidate_sync.py:36 in 50190dfaaf outdated

  31 | +        self.nodes[1].invalidateblock(node1blocks[0])
  32 | +
  33 | +        if (self.nodes[0].getblockcount() != cnt):
  34 | +            raise AssertionError("Failed to invalidate initial blocks")
  35 | +
  36 | +        # The test framework is a little dense wrt mining on nodes without wallets -

jnewbery commented at 6:42 PM on November 1, 2019:

This comment is wrong. All nodes will generate to a deterministic address (whether wallet is enabled or not).

Other ways to achieve this:

call `self.nodes[0].generatetoaddress(17, self.nodes[0].getnewaddress()) to get a new address (and hence new block hash)
call sleep(1) to get a new timestamp (and hence new block hash)
~use setmocktime() to get a new timestamp (and hence new block hash)~ probably not a good idea if you're testing p2p behaviour.

jnewbery commented at 6:44 PM on November 1, 2019:

also: I'd gently encourage you not to call other contributors' code 'a little dense'

TheBlueMatt commented at 7:50 PM on November 1, 2019:

I tried getnewaddress first - sadly this is just an issue for non-wallet nodes. Wallet'd nodes will generate a new deterministic address, but the test framework just hard-codes a bunch. That's totally fine, but it is a bit dense. I'm also a bit dense, and so is lots of code I write, cause its just a hack to get it to work. Didn't mean it at all as an insult, just describing the state of it.

jnewbery commented at 8:13 PM on November 1, 2019:

It's not dense. It's how we get tests to work with wallet enabled and disabled.

Can you remove the incorrect comment please?

TheBlueMatt commented at 5:08 PM on November 4, 2019:

Fair, likely should not have used the word "dense". I cleared up the comment, though I still think this approach is the easiest?

Add test for syncing blocks generated after invalidateblock.

Suhas wrote this test to demonstrate a bug in invalidateblock back
in 2015, and at some point between then and now it started passing,
so best to just merge it so we don't regress.

Closes #5806 (the original issue in which this test was written)

df117c8212

TheBlueMatt force-pushed on Nov 4, 2019

in test/functional/p2p_post_invalidate_sync.py:37 in df117c8212

  32 | +
  33 | +        if (self.nodes[0].getblockcount() != cnt):
  34 | +            raise AssertionError("Failed to invalidate initial blocks")
  35 | +
  36 | +        # The test framework uses a static per-node address which will generate
  37 | +        # a deterministic block if we have no wallet.

jnewbery commented at 8:55 PM on November 4, 2019:

'if we have no wallet' is inaccurate. All calls to generate in the functional test framework will generate blocks to a hardcoded address.

jnewbery commented at 9:03 PM on November 4, 2019:

I think it's slightly clearer if all blocks are generated on node0 and sync'ed to node1. It makes it more obvious to someone reading the test that this is testing node1 being able to resync to an less-work chain if it previously invalidated a different chain.

in test/functional/p2p_post_invalidate_sync.py:16 in df117c8212

  11 | +
  12 | +class InvalidateBlockTest(BitcoinTestFramework):
  13 | +    def set_test_params(self):
  14 | +        self.num_nodes = 2
  15 | +        self.setup_clean_chain = True
  16 | +        self.extra_args = [[],[]]

jnewbery commented at 8:55 PM on November 4, 2019:

This isn't required if you're not adding any extra args.

in test/functional/p2p_post_invalidate_sync.py:42 in df117c8212

  37 | +        # a deterministic block if we have no wallet.
  38 | +        # Instead, mine on nodes[0], which will use a different hardcoded address
  39 | +        # than the one we previously used, making this block unique.
  40 | +        self.nodes[0].generate(17)
  41 | +
  42 | +        print("All blocks generated, trying to sync")

jnewbery commented at 8:57 PM on November 4, 2019:

Use self.log.info() rather than print

in test/functional/p2p_post_invalidate_sync.py:15 in df117c8212

  10 | +from test_framework.test_framework import BitcoinTestFramework
  11 | +
  12 | +class InvalidateBlockTest(BitcoinTestFramework):
  13 | +    def set_test_params(self):
  14 | +        self.num_nodes = 2
  15 | +        self.setup_clean_chain = True

jnewbery commented at 8:59 PM on November 4, 2019:

If you remove this line, you can also remove the following lines from run_test:

        self.nodes[0].generate(1) # Leave IBD
        self.sync_all()

The test will run more quickly and it'll be clearer to readers what the test is for.

jnewbery commented at 9:03 PM on November 4, 2019: member

Getting better. You'll be a python developer soon!

promag commented at 8:42 AM on November 7, 2019: member

Concept ACK.

jnewbery commented at 4:54 PM on December 15, 2019: member

Suggeted fixups: https://github.com/jnewbery/bitcoin/commit/3357670144485dcde14ddf98a90fab0fca6965a3 (in branch https://github.com/jnewbery/bitcoin/tree/pr17335.1)

DrahtBot commented at 6:56 PM on June 12, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jun 12, 2020

jnewbery commented at 8:04 PM on June 12, 2020: member

This has unaddressed review comments from > 6 months ago. Closing and marking 'up for grabs'. @TheBlueMatt - if you want to pick this up again, let me know and I'll reopen.

jnewbery closed this on Jun 12, 2020

jnewbery added the label Up for grabs on Jun 12, 2020

jonatack commented at 6:07 PM on June 27, 2020: member

Picked this up in #19397.

MarcoFalke removed the label Needs rebase on Jun 28, 2020

MarcoFalke removed the label Up for grabs on Jun 28, 2020

DrahtBot locked this on Feb 15, 2022