Refactor revalidators

[hannahhoward]

Goals

Implement #297

Implementation

The changes here are, to be blunt, not small. I recommend reviewing in chunks:

• The changes to the state machine and new events
• The changes to the message library
• The changes in validation and event processing

The major overall change is to the validation process. Here's our new validation interface:

// ValidationResult describes the result of validating a voucher
type ValidationResult struct {
	// Accepted indicates where the request was accepted if a request is not
	// accepted, the request fails. This is true for revalidation as well
	Accepted bool
	// VoucherResult provides information to the other party about what happened
	// with the voucher
	VoucherResult
	// LeaveRequestPaused indicates whether the request should stay paused
	// even if the request was accepted
	LeaveRequestPaused bool
	// DataLimit specifies how much data this voucher is good for. When the amount
	// amount data specified is reached (or shortly after), the request will pause
	// pending revalidation. 0 indicates no limit.
	DataLimit uint64
	// RequiresFinalization indicates at the end of the transfer, the channel should
	// be left open for a final settlement
	RequiresFinalization bool
}

// RequestValidator is an interface implemented by the client of the
// data transfer module to validate requests
type RequestValidator interface {
	// ValidatePush validates a push request received from the peer that will send data
	// -- All information about the validation operation is contained in ValidationResult,
	// including if it was rejected. Information about why a rejection occurred is embedded
	// in the VoucherResult.
	// -- error indicates something went wrong with the actual process of trying to validate
	ValidatePush(
		chid ChannelID,
		sender peer.ID,
		voucher Voucher,
		baseCid cid.Cid,
		selector ipld.Node) (ValidationResult, error)
	// ValidatePull validates a pull request received from the peer that will receive data
	// -- All information about the validation operation is contained in ValidationResult,
	// including if it was rejected. Information about why a rejection occurred should be embedded
	// in the VoucherResult.
	// -- error indicates something went wrong with the actual process of trying to validate
	ValidatePull(
		chid ChannelID,
		receiver peer.ID,
		voucher Voucher,
		baseCid cid.Cid,
		selector ipld.Node) (ValidationResult, error)

	// Revalidate revalidates a request with a new voucher
	// -- All information about the validation operation is contained in ValidationResult,
	// including if it was rejected. Information about why a rejection occurred should be embedded
	// in the VoucherResult.
	// -- error indicates something went wrong with the actual process of trying to validate
	Revalidate(channelID ChannelID, channel ChannelState) (ValidationResult, error)
}

What changed:

• Revalidators went away entirely -- every voucher validator should be able to revalidate from the channel state
• The isRestart param went away -- a restart triggers a revalidation
• A validation now produces a LOT more information
  • First, the error parameter in the validation was way overloaded
    • it groups "errors" i.e. something went wrong with my server when I validated with "rejections" -- i.e. your voucher has a problem and I'm not accepting your request (you could think of this as analogous to 500 vs 400 response codes)
      • now an error is only the "something went wrong with my server"
      • the Accepted parameter of ValidationResult now identifies whether a rejection occurred
        • if you need to pass information back to the initiator about why things failed, it goes in the VoucherResult. The previous errors were not returned to the caller anyway, and go-fil-markets retrieval code already relies on sending data in the VoucherResult
    • we also use the "error" to send a Pause/Resume command. This made things even more annoying.
      • now a pause / resume is expressed by LeaveRequestPaused
  • DataLimit allows you to tell data transfer ahead of time how much data this voucher is good for. When the limit gets hit, DataTransfer handles pausing for you. This replaces the OnPushDataReceived / OnPullDataSent methods of the revalidators
  • RequireFinalization allows you to tell data transfer ahead of time that you're gonna need one more round of validation at the end. If set true, data transfer will leave the channel open for you to send some vouchers back and forth -- this replaces the OnComplete method in the revalidator.

One design quirk I'm still thinking about:

• the way you tell a transfer stopped on a datalimit to resume is setting LeaveRequestPaused false. This doesn't take into account the current data limit, and whether you actually changed it. I wonder if we should put this check in, have the unpause happen from the data limit change and going over the current sent/received, and then rename LeaveRequestPaused as an affirmative "Pause the request now" -- as in pause it if we're not paused already, but don't resume.

So that's the main thing, but it was a big enough change I had to make a lot of code changes to do it, and that meant I had to work through a lot of code that I wrote (or others wrote) almost a year ago.... and boy it was hard not to do cleanups, especially since I had to comment to work out what it was doing again. (OY) The upside is this means the code has a lot of comments and that should make reviewing easier. Also I moved all the code related to handling request messages to it's own file, cause that has a ton of logic.

