Refactor tlc saving in channel state and fix a few bugs #72
Conversation
contrun
force-pushed
the
keep-tlc-on-remove
branch
2 times, most recently
from
7bf2b7c
to
c1c4557
Compare
src/ckb/channel.rs
Outdated
| } else { | ||
| self.to_remote_amount += current.tlc.amount; | ||
| } | ||
| current.removal_info = Some((commitment_numbers, reason)); |
There was a problem hiding this comment.
is it better we remove the current entry from self.tlcs?
otherwise tlcs will keep growing, and we need to serialize and store this field.
There was a problem hiding this comment.
We need this historical tlc information to construct valid revocation transaction. Imagine that the adversary broadcasts an older commitment transaction, and we want to revoke this. In order to do so, we will need to fill in all the tlcs that are committed commitment transaction in the witness. This is the reason why we need to keep tlcs forever. Of course, we can also save the witnesses for commitment transaction (just hook into this event https://github.com/nervosnetwork/cfn-node/blob/c1c4557a15d6457b23008328e1fc5cdf82e9611b/src/ckb/network.rs#L152C5-L152C27). I personally prefer the latter method. But I haven't think carefully about the tradeoffs. I think it is not such so important to optimize this yet.
| a.sort_by(|x, y| u64::from(x.0.tlc.id).cmp(&u64::from(y.0.tlc.id))); | ||
| b.sort_by(|x, y| u64::from(x.0.tlc.id).cmp(&u64::from(y.0.tlc.id))); |
There was a problem hiding this comment.
BTreeMap is sorted, I don't think we need sort again here. btw, we need unit test for this fn.
src/ckb/channel.rs
Outdated
| self.tlcs.insert(tlc.id, detailed_tlc); | ||
| if tlc.is_offered() { | ||
| self.to_local_amount -= tlc.amount; |
There was a problem hiding this comment.
@chenyukang a reminder, we need to check to_local_amount / to_remote_amount - tlc.amount > min_amount here before inserting.
contrun
force-pushed
the
keep-tlc-on-remove
branch
7 times, most recently
from
45cb8e4
to
75a1ac7
Compare
Add notification on commitment transaction signed locally Fix initial commitment number Bundle local and remote commitment numbers in channel state Bundle offering and received tlc ids in channel state Use enum for TLC id Use a unified field to represent all tlcs in channel state Use btree map instead of hash map for tlcs Refactor add tlc Greatly simplify tlc handling Remove two subtle bugs due to tlc data structure change Fix json serde error because keys in objects must be strings Refactor get active tlc implementation Remove outdated code Fix balance calculation in removing tlc
Commitment numbers of each parties are independent. Updating remote commitment number after received RevokeAndAck effectively means we are using the same commitment number.
A remote commitment point is only obtained when RevokeAndAck is received from the remote party. And RevokeAndAck is sent only when the remote party receives a CommitmentSigned message. This is exactly when the local commitment transaction is updated.
https://stackoverflow.com/questions/56726571/why-choosing-unwrap-or-else-over-unwrap-or Using unwrap_or will always evaluate the argument. This is undesirable because we may send the message twice here.
contrun
force-pushed
the
keep-tlc-on-remove
branch
from
75a1ac7
to
c41657c
Compare
This PR refactored TLC fields in channel state. We will save all the tlcs in a unified tlcs field. This make it more convenient to see when a tlc is added/removed. When refactoring this I also found a few bugs. Below are the changes in this PR.
LocalCommitmentSignedis added. This may be used to notify watch tower that some transactions are now broadcastable by the peer.RevokeAndAckfrom the peer received. This effectively makes local/remote commitment number always the same. We should instead update commitment numbers onCommitmentSignedmessages sent or received. This PR fixes that.RevokeAndAckis received from the remote party. AndRevokeAndAckis sent only when the remote party receives aCommitmentSignedmessage. This is exactly when the local commitment transaction is updated.