Skip to content

Conversation

@0xMimir
Copy link
Contributor

@0xMimir 0xMimir commented Feb 5, 2025

Resolves #1055, continuation of #1001

@0xMimir 0xMimir force-pushed the feat/delayed-pubsub-broadcast branch from c464ace to 9e72b92 Compare February 5, 2025 18:09
is_local: is_sender_local,
});
} else {
for (tx, _) in rejected {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@0xMimir this is not correct, because the full diff may have been rejected because of one particular transaction, but here we are reacting to every rejected transaction.

I have to think about it more, but I am unsure about the approach in general. This probably should work on the diff level, not per transaction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To add a bit more context, diffs will only be rejected if one of the transactions is rejected because of one of these reasons:

https://github.com/openmina/openmina/blob/b152745fce013d2cee65f78f9a51ae1e776d0726/ledger/src/transaction_pool.rs#L239

Copy link
Contributor Author

@0xMimir 0xMimir Feb 5, 2025

Choose a reason for hiding this comment

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

I could filter by error if tx error is grounds_for_diff_rejection, reject it, otherwise continue

Copy link
Collaborator

Choose a reason for hiding this comment

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

The "diff" is lost here:

https://github.com/openmina/openmina/blob/develop/p2p/src/network/pubsub/p2p_network_pubsub_reducer.rs#L577-L586

I guess it was implemented that way to make it more compatible with the way the webrtc layer works, but that makes it harder to work at the diff level. It also means that the transactions will be verified one by one instead of batched (in webrtc they are batched [here](https://github.com/openmina/openmina/blob/44ba15084f97b13f360ba4b88145ba07a78e638c/node/src/transaction_pool/candidate/transaction_pool_candidate_reducer.rs#L98, but that will not help the libp2p part).

You can also see here that the action dispatched to verify the transactions also supports multiple transactions (even if the callback always constructs the call using a single transaction):

https://github.com/openmina/openmina/blob/44ba15084f97b13f360ba4b88145ba07a78e638c/node/src/state.rs#L535-L538

I don't know if in practice we receive more than one transaction per diff from libp2p's gossip btw (you could add some logging and run the node for some time to check).

So:

  • P2pChannelsTransactionAction::Libp2pReceived should contain all the transactions, not just one
  • on_p2p_channels_transaction_libp2p_received callback should receive the full list of transactions, not just one. And then dispatch StartVerify with all the transactions at once.
  • StartVerify and the related actions should probably contain the message id (as an Option similar to how it contains a reference to the rpc from where the transaction may have come from if it was sent by the user directly using the API)
  • Then at the end of the sequence when handling ApplyVerifiedDiffWithAccounts you will have access to the actual message id (if any) from which those transactions came from.

Btw those actions probably need to be split in more, but lets leave that for later

Copy link
Collaborator

@tizoc tizoc Feb 5, 2025

Choose a reason for hiding this comment

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

Another idea is that instead of having from_rpc and from_gossip separately (because you cannot have both), there could be a single from_origin that is either FromRpc(RpcId) | FromGossip(MessageId) | Other (or WebRtc instead of Other, I don't think there is another possibility) so that you can react accordingly depending on the origin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One last thing, and this should be left for later. But transactions should first go into the TransactionPoolState::candidates and with TransactionPoolCandidatesState extended to support storing the libp2p received messages grouped by message id. The logic should also be extended to handle libp2p gossip as an origin. This will make it much easier to later fix the issue of us trying to validate the pool before the node is even synced.

With that approach we would still receive and store the gossip messages but wait until we are synced to validate them.

Anyway, this is for later, first thing to solve is what I mentioned in the above comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated logic in pubsub and transaction pool, to use message id instead of transaction hashes, also I have been running node for almost a day now and every time transaction diff, had only one transaction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, thats useful to know (still we need to consider multiple transactions because the protocol allows it)

@0xMimir 0xMimir force-pushed the feat/delayed-pubsub-broadcast branch from 9e72b92 to 0fee577 Compare February 6, 2025 13:49
@0xMimir 0xMimir force-pushed the feat/delayed-pubsub-broadcast branch 2 times, most recently from 84db0dc to 803195e Compare February 6, 2025 16:18
@0xMimir 0xMimir force-pushed the feat/delayed-pubsub-broadcast branch from 803195e to 6d58622 Compare February 6, 2025 16:33
@0xMimir 0xMimir force-pushed the feat/delayed-pubsub-broadcast branch from 6d58622 to 4380861 Compare February 6, 2025 17:48
@0xMimir 0xMimir force-pushed the feat/delayed-pubsub-broadcast branch 8 times, most recently from 3e391dd to b51c7a3 Compare February 6, 2025 19:57
@0xMimir 0xMimir force-pushed the feat/delayed-pubsub-broadcast branch from b51c7a3 to da83a61 Compare February 6, 2025 20:10
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 (left some comments re: comments to add, please add them before merging)

@0xMimir 0xMimir merged commit f2bf64c into o1-labs:develop Feb 7, 2025
30 checks passed
@0xMimir 0xMimir deleted the feat/delayed-pubsub-broadcast branch February 17, 2025 09: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.

As a developer, I want to broadcast snark pool and transactions only after they have been fully validated, to avoid broadcasting invalid ones

2 participants