feat(p2p): no tips store in the p2p client #12395

Maddiaa0 · 2025-03-01T17:25:57Z

Overview

Migrates the p2p client to use the archiver's event emitter that was created in the pr above in this stack.

This removes usage of the L2BlockStream from the p2p client for dealing with state changes, and instead uses events to deal with these situations

Maddiaa0 · 2025-03-01T17:26:12Z

feat(p2p): no tips store in the p2p client #12395 👈 (View in Graphite)
fix(p2p): slashing test + slasher client does not store own state #12380
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

LHerskind

Looks good. Tiny comment around the epochs to avoid a potential race condition.

LHerskind · 2025-03-03T16:45:31Z

yarn-project/sequencer-client/src/slasher/slasher_client.ts

@@ -150,31 +150,18 @@ export class SlasherClient extends WithTracer {
    return EthAddress.fromString(payloadAddress);
  }

-  public handleBlockStreamEvent(event: L2BlockSourceEvent): Promise<void> {


LHerskind · 2025-03-03T16:50:37Z

yarn-project/archiver/src/archiver/archiver.ts

@@ -409,14 +409,15 @@ export class Archiver extends EventEmitter implements ArchiveSource, Traceable {
        );
      }

+      const [localProvenEpochNumber, localProvenBlockNumber] = await Promise.all([
+        this.store.getProvenL2EpochNumber(),


As in #12380 might be better if these depends on each other, and not be pulled as there could be a race condition. e.g., given the block number we should be able to get the slot number and then the epoch number.

epoch can be inferred from block, gotcha

epoch number is entirely not required, here, opening pr to remove

alexghr

Just a question on synching really, otherwise the code looks good and lot simpler than before!

With these changes do we really don't have to ask the P2P client if it's synched up the latest block? 🤔

alexghr · 2025-03-03T17:13:42Z

yarn-project/sequencer-client/src/slasher/slasher_client.ts

  /**
   * Allows consumers to stop the instance of the slasher client.
   * 'ready' will now return 'false' and the running promise that keeps the client synced is interrupted.
   */
  public stop() {
    this.log.debug('Stopping Slasher client...');
-    this.l2BlockSource.removeListener(L2BlockSourceEvents.L2PruneDetected, this.handlePruneL2Blocks.bind(this));
+    this.l2BlockSource.removeListener(L2BlockSourceEvents.ChainPruned, this.handlePruneL2Blocks.bind(this));


I realise this isn't new code (in this PR) but this bind will create a new function instance and won't remove the original.

alexghr · 2025-03-03T17:16:28Z

yarn-project/p2p/src/client/p2p_client.test.ts

  it('deletes txs once block is proven', async () => {
    blockSource.setProvenBlockNumber(0);
    await client.start();
    expect(txPool.deleteTxs).not.toHaveBeenCalled();

-    await advanceToProvenBlock(5);
+    advanceToProvenBlock(5);
+    await sleep(100); // wait for the event to be emitted and handled


I guess this is needed because advanceToProvenBlock is no longer async? Slightly worried this might flake in the future, but probably fine for now 😁

alexghr · 2025-03-03T17:17:16Z

yarn-project/p2p/src/client/p2p_client.ts

+    // Store references to the wrapper functions for later removal
+    this.eventHandlers = {
+      handleChainPruned,
+      handleBlocksAdded,
+      handleChainProven,
+    };


alexghr · 2025-03-03T17:19:42Z

yarn-project/p2p/src/client/p2p_client.ts

-  private async startServiceIfSynched() {
-    if (
-      this.currentState === P2PClientState.SYNCHING &&
-      (await this.getSyncedLatestBlockNum()) >= this.latestBlockNumberAtStart &&
-      (await this.getSyncedProvenBlockNum()) >= this.provenBlockNumberAtStart
-    ) {
-      this.log.debug(`Synched to blocks at start`);
-      this.setCurrentState(P2PClientState.RUNNING);
-      if (this.syncResolve !== undefined) {
-        this.syncResolve();
-        await this.p2pService.start();
-      }
-    }


Is this not needed anymore? My understanding was that this function would block the node from 'starting' until all of its components had synched.

Is there a risk that after a restart a node would have components at different heights because they synched at different speeds from the archiver?

You're completely correct, myself and lasse have just discussed this. I'm currently bringing this logic back!

alexghr · 2025-03-03T17:23:33Z

yarn-project/sequencer-client/src/sequencer/sequencer.ts

      this.l1ToL2MessageSource.getBlockNumber(),
    ] as const);

-    const [worldState, l2BlockSource, p2p, l1ToL2MessageSource] = syncedBlocks;
+    const [worldState, l2BlockSource, l1ToL2MessageSource] = syncedBlocks;


I think we should still ask the P2P node if it's synched up the latest block otherwise the sequencer could start building a block before the P2P client had a chance to react to the BlocksAdded event.

This happened in the ci 👍

This is exactly what happened in the failing prover test!

Maddiaa0 mentioned this pull request Mar 1, 2025

fix(p2p): slashing test + slasher client does not store own state #12380

Merged

Maddiaa0 force-pushed the md/fix-slasher-client branch from 63c9281 to bc0c510 Compare March 1, 2025 17:26

Maddiaa0 force-pushed the md/remove-p2p-state branch from 9e603da to 3190e47 Compare March 1, 2025 17:26

Maddiaa0 changed the title ~~feat(p2p): no store in the p2p client~~ feat(p2p): no tips store in the p2p client Mar 3, 2025

Maddiaa0 force-pushed the md/fix-slasher-client branch from 2d6e4e9 to e64590b Compare March 3, 2025 15:32

Maddiaa0 requested a review from charlielye as a code owner March 3, 2025 15:32

Maddiaa0 force-pushed the md/remove-p2p-state branch from 6008722 to adfec1f Compare March 3, 2025 15:32

Base automatically changed from md/fix-slasher-client to master March 3, 2025 16:03

Maddiaa0 force-pushed the md/remove-p2p-state branch from adfec1f to a75a866 Compare March 3, 2025 16:27

Maddiaa0 requested review from alexghr and LHerskind March 3, 2025 16:29

LHerskind approved these changes Mar 3, 2025

View reviewed changes

alexghr reviewed Mar 3, 2025

View reviewed changes

Maddiaa0 added 4 commits March 3, 2025 19:59

feat(p2p): no store in the p2p client

c32e9df

fix: in memory sync state

2165335

some reordering

9581d47

fmt

1c82e22

Maddiaa0 force-pushed the md/remove-p2p-state branch from ba1798f to 1c82e22 Compare March 3, 2025 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(p2p): no tips store in the p2p client #12395

feat(p2p): no tips store in the p2p client #12395

Maddiaa0 commented Mar 1, 2025 •

edited

Loading

Maddiaa0 commented Mar 1, 2025 •

edited

Loading

LHerskind left a comment

LHerskind Mar 3, 2025

LHerskind Mar 3, 2025

Maddiaa0 Mar 3, 2025

Maddiaa0 Mar 3, 2025

Maddiaa0 Mar 3, 2025

alexghr left a comment

alexghr Mar 3, 2025

Maddiaa0 Mar 3, 2025

alexghr Mar 3, 2025

alexghr Mar 3, 2025

alexghr Mar 3, 2025

Maddiaa0 Mar 3, 2025

alexghr Mar 3, 2025

LHerskind Mar 3, 2025

Maddiaa0 Mar 3, 2025

feat(p2p): no tips store in the p2p client #12395

Are you sure you want to change the base?

feat(p2p): no tips store in the p2p client #12395

Conversation

Maddiaa0 commented Mar 1, 2025 • edited Loading

Overview

Maddiaa0 commented Mar 1, 2025 • edited Loading

LHerskind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexghr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Maddiaa0 commented Mar 1, 2025 •

edited

Loading

Maddiaa0 commented Mar 1, 2025 •

edited

Loading