Make specification of protocol in SyncRequester::send_request explicit #10295

elferdo · 2019-02-04T15:40:06Z

The function SyncRequester::send_request has a simple logic to match a given packet_id to its corresponding protocol. The logic does not seem to be robust, though, and expecting clients to provide the protocol themselves is not too much burden. If needed, we can still reimplement the matching logic separately.

As a consequence, SyncIO needs no longer provide send and send_protocol.

Fixes #8605

explicit when calling "SyncRequester::send_packet". * Remove "SyncIO::send" and leave only "SyncIO::send_protocol"

parity-cla-bot · 2019-02-04T15:40:11Z

It looks like @elferdo hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-02-04T15:40:11Z

It looks like @elferdo hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

elferdo · 2019-02-04T15:42:55Z

[clabot:check]

parity-cla-bot · 2019-02-04T15:42:59Z

It looks like @elferdo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

parity-cla-bot · 2019-02-04T15:42:59Z

It looks like @elferdo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

tomusdrw

Looks good, just one worrisome place.

ethcore/sync/src/chain/propagator.rs

ngotchac · 2019-02-04T15:53:42Z

ethcore/sync/src/chain/mod.rs

 use ethcore::client::{BlockChainClient, BlockStatus, BlockId, BlockChainInfo, BlockQueueInfo};
 use ethcore::snapshot::{RestorationStatus};
 use sync_io::SyncIo;
 use super::{WarpSync, SyncConfig};
 use block_sync::{BlockDownloader, DownloadAction};
 use rand::Rng;
 use snapshot::{Snapshot};
-use api::{EthProtocolInfo as PeerInfoDigest, WARP_SYNC_PROTOCOL_ID, PriorityTask};
+use api::{EthProtocolInfo as PeerInfoDigest, ETH_PROTOCOL, WARP_SYNC_PROTOCOL_ID, PriorityTask};


I think we should rename WARP_SYNC_PROTOCOL_ID to WARP_SYNC_PROTOCOL (as there are already ETH_PROTOCOL and LIGHT_PROTOCOL)

ethcore/sync/src/chain/mod.rs

ethcore/sync/src/chain/requester.rs

a devp2p packet.

elferdo · 2019-02-06T10:23:58Z

Any thoughts how to proceed on this? How can I test that the changes work? I have tried running a node and connecting to just one reserved peer, a parity node, and synchronization seems to proceed fine both with and without warp.

tomusdrw

@elferdo I'm pretty sure we won't break anything in this PR. As @grbIzl noticed, the protocol of the network is often overwritten using with_context function - IMHO this seems super confusing, I'd prefer to be explicit and use send_protocol always, alternatively as we spoke in DM we could tangle packet and protocol together (using an enum and trait Packet{ fn id() -> u8, fn protocol() -> T } for instance) to prevent any amibguity.

If the sync works and we pass all the tests I'm ok with this PR (modulo the panic). Maybe @grbIzl can suggest how to test private transaction stuff.

ethcore/sync/src/tests/helpers.rs

in chain sync.

tomusdrw · 2019-02-08T09:13:38Z

@elferdo I just noticed those new commits here, I think it would be better to merge this PR asap and then do any follow up work in susbsequent prs. This one is already reviewed and marked as good.

elferdo · 2019-02-08T09:14:17Z

@elferdo I'm pretty sure we won't break anything in this PR. As @grbIzl noticed, the protocol of the network is often overwritten using with_context function - IMHO this seems super confusing, I'd prefer to be explicit and use send_protocol always, alternatively as we spoke in DM we could tangle packet and protocol together (using an enum and trait Packet{ fn id() -> u8, fn protocol() -> T } for instance) to prevent any amibguity.

I implemented something like what you propose with trait Packet[...], just in the context of syncing, that means ethcore-devp2p still uses both protocol and packet_id as arguments.

elferdo · 2019-02-08T09:15:45Z

@elferdo I just noticed those new commits here, I think it would be better to merge this PR asap and then do any follow up work in susbsequent prs. This one is already reviewed and marked as good.

Oh, ok, sorry. Should I revert these commits and open a new PR then?

tomusdrw · 2019-02-08T09:32:10Z

Should I revert these commits and open a new PR then?

Yes, if you don't mind. I think that will be better, cause otherwise we may end up with never ending story of commit-review cycle and never get anything merged. And it's going to be harder and harder to keep that PR mergeable to master.

elferdo · 2019-02-08T09:35:31Z

Sure, that makes sense. I'll revert the last changes.

This reverts commit 0bef1ce.

ethcore/sync/src/chain/propagator.rs

grbIzl · 2019-02-08T11:11:48Z

Ok. Let's try to close this one and go further

