A few minor nits on #462 #535
Conversation
Instead of passing a Vec of Vecs drop them into one as we go in ChannelMonitor, hopefully avoiding a bit of memory fragmentation and improving readability.
| @@ -2010,7 +2010,7 @@ impl<ChanSigner: ChannelKeys> ChannelMonitor<ChanSigner> { | |||
| watch_outputs.push(new_outputs); | |||
| } | |||
| } | |||
| claimable_outpoints.push(new_outpoints); | |||
| claimable_outpoints.append(&mut new_outpoints); | |||
There was a problem hiding this comment.
It was done this way to avoid mixing spend between revoked commitment txn and HTLC-txn in same block. Our tracking stuff should be reliable enough in case of one of them being reorg-out but still, make harder to reason on it...
There was a problem hiding this comment.
Hmm, right, but it doesn't do that on master, either. the behavior didn't change in this commit, your last refactor before merge changed that.
There was a problem hiding this comment.
And, also, I think we should aggregate such outputs - reorgs are rare-ish, and if we have to fall back to individual transactions to deal with them so be it, but in the common case it saves a good chunk on fees, so we should do it.
There was a problem hiding this comment.
No my last change was segregating them inside Vec<> and then iterating inside OnChainTxHandler::block_connected ? Let's move forward with this but add an issue somewhere we should add test-coverage for this case
There was a problem hiding this comment.
They all get added to the same aggregated_claim map, though.
Codecov Report
@@ Coverage Diff @@
## master #535 +/- ##
==========================================
+ Coverage 89.73% 89.84% +0.10%
==========================================
Files 34 34
Lines 18997 18991 -6
==========================================
+ Hits 17047 17062 +15
+ Misses 1950 1929 -21
Continue to review full report at Codecov.
|
This comment was stale and referred to a previous implementation of lightningdevkit#462, which changed before it was merged.
The API to rust-bitcoin to check a transaction correctly spends another changed some time ago, but we still have a lot of needless .clone()s in our tests.
This reintroduces a check_spends!() removed in 3d640da due to check_spends not being able to check a transaction which spends multiple other transactions. It also simplifies a few calls in claim_htlc_outputs_single_tx by using check_spends!().
TheBlueMatt
force-pushed
the
2020-03-462-nits
branch
from
b4580fc to
6abce81
Compare
|
ACK 6abce81 |
Pretty trivial nits that I saw before merging, but definitely weren't worth holding up Progress (tm) for.