It is possible that that there are multiple HTLCs with different values,
but the same public key script. As such, a check against the value should
be performed when looking for HTLC outputs in a commitment transaction.
Create a new helper method called genHtlcScript which will
generate the public key scripts for a supplied HTLC. This functionality
from addHTLC is removed, and addHTLC will instead call this new
method.
In restoreStateLogs we will regenerate the public key scripts for the
HTLCs with genHtlcScript and restore the proper values.
When an HTLC is either cancelled or settled we must properly set the
pkScript for the HTLC on the remote commitment, such that we can
generate a valid ChannelDelta.
Description of bug:
When calling ReceiveNewCommitment() we will progress through methods
fetchCommitmentView and addHTLC which will add HTLC outputs to the
commitment transaction in the local commitment chain and save the
pkScript to the relevant PaymentDescriptor which resides in the
corresponding updateLog. Finally the local commitment will be added
to the local commitment chain.
When the same user next calls SignNextCommitment we will again
progress through fetchCommitmentView and addHTLC. In addHTLC we will
now overwrite the pkScripts in the PaymentDescriptors with the
pkScript from the context of the remote commitment. When we later
call RevokeCurrentCommitment and proceed into toChannelDelta, we
will not be able to find the correct pkScript in the PaymentDescriptor
to match it against the outputs in the commitment transaction.
This will lead to the nested function locateOutputIndex returning
incorrect values.
Fixing the bug:
We introduce three new fields in PaymentDescriptor:
* ourPkScript
* theirPkScript
* theirPrevPkScript
ourPkScript will include the pkScript for the HTLC from the context
of the local commitment.
theirPkScript will take the value of the latest pkScript for the HTLC
from the context of the remote commitment.
theirPrevPkScript will take the second-latest pkScript for the HTLC
from the context of the remote commitment. This is the value we use
in toChannelDelta when we save a revoked commitment from our peer.
The appropriate value of these fields are set in the addHTLC method.
Additionally we pass a boolean value to toChannelDelta so we know
whether we are operating on a local or remote commitment and grab
the correct pkScript in locateUpdateIndex.
If a forceClose happens and the to-self output is below the dust value,
then that output will be non-existant in the commitment transaction.
UtxoNursery now handles that case by checking to see if the accompanying
selfOutPutSignDescriptor is nil. If it is nil, then it will ignore it.
If the value of the to-local output is below the dust limit, the
ForceCloseSummary should not include a sign descriptor for this output.
We also find the proper to-self output by looking for the expected public
key script and not assume that no HTLC outputs exist.
Currently non-HTLC outputs will be accepted in the commitment
transaction as long as it is non-zero. We change this by not allowing
outputs with a value lower than the dust limit. The value of such
an output will go towards transaction fees.
Introduce TimelockShift which is used to make sure the commitment
transaction is spendable by setting the locktime with it so that
it is larger than 500,000,000, thus interpreting it as Unix epoch
timestamp and not a block height. It is also smaller than the current
timestamp which has bit (1 << 30) set, so there is no risk of having
the commitment transaction be rejected. This way we can safely use
the lower 24 bits of the locktime field for part of the obscured
commitment transaction number.
Add tests to assert maximum state can be used. Also test
that more than one input in the commitment transaction
will fail and that having state number larger than
maxStateHint will fail.
Fix SetStateNumHint and GetStateNumHint to properly
set and get the stateNumHints using the lower 24 bits
of the locktime of the commitment transaction as the
lower 24 bits of the obfuscated state number and the
lower 24 bits of the sequence field as the higher 24
bits.
rHashMap is used to store the PaymentDescriptor belonging to a received
HTLC's revocation hash. This improves the efficiency of looking up
PaymentDescriptors from their RHash whenever we want to settle or cancel
that HTLC.
This fixes the bug reproduced in #114. The prevHop map in newRoute may
include many more edges than what is used to produce the final route, and
thus the check prior to building the route could result with incorrect
errors being reported. We move this check to after the number of edges
to be used for the route is deduced.
