Skip to content

fix: discv5 test failure #13653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Maddiaa0 merged 2 commits into from
Apr 17, 2025
Merged

fix: discv5 test failure #13653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4909aaa
fix: discv5 flake
Maddiaa0 Apr 17, 2025
a6b88ef
fix: debugging info
Maddiaa0 Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion yarn-project/p2p/src/services/discv5/discV5_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export class DiscV5Service extends EventEmitter implements PeerDiscoveryService
) {
super();
const { p2pIp, p2pPort, p2pBroadcastPort, bootstrapNodes, trustedPeers, privatePeers } = config;

this.bootstrapNodeEnrs = bootstrapNodes.map(x => ENR.decodeTxt(x));
const privatePeerEnrs = new Set(privatePeers);
this.trustedPeerEnrs = trustedPeers.filter(x => !privatePeerEnrs.has(x)).map(x => ENR.decodeTxt(x));
Expand All @@ -63,6 +64,7 @@ export class DiscV5Service extends EventEmitter implements PeerDiscoveryService

// If no overridden broadcast port is provided, use the p2p port as the broadcast port
if (!p2pBroadcastPort) {
this.logger.warn('No p2pBroadcastPort provided, using p2pPort as broadcast port');
config.p2pBroadcastPort = p2pPort;
}

Expand Down Expand Up @@ -204,7 +206,7 @@ export class DiscV5Service extends EventEmitter implements PeerDiscoveryService
}
}

public getAllPeers(): ENR[] {
public getKadValues(): ENR[] {
return this.discv5.kadValues();
}

Expand Down
51 changes: 26 additions & 25 deletions yarn-project/p2p/src/services/discv5/discv5_service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const waitForPeers = (node: DiscV5Service, expectedCount: number, timeout = 15_0
}, timeout);

