ethereum / devp2p

Ethereum peer-to-peer networking specifications
979 stars 275 forks source link

caps/wit.md: Ethereum Witness Protocol v0 #167

Closed carver closed 3 years ago

carver commented 3 years ago

This simple budding protocol has been successfully tested among a couple cooperating clients. It is tremendously useful for Beam Sync. Let me know if there's anything I can do to help get it approved & merged!

Large chunks were shamelessly stolen from the snap protocol PR, so thanks to Peter for that.

pipermerriam commented 3 years ago

Love seeing this move forward. One thought (which I've already brought up privately with carver) is that this protocol might be best modeled similar to how the transaction pool messages work, with both a gossip, and request/response component.

Reasoning here is that a client which is beam syncing will have some amount of lag behind the head of the chain and we don't want to have to do things like polling a connected peer until they have a witness available. By having an explicit advertisement mechanism we can eliminate un-necessary requests that would require empty responses.

gsalgado commented 3 years ago

AdvertiseWitnessHashes -> gossip message to advertise what witness hashes you have available

Its name seems to imply the payload would contain the witnesses themselves, but I guess what we really need to know is the block hashes for which a given peer has witnesses, no? Maybe that's what you meant, but in that case I think a name like AdvertiseWitnessesForBlock would be better

tkstanczak commented 3 years ago

AdvertiseWitnessHashes -> gossip message to advertise what witness hashes you have available

Its name seems to imply the payload would contain the witnesses themselves, but I guess what we really need to know is the block hashes for which a given peer has witnesses, no? Maybe that's what you meant, but in that case I think a name like AdvertiseWitnessesForBlock would be better

I think it should be pull based. GetWitnessedBlocks() -> return which of the last 256 hashes the client has WitnessedBlocks()

tkstanczak commented 3 years ago

AdvertiseWitnessHashes -> gossip message to advertise what witness hashes you have available

Its name seems to imply the payload would contain the witnesses themselves, but I guess what we really need to know is the block hashes for which a given peer has witnesses, no? Maybe that's what you meant, but in that case I think a name like AdvertiseWitnessesForBlock would be better

I think it should be pull based. GetWitnessedBlocks() -> return which of the last 256 hashes the client has WitnessedBlocks()

eth65 changes the very inefficient behaviour of transaction broadcasting -> let us not repeat it also NewBlock NewBlockHint are methods to listen to.

gsalgado commented 3 years ago

GetWitnessedBlocks() -> return which of the last 256 hashes the client has

That seems wasteful to me as nodes will have to send GetWitnessedBlocks() requests quite frequently to avoid sending GetBlockWitnessHashes() to peers that don't have witnesses for a certain block, and each WitnessedBlocks() response will contain mostly the same data sent in a previous WitnessedBlocks(). Whereas with the gossip approach, every msg would (AIUI, but @pipermerriam can confirm?) contain only one block hash

pipermerriam commented 3 years ago

@tkstanczak

I'm not entirely following your two comments. They seem to propose different things.

Re-proposing what I proposed above with (hopefully) clearer language.

  1. AdvertiseBlockWitnesses: List of block hashes that the client has witnesses available for. Clients would keep track of what hashes they've already sent and only include hashes that have not already been advertised.
  2. GetBlockWitnesses: request witnesses by their block hash. A client can ensure that they only issue requests for witnesses that have been advertised ensuring that the majority of requests can be served.
  3. BlockWitnesses: response message, including the requested witnesses.

IIUC this should satisfy both of @tkstanczak concerns. 1) The actual transmission of the witnesses is pull based. 2) Leveraging the efficiency that eth65 applied to transaction and block gossip.

Any objections?

gsalgado commented 3 years ago

@pipermerriam

In discussions with @carver we agreed that we can only store witness hashes for recent blocks, and since AdvertiseBlockWitnesses will only incrementally advertise new block hashes, there's a chance peers will make witness requests that we can no longer fulfill. I can think of two alternatives to avoid that:

Thoughts?

tkstanczak commented 3 years ago

I think that there are two roles really: full nodes that always can be expected to have witnesses and beam syncing nodes that have very inconsistent witnesses

tkstanczak commented 3 years ago

FullNode that participtes in wit protocol should have the witnesses for the last FULL_NODE_HISTORY_SIZE blocks (I would say 1024) BeamSync node that participates in wit protocol may have some of the witnesses (for example witnesses for blocks 800, 805, 806, 808) <- with gaps. Also during reorganizations both full nodes and beam sync nodes may have non-canonical witnesses.

I think in Nethermind we can advertise witnesses but we will probably ignore tracking what others advetise and will just keep asking other nodes about latest witnesses that we need (I think this strategy will be better for the synchronizing nodes game-theoretically).

gsalgado commented 3 years ago

@carver It just occurred to me we should probably introduce a Status msg, including the list of block hashes for which the node has witness hashes

tkstanczak commented 3 years ago

@carver It just occurred to me we should probably introduce a Status msg, including the list of block hashes for which the node has witness hashes

Do we expect the beam sync to ever sync historical blocks?

tkstanczak commented 3 years ago

I see it this way (BN, FN -> BeamNode, FullNode)

Scenario with status:

BN->FN Status (empty) FN->BN Status (full 256 blocks) BN->FN requests witness for the latest block only or does not issue any request FN->BN responds with witness

Scenario without status:

BN->FN requests witness for the latest block only FN->BN responds with witness or empty if missing

Second scenario is less verbose.

tkstanczak commented 3 years ago

Also I believe that BN should simply always send the GetBLockWitnessHashes to all the wit/0 nodes that send the NewBlockMessage to it.

tkstanczak commented 3 years ago

I would focus now on trying to get the blocks synced with this protocol and check how it behaves. This should give us some insight into what the problems could be.

gsalgado commented 3 years ago

Do we expect the beam sync to ever sync historical blocks?

I don't think so. Why?

carver commented 3 years ago

I would focus now on trying to get the blocks synced with this protocol and check how it behaves. This should give us some insight into what the problems could be.

I agree. It seems like we can just treat every NewBlock announcement from a peer with wit devp2p protocol as an announcement that they probably have the witness.

I don't expect clients to end up wasting much time polling for witnesses (like I guess can be a problem with transactions). We have so much information about the consensus about which blocks are at the tip, and which blocks our peers claim to have.

fjl commented 3 years ago

Is this ready for merging? It's implemented by several clients at this point.

carver commented 3 years ago

I'll clean up the lint issues, squash it, and mark it as ready for proper review & merge.