Page MenuHomePhabricator

[avalanche] Add proof-poll responses
ClosedPublic

Authored by tyler-smith on Oct 23 2021, 05:08.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC3491900468b7: [avalanche] Add proof-poll responses
Summary

Nodes will need to come to consensus on which proofs are currently active for any given stake, so they will need to handle Avalanche polls about Avalanche stake proofs themselves.

This change adds basic responses for proof polls.

Depends on D10686, D10904.

Test Plan

ninja all check-all

python test/functional/abc_p2p_avalanche_voting_proofs.py

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Squashing down patches into single commit.

Fabien requested changes to this revision.Dec 10 2021, 08:48

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.

Fabien requested changes to this revision.Dec 17 2021, 10:02

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
Fabien requested changes to this revision.Dec 20 2021, 21:55
  • 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.

Remove unnecessary format changes.

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

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.

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?

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.

This revision now requires changes to proceed.Jan 26 2022, 14:09

Re-work locks to reduce their scopes.

Undo unnecessary/ineffectual code re-ordering.

Fabien requested changes to this revision.Feb 2 2022, 18:27

One remaining nit, but apart from that it looks good

src/net_processing.cpp
3350 ↗(On Diff #32130)

That no longer a param

This revision now requires changes to proceed.Feb 2 2022, 18:27

Update function params documentation.

Fabien added inline comments.
test/functional/abc_p2p_avalanche_voting_proofs.py
2 ↗(On Diff #32185)

2022 or 2021, but certainly not 2020

This revision is now accepted and ready to land.Feb 4 2022, 08:44
This revision was automatically updated to reflect the committed changes.