fuel-sync design

[xgreenx]

Rendered

I described the implementation, assuming that we can't change the block producer. Or we know the block producer at the height X because Ethereum blocks are synced, and we can get information about the producer.

The responsibility from whom and how requests the blocks on the P2P side. But it also is possible to move this responsibility into FS, but it requires corresponding modification of traits.

[vlopes11]

looks solid overall; couple of comments

[vlopes11]

Shouldn't we require async/await only for operations that are I/O? It might be overkill to have everything async, unless ofc everything is I/O

[xgreenx]

Yes, I meant that we could call methods directly. I will rename it to "sync/async".

[vlopes11]

This is ofc desirable, but then we need concrete types to be sent as message (via channels?). We don't know about the internals of the service, but then we need to know about the messages the service is ready to accept.

Or are we going to have a broker in the middle that will do that for us?

[xgreenx]

I didn't think about this rule a lot because the usage of channels makes the code more asynchronous but less clear. Also, most services can work synchronously, and channels make the development harder. For example current implementation of the TX pool use channels. To get the list of transactions, we need to write a lot of boilerplate code using async where we don't need it.

[vlopes11]

nit: these channel types must be owned (here its a semi-slice, and not a vec or array)

[xgreenx]

Yes, it is only pseudocode representing the idea of what should be received from the channel.

After implementation, we will use more concrete types.

[vlopes11]

Shouldn't we have a mechanism to enforce the data to identify who is reporting? (ie AsRef<SomeKey>)

[xgreenx]

I think it is not essential "which" services report it. More critical "about what" we want to report. If we report about blocks, it can have one score system, if about transaction -> another.

Also, we don't need to know about other services, we only need to know about the data(P2P already knows about data).

[vlopes11]

Shouldn't we return the sender as owned? I know the user can clone it himself, but I think its always going to be used as owned

[xgreenx]

Returning the reference may be helpful if you want to check something but don't subscribe. Like is_closed or something else. On the interface level, we don't need to force the user to actually subscribe=) If he needs it, he will clone.

[bvrooman]

A pattern we use now for owned senders is a subscribe method. We could have both an owned for creating a subscription and unowned version for read-only inquiries.

[xgreenx]

We can have both methods, but I want to keep the interface as minimal as possible=) sender adds flexibility, but I don't like naming

[Voxelot]

Wouldn't this be a Receiver? Sender implies that anyone who obtains this could send a broadcast, instead of just listening for broadcasts.

[xgreenx]

You are right, the idea was to use Receiver to be able only to receive information.

I thought Sender.send requires mutability, but it does not. So yea, then better to name the method subscribe and return Receiver<...>

[vlopes11]

Maybe we rename it? Ping is traditionally a request with a response (and the response is the indicator that everything is fine). In this case, we are not expecting a response, so we could instead call it heartbeat

AFAIU we are expecting the service to take some action in some channel. Do we have a way to enforce that in the signature? Otherwise, a service can implement this trait, and optionally implement it correctly/the intended way

[xgreenx]

Actually, maybe better to remove ping at all and add this request to fetch.

I didn't get the second part=(

[vlopes11]

hmm this one is a hard question. I'm not sure we can passively receive forks without being the case of a punishable action. Lets open a thread to discuss this one?

[xgreenx]

I guess you are talking about "Yes -> Ignore(Maybe report some neutral error to P2P to prevent DDoS)".

Yes, we need to punish. Actually, it should be solved on the P2P level. Because P2P should punish nodes that send not requested information.

Each node can report about new height one time, each next time the reputation should decrease(if we didn't request it).

[Voxelot]

I think we could generalize the connection point between FS and CM regardless of whether we are in PoS mode or not.

The benefit of this approach is that the synchronizer isn't required to contain business logic about how to validate the sealed block consensus metadata, and just defers that to whichever configuration the CM is running in (PoA or PoS).

[xgreenx]

We can=) I see CM as descending service for FS in this case. So FS will execute something like the check(blocks: &[Sealed<Block>]) -> bool method and after deciding what to do with blocks(reject or commit).

[Voxelot]

So it should be decided individually.

