Set custody subnets on tests #6382
Conversation
ackintosh
added
work-in-progress
| let node_id = peer_id_to_node_id(&peer_id).expect("convert peer_id to node_id"); | ||
| let subnets = DataColumnSubnetId::compute_custody_subnets::<E>( | ||
| node_id.raw(), | ||
| spec.custody_requirement + 4, |
There was a problem hiding this comment.
spec.custody_requirement did not fix the flakiness; the failure where cx.get_custodial_peers(self.column_index) returns only a supernode still occurs. Based on my testing, spec.custody_requirement + 4 (or more) seems to fix it.
There was a problem hiding this comment.
Why not have two supernodes in the retry test + set_custody_subnets in all tests for completeness?
There was a problem hiding this comment.
That makes sense to me💡 Thanks!
There was a problem hiding this comment.
Yeah the random generated peer ids doesn’t guarantee that peers are distributed across all subnets evenly. we need to either depend on more super nodes or have deterministic node ids in the test. In practice, LH will need to continuously maintain peers across all subnets.
ackintosh
added
ready-for-review
jimmygchen
added
ready-for-merge
|
@mergify queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 351dd6c |
Issue Addressed
Closes #6319
Proposed Changes
__add_connected_peer_testing_onlyto set custody subnetssampling_with_retriestest to ensure that the node can retry