test: add functional test for txrelay during and after IBD #19423

glozow commented at 5:03 AM on July 1, 2020: member

This is a followup to #19204 which uses minfeefilter=MAX_MONEY to effectively shut off txrelay, thereby reducing inv traffic, when nodes are in IBD. It was missing a functional test.

fanquake added the label Tests on Jul 1, 2020

glozow commented at 5:05 AM on July 1, 2020: member

Btw thanks @jonatack for input on the test 😄 added you as coauthor. I added a loop for peerinfo so this test should work with any number of nodes.

in test/functional/p2p_ibd.py:21 in 502950d517 outdated

  16 | +        self.setup_clean_chain = True
  17 | +        self.num_nodes = 2
  18 | +        self.extra_args = [["-minrelaytxfee=0.00000100"], ["-minrelaytxfee=0.00000100"]]
  19 | +
  20 | +    def skip_test_if_missing_module(self):
  21 | +        self.skip_if_no_wallet()

jonatack commented at 5:18 AM on July 1, 2020:

I think you can remove these two lines about the wallet. This should be unrelated to wallet presence.

glozow commented at 6:03 AM on July 1, 2020:

woops

glozow force-pushed on Jul 1, 2020

glozow commented at 5:46 AM on July 1, 2020: member

Pushing to rerun travis because a job timed out :(

in test/functional/p2p_ibd.py:30 in c33c390a85 outdated

  28 | +        for node in self.nodes:
  29 | +            assert node.getblockchaininfo()['initialblockdownload']
  30 | +            info = node.getpeerinfo()
  31 | +            assert_equal(info[0]['minfeefilter'], MAX_FEE_FILTER)
  32 | +            for conn_info in info:
  33 | +                assert "inv" not in conn_info["bytesrecv_per_msg"].keys()

jonatack commented at 5:48 AM on July 1, 2020:

Nice idea to add the inner loop. Can also assert the filter for each element.

-            assert_equal(info[0]['minfeefilter'], MAX_FEE_FILTER)
             for conn_info in info:
+                assert_equal(conn_info['minfeefilter'], MAX_FEE_FILTER)

in test/functional/p2p_ibd.py:41 in c33c390a85 outdated

  40 | +        self.log.info("Check that nodes reset minfilter and receive invs after coming out of IBD")
  41 | +        for node in self.nodes:
  42 | +            assert not node.getblockchaininfo()['initialblockdownload']
  43 | +            info = node.getpeerinfo()
  44 | +            assert_equal(info[0]['minfeefilter'], NORMAL_FEE_FILTER)
  45 | +            for conn_info in info:

jonatack commented at 5:49 AM on July 1, 2020:

idem!

-            assert_equal(info[0]['minfeefilter'], NORMAL_FEE_FILTER)
             for conn_info in info:
+                assert_equal(conn_info['minfeefilter'], NORMAL_FEE_FILTER)

in test/functional/p2p_ibd.py:25 in c33c390a85 outdated

  20 | +    def skip_test_if_missing_module(self):
  21 | +        self.skip_if_no_wallet()
  22 | +
  23 | +    def run_test(self):
  24 | +        MAX_FEE_FILTER = Decimal(str(9170997 / COIN))
  25 | +        NORMAL_FEE_FILTER = Decimal(str(100 / COIN))

jonatack commented at 5:49 AM on July 1, 2020:

If you like, the constants can be pulled out to the top to de-clutter the test. I think that's generally what is done but that could be a misconception on my part.

jonatack commented at 5:56 AM on July 1, 2020: member

Concept ACK :smile:

A few suggestions below.

jonatack commented at 5:58 AM on July 1, 2020: member

Pushing to rerun travis because a job timed out :(

You can also ask for a travis re-kick on irc and someone will restart just the recalcitrant job. In this case, I'll keep a sharp weather eye and re-kick any jobs needed here.

glozow force-pushed on Jul 1, 2020

jonatack commented at 7:05 AM on July 1, 2020: member

LGTM

in test/functional/test_runner.py:249 in 236cd1c128 outdated

 245 | @@ -246,6 +246,7 @@
 246 |      'rpc_help.py',
 247 |      'feature_help.py',
 248 |      'feature_shutdown.py',
 249 | +    'p2p_ibd.py',

naumenkogs commented at 10:12 AM on July 1, 2020:

This is a confusing name because it doesn't really test IBD (the main purpose of which is to sync the tip). Maybe 'p2p_tx_relay_while_ibd.py'

MarcoFalke commented at 11:59 AM on July 1, 2020:

p2p_feefilter_ibd ?

glozow commented at 7:29 PM on July 1, 2020:

p2p_ibd_txrelay?

in test/functional/p2p_ibd.py:13 in 236cd1c128 outdated

   8 | +
   9 | +from test_framework.messages import COIN
  10 | +from test_framework.test_framework import BitcoinTestFramework
  11 | +from test_framework.util import assert_equal
  12 | +
  13 | +MAX_FEE_FILTER = Decimal(str(9170997 / COIN))

MarcoFalke commented at 12:01 PM on July 1, 2020:

Seems a bit odd to use floating point, but then put the result in a decimal. Why not Decimal(9170997)/COIN?

in test/functional/p2p_ibd.py:14 in 236cd1c128 outdated

   9 | +from test_framework.messages import COIN
  10 | +from test_framework.test_framework import BitcoinTestFramework
  11 | +from test_framework.util import assert_equal
  12 | +
  13 | +MAX_FEE_FILTER = Decimal(str(9170997 / COIN))
  14 | +NORMAL_FEE_FILTER = Decimal(str(100 / COIN))

MarcoFalke commented at 12:01 PM on July 1, 2020:

same

in test/functional/p2p_ibd.py:21 in 236cd1c128 outdated

  16 | +
  17 | +class P2PIBDTest(BitcoinTestFramework):
  18 | +    def set_test_params(self):
  19 | +        self.setup_clean_chain = True
  20 | +        self.num_nodes = 2
  21 | +        self.extra_args = [["-minrelaytxfee=0.00000100"], ["-minrelaytxfee=0.00000100"]]

MarcoFalke commented at 12:02 PM on July 1, 2020:

Could use the constant?

        self.extra_args = [
        ["-minrelaytxfee={}".format(NORMAL_FEE_FILTER)],
        ["-minrelaytxfee={}".fromat(NORMAL_FEE_FILTER)],
         ]

in test/functional/p2p_ibd.py:43 in 236cd1c128 outdated

  38 | +        for node in self.nodes:
  39 | +            assert not node.getblockchaininfo()['initialblockdownload']
  40 | +            info = node.getpeerinfo()
  41 | +            for conn_info in info:
  42 | +                assert_equal(info[0]['minfeefilter'], NORMAL_FEE_FILTER)
  43 | +                assert "inv" in conn_info["bytesrecv_per_msg"].keys() or "inv" in conn_info["bytessent_per_msg"].keys()

MarcoFalke commented at 12:05 PM on July 1, 2020:

This is a inv of type block, no?

I am not sure if there is a way to ask Bitcoin Core which inv type it received. Maybe the only way is to use mininode?

glozow commented at 6:22 PM on July 2, 2020:

Well, to be honest, this test doesn't even produce any transactions to send invs about right now 😅 I need to work that out.

Mininodes are useful for seeing what nodes send out, but I'm not sure how useful they for seeing what nodes receive... I could have a mininode manually send a feefilter and make sure it doesn't receive anything, but then I'm testing the feefilter message instead of IBD behavior.

MarcoFalke commented at 6:39 PM on July 2, 2020:

Isn't the point to test what the node sends out? I.e. test whether MAX_FILER or not was sent out?

glozow commented at 6:41 PM on July 2, 2020:

Yep, feefilter is already tested - MAX_FILTER during IBD, then reset to normal after. I had been hoping to also test that they don't receive tx invs, but it seems more complicated than what I was doing originally.

glozow commented at 12:38 AM on July 3, 2020:

@MarcoFalke if you have time to check out https://github.com/bitcoin/bitcoin/pull/19423/commits/7bc653e7e3d21c675c6a1257f658222b97e166cb please let me know what you think :)

MarcoFalke commented at 12:05 PM on July 1, 2020: member

Very nice. Concept ACK

glozow force-pushed on Jul 2, 2020

MarcoFalke commented at 6:48 PM on July 2, 2020: member

ACK fc1a714699a66f9feec9e8ab0d6be92a07244a74

glozow commented at 9:26 PM on July 2, 2020: member

Status: I think https://github.com/bitcoin/bitcoin/commit/fc1a714699a66f9feec9e8ab0d6be92a07244a74 (with just the minfeefilter) is sufficient for testing #19204.

I figured out a way to test transaction relay too in https://github.com/bitcoin/bitcoin/pull/19423/commits/7bc653e7e3d21c675c6a1257f658222b97e166cb, but it's not perfect - there isn't an obvious way to see what tx invs a node has received since bitcoind doesn't provide that information through rpc.

in test/functional/p2p_ibd_txrelay.py:38 in 7bc653e7e3 outdated

  42 | +        self.nodes[1].setmocktime(tip_time + MAX_TIP_AGE + 1)
  43 | +        connect_nodes(self.nodes[0], 1)
  44 | +        connect_nodes(self.nodes[1], 0)
  45 | +
  46 | +        self.log.info("Check that nodes set minfilter to MAX_MONEY and do not receive tx invs while still in IBD")
  47 | +        assert self.nodes[1].getblockchaininfo()['initialblockdownload']

jnewbery commented at 12:59 AM on July 3, 2020:

I'm worried that this might be intermittent. IsInitialBlockDownload is a latch. The first time it returns false causes it to return false on all subsequent calls. Is there a window between restarting the node and fastforwarding time with setmocktime where somthing could call IsInitialBlockDownload() and cause it to latch to false?

glozow commented at 3:33 PM on July 3, 2020:

Gooooooood point. A few ideas I have:

Wondering if I can pass in an argument for start to get the test node to start up at a particular mocktime?
I could... setmocktime backwards to begin with and maybe when it restarts, it'll be outdated from the get go?

jnewbery commented at 3:43 PM on July 3, 2020:

~1. I don't think there's a way to start a bitcoind node with a mocktime. It can only be set by an RPC after the node has started.~ 2. Yes, I think this would work!

EDIT. You were right, I was wrong. You can start bitcoind with a -mocktime: https://github.com/bitcoin/bitcoin/blob/3276c148c4cac7b7c9adbaab5997b26488612085/src/init.cpp#L527

jonatack commented at 5:01 PM on July 3, 2020:

I'm worried that this might be intermittent. IsInitialBlockDownload is a latch.

Yes, I'm seeing the test fail at that line.

Great stuff, will check out your next push asap @gzhao408!

glozow commented at 5:31 PM on July 3, 2020:

~Oo hmmmmm these args are only used in test_framework.add_nodes to instantiate the TestNode objects, and then can't be changed afterward. So I can't actually restart the node with a new -mocktime.~ Just kidding, I'm just struggling to make it work for some reason 😂

glozow commented at 5:34 PM on July 3, 2020:

Not sure 🤔

MarcoFalke commented at 5:44 PM on July 3, 2020:

extra_args can be overwritten for start_node(s)

glozow force-pushed on Jul 3, 2020

glozow commented at 8:50 PM on July 3, 2020: member

Soooo I just had an excellent adventure through the mock time stuff... I can't get -mocktime to work (see this test which passes for setmocktime but not -mocktime) so I implemented this by fastforwarding one node by 2 days so it thinks it's still in IBD even when receiving blocks (all > 1 day old). Then we don't need to restart the node.

promag commented at 11:09 AM on July 7, 2020: member

Concept ACK, just read the code and looks good to me.

practicalswift commented at 9:03 PM on July 10, 2020: contributor

Concept ACK

Thanks for improving our testing!

MarcoFalke commented at 10:21 AM on July 11, 2020: member

The test fails on the second commit. I am happy to merge the first commit, though, and we can leave the second commit for a later pull?

glozow commented at 6:38 PM on July 11, 2020: member

@MarcoFalke thanks! Would you happen to have a log for the test fail? (Also it seems like a travis job timed out but otherwise green). I can definitely separate the second commit, but currently it's working for me so I'm not sure how to fix it 😕

jonatack commented at 6:44 PM on July 11, 2020: member

@gzhao408 here's the link (you can click through to it on the :x: ) https://travis-ci.org/github/bitcoin/bitcoin/jobs/704781135. I restarted the job a few days ago to see if it would re-fail.

jonatack commented at 6:47 PM on July 11, 2020: member

(Do you have travis linked to your personal fork of the bitcoin repo? You can restart failing jobs there.)

glozow force-pushed on Jul 11, 2020

[test] feefilter during and after IBD

Co-authored-by: Jon Atack <jon@atack.com>

cb31ee01b4

glozow force-pushed on Jul 15, 2020

glozow commented at 12:16 AM on July 16, 2020: member

Sorry for the delay 😓 ready for review again.

Pushed it back to the first commit and fixed the error - this tests the fee filter during and after IBD, no txrelay. I'll work on txrelay and will make it a followup.

jnewbery commented at 2:31 PM on July 16, 2020: member

utACK cb31ee01b42af58c64b5d3c0eabae4bcd7fd67cf

glozow requested review from MarcoFalke on Jul 17, 2020

MarcoFalke merged this on Jul 17, 2020

MarcoFalke closed this on Jul 17, 2020

Fabcien referenced this in commit ca15bc5ccd on Sep 2, 2021

DrahtBot locked this on Feb 15, 2022