node.on('peer:discovered', () => {
if (node.getAllPeers().length >= expectedCount) {
if (node.getKadValues().length >= expectedCount) {
clearTimeout(timeoutId);
resolve();
}
Expand All @@ -38,7 +38,7 @@ describe('Discv5Service', () => {
let bootNodePeerId: PeerId;
let basePort = 7890;

const baseConfig: BootnodeConfig = {
const bootnodeConfig: BootnodeConfig = {
p2pIp: '127.0.0.1',
p2pPort: basePort + 100,
listenAddress: '127.0.0.1',
Expand All @@ -56,7 +56,7 @@ describe('Discv5Service', () => {
const telemetryClient = getTelemetryClient();
store = await openTmpStore('test');
bootNode = new BootstrapNode(store, telemetryClient);
await bootNode.start(baseConfig);
await bootNode.start(bootnodeConfig);
bootNodePeerId = bootNode.getPeerId();
});

Expand All @@ -68,15 +68,15 @@ describe('Discv5Service', () => {
const startNodes = (...nodes: { start: () => Promise<void> }[]) => Promise.all(nodes.map(node => node.start()));
const stopNodes = (...nodes: { stop: () => Promise<void> }[]) => Promise.all(nodes.map(node => node.stop()));
const getPeers = (node: DiscV5Service) =>
Promise.all(node.getAllPeers().map(async peer => (await peer.peerId()).toString()));
Promise.all(node.getKadValues().map(async peer => (await peer.peerId()).toString()));

it('should initialize with default values', async () => {
const node = await createNode();
expect(node.getStatus()).toEqual(PeerDiscoveryState.STOPPED); // not started yet
await node.start();
expect(node.getStatus()).toEqual(PeerDiscoveryState.RUNNING);
const peers = node.getAllPeers();
const bootnode = peers[0];
const kadValues = node.getKadValues();
const bootnode = kadValues[0];
expect((await bootnode.peerId()).toString()).toEqual(bootNodePeerId.toString());
await node.stop();
});
Expand All @@ -96,8 +96,8 @@ describe('Discv5Service', () => {
await startNodes(node1, node2);

// nodes should be connected to boostrap
expect(node1.getAllPeers()).toHaveLength(1);
expect(node2.getAllPeers()).toHaveLength(1);
expect(node1.getKadValues()).toHaveLength(1);
expect(node2.getKadValues()).toHaveLength(1);

await Promise.all([
waitForPeers(node2, 2),
Expand Down Expand Up @@ -170,8 +170,8 @@ describe('Discv5Service', () => {
const node1 = await createNode({ l1ChainId: 13, bootstrapNodeEnrVersionCheck: true });
const node2 = await createNode({ l1ChainId: 14, bootstrapNodeEnrVersionCheck: false });
await startNodes(node1, node2);
expect(node1.getAllPeers()).toHaveLength(0);
expect(node2.getAllPeers()).toHaveLength(1);
expect(node1.getKadValues()).toHaveLength(0);
expect(node2.getKadValues()).toHaveLength(1);
await stopNodes(node1, node2);
});

Expand Down Expand Up @@ -226,7 +226,7 @@ describe('Discv5Service', () => {
await node2.start();
await waitForPeers(node2, 1);

const node2Peers = await Promise.all(node2.getAllPeers().map(async peer => (await peer.peerId()).toString()));
const node2Peers = await Promise.all(node2.getKadValues().map(async peer => (await peer.peerId()).toString()));
// NOTE: bootnode seems to still be present in list of peers sometimes, will investigate
// expect(node2Peers).toHaveLength(1);
expect(node2Peers).toContain(node1.getPeerId().toString());
Expand Down Expand Up @@ -257,12 +257,12 @@ describe('Discv5Service', () => {

await startNodes(node1, node2, node3, trustedNode);

expect(node1.getAllPeers()).toHaveLength(0);
expect(trustedNode.getAllPeers()).toHaveLength(0);
expect(node1.getKadValues()).toHaveLength(0);
expect(trustedNode.getKadValues()).toHaveLength(0);

// Verify node2 and node3 are connected to the trusted peer
expect(node2.getAllPeers().length).toBe(1);
expect(node3.getAllPeers().length).toBe(1);
expect(node2.getKadValues().length).toBe(1);
expect(node3.getKadValues().length).toBe(1);
expect(await getPeers(node2)).toContain(trustedNode.getPeerId().toString());
expect(await getPeers(node3)).toContain(trustedNode.getPeerId().toString());

Expand All @@ -281,7 +281,7 @@ describe('Discv5Service', () => {
})(),
]);

expect(node1.getAllPeers()).toHaveLength(0);
expect(node1.getKadValues()).toHaveLength(0);

// Verify node2 and node3 discovered each other through the trusted peer
const node2Peers = await getPeers(node2);
Expand Down Expand Up @@ -320,10 +320,10 @@ describe('Discv5Service', () => {

await startNodes(node1, node2, node3, privateNode);

expect(node1.getAllPeers()).toHaveLength(0);
expect(node2.getAllPeers()).toHaveLength(0);
expect(node3.getAllPeers()).toHaveLength(0);
expect(privateNode.getAllPeers()).toHaveLength(0);
expect(node1.getKadValues()).toHaveLength(0);
expect(node2.getKadValues()).toHaveLength(0);
expect(node3.getKadValues()).toHaveLength(0);
expect(privateNode.getKadValues()).toHaveLength(0);

await sleep(2000); // wait for peer discovery to be able to start
for (let i = 0; i < 3; i++) {
Expand All @@ -334,10 +334,10 @@ describe('Discv5Service', () => {
await sleep(100);
}

expect(node1.getAllPeers()).toHaveLength(0);
expect(node2.getAllPeers()).toHaveLength(0);
expect(node3.getAllPeers()).toHaveLength(0);
expect(privateNode.getAllPeers()).toHaveLength(0);
expect(node1.getKadValues()).toHaveLength(0);
expect(node2.getKadValues()).toHaveLength(0);
expect(node3.getKadValues()).toHaveLength(0);
expect(privateNode.getKadValues()).toHaveLength(0);

await stopNodes(node1, node2, node3, privateNode);
}, 30_000);
Expand All @@ -348,8 +348,9 @@ describe('Discv5Service', () => {
const peerId = await createSecp256k1PeerId();
const config: P2PConfig = {
...getP2PDefaultConfig(),
...baseConfig,
...emptyChainConfig,

@Maddiaa0 Maddiaa0 Apr 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the culprit.

...baseConfig was passed to the bootstrap node on line 59. So it was being mutated in between tests.

@LHerskind LHerskind Apr 17, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fatality.

p2pIp: `127.0.0.1`,
listenAddress: `127.0.0.1`,
p2pPort: port,
bootstrapNodes: useBootnode ? [bootnodeAddr] : [],
blockCheckIntervalMS: 50,
Expand Down
4 changes: 2 additions & 2 deletions yarn-project/p2p/src/services/dummy_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@ export class DummyPeerDiscoveryService extends EventEmitter implements PeerDisco
}
/**
* Called to discover peers in the network.
* @returns An array of discovered peer addresses.
* @returns An array of Enrs.
*/
public getAllPeers() {
public getKadValues() {
return [];
}

Expand Down
6 changes: 3 additions & 3 deletions yarn-project/p2p/src/services/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,10 @@ export interface PeerDiscoveryService extends EventEmitter {
stop(): Promise<void>;

/**
* Gets all peers.
* @returns An array of peer ENRs.
* Gets all KadValues.
* @returns An array of ENRs.
*/
getAllPeers(): ENR[];
getKadValues(): ENR[];

/**
* Runs findRandomNode query.
Expand Down