Page MenuHomePhabricator

[avalanche] Add getavalancheproofs RPC to retrieve tracked proof ids
ClosedPublic

Authored by sdulfari on Sep 26 2022, 18:18.

Details

Summary

Useful for debugging and for node operators during setup and evaluating avalanche node health.
Adding this RPC removes the time consuming step of digging through the logs for proofids in
order to evalutate their statuses (valid, conflicting, immature).

Test Plan
ninja
./test/functional/test_runner.py abc_rpc_getavalancheproofs

./bitcoind -daemon -testnet
./bitcoin-cli -testnet getavalancheinfo

Diff Detail

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

Event Timeline

Failed tests logs:

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

------- Stdout: -------
2022-09-26T18:29:33.778000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220926_182323/abc_rpc_getavalancheproofs_204
2022-09-26T18:29:34.367000Z TestFramework (INFO): The test node has no proof
2022-09-26T18:29:34.368000Z TestFramework (INFO): The test node has a proof
2022-09-26T18:29:35.028000Z 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_rpc_getavalancheproofs.py", line 80, in run_test
    "immature": [],
AssertionError
2022-09-26T18:29:35.078000Z TestFramework (INFO): Stopping nodes
2022-09-26T18:29:35.279000Z TestFramework (WARNING): Not cleaning up dir /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220926_182323/abc_rpc_getavalancheproofs_204
2022-09-26T18:29:35.279000Z TestFramework (ERROR): Test failed. Test logging available at /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220926_182323/abc_rpc_getavalancheproofs_204/test_framework.log
2022-09-26T18:29:35.280000Z TestFramework (ERROR): 
2022-09-26T18:29:35.280000Z TestFramework (ERROR): Hint: Call /work/test/functional/combine_logs.py '/work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220926_182323/abc_rpc_getavalancheproofs_204' to consolidate all logs
2022-09-26T18:29:35.280000Z TestFramework (ERROR): 
2022-09-26T18:29:35.280000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-09-26T18:29:35.280000Z TestFramework (ERROR): https://github.com/Bitcoin-ABC/bitcoin-abc/issues
2022-09-26T18:29:35.280000Z TestFramework (ERROR):

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

Fix a race condition in the test

Fabien requested changes to this revision.Sep 27 2022, 08:56
Fabien added a subscriber: Fabien.
Fabien added inline comments.
doc/release-notes.md
16 ↗(On Diff #35196)

Needs rebase

src/rpc/avalanche.cpp
956 ↗(On Diff #35196)

Unrelated but we need to factor this, it's duplicated over every RPC

961–980 ↗(On Diff #35196)
1316 ↗(On Diff #35196)

getavalancheproofids ?

test/functional/abc_rpc_getavalancheproofs.py
63 ↗(On Diff #35196)

not needed

136 ↗(On Diff #35196)

You might want to assert that ready_to_poll is true at this point

172 ↗(On Diff #35196)

This is not very useful since you already have the status via the RPC, asserting the logs should be a last resort rather than an expectation

180 ↗(On Diff #35196)

Suggestion: mine a block and check the immature goes into valid.

This revision now requires changes to proceed.Sep 27 2022, 08:56

Will work on the remaining feedback.

src/rpc/avalanche.cpp
956 ↗(On Diff #35196)

Agreed. I briefly looked into this but discovered that some RPCs need more attention than simply refactoring this check. Some would benefit from checking that avalanche is actually enabled, not just initialized. I didn't have time to dig into details yet.

1316 ↗(On Diff #35196)

I considered this. The RPC naming is a little strange since it does not match the code 1 to 1. To fetch an object, it's always getraw<object> and RPCs that return ids do not have that in the name. The only benefit I can see from this is getavalancheproofs could later have a verbose flag to return full objects and the name would still make sense. This will not make sense with getavalancheproofids.

Address feedback and make new ProofPool getters const

Fabien added inline comments.
doc/release-notes.md
10 ↗(On Diff #35224)

Make a full sentence:
Add a new ...

This revision is now accepted and ready to land.Sep 27 2022, 18:21