Add blocknotify and walletnotify functional tests

promag commented at 11:10 am on July 27, 2017: member

This patch adds the missing functional tests for -blocknotify and -walletnotify notifications. The -alertnotify test file forknotify.py is renamed to notifications.py to accommodate the new tests. Credits to @jnewbery for this cleanup and unification.

in test/functional/blocknotify.py:32 in cebf6e05da outdated

27+        self.extra_args = [['-blocknotify=echo %%s >> %s' % path]]
28+
29+    def run_test(self):
30+        # generate 5 blocks
31+        blocks = self.nodes[0].generate(5)
32+        sleep(1)

promag commented at 11:13 am on July 27, 2017:

Without sleep sometimes the file doesn’t have all blocks because runCommand() is aync. Any idea to avoid the sleep without pooling or blocking reads?

laanwj added the label Tests on Jul 27, 2017

in test/functional/blocknotify.py:9 in cebf6e05da outdated

0@@ -0,0 +1,42 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017 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+"""Test blocknotify.
6+
7+Verify that a bitcoind node calls the given command for each block
8+"""
9+from os import close, remove

practicalswift commented at 11:57 am on July 27, 2017:

Replace with import os. os.close and os.remove is used below :-)

promag commented at 12:00 pm on July 27, 2017:

Lol yes. What do you guys prefer? Import module or symbols?

practicalswift commented at 12:10 pm on July 27, 2017:

We have a winner:

0$ git grep -E "(import os|from os)" -- "*.py" | cut -f2 -d: | sort | uniq -c
1     25 import os
2      2 import os.path

