This PR adds a functional test for anchors.dat.
It creates a node and adds 2 outbound block-relay-only connections and 5 inbound connections. When the node is down, anchors.dat should contain the 2 addresses from the outbound block-relay-only connections.
Concept ACK
I was thinking this should be a p2p_ test rather than a feature_ test but maybe you're right...
I added feature_anchors.py in BASE_SCRIPTS
CI Tests are failing for two reasons so far:
['feature_anchors.py']. Check the test lists in test_runner.pyfeature_anchors.py:31:1: W293 blank line contains whitespace
feature_anchors.py:77:25: W292 no newline at end of file
Thanks, @jaonoctus. Fixed!
Concept ACK
56 | + self.log.info("Check anchors.dat file in the node directory") 57 | + assert os.path.exists(os.path.join(self.nodes[0].datadir + '/regtest', 'anchors.dat')) 58 | + 59 | + # It should contains only the block-relay-only addresses 60 | + self.log.info("Check the addresses in anchors.dat") 61 | + for line in open(os.path.join(self.nodes[0].datadir + '/regtest', 'anchors.dat'), 'rb'):
for line in open(os.path.join(self.nodes[0].datadir + '/regtest', 'anchors.dat'), 'rb', encoding='utf8'):
I think it won't work because it's binary mode.
Anyway, all of the linters must pass.
Tested ACK. I'm pretty sure It's a false-flag.
encoding - passes
encoding - fail
I also have checked other tests that are reading binaries. Such as p2p_message_capture.py (line 37) or rpc_dumptxoutset.py (line 40). None have the encoding parameter, which makes sense.
Python's open(...) seems to be used to open text files without explicitly specifying encoding='utf8'
Concept ACK.
52 | + 53 | + self.log.info("Stop node 0") 54 | + self.stop_node(0) 55 | + 56 | + self.log.info("Check anchors.dat file in the node directory") 57 | + assert os.path.exists(os.path.join(self.nodes[0].datadir + '/regtest', 'anchors.dat'))
could make the path a named variable to avoid repeating the join three times?
Yeah, no problem! thanks!
Done!
Approach ACK.
Tested https://github.com/bitcoin/bitcoin/pull/21338/commits/53046529155a7dd9875604059a7134221d176522 on Ubuntu 20.04.
The test has failed intermittently when run several times.
The reason is the file anchors.dat sometimes has 2 lines, as shown below.
A simple solution for this would be concatenate all line before the validations.
2021-03-02T19:52:13.969000Z TestFramework (INFO): Check the addresses in anchors.dat
item (first line): fabfb5da02fc5a030000e1f505000000000000000000000000000000000000ffff7f0000012b6efc5a030000e1f505000000000000000000000000000000000000ffff7f0000012b6d1c665ef1c1d383c4869170e021306a945bdbfb9f39c12bb2e0
ip_port: 7f0000012b6e
ip_port in item: True
ip_port: 7f0000012b6d
ip_port in item: True
item (second line): 279f548ecaf3
ip_port: 7f0000012b6e
ip_port in item: False
2021-03-02T19:52:13.969000Z TestFramework (ERROR): Assertion failed
@leonardojobim Amazing review! I'm working on it at this moment! Thank you!
@leonardojobim fixed! could you please test it again?
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
Tested ACK https://github.com/bitcoin/bitcoin/pull/21338/commits/a83908f9f72ea2979a02ab051e1fc0846d0262af
The tests were run multiple times successfully.
6 | + 7 | +from test_framework.p2p import P2PInterface 8 | +from test_framework.test_framework import BitcoinTestFramework 9 | +from test_framework.util import assert_equal 10 | + 11 | +import os
nit, per PEP8 and test/functional/README.md, this import should be before the framework ones (separated by a blank line)
https://www.python.org/dev/peps/pep-0008/#imports
Imports should be grouped in the following order:
You should put a blank line between each group of imports.
Thanks, just fixed!
24 | + self.setup_nodes() 25 | + 26 | + def run_test(self): 27 | + self.log.info("Add 2 block-relay-only connections to node 0") 28 | + for i in range(2): 29 | + self.log.info(f"block-relay-only: {i}")
self.log.debug(f"block-relay-only: {i}")
Just changed it, it's better debug instead of info for this case.
29 | + self.log.info(f"block-relay-only: {i}") 30 | + self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=i, connection_type="block-relay-only") 31 | + 32 | + self.log.info("Add 5 inbound connections to node 0") 33 | + for i in range(5): 34 | + self.log.info(f"inbound: {i}")
self.debug.info(f"inbound: {i}")
Just changed it, but the right format is self.log.debug as you suggested above I guess.
56 | + node0_anchors_path = os.path.join(self.nodes[0].datadir + '/regtest', 'anchors.dat') 57 | + 58 | + self.log.info("Check anchors.dat file in the node directory") 59 | + assert os.path.exists(node0_anchors_path) 60 | + 61 | + # It should contains only the block-relay-only addresses
# It should contain only the block-relay-only addresses
Concept ACK and some very light review, will review a bit more soon.
The file permission of the new test file should be 755 instead of 644.
-rwxr-xr-x 1 1685 Jan 21 20:54 test/functional/feature_abortnode.py*
-rw-r--r-- 1 2902 Mar 6 13:37 test/functional/feature_anchors.py
-rwxr-xr-x 1 4242 Mar 6 13:10 test/functional/feature_asmap.py*
In order to run the test, I had to first run:
sudo chmod 755 test/functional/feature_anchors.py
Nit: in terms of logging and function naming, I prefer "Test" to "Check that/if"
Don't hesitate to try running Python linters on your functional test changes like pycodestyle and black to see their suggestions.
55 | + 56 | + self.log.info("Stop node 0") 57 | + self.stop_node(0) 58 | + 59 | + node0_anchors_path = os.path.join( 60 | + self.nodes[0].datadir + "/regtest", "anchors.dat"
Hello Bruno. I think we should concatenate regtest sub directory using the os.path.join() function instead of using + "/regtest" (just to avoid assuming the test is running in a Unix based OS).
node0_anchors_path = os.path.join( self.nodes[0].datadir, "regtest", "anchors.dat" )
Good point.
In general, don't hesitate to refer to test/functional/feature_asmap.py for an example of a similar kind of test file.
self.datadir = os.path.join(self.node.datadir, self.chain)
self.default_asmap = os.path.join(self.datadir, DEFAULT_ASMAP_FILENAME)
self.asmap_raw = os.path.join(os.path.dirname(os.path.realpath(__file__)), ASMAP)
Hello, Abel.. Thanks a lot for the review! Just changed it.
Approach ACK.
Tested 0605a3c on Linux Mint 20.
I tried breaking the test without successing with the following:
Executing the new test alone from command line works correctly (test/functional/feature_anchors.py).
Executing all functional tests with test/functional/test_runner.py includes new test:
187/202 - feature_anchors.py passed, Duration: 4 s
TEST | STATUS | DURATION
example_test.py | ✓ Passed | 11 s feature_abortnode.py | ✓ Passed | 34 s feature_anchors.py | ✓ Passed | 4 s . . .
ACK https://github.com/bitcoin/bitcoin/pull/21338/commits/0605a3cbf8ddec39dfbe5c490a872158dc8a6faa, I ran tests OK on Debian 10.8 (buster) and reviewed the code and it looks fine for me.
64 | + assert os.path.exists(node0_anchors_path) 65 | + 66 | + # It should contain only the block-relay-only addresses 67 | + self.log.info("Check the addresses in anchors.dat") 68 | + 69 | + anchors = ""
wouldn't it make more sense for this to be an array?
I don't think so. I am concatenating the lines to verify if the addresses are into this. I don't think an array would be useful here.
Let's assume ip_port is aa and anchors is ['fa', 'af']
With array:
>>> 'aa' in ['fa', 'af']
False
With string
>>> 'aa' in 'faaf'
True
Am I missing something?
I'm using string for the reason you're showing with your example. I think the question about using array or string is: how is anchors.dat structured?
Let's suppose:
ip_port is 7f0000013e01 and the file content is: fabfb5da02fc5a030000e1f505000000000000000000000000000000000000ffff7f0000013e01fc5a030000e1f505000000000000000000000000000000000000ffff7f0000013e0075f7d7cf33e71e42f8add50c305f091fb881b6fa5633932559d86cd9f6141260
but we have:
fabfb5da02fc5a030000e1f505000000000000000000000000000000000000ffff7f00 (breaks line)
00013e01fc5a030000e1f505000000000000000000000000000000000000ffff7f000 (breaks line)
0013e0075f7d7cf33e71e42f8add50c305f091fb881b6fa5633932559d86cd9f6141260 (breaks line)
in this case, my logic using string would be useful. But how is the file structure for this case?
Anyway, almost 100% of my tests showed me that anchors.dat usually has just one line.
anchors.dat is binary data, so why would newlines matter and what is the point of rstrip?
I can remove rstrip I guess, it doesn't affect anything, thanks.
So, can I read only the first line?
I was using array but the test `was failing, assuming:
item (first line): fabfb5da02fc5a030000e1f505000000000000000000000000000000000000ffff7f0000012b6efc5a030000e1f505000000000000000000000000000000000000ffff7f0000012b6d1c665ef1c1d383c4869170e021306a945bdbfb9f39c12bb2e0
ip_port: 7f0000012b6e
ip_port in item: True
ip_port: 7f0000012b6d
ip_port in item: True
item (second line): 279f548ecaf3
ip_port: 7f0000012b6e
ip_port in item: False`
If newlines don't have a meaning, they shouldn't be interpreted. You can just use read()?
Yes, just pushed the changes. It makes sense. Could you please check?
59 | + node0_anchors_path = os.path.join( 60 | + self.nodes[0].datadir, "regtest", "anchors.dat" 61 | + ) 62 | + 63 | + self.log.info("Check anchors.dat file in the node directory") 64 | + assert os.path.exists(node0_anchors_path)
I think this check can be skipped (will be checked by open already)
Make sense.
When the file doesn't exit in the node directory the open function will return a FileNotFoundError.
Thanks!
Done!
Seems fine. Left two comments
Tested ACK. I ran it 100x and they all passed. OS: Archlinux.
Concept ACK 581791c62067403fbeb4e1fd88c1d80549627c94
9 | +from test_framework.p2p import P2PInterface 10 | +from test_framework.test_framework import BitcoinTestFramework 11 | +from test_framework.util import assert_equal 12 | + 13 | + 14 | +def check_node_connections(*, node, num_in, num_out):
This code is used once only. What are benefits of the separated function here?
0 | @@ -0,0 +1,85 @@ 1 | +#!/usr/bin/env python3 2 | +# Copyright (c) 2020 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 Anchors functionality"""
nit:
"""Test block-relay-only anchors functionality"""
23 | + 24 | + def setup_network(self): 25 | + self.setup_nodes() 26 | + 27 | + def run_test(self): 28 | + self.log.info("Add 2 block-relay-only connections to node 0")
nit: here and after s/"node 0"/"node"/ as it is the only node in this test.
Perfect, thanks!
24 | + def setup_network(self): 25 | + self.setup_nodes() 26 | + 27 | + def run_test(self): 28 | + self.log.info("Add 2 block-relay-only connections to node 0") 29 | + for i in range(2):
nit: Numbers of connections, i.e., 2 and 5, could be named variables.
76 | + 77 | + self.log.info("Start node 0") 78 | + self.start_node(0) 79 | + 80 | + self.log.info("When node starts, check if anchors.dat doesn't exist anymore") 81 | + assert not os.path.exists(node0_anchors_path)
The same assertion seems reasonable to place in the test beginning too, no?
Yes, make sense.
ACK 581791c62067403fbeb4e1fd88c1d80549627c94