Other changes:

• Core logic for data limits in OnDataQueued/OnDataReceived:

  • I essentially copied the strategy used with tracking data progress without going to the state machine, caching information in memory
  • BOY am I ready to throw them state machines out and replace them with something that gives us an in memory copy all the time but that's a refactor for another time

• I changed the IsVoucherResult method to IsValidationResult for clarity -- this removes a weird pairing of IsVoucherResult & EmptyVoucherResult methods -- IsVoucherResult and EmptyVoucherResult could be true at the same time, which was confusing

• Added an imperative SendVoucherResult method. Honestly, I'd rather this not exist, but when we update the retrieval code, I want to maintain compatibility, and this will allow us to send the same information from the server we were already sending when the channel pauses

There were also some low hanging refactors that drove me crazy as I added changes -- The ChannelState implementation was holding it's own copy of the channelstate which had to be updated and copied on -- now it just embeds and internalChannelState struct.

There were two implementations of mockChannelState that I consolidated.

That's most of it. I don't think this can merge until I complete at least a draft implementation of updating the retrieval code in go-fil-markets - to verify that we actually made possible the big simplifications I'm hoping for.

Also I think this is breaking enough that it should release (along with the transport refactors) as a 2.0 package.

[codecov-commenter]

Codecov Report

Merging #308 (86b6976) into master (00538ea) will decrease coverage by 0.79%.
The diff coverage is 68.47%.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
- Coverage   66.06%   65.26%   -0.80%     
==========================================
  Files          29       30       +1     
  Lines        3439     3501      +62     
==========================================
+ Hits         2272     2285      +13     
- Misses        814      850      +36     
- Partials      353      366      +13     

Impacted Files	Coverage Δ
impl/receiver.go	77.35% <ø> (ø)
message/message1_1prime/message.go	48.25% <0.00%> (-5.66%)	⬇️
impl/impl.go	63.05% <45.45%> (-1.90%)	⬇️
impl/receiving_requests.go	67.46% <67.46%> (ø)
channels/channels_fsm.go	71.42% <68.42%> (+2.59%)	⬆️
channels/channel_state.go	75.32% <70.00%> (-4.90%)	⬇️
channels/channels.go	71.42% <71.42%> (-1.03%)	⬇️
channels/caches.go	77.46% <77.46%> (ø)
impl/restart.go	58.13% <77.77%> (-3.66%)	⬇️
impl/events.go	87.05% <84.90%> (+9.96%)	⬆️
... and 6 more

[dirkmc]

nit: RequiresFinalization => requiresFinalization

[dirkmc]

Looks like the implementation for this function is missing

[dirkmc]

IIRC we need to know at the markets layer if the pull request is for a restart. Maybe check when you implement the markets layer changes and revisit.

[hannahhoward]

see implementation ticket

[dirkmc]

I suggest renaming this to Paused

[hannahhoward]

ultimately changed to ForcePause in subsequent PR

[rvagg]

ResponderFinalizing doesn't come into play here at all?

[hannahhoward]

ResponderFinalizing is the for the initiator, indicating the responder is finalizing. For the moment, we only care about the responder being in a finalization state, as it only affects their logic.

Probably and maybe we should add it to the list but I just want to be careful.

[hannahhoward]

I think I'll just go ahead and add it

[rvagg]

ResponderFinalizing?

[hannahhoward]

that's fair.

[rvagg]

what are the cases where this is nil? is it just DataSent calls? and if so, the only value in calling this function is to set the progress of the sent index, correct?

very minor nit - the naming of this function could be improved, current name doesn't imply mutation but that's one of its two core functions, updateIndexAndCheckEvents() might be the verbose option.

[hannahhoward]

yea agree on all fronts. I'm inclined to wait and update when the transport refactor happens, cause I think data limit logic will move downward to the transport

[rvagg]

this is interesting, both IsNew and IsRestart jumping outside of the previously containing if, was it wrong before not to check this in all but the IsComplete case?

[hannahhoward]

honestly they're just unneccesarily nested. IsNew = true & IsRestart = tree imply IsValidationResult == true, as does IsComplete = true.

So basically, the IsValidationResult handles the "!Accepted" condition and then the other things are determining behavior on an accepted response.

[hannahhoward]

submitting