Use do-not-send-first-blocks extension for restarts #257

dirkmc · 2021-10-01T15:56:48Z

Fixes #256

codecov-commenter · 2021-10-01T15:59:01Z

Codecov Report

Merging #257 (0961515) into master (b06ea85) will decrease coverage by 0.17%.
The diff coverage is 70.33%.

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
- Coverage   68.00%   67.83%   -0.18%     
==========================================
  Files          24       24              
  Lines        3069     3146      +77     
==========================================
+ Hits         2087     2134      +47     
- Misses        627      652      +25     
- Partials      355      360       +5

Impacted Files	Coverage Δ
impl/restart.go	`63.85% <0.00%> (ø)`
channels/channel_state.go	`76.04% <33.33%> (-1.38%)`	⬇️
network/libp2p_impl.go	`66.27% <62.50%> (-2.12%)`	⬇️
channels/channels.go	`73.73% <66.66%> (-0.91%)`	⬇️
impl/receiver.go	`69.23% <75.00%> (ø)`
transport/graphsync/graphsync.go	`77.47% <75.51%> (-0.51%)`	⬇️
channels/channels_fsm.go	`70.74% <100.00%> (+0.20%)`	⬆️
impl/events.go	`75.08% <100.00%> (ø)`
message/message1_1/transfer_request.go	`78.37% <100.00%> (ø)`
message/message1_1/transfer_response.go	`96.22% <100.00%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b06ea85...0961515. Read the comment docs.

hannahhoward

Can you explain the rationale for the blocksReceivedIndex? Why not just add a value to the main data transfer state? It's just a number -- the only reason we store received cids list seperately is they are very large

hannahhoward

See previous comment about blocks received index.

Comment about tracking peers outside the go-graphsync extension and tracking peers outside the network impl is NON-BLOCKING.

hannahhoward · 2021-10-01T16:12:37Z

transport.go

 		dataSender peer.ID,
 		channelID ChannelID,
 		root ipld.Link,
 		stor ipld.Node,
-		doNotSendCids []cid.Cid,
-		msg Message) error
+		channel ChannelState,


sidebar +++1 for this change generally. I've never liked the specificity of doNodeSendCids. Technically, we can probably drop a bunch of other parameters if we do this (I believe root/selector/sender/channelID can all be extracted from channel state)

I see that wouldn't quite work for the code here just yet (you're counting on channel == nil being evidence of a not-restart), but a great thing for future refactor!

hannahhoward · 2021-10-01T16:26:22Z

transport/graphsync/graphsync.go

+	}
+
+	// Get the peer's protocol version
+	protocol, err := t.peerProtocol.Protocol(ctx, p)


So I want to point out something that's a potential reason to augment the Graphsync extension as well: in the case of a pull request, you may still not have this cached even for a restart, as the only communication may be over graphsync. I don't know whether this matters a lot, but the idea would be to maintain your protocol list outside the network implementation, so you can update it for an incoming GS request/response as well as well. The only penalty is a new libp2p message I guess, so not the worst thing but still.

I see what you mean, that may save us an extra round trip.

If I understand correctly, if we add another extension protocol, then whenever we send a message, we also include an encoded payload for that extension. Is that correct? So for example if we have an extension for go-data-transfer-v1.2, the message will look like:

<message data> extensions: fil/data-transfer/1.2: <encoded data> fil/data-transfer/1.1: <encoded data> fil/data-transfer: <encoded data>

Maybe easier to explain with a screenshot from the debugger:

:

yes it's an additional copy of the exact same mesage... so arguably this ones a bit a bummer, especially since there's no difference in the message format.... it's literally just the extension name.

BTW we can probably deprecate 1_0 at this point -- hasn't existed since pre-mainnet.

Also Aarsh pointed out that for a pull request it's the data receiver who sends the extension, so the data receiver is not going to know the protocol of the data sender.
So I think it makes most sense not to use an additional extension for now.

dirkmc · 2021-10-01T16:48:04Z

Can you explain the rationale for the blocksReceivedIndex

You're right it makes more sense to store it on the state. I've pushed a commit to do that.

aarshkshah1992

@dirkmc First set of comments .

aarshkshah1992 · 2021-10-04T10:19:07Z

channels/channels.go

@@ -241,8 +241,27 @@ func (c *Channels) DataQueued(chid datatransfer.ChannelID, k cid.Cid, delta uint
 }

 // Returns true if this is the first time the block has been received
-func (c *Channels) DataReceived(chid datatransfer.ChannelID, k cid.Cid, delta uint64) (bool, error) {
-	return c.fireProgressEvent(chid, datatransfer.DataReceived, datatransfer.DataReceivedProgress, k, delta)
+func (c *Channels) DataReceived(chid datatransfer.ChannelID, k cid.Cid, delta uint64, index int64) (bool, error) {


@dirkmc

So, Graphsync will keep track of the index/offset for each block in the DAG and pass that to the Data Receieved callback ?

After a restart -> GS will fire these notifications for blocks we have already received, right ? So, we'll re-update the offset for each block all the way to the last received block and therefore the latest offset, right ?

aarshkshah1992 · 2021-10-04T10:24:12Z

message.go

+	// version of data-transfer (supports do-not-send-first-blocks extension)
+	ProtocolDataTransfer1_2 protocol.ID = "/fil/datatransfer/1.2.0"
+
+	// ProtocolDataTransfer1_2 is the protocol identifier for the version


Suggested change

// ProtocolDataTransfer1_2 is the protocol identifier for the version

// ProtocolDataTransfer1_1 is the protocol identifier for the version

aarshkshah1992 · 2021-10-04T12:38:04Z

network/libp2p_impl.go

 	return nil
 }
+
+func (impl *libp2pDataTransferNetwork) Protocol(ctx context.Context, id peer.ID) (protocol.ID, error) {
+	// Check the cache for the peer's protocol version


@dirkmc You don't need this cache as libp2p maintains it's own cache. You can use impl.host.Peerstore().FirstSupportedProtocol() for the cache lookup and then go do the open stream thing it it doesen't have anything for you.

feat: do-not-send-first-blocks

2570acc

jennijuju requested a review from hannahhoward October 1, 2021 16:00

hannahhoward reviewed Oct 1, 2021

View reviewed changes

hannahhoward requested changes Oct 1, 2021

View reviewed changes

refactor: move received blocks total onto channel state

93b59f1

hannahhoward approved these changes Oct 1, 2021

View reviewed changes

jennijuju added this to the v1.13.0 milestone Oct 4, 2021

fix: record stream protocol on stream open

0961515

dirkmc merged commit 717d0bf into master Oct 4, 2021

dirkmc deleted the feat/do-not-send-first-blocks branch October 4, 2021 12:15

aarshkshah1992 reviewed Oct 4, 2021

View reviewed changes

This was referenced Oct 4, 2021

use go-fil-markets v1.13.0 filecoin-project/lotus#7441

Closed

release: v1.11.0 #262

Merged

release: v1.13.0 filecoin-project/go-fil-markets#635

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use do-not-send-first-blocks extension for restarts #257

Use do-not-send-first-blocks extension for restarts #257

dirkmc commented Oct 1, 2021

codecov-commenter commented Oct 1, 2021 •

edited

Loading

hannahhoward left a comment

hannahhoward left a comment

hannahhoward Oct 1, 2021

hannahhoward Oct 1, 2021

hannahhoward Oct 1, 2021

dirkmc Oct 1, 2021

hannahhoward Oct 1, 2021

dirkmc Oct 4, 2021

dirkmc commented Oct 1, 2021

aarshkshah1992 left a comment

aarshkshah1992 Oct 4, 2021

aarshkshah1992 Oct 4, 2021

dirkmc Oct 4, 2021

aarshkshah1992 Oct 4, 2021

aarshkshah1992 Oct 4, 2021

	// ProtocolDataTransfer1_2 is the protocol identifier for the version
	// ProtocolDataTransfer1_1 is the protocol identifier for the version

Use do-not-send-first-blocks extension for restarts #257

Use do-not-send-first-blocks extension for restarts #257

Conversation

dirkmc commented Oct 1, 2021

codecov-commenter commented Oct 1, 2021 • edited Loading

Codecov Report

hannahhoward left a comment

Choose a reason for hiding this comment

hannahhoward left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmc commented Oct 1, 2021

aarshkshah1992 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 1, 2021 •

edited

Loading