I think this is the correct approach, and focuses on using the right tool for each job instead of complicating simple APIs like the txpool with channels just for the sake of consistency.

[Voxelot]

This may be a good usecase for FuturesUnordered, which allows joining on a set of parallel tasks.

The consumer of the API would have the option of using poll_next if they want to process the data immediately as it becomes available instead of having to wait for all the futures to complete.

[Voxelot]

I think I'd prefer using a concrete enum over all gossipable data types instead of a generic param. This makes it very clear what kind of data is actually supported for gossip, and also gives us more compiler guarantees that gossiping is implemented across all of our types via exhaustive matching.

[leviathanbeak]

I don't see how a trait solves this problem, Gossipsub Messages are already clearly defined in fuel-p2p, the issue #595 that is referenced is more when there is a "valid" type, like a Transaction, but it's invalid in terms of its state on the chain itself.

[xgreenx]

We have several validity checks:

• Validate that entity can be properly decoded from the bytes. It should be done on the P2P side.
• Validate that transactions are valid in business logic(all input arguments are unique, all output are valid, use CheckedTransaction). It is not P2P responsibility. It seems part of the TXPool.
• Validate that transition already in the TXPool(we don't need to gossip the same transaction). This can be done only by TXPool.

[Voxelot]

Validate that transition already in the TXPool(we don't need to gossip the same transaction). This can be done only by TXPool.

It's debatable whether compact blocks over p2p are really useful. In practice, they can slow down consensus if nodes have to query peers for txs they are missing in their txpool. I think we should assume a broadcast of full blocks. Full blocks are also required for syncing old blocks, as the txs will no longer exist in anyone's txpool.

[xgreenx]

Why should nodes query transactions that missing in node pool?

[Voxelot]

Ah wait, I misunderstood validating if a transaction already in txpool would be used for compact blocks. Re-read and realized it's just to prevent re-gossiping txs we've already received.

[Voxelot]

We can defer ethereum height checking to the BI, to consolidate this check between CM->BI and FS->BI.

If we are behind on the ethereum height, the BI should decide whether to wait on a better height or reject the block.

[xgreenx]

But it means that we should check the consensus details when the ethereum block is available. This check should be done by CM then. CM can wait for a new height from the relayer

[Voxelot]

Ah good point! Maybe we should check it in a few places then, since it'll up to the CM to decide what details from ethereum it needs (or not). Whereas the block importer needs to provide some guarantees that the messages included in a block are going to exist before passing the block to the executor.

[xgreenx]

Then it's decided=) SealValidator::valdiate is async. If there is no Ethereum block, then CM waits for it(In the PoA, it is the default behavior for all block headers, in the PoS, we need to think about cases with fake height). When BI will ask the relayer about the information, it will already be available(because CM awaited it). BlocksCommitter::commit also is async.

With this approach, we have a problem: we can have many async pending requests, and each consumes a memory. Maybe we need to limit this somehow.

[Voxelot]

The first place we can restrict this is via the active set size, since that's equivalent to the buffer size of blocks we're trying to sync.

The other way to restrict this is to validate blocks with the CM and commit to BI one at a time. This assumes that the CM doesn't wait for the eth height until it's already verified things like the signature first to avoid this becoming an attack vector.

[xgreenx]

Could you elaborate more on the second solution, please? I didn't get it=(

The CM can't verify the signature(that it is done by the correct signer) because it doesn't know who the signer is(Eth is not synced enough).

[Voxelot]

Ah true, I suppose we could use some heuristic where we check if our relayer node is synced and the da_height is greater than some margin to reject. This way we add some tolerance in case our relayer's eth node claims to be synced when it really isn't.

For example:

flowchart
    a["if block.da_height > relayer.da_height"] -- no --> b["check signature"]
    b -- valid --> s["success"]
    b -- invalid --> f["reject"]
    a -- yes --> c["wait for relayer to sync"]
    c --> d["is block.da_height > relayer.da_height + 10(adjustable margin)"]
    d -- yes --> f
    d -- no --> a
    

[xgreenx]

Yep, added the final description of what to do in the case when the Relayer is behind.

[Voxelot]

Redo to match the current state of sync without reputation. Add reputation in a follow-up.