⚙ D10387 [avalanche] Add proof-poll responses

Squashing down patches into single commit.

Harbormaster completed remote builds in B17600: Diff 31346.Dec 10 2021, 03:15

This diff should have a dependency to the diff that creates getAvalancheVoteForBlock. What you need to do is to create a diff for the commit that adds the function, rebase this one on top of it in your local tree, and add a dependency in this diff summary. This can be done by editing the summary and add Depends on Dxxxx to it, where Dxxxx is the diff id of the previous commit.

src/net_processing.cpp
5208 ↗	(On Diff #31346)	You can take a look at the accessors in the peer manager (like isBoundToPeer or isOrphan) to get a more precise status.
src/net_processing.h
236 ↗	(On Diff #31346)	Use the dedicated type, i.e. BlockHash and pass it by const ref
237 ↗	(On Diff #31346)	Same with ProofId
test/functional/abc_p2p_avalanche_voting_proofs.py
39 ↗	(On Diff #31346)	is that really needed ?
51 ↗	(On Diff #31346)	Nit: you don't need that much blocks to be mined since you're only using the first one
73 ↗	(On Diff #31346)	Now I understand your comment regarding the proofid. The correct way to do it is to extract it from the proof you just built by deserializing it into an object that has the proofid as a property: proofobj = FromHex(LegacyAvalancheProof(), proof) # Now you can access the id as an int proofobj.proofid
91 ↗	(On Diff #31346)	Since you're testing answering the polls and not sending them, you don't need the whole quorum. You can also simplify a bit by using `sendavalancheproof` instead of `addavalanchenode`
98 ↗	(On Diff #31346)	The REJECTED case is missing
test/functional/test_framework/avatools.py
211 ↗	(On Diff #31346)	This change is good and can be its own diff

This revision now requires changes to proceed.Dec 10 2021, 08:48

Address non-functional feedback items.

tyler-smith edited the summary of this revision. (Show Details)Dec 15 2021, 15:49

tyler-smith added a parent revision: D10685: [avalanche] Make send_poll test helper accept optional type argument..

Harbormaster completed remote builds in B17649: Diff 31418.Dec 15 2021, 15:58

Fix stacking against D10387.

tyler-smith edited the summary of this revision. (Show Details)Dec 15 2021, 16:08

tyler-smith edited the summary of this revision. (Show Details)Dec 15 2021, 16:38

tyler-smith added a parent revision: D10686: [avalanche] Move block-vote decision into getAvalancheVoteForBlock..

Harbormaster completed remote builds in B17651: Diff 31427.Dec 15 2021, 16:41

This will need a rebase after you fixed the dependencies.

test/functional/abc_p2p_avalanche_voting_proofs.py
35 ↗	(On Diff #31427)	You're not using the second node
49 ↗	(On Diff #31427)	`[blockhashes[0]] == blockhashes`

This revision now requires changes to proceed.Dec 17 2021, 10:02

Rebase onto new version of D10686

Harbormaster completed remote builds in B17674: Diff 31458.Dec 20 2021, 00:21

Please fix the linter issues.
The second node is still not used
The orphan case is missing

test/functional/abc_p2p_avalanche_voting_proofs.py
97 ↗	(On Diff #31458)	You can create a small helper to reduce the boilerplate. Also these proof have something special, and can benefit from better name (e.g. this one is conflicting with proof_0, can be conflicting_proof or something like that).
126 ↗	(On Diff #31458)	This will throw an exception on error, no need for the assert
137 ↗	(On Diff #31458)	You can keep this consistent through the test

This revision now requires changes to proceed.Dec 20 2021, 21:55

Get tests working correctly.

Fix arc lint issues.

Remove unnecessary format changes.

Harbormaster completed remote builds in B17921: Diff 31821.Jan 17 2022, 03:08

Harbormaster completed remote builds in B17919: Diff 31819.Jan 17 2022, 03:12

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: abc_p2p_avalanche_voting_proofs.py ======

------- Stdout: -------
2022-01-17T03:10:52.265000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220117_030546/abc_p2p_avalanche_voting_proofs_185
2022-01-17T03:10:53.007000Z TestFramework (INFO): Trigger polling from the node...
2022-01-17T03:10:53.107000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_avalanche_voting_proofs.py", line 112, in run_test
    AvalancheVote(AvalancheProofVoteResponse.UNKNOWN, proof_4_id)])
  File "/work/test/functional/abc_p2p_avalanche_voting_proofs.py", line 94, in poll_assert_response
    response = ava_node.wait_for_avaresponse()
  File "/work/test/functional/test_framework/avatools.py", line 215, in wait_for_avaresponse
    timeout=timeout)
  File "/work/test/functional/test_framework/p2p.py", line 485, in wait_until
    timeout_factor=self.timeout_factor)
  File "/work/test/functional/test_framework/util.py", line 270, in wait_until_helper
    if predicate():
  File "/work/test/functional/test_framework/p2p.py", line 481, in test_function
    assert self.is_connected
AssertionError
2022-01-17T03:10:53.159000Z TestFramework (INFO): Stopping nodes
------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/test_framework/authproxy.py", line 116, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1069, in _send_output
    self.send(chunk)
  File "/usr/lib/python3.7/http/client.py", line 991, in send
    self.sock.sendall(data)
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/work/test/functional/abc_p2p_avalanche_voting_proofs.py", line 166, in <module>
    AvalancheTest().main()
  File "/work/test/functional/test_framework/test_framework.py", line 152, in main
    exit_code = self.shutdown()
  File "/work/test/functional/test_framework/test_framework.py", line 280, in shutdown
    self.stop_nodes()
  File "/work/test/functional/test_framework/test_framework.py", line 514, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/work/test/functional/test_framework/test_node.py", line 422, in stop_node
    self.stop(wait=wait)
  File "/work/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 161, in __call__
    'POST', self.__url.path, postdata.encode('utf-8'))
  File "/work/test/functional/test_framework/authproxy.py", line 122, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1030, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 970, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 942, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/lib/python3.7/socket.py", line 727, in create_connection
    raise err
  File "/usr/lib/python3.7/socket.py", line 716, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_p2p_avalanche_voting_proofs.py

Harbormaster completed remote builds in B17920: Diff 31820.Jan 17 2022, 03:17

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: abc_p2p_avalanche_voting_proofs.py ======

------- Stdout: -------
2022-01-17T03:16:27.112000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220117_031108/abc_p2p_avalanche_voting_proofs_185
2022-01-17T03:16:27.851000Z TestFramework (INFO): Trigger polling from the node...
2022-01-17T03:16:27.952000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_avalanche_voting_proofs.py", line 112, in run_test
    AvalancheVote(AvalancheProofVoteResponse.UNKNOWN, proof_4_id)])
  File "/work/test/functional/abc_p2p_avalanche_voting_proofs.py", line 94, in poll_assert_response
    response = ava_node.wait_for_avaresponse()
  File "/work/test/functional/test_framework/avatools.py", line 215, in wait_for_avaresponse
    timeout=timeout)
  File "/work/test/functional/test_framework/p2p.py", line 485, in wait_until
    timeout_factor=self.timeout_factor)
  File "/work/test/functional/test_framework/util.py", line 270, in wait_until_helper
    if predicate():
  File "/work/test/functional/test_framework/p2p.py", line 481, in test_function
    assert self.is_connected
AssertionError
2022-01-17T03:16:28.003000Z TestFramework (INFO): Stopping nodes
------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/test_framework/authproxy.py", line 116, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1069, in _send_output
    self.send(chunk)
  File "/usr/lib/python3.7/http/client.py", line 991, in send
    self.sock.sendall(data)
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/work/test/functional/abc_p2p_avalanche_voting_proofs.py", line 166, in <module>
    AvalancheTest().main()
  File "/work/test/functional/test_framework/test_framework.py", line 152, in main
    exit_code = self.shutdown()
  File "/work/test/functional/test_framework/test_framework.py", line 280, in shutdown
    self.stop_nodes()
  File "/work/test/functional/test_framework/test_framework.py", line 514, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/work/test/functional/test_framework/test_node.py", line 422, in stop_node
    self.stop(wait=wait)
  File "/work/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 161, in __call__
    'POST', self.__url.path, postdata.encode('utf-8'))
  File "/work/test/functional/test_framework/authproxy.py", line 122, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1030, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 970, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 942, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/lib/python3.7/socket.py", line 727, in create_connection
    raise err
  File "/usr/lib/python3.7/socket.py", line 716, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_p2p_avalanche_voting_proofs.py

Fabien requested changes to this revision.Jan 17 2022, 09:05

Fabien added inline comments.

src/net_processing.cpp
2942 ↗	(On Diff #31821)	We probably want to reject an orphan, it's currently invalid after all
2947 ↗	(On Diff #31821)	Same here, this should be a rejecting error code
4453 ↗	(On Diff #31821)	So I didn't look at the failure detail, but you are locking cs_peermanager while holding cs_main which is very likely why the debug build is failing (out of order locking is a potential deadlock and the debug build will catch this). You need to release cs_main before locking the peer manager.
test/functional/abc_p2p_avalanche_voting_proofs.py
32 ↗	(On Diff #31821)	The second node is not used. This is the third time I give you this feedback in the same diff, please fix that.
57 ↗	(On Diff #31821)	This will be much clearer to pass the stakes to the helper. This lets you know remove some of the boilerplate and/or use meaningful names for the stakes
146 ↗	(On Diff #31821)	you can use `assert_raises_rpc_error` to check the rpc exception is what is expected
test/functional/test_framework/avatools.py
160 ↗	(On Diff #31821)	You're not returning a serialized proof id
172 ↗	(On Diff #31821)	Not bad per se, but you're not using it in this diff ?
198 ↗	(On Diff #31821)	Dito
228 ↗	(On Diff #31821)	This can be its own diff, there are more use cases for this helper

This revision now requires changes to proceed.Jan 17 2022, 09:05

Fix latest feedback items. Fix failures due to lock checks by taking all required locks in once place.

Harbormaster completed remote builds in B18022: Diff 31992.Jan 26 2022, 11:29

tyler-smith edited the summary of this revision. (Show Details)Jan 26 2022, 11:30

tyler-smith added a parent revision: D10904: [avalanche] Add send_proof test helper..

tyler-smith added inline comments.Jan 26 2022, 11:32

src/net_processing.cpp
2550 ↗	(On Diff #31992)	@Fabien Any insight into why this lint check started failing even though I didn't touch this part?

Fabien added inline comments.Jan 26 2022, 12:34

src/net_processing.cpp
2550 ↗	(On Diff #31992)	I have the same issue and bissected it, but don't know wtf cppcheck is doing. I'll work around it

Fabien requested changes to this revision.Jan 26 2022, 14:09

Fabien added inline comments.

src/net_processing.cpp
4743 ↗	(On Diff #31992)	This is increasing the scope of 3 locks instead of reducing the scope of a single one. You should rather try to reduce the scope of cs_main to what's really needed. It can even be its own diff.