* master: no volumes are needed, just run -v volume:/path/in/the/container (#10345) Fixed misstype (#10351) snap: prefix version and populate candidate channel (#10343) Bundle protocol and packet_id together in chain sync (#10315) role back docker build image and docker deploy image to ubuntu:xenial based (#10338) change docker image based on debian instead of ubuntu due to the chan… (#10336) Don't add discovery initiators to the node table (#10305) fix(docker): fix not receives SIGINT (#10059) snap: official image / test (#10168) fix(add helper for timestamp overflows) (#10330) Additional error for invalid gas (#10327) Revive parity_setMinGasPrice RPC call (#10294) Add Statetest support for Constantinople Fix (#10323) fix(parity-clib): grumbles that were not addressed in #9920 (#10154) fix(light-rpc): Make `light_sync` generic (#10238) fix publish job (#10317) Secure WS-RPC: grant access to all apis (#10246) Make specification of protocol in SyncRequester::send_request explicit (#10295)

* Make the specification of the protocol to which a packet_id belongs to

27a6d98

explicit when calling "SyncRequester::send_packet". * Remove "SyncIO::send" and leave only "SyncIO::send_protocol"

elferdo requested review from tomusdrw and ngotchac February 4, 2019 15:40

elferdo mentioned this pull request Feb 4, 2019

Generic request sender in sync is not generic enough #8605

Closed

tomusdrw reviewed Feb 4, 2019

View reviewed changes

ethcore/sync/src/chain/propagator.rs Show resolved Hide resolved

ngotchac reviewed Feb 4, 2019

View reviewed changes

ethcore/sync/src/chain/mod.rs Show resolved Hide resolved

grbIzl reviewed Feb 4, 2019

View reviewed changes

ethcore/sync/src/chain/requester.rs Show resolved Hide resolved

Fernando Herrero Carrón added 2 commits February 4, 2019 17:10

Adapt tests to new code.

584ed4f

Strengthen tests to check if packet_id and protocol match when sending

aecfb1f

a devp2p packet.

tomusdrw approved these changes Feb 6, 2019

View reviewed changes

ethcore/sync/src/tests/helpers.rs Show resolved Hide resolved

5chdn added this to the 2.4 milestone Feb 7, 2019

5chdn added A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust. labels Feb 7, 2019

Fernando Herrero Carrón added 6 commits February 7, 2019 17:41

Early proposal for binding packet_id and protocol together

0bef1ce

in chain sync.

Replace PacketId with SyncPacketId in chain sync.

5d8bc34

Rename module.

afd1431

Remove send_protocol from SyncIo and leave just send.

1d9ae78

Rename trait Packet to PacketInfo.

16afb11

Document.

81022bf

Fernando Herrero Carrón and others added 2 commits February 8, 2019 10:37

Revert "Early proposal for binding packet_id and protocol together"

9dbdd15

This reverts commit 0bef1ce.

Merge branch 'master' into fhc-explicit-protocol-send-request

349eae0

grbIzl reviewed Feb 8, 2019

View reviewed changes

ethcore/sync/src/chain/propagator.rs Outdated Show resolved Hide resolved

Fix conflict merge error.

7b245ba

grbIzl approved these changes Feb 8, 2019

View reviewed changes

elferdo merged commit 046b8bb into master Feb 8, 2019

elferdo deleted the fhc-explicit-protocol-send-request branch February 8, 2019 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make specification of protocol in SyncRequester::send_request explicit #10295

Make specification of protocol in SyncRequester::send_request explicit #10295

elferdo commented Feb 4, 2019 •

edited

Loading

parity-cla-bot commented Feb 4, 2019

parity-cla-bot commented Feb 4, 2019

elferdo commented Feb 4, 2019

parity-cla-bot commented Feb 4, 2019

parity-cla-bot commented Feb 4, 2019

tomusdrw left a comment

ngotchac Feb 4, 2019

elferdo commented Feb 6, 2019

tomusdrw left a comment

tomusdrw commented Feb 8, 2019

elferdo commented Feb 8, 2019

elferdo commented Feb 8, 2019

tomusdrw commented Feb 8, 2019

elferdo commented Feb 8, 2019

grbIzl commented Feb 8, 2019

Make specification of protocol in SyncRequester::send_request explicit #10295

Make specification of protocol in SyncRequester::send_request explicit #10295

Conversation

elferdo commented Feb 4, 2019 • edited Loading

parity-cla-bot commented Feb 4, 2019

parity-cla-bot commented Feb 4, 2019

elferdo commented Feb 4, 2019

parity-cla-bot commented Feb 4, 2019

parity-cla-bot commented Feb 4, 2019

tomusdrw left a comment

Choose a reason for hiding this comment

ngotchac Feb 4, 2019

Choose a reason for hiding this comment

elferdo commented Feb 6, 2019

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw commented Feb 8, 2019

elferdo commented Feb 8, 2019

elferdo commented Feb 8, 2019

tomusdrw commented Feb 8, 2019

elferdo commented Feb 8, 2019

grbIzl commented Feb 8, 2019

elferdo commented Feb 4, 2019 •

edited

Loading