We shouldn't use hardcoded values here

[aarani]

This code assumes that the person who broadcasted the commitment
transaction is always the funder and the person who tried to spend
the broadcasted remote commitment is always funder.
This passed our test because that's the only case we test in our
end-to-end tests.

[knocte]

should this be applied to m4 first?

[aarani]

There is two revocation implementation currently in DNL.Kiss one which uses private key and tries to spend whatever commitment tx it can (whether it's broadcasted by us or by the remote party)
There is another one (which we recently merged) which only returns the punishment Tx to store and broadcast later in case of an revoked commitment broadcasted by the other party
This bug only affects the first one (previous m4 implementation ) and not the second one.
So for example if we add a force close option to geewallet in future this bug will prevent both funder and fundee (using geewallet) from spending the commitment tx if fundee tries to force-close the channel on the other hand if the remote party try to cheat by broadcasting a revoked tx our recently merged code will be able to spend the tx anyway

[knocte]

So for example if we add a force close option to geewallet in future this bug will prevent both funder and fundee (using geewallet) from spending the commitment tx if fundee tries to force-close the channel.

Given that Andrew is going to implement force-closing in m4, then we need this bugfix in m4 first?

[aarani]

So for example if we add a force close option to geewallet in future this bug will prevent both funder and fundee (using geewallet) from spending the commitment tx if fundee tries to force-close the channel.

Given that Andrew is going to implement force-closing in m4, then we need this bugfix in m4 first?

Yes this bug should get fixed for that to work

[knocte]

Yes this bug should get fixed before m4 finishes

Ok then re-target this PR to geewalletLightningMilestone4 instead of master.

[aarani]

The master branch is broken too but yeah will create a new pull request for that branch

[aarani]

#19 retargeted.

[knocte]

The master branch is broken too

I know.

will create a new pull request for that branch

No need, I will rebase master branch on top of geewalletLightningMilestone4 after merging this.

Take in account, given that DNL.Kiss is a fork of DNL, it will be constantly a moving target (having force-pushes all the time for the master branch).

[knocte]

@canndrew please review

[knocte]

#19 retargeted.

Closing this one then, actually.