[avalanche] Make sure each proof is added to contender cache before mining the next block in contender voting test
ClosedPublic
Actions

Authored by roqqit on Thu, Dec 12, 22:08.

Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABC0657e8a86eeb: [avalanche] Make sure each proof is added to contender cache before mining the…

Summary

As part of generating proofs, it is possible for the node to mine the next block before adding the proof to the contender cache because mining a block takes a lock on cs_main, for which addStakeContender() also requires a lock. This patch ensures the proof was added to the cache before generating the next proof and mining the block associated with that.

This should fix some flakiness with the contender voting test.

Test Plan

./test/functional/test_runner.py abc_p2p_avalanche_contender_voting

Diff Detail

Repository

rABC Bitcoin ABC

Branch

contender-test-race

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 31714
Build 62924: Build Diff	lint-circular-dependencies · build-clang-tidy · build-debug · build-clang · build-without-wallet · build-diff
Build 62923: arc lint + arc unit

Event Timeline

roqqit created this revision.Thu, Dec 12, 22:08

Owners added a reviewer: Restricted Owners Package.Thu, Dec 12, 22:08

Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Dec 12, 22:08

roqqit requested review of this revision.Thu, Dec 12, 22:08

Harbormaster completed remote builds in B31634: Diff 51564.Thu, Dec 12, 22:21

Fabien requested changes to this revision.Fri, Dec 13, 08:22

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/avalanche/processor.cpp
1001 ↗	(On Diff #51564)	This will bloat the avalanche debug log with no useful info as it's sole purpose is to help you in this test. Please find another way to achieve the same result. Maybe a debug RPC ?

This revision now requires changes to proceed.Fri, Dec 13, 08:22

waiting for len(getavalanchepeerinfo) == QUORUM_NODE_COUNT should be enough here

Use a debug RPC. Waiting for getavalanchepeerinfo is not sufficient here because its possible for this value to be updated before the contender is added to the cache.

Harbormaster completed remote builds in B31690: Diff 51637.Mon, Dec 16, 18:00

Fabien requested changes to this revision.Tue, Dec 17, 10:14

Fabien added inline comments.

src/avalanche/processor.cpp
1003 ↗	(On Diff #51637)	imo you should just return the vote status instead of a bool (that's your function signature btw). It's just more versatile. Or maybe use getStakeContenderStatus which already exists (and will tell you if computation is pending) ?
src/rpc/avalanche.cpp
1807 ↗	(On Diff #51637)	same, getstakecontendervote is just more versatile. Also just take the prevblockhash and the proofid so the rpc computes the stakecontenderid locally. No need to expose the implementation details.