in test/functional/blocknotify.py:12 in cebf6e05da outdated

 7+Verify that a bitcoind node calls the given command for each block
 8+"""
 9+from os import close, remove
10+from tempfile import mkstemp
11+from test_framework.test_framework import BitcoinTestFramework
12+from test_framework.util import assert_equal, assert_raises_jsonrpc

practicalswift commented at 11:57 am on July 27, 2017:

Remove assert_raises_jsonrpc. Not used :-)

in test/functional/blocknotify.py:21 in cebf6e05da outdated

16+
17+    def __init__(self):
18+        super().__init__()
19+
20+        # get a temporary file path to store the notified blocks
21+        (fd, path) = mkstemp(text = True)

practicalswift commented at 11:58 am on July 27, 2017:

Nit: fd, path = mkstemp(text=True) will do :-)

practicalswift commented at 11:59 am on July 27, 2017: contributor

Concept ACK :-)

promag force-pushed on Jul 27, 2017

promag commented at 9:35 pm on July 27, 2017: member

@practicalswift thanks for the review. Added a few comments and fixed the nits.

promag force-pushed on Jul 27, 2017

in test/functional/blocknotify.py:24 in 4e9b9b22ac outdated

19+
20+    def __init__(self):
21+        super().__init__()
22+
23+        # get a temporary file path to store the notified blocks
24+        fd, path = tempfile.mkstemp(text = True)

MarcoFalke commented at 9:13 am on July 28, 2017:

Please use self.options.tmpdir, so that files are contained in a single tree for each test.

Also nit: Please no whitespace for named args. (text=True)

promag commented at 10:13 am on July 28, 2017:

self.options is not defined at this point.

in test/functional/blocknotify.py:39 in 4e9b9b22ac outdated

34+    def run_test(self):
35+        # generate 3 blocks
36+        blocks = self.nodes[0].generate(3)
37+
38+        # wait while the command runs asynchronously
39+        time.sleep(3)

MarcoFalke commented at 9:16 am on July 28, 2017:

Please no fixed sleeps. This might be too much time on a fast system or too less time on a slow io system.

You could wrap it into a while loop with timeout on failure and break on success. Alternatively there should be some helper function wait_until if I am not mistaken.

MarcoFalke commented at 9:17 am on July 28, 2017: member

Concept ACK, but needs travis fixed:

0WARNING! The following scripts are not being run: ['blocknotify.py']. Check the test lists in test_runner.py.

in test/functional/blocknotify.py:44 in 4e9b9b22ac outdated

39+        time.sleep(3)
40+
41+        # file content should equal the generated blocks hashes
42+        with open(self.path, 'r') as f:
43+            assert_equal(''.join(blocks), f.read())
44+

jnewbery commented at 9:26 am on July 28, 2017:

nit: whitespace.

Consider using a linter to catch nits.

in test/functional/blocknotify.py:27 in 4e9b9b22ac outdated

22+
23+        # get a temporary file path to store the notified blocks
24+        fd, path = tempfile.mkstemp(text = True)
25+        os.close(fd)
26+
27+        self.path = path

jnewbery commented at 9:29 am on July 28, 2017:

can assign self.path above rather than assign to a local variable path

jnewbery commented at 9:33 am on July 28, 2017: member

A couple of nits to add to @MarcoFalke’s

You’ll need to add blocknotify.py to BASE_SCRIPTS in test_runner.py

promag force-pushed on Jul 28, 2017

promag force-pushed on Jul 31, 2017

promag commented at 8:05 pm on July 31, 2017: member

@MarcoFalke done. However can’t get tmpdir in the constructor. Any other suggestion?

MarcoFalke commented at 8:15 pm on July 31, 2017: member

You could try to overwrite setup_network:

0def setup_network():
1    self.extra_args = [bla]
2    super().setup_network()

On Mon, Jul 31, 2017 at 10:06 PM, João Barbosa notifications@github.com wrote:

@MarcoFalke https://github.com/marcofalke done. However can’t get tmpdir in the constructor. Any other suggestion?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10941#issuecomment-319180756, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmv0EceWaRTPKc4AFnANOaqKf4O_Jqks5sTjOogaJpZM4OlIuV .

promag force-pushed on Jul 31, 2017

promag commented at 8:54 pm on July 31, 2017: member

@MarcoFalke ty, all comments addressed.

promag force-pushed on Jul 31, 2017

practicalswift commented at 4:51 am on August 1, 2017: contributor

utACK d2c730c77f9b4011040e30f2bc2a4c8c6deb13f2

in test/functional/blocknotify.py:36 in d2c730c77f outdated

31+
32+    def run_test(self):
33+        block_count = 10
34+        blocks = self.nodes[0].generate(block_count)
35+
36+        # wait for expected data length

jnewbery commented at 5:37 pm on August 2, 2017:

A few comments on this loop:

I’d make the sleep longer (say 0.1 seconds). I’m also not sure you need the back-off. Just a time.sleep(0.1) should be fine.
The total time allowed for the file to be written is 0.45 seconds. That needs to be longer (up to 10 seconds) since Travis can block i/o for many seconds at a time. See #10072.
Please make the comment clear about the total time allowed for the loop.

jnewbery commented at 5:38 pm on August 2, 2017: member

tested ACK with one change needed to the timing of the while loop

promag force-pushed on Aug 12, 2017

promag commented at 2:03 am on August 12, 2017: member

@jnewbery done. Also rebased.

MarcoFalke commented at 11:29 am on August 12, 2017: member

Needs rebase

promag force-pushed on Aug 12, 2017

promag commented at 1:56 pm on August 12, 2017: member

@MarcoFalke done.

in test/functional/blocknotify.py:38 in 2b03bc1fb8 outdated

33+        block_count = 10
34+        blocks = self.nodes[0].generate(block_count)
35+
36+        # wait at most 10 seconds for expected file size before reading the content
37+        attempts = 0
38+        while os.stat(self.path).st_size < (block_count * 65) and attempts < 100:

MarcoFalke commented at 2:25 pm on August 12, 2017:

Maybe wait until the file is created? I’d really suggest using some sort of wrapper, like the wait_until from mininode.

0wait_until(lambda: os.path.isfile(self.path), timeout=1)
1wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)

MarcoFalke commented at 3:13 pm on August 15, 2017:

Just to make sure, that this is causing failures on my machine:

0  File "./test/functional/blocknotify.py", line 38, in run_test
1    while os.stat(self.path).st_size < (block_count * 65) and attempts < 100:
2FileNotFoundError: [Errno 2] No such file or directory: '/tmp/bitcoin_test_runner_20170815_171120/blocknotify_621/blocks.txt'

MarcoFalke commented at 6:59 pm on October 2, 2017:

ping @promag: Are you still working on this?

ryanofsky commented at 1:27 pm on October 3, 2017:

ping @promag: Are you still working on this?

I’m not seeing any errors when I run the test, but this is probably caused by a race between the start of this loop and the first notification. Could be fixed by adding a not os.path.exists(self.path) check here, or adding with open(self.path, "w"): pass to setup_network.

MarcoFalke commented at 3:38 pm on October 3, 2017:

Indeed. The code I posted a few comments above fixed it for me.

c.f.

0wait_until(lambda: os.path.isfile(self.path), timeout=1)
1wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)

jnewbery commented at 5:24 pm on August 14, 2017: member

I still believe that this test should be merged with #10966 and the forknotify.py test. They’re testing very similar functionality and there’s a lot of shared code between the three tests.

in test/functional/blocknotify.py:24 in 2b03bc1fb8 outdated

24+
25+    def setup_network(self):
26+        self.path = os.path.join(self.options.tmpdir, 'blocks.txt')
27+
28+        # the command will append the notified block hash to file
29+        self.extra_args = [['-blocknotify=echo %%s >> %s' % self.path]]

ryanofsky commented at 1:21 pm on October 3, 2017:

Maybe escape path with shlex.quote(self.path)

promag commented at 2:12 pm on October 4, 2017:

tmpdir is used elsewhere (should be secure) and blocks.txt doesn’t need to be quoted. Is there a strong argument for it?

promag commented at 2:16 pm on October 4, 2017:

Of you mean to quote the command echo %%s >> %s?

ryanofsky commented at 2:50 pm on October 4, 2017:

Quoting the entire command would be incorrect because it is passed literally to system(3). Quoting the path is correct because it is being passed as a shell token. Anyway just suggested this in case you wanted the code to be more correct. There are no practical implications (security or otherwise) either way.

promag commented at 2:53 pm on October 4, 2017:

Right, otherwise it wouldn’t work 😃 I’m going for simplicity if you don’t mind.

ryanofsky commented at 1:36 pm on October 3, 2017: member

Tested ACK 2b03bc1fb844980ffc67c6eaa52b034e978a9826 as long as the race condition is fixed.

I agree with others the test setup should be merged with walletnotify test setup in #10966 so code is not duplicated, but I don’t think it matters if this is done in one or two prs.

promag commented at 6:59 am on October 4, 2017: member

As suggested I’ll merge the test and use wait_until.

promag force-pushed on Oct 4, 2017

promag commented at 2:14 pm on October 4, 2017: member

Rebased and updated as per suggestions. Still in a separate test file because IMO that should go to a different PR since forknotify.py needs to be changed.

jnewbery commented at 3:33 pm on October 4, 2017: member

Please see https://github.com/jnewbery/bitcoin/tree/fork_notify. This adds blocknotify and walletnotify to the existint fork_notify.py test script.

in test/functional/blocknotify.py:37 in e7408a4447 outdated

32+        # wait at most 10 seconds for expected file size before reading the content
33+        wait_until(lambda: os.stat(self.path).st_size >= (block_count * 65), timeout=10)
34+
35+        # file content should equal the generated blocks hashes
36+        with open(self.path, 'r') as f:
37+            assert_equal(blocks, f.read().splitlines())

MarcoFalke commented at 7:41 pm on October 9, 2017:

Hmm. I don’t think boost::thread guarantees that the thread run in the order they are constructed. Am I wrong?

The arrays should have the same content, but a different order.

promag commented at 0:20 am on October 10, 2017:

Correct, in -walletnotify (#10966) there’s:

0            # txids are sorted because there is no order guarantee in notifications
1            assert_equal(sorted(txids_rpc), sorted(txids_file))

So I’ll do the same here for correctness.

MarcoFalke commented at 7:41 pm on October 9, 2017: member

utACK e7408a444 but needs clarification

promag renamed this:
~~Add blocknotify functional test~~
Add blocknotify and walletnotify functional tests
on Oct 10, 2017

promag force-pushed on Oct 10, 2017

promag commented at 0:37 am on October 10, 2017: member

@jnewbery Picked your branch and added a commit that adds -rescan to node[1] (needs squash). @MarcoFalke the block sort was already on the @jnewbery branch.

jnewbery commented at 4:51 pm on October 10, 2017: member

Tested ACK 6d9c1593497e9f295b5f335c7c7a0df7966d7590. As you’ve pointed out, should be squashed prior to merge.

promag force-pushed on Oct 10, 2017

promag commented at 10:39 pm on October 10, 2017: member

@jnewbery Done.

in test/functional/notifications.py:5 in 22a3487e22 outdated

0@@ -0,0 +1,86 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2016 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+"""Test the -alertnotify and -blocknotify options."""

mess110 commented at 10:42 pm on October 10, 2017:

You are also testing -walletnotify, could you please update the comment?

in test/functional/notifications.py:11 in 22a3487e22 outdated

 6+import os
 7+
 8+from test_framework.test_framework import BitcoinTestFramework
 9+from test_framework.util import assert_equal, wait_until, connect_nodes_bi
10+
11+class ForkNotifyTest(BitcoinTestFramework):

mess110 commented at 10:42 pm on October 10, 2017:

Is the class name still correct? I think it should be something like NotificationsTest

[tests] Tidy up forknotify.py 9c72a464f8

promag force-pushed on Oct 10, 2017

promag commented at 10:45 pm on October 10, 2017: member

@mess110 Done, thank you.

[tests] Add -blocknotify functional test df18d29a02

[tests] Add -walletnotify functional test 857b32b4b2

promag force-pushed on Oct 10, 2017

mess110 commented at 10:49 pm on October 10, 2017: contributor

ACK https://github.com/bitcoin/bitcoin/pull/10941/commits/857b32b4b280f13997cf2fa471802ad6a27075fb

(or the squashed version of this)

promag commented at 10:52 pm on October 10, 2017: member

I think makes sense to have individual commits.

laanwj commented at 9:24 am on October 11, 2017: member

I think makes sense to have individual commits.

Certainly if multiple authors are involved. And the different commits concern different aspects.

What we want to avoid with squashing is sequential commits like “fix the previous commit” which are internal to the author’s working and are only confusing when reading the history. But that’s not the case here.

utACK 857b32b

laanwj merged this on Oct 11, 2017

laanwj closed this on Oct 11, 2017

practicalswift referenced this in commit 364da2c529 on Oct 11, 2017

PastaPastaPasta referenced this in commit 2a7b47220b on Dec 22, 2019

PastaPastaPasta referenced this in commit 6d164bf997 on Jan 2, 2020

PastaPastaPasta referenced this in commit a7ced12270 on Jan 4, 2020

PastaPastaPasta referenced this in commit e618e3f088 on Jan 12, 2020

PastaPastaPasta referenced this in commit 137e0a6509 on Jan 12, 2020

PastaPastaPasta referenced this in commit 7ba71fd26b on Jan 12, 2020

PastaPastaPasta referenced this in commit efebec8a9f on Jan 12, 2020

PastaPastaPasta referenced this in commit 177696874d on Jan 12, 2020

PastaPastaPasta referenced this in commit 31bdc10c51 on Jan 12, 2020

PastaPastaPasta referenced this in commit 7f2ebba617 on Jan 12, 2020

ckti referenced this in commit a6a5bfc7e0 on Mar 28, 2021

MarcoFalke locked this on Sep 8, 2021

Add blocknotify and walletnotify functional tests #10941