Page MenuHomePhabricator

[avalanche] Cleanup unrequested radix tree after a timeout
ClosedPublic

Authored by Fabien on Jun 10 2022, 13:30.

Details

Summary

This ensures that the radix tree don't add up too much if the compact proofs are never requested.
The cleanup is done in the same thread than the periodic getavaaddr requests in order to reuse some of the code. The thread will later also be extended with getavaproofs requests so this is renamed appropriately.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Jun 10 2022, 13:30
Fabien planned changes to this revision.Jun 10 2022, 13:34

Failed tests logs:

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

------- Stdout: -------
2022-06-10T13:47:48.508000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220610_134352/abc_p2p_compactproofs_208
2022-06-10T13:47:49.414000Z TestFramework (INFO): Check we send a getavaproofs message to our avalanche outbound peers
2022-06-10T13:47:50.956000Z TestFramework (INFO): Check we send a getavaproofs message to our manually connected peers that support avalanche
2022-06-10T13:47:52.762000Z TestFramework (INFO): Check the node responds to getavaproofs messages
2022-06-10T13:47:54.754000Z TestFramework (INFO): Check the node requests the missing proofs after receiving an avaproofs message
2022-06-10T13:47:55.669000Z TestFramework (INFO): The node ignores unsollicited avaproofs
2022-06-10T13:47:55.780000Z TestFramework (INFO): Check no proof is requested if there is no shortid
2022-06-10T13:47:55.934000Z TestFramework (INFO): Check the node requests all the proofs if it known none
2022-06-10T13:47:56.086000Z TestFramework (INFO): Check the node requests only the missing proofs
2022-06-10T13:47:56.240000Z TestFramework (INFO): Check the node don't request prefilled proofs
2022-06-10T13:47:56.394000Z TestFramework (INFO): Check the node requests no proof if it knows all of them
2022-06-10T13:47:56.564000Z TestFramework (INFO): Check out of bounds index
2022-06-10T13:47:56.789000Z TestFramework (INFO): An invalid prefilled proof will trigger a ban
2022-06-10T13:47:57.010000Z TestFramework (INFO): Check the node respond to missing proofs requests
2022-06-10T13:47:57.563000Z TestFramework (INFO): Unsollicited requests are ignored
2022-06-10T13:47:57.769000Z TestFramework (INFO): Sending an empty request has no effect
2022-06-10T13:47:57.821000Z TestFramework (INFO): Check the requested proofs are sent by the node
2022-06-10T13:47:59.277000Z TestFramework (INFO): Check the node will not send the proofs if not requested before the timeout elapsed
2022-06-10T13:47:59.542000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 136, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_compactproofs.py", line 478, in run_test
    self.test_send_missing_proofs()
  File "/work/test/functional/abc_p2p_compactproofs.py", line 443, in test_send_missing_proofs
    assert_equal(len(slow_peer.get_proofs()), 0)
  File "/work/test/functional/test_framework/util.py", line 60, in assert_equal
    for arg in (thing1, thing2) + args)))
AssertionError: not(10 == 0)
2022-06-10T13:47:59.596000Z TestFramework (INFO): Stopping nodes
2022-06-10T13:47:59.751000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220610_134352/abc_p2p_compactproofs_208
2022-06-10T13:47:59.751000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220610_134352/abc_p2p_compactproofs_208/test_framework.log
2022-06-10T13:47:59.751000Z TestFramework (ERROR): 
2022-06-10T13:47:59.752000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-diff/test/tmp/test_runner_₿₵_  _20220610_134352/abc_p2p_compactproofs_208' to consolidate all logs
2022-06-10T13:47:59.752000Z TestFramework (ERROR): 
2022-06-10T13:47:59.752000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-06-10T13:47:59.752000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2022-06-10T13:47:59.753000Z TestFramework (ERROR):

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

Fabien planned changes to this revision.Jun 10 2022, 13:55

Fix intermittent failure. The scheduler doesn't run immediately so we need to wait for it.
A debug message is used for that purpose.

This revision is now accepted and ready to land.Jun 10 2022, 17:37
sdulfari requested changes to this revision.Jun 10 2022, 18:16
sdulfari added inline comments.
src/net_processing.cpp
1707 ↗(On Diff #33966)

Shouldn't the g_avalanche check encompass this whole function? Or better: just short circuit at the beginning of the function if !g_avalanche

This revision now requires changes to proceed.Jun 10 2022, 18:16
Fabien requested review of this revision.Jun 10 2022, 19:18
Fabien added inline comments.
src/net_processing.cpp
1707 ↗(On Diff #33966)

The scheduler is created before g_avalanche is initialized so you don't want to bypass the whole function, but re-schedule. Another solution would be a lambda or a goto but this is not really making the readability much better imo.

src/net_processing.cpp
1707 ↗(On Diff #33966)

Ok, that part makes sense. But not checking for g_avalanche before doing the above avalanche logic looks broken to me.

Jump as soon as possible to the re-schedule

This revision is now accepted and ready to land.Jun 10 2022, 20:39