Skip to content

Conversation

@0xMimir
Copy link
Contributor

@0xMimir 0xMimir commented Dec 18, 2024

Added callback to prevalidate pubsub message before broadcasting them to peers, delayed block broadcast until block is applied.

Previous flow when pubsub message is received:

Message received -> Message Broadcasted -> Message processed

Flow after this PR will be:

Message received -> Message prevalidated -> Message processed -> Message broadcasted 

After block is received in pubsub it gets stored in state, message gets removed from state either by being to old, (older than 10minutes), or by being broadcasted, message cleanup is trigger by dispatching P2pNetworkPubsubAction::PruneMessages which is done from P2pState::p2p_timeout_dispatch

@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch 4 times, most recently from d07c188 to 1fb0caf Compare December 19, 2024 09:10
@0xMimir 0xMimir requested a review from vlad9486 December 19, 2024 13:31
@0xMimir 0xMimir marked this pull request as ready for review December 19, 2024 13:32
@tizoc
Copy link
Collaborator

tizoc commented Dec 19, 2024

@0xMimir when you think this is ready please expand the summary in the PR with mode details about the changes to ease reviewing (like for example what the new flow looks like compared to what it was before, how cleanup works , etc)

@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch 3 times, most recently from 451d363 to 96369ab Compare January 8, 2025 15:34
@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch 4 times, most recently from 9e9b439 to 0c895ba Compare January 13, 2025 12:19
@0xMimir 0xMimir removed the request for review from vlad9486 January 14, 2025 10:03
@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch from 0c895ba to d2b78db Compare January 14, 2025 12:28
@0xMimir 0xMimir requested review from tizoc and vlad9486 January 14, 2025 12:28
Copy link
Collaborator

@tizoc tizoc left a comment

Choose a reason for hiding this comment

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

I left a bunch of comments, some are about issues that are probably easy to solve, but there are two aspects that may need a bit more discussion:

  • When the block message really needs to be re-broadcasted (now it is happening too early)
  • That the block validation callback is not going through the full flow (right now it only considers block pre-validation, not block application, it is only after that point that you really know if it is Valid)

@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch 3 times, most recently from e1ff8bf to 9c419ff Compare January 15, 2025 17:06
@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch from 9c419ff to 43f3d2f Compare January 15, 2025 17:30
@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch 2 times, most recently from 4585b73 to c198521 Compare January 15, 2025 18:33
@0xMimir 0xMimir force-pushed the feat/block-validation-in-pubsub branch from c198521 to ee2186e Compare January 15, 2025 18:44
Copy link
Collaborator

@tizoc tizoc left a comment

Choose a reason for hiding this comment

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

LGTM

@0xMimir not for this PR, but for the future. How hard do you think it would be to write tests for the different situations handled here?

@0xMimir 0xMimir merged commit 7db7078 into o1-labs:develop Jan 16, 2025
30 checks passed
@0xMimir 0xMimir deleted the feat/block-validation-in-pubsub branch January 20, 2025 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants