Two post-#230 fixups #255
Two post-#230 fixups #255
Conversation
TheBlueMatt
force-pushed
the
2018-11-230-ext
branch
from
1422b71 to
f633008
Compare
This extends 1b33064 by re-simplifying the ChannelMonitor <-> Channel interface a bit as we never have any use for the latest remote commitment point until we have knowledge of a remote transaction generated using it.
This fixes a bug in 3518f1f where we may generate an output event for a P2WPKH output which is not ours if the transaction has a sequence/lock_time combination which false-positives our remote tx detection. Also note that the TODO is removed as this should already be covered without issue if the client properly replays the chain on restart.
TheBlueMatt
force-pushed
the
2018-11-230-ext
branch
from
f633008 to
3af20fd
Compare
| @@ -929,23 +938,12 @@ impl ChannelMonitor { | |||
| htlc_idxs.push(None); | |||
| values.push(outp.value); | |||
| total_value += outp.value; | |||
| } else if outp.script_pubkey.is_v0_p2wpkh() { | |||
There was a problem hiding this comment.
Not sure for the bug fixes, the only P2WPKH from a remote revoked tx is ours following bolt 3, how the sequence/lock-time mess can false-positives the tx detection ?
There was a problem hiding this comment.
Technically at this point in check_spend_remote_transaction we aren't even sure if the transaction spends our funding transaction or if its just some random transaction that had the right bits set in its input-0-nSequence and lock time to make it match our commitment number xor check. I guess in the non-watchtower case it should be the case that self.funding_txo.is_some() which would mean we do know for sure, but to keep things consistent its easier to just not assume that here.
There was a problem hiding this comment.
Ah yes you're right, in non-watchtower were we don't have funding_txo set (and can't assume we face commitment tx) this could generate falses-positives
|
I think that 90b0ed9 is good, I read again the conversation in #230 on this point, and I think we misunderstood each other, what I would like to say is that in case of sizeable push_msat, we don't need remote_tx info only per_commitment_point and you said that we use per_commitment_point only in conjunction with remote_tx info. So yes finally it's indifferent that we provide per_commitment_point at same time with provide_latest_remote_commitment_tx_info, in some cases we will use both infos to generate tx, sometimes not. Thanks to prune superflous provisions (but on this point maybe that 394b11c solved it without me notifying) |
The first simplifies the API in a way that @ariard seemed to indicate was incorrect but the tests appear to still pass and I'm not sure why, so I'll let him tell me why I'm wrong here. The second fixes an I-believe-bug and also makes things a bit more performant.