Minor nit: the variable name and the godoc comment don't match up. This should be changed to read IsDust.

Hmm, I need to check the spec, but we'll need to ensure that we're storing their value for the dust-limit. This is necessary since we modify their commitment transactions with updates and will need to ensure we adhere to their required dust limit. We don't care about what their dust-limit is as long as they respect our limit.

As I described below I don't separate the dust limit on their and our , the dust limit which I store it is negotiated value and it should be identical for both sides.

Minor suggestion: we can eliminate some unnecessary nesting here if we modify the fragment to read:

if _, err := r.Read(scratch[:1]); err != nil {
    retur nil, err
}

if scratch[0] == 1 {
    h.Incoming = true
} else {
    h.Incoming = false
}

Same applies to the fragment below.

We currently don't have a "default" configuration yet for values like the dust-limit, so can you add a TODO here to use the default dust-limit value (or perhaps query a dynamic indicator here?

As written now, this isn't strictly correct since it it just echoes back what they chose, but in practice it works currently.

Hmm, so on second thought, I think we actually need to retain the state of the dust-limit of both sides since the state machine needs to decide separately for incoming/outgoing HTLC's. Otherwise we may run into a "commitment divergence" due to different decisions about if a HTLC constituted a dust value or not.

As far as I understand we negotiate the dust limit on funding request/response messages. After that we have identical dust limit on both sides and may make the decision - should we include HTLC in commitment transaction or not - relaying on this negotiated dust limit, could you please elaborate why we need to have different dust limits for both sides?

The dust limits aren't negotiated like the fees are within the current spec draft. Instead, both sides have their own distinct dust limits, as both sides ay have different degrees of concfomrat w.r.t to what they think a valid dust-limit is.

As a result, there needs to be two dust-limits, one which must be adhered to when the remote side adds HTLC's to our commitment transactions, and one which we need to be cognizant of where we're triggering updates to their commitment transaciton.

The error here shouldn't be ignored.

One additional assertion we can throw in is that if we obtain the current commitment transaction for both sides, the HTLC output shouldn't be present on either of them.

I have a branch locally which starts to factor out paramter like this into a FundingConfig struct which includes all the parameters as are currently defined within the spec.

Nice catch! I think one of my prior commits adding a new test-case accidentally removed this test.

Nice, happy to see this removed. This was left over from when we were toying around with a CLTV based channel construction (assuming there isn't a malleability fix).

Minor nit: can you drop the Our|Their prefix from the DustLimit fields here? The PoV context really doesn't apply here since these are wire messages rather than state which is stored locally.

Same comment here about dropping the prefix.

Nice catch, this bit of code was unnecessary due to similar logic already residing within the fundingManager.handleErrorGeneric method.

This line causes the travis build to fail:

--- FAIL: TestCheckDustLimit (0.01s)
channel_test.go:875: for this test to work bob dust limit should be bigger than alice dust limit at least on 2 satoshi

I don't think this line is needed. It seems that the value of the HTLC within the test simply needs to be above Alice's dust limit, but below Bob's dust limit .

Aghhh... :) Thanks! Because of this multi-hop test I had started forget to check the reason of test failure...

I don't think this line is needed. It seems that the value of the HTLC within the test simply needs to be above Alice's dust limit, but below Bob's dust limit.

done