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

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

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC069441490afd: [avalanche] Add getavalancheproofs RPC to retrieve tracked proof ids

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

sdulfari created this revision.Sep 26 2022, 18:18

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 26 2022, 18:18

sdulfari requested review of this revision.Sep 26 2022, 18:18

sdulfari mentioned this in D12013: [avalanche] Add a way to retrieve proof ids from each pool.Sep 26 2022, 18:22

Harbormaster completed remote builds in B20181: Diff 35192.Sep 26 2022, 18:30

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

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

sdulfari added a child revision: D12064: [avalanche] Cleanup redundant proof count getters in PeerManager API.Sep 26 2022, 18:32

Fix a race condition in the test

Harbormaster completed remote builds in B20185: Diff 35196.Sep 26 2022, 19:02

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.