Comments (13)
Was looking on it, seems to me than for some chain_monitor TODO we need to pass an Arc to ChannelMonitor struct and if so, maybe it's make sense to drop it from SimpleManyChannelMonitor?
from rust-lightning.
I assume you're referring to "TODO: Need to register the given script here with a chain_monitor" and the two "TODO: Register all outputs in commitment_tx with the ChainWatchInterface!"? I'm not 100% sure by what you mean by "drop it from SimpleManyChannelMonitor", though? In any case, yea, not sure if it makes more sense to return such values to the callers (ala the Channel::block_connected telling ChannelManager::block_connected how it should handle it) or if it makes more sense to give it yet another Arc.
from rust-lightning.
Yes I'm referring to this ones, I prefer your suggestion with Channel - ChannelManager because I started on the other side and passing ChainWatchInterface means we have to deal with it in channel_monitor serialization/deserialization. Will open a PR soon on this
from rust-lightning.
Following BOLT 5 there is a lot of different cases to claim in case of remote node broadcasting a commitment tx.
The Bad Way :
- to_remote outputs via P2WPKH
- offered htlc outputs via payment preimage tx
- received htlc outputs via timeout tx
The Ugly Way :
- to_remote outputs via P2WPKH (same as above)
- to_local outputs via revocation key (tested in #199 )
- offered/received htlc outputs via revocation key (tested in #184 )
- HTLC-timeout/HTLC-success outputs via revocation key (with #163)
Seems to me that currently, channel_monitor_network_test currently does only Bad Way 2,3 and Ugly Way 4, do I forget any case ?
from rust-lightning.
(Alice broadcasting a commitment tx, speaking as Bob viewpoint here)
from rust-lightning.
"to_remote outputs via P2WPKH" should be covered by #81 (ie once we have a way for clients to specify the keys they want they should be monitoring the chain for at least the non-derived keys, and obviously documentation should mention this, this also applies to shutdown keys).
The "via revocation key" HTLC outputs are implemented, probably, but aren't tested yet (which you indicated, just being thourough).
The to_local outputs via revocation key I think we impelement? Maybe I'm misunderstanding what the BOLT is referring to, but note that I think we need an interface so that we can tell the client about the availability of an output+the key needed to claim it when we believe the remote side cannot take it (eg its a timelocked output for our latest commitment transaction which we don't think we've ever revoked...we are in no rush to spend it and can let the client do so at their leisure).
from rust-lightning.
Yes sorry I checked the code in fact the to_local outputs via revocation is implemented, but seems to me there is not test only for it because in some tests (all?) it's trimmed so it doesn't appear specifically.
#184 is tests for "via revocation key" HTLC outputs, need review. While I was writing tests, I was wandering maybe we could cache tx given to check_spend_remote_tx after 1st pass in case of block re-scanning to avoid generate again same punishment tx.
"to_remote_outputs via P2WPKH", seems fine to be covered by #81, noted.
from rust-lightning.
And yeah for stuff in force_close about ChannelManager I think we need an interface to have the ChainWatchInterface to pass up the payment_preimage extracted from on-chain HTLC-Success input to the ChannelManager and this last one claiming backwards. Seems for HTLC-timeout case, failing backward. Working on it
from rust-lightning.
I'd hope it doesn't need to get added in ChainWatchInterface, maybe ChannelMonitors should have a different block_connected fn that ChannelManager calls that returns more information and watchtowers (or other calls into ChannelMonitor via ChainWatchInterface) can just ignore the extra data.
The which-object-is-registered-with-ChainWatchInterface stuff is already a mess (see #80) so I don't think rejiggering what calls what is that big a deal right now. Eventually we'll need to rewrite it all and make it harder to misuse but that doesn't need to happen ASAP.
from rust-lightning.
Hmm, just a side-note to not forget to add the case of a revoked commitment tx solved by a justice tx, detect it and fail backward. Will go in #198
from rust-lightning.
Some notes to check if we have covered everything on on-chain Channel resolution.
Local Node seeing a Local Commitment Tx :
-
commitment tx :
- to_local output, resolution by output descriptor generated, implemented by ChannelMonitor::broadcast_by_local_state, tested by test_claim_sizeable_push_msat
- to_remote output, nothing to be done
- offered_htlc_output : resolution by HTLC-Timeout tx generated and failure passed backward if needed, implemented by ChannelMonitor::broadcast_by_local_state, tested by channel_monitor_network_test, test_htlc_on_chain_timeout
- received_htlc_output : resolution by HTLC-Success tx generated, implemented by ChannelMonitor::broadcast_by_local_state, tested by channel_monitor_network_test
-
HTLC-Timeout tx (from any commitment tx offered_htlc_output) :
- unique output : resolution by output descriptor generated for unique output, implemented by ChannelMonitor::broadcast_by_local_state, tested by test_dynamic_spendable_outputs_local_htlc_timeout_tx
-
HTLC-Success tx (from any commitment tx received_htlc_output) :
- unique output : resolution by output descriptor generated for unique output, implemented by ChannelMonitor::broadcast_by_local_state, tested by test_dynamic_spendable_outputs_local_htlc_success_tx
Local Node seeing a Non-Revoked Remote Commitment Tx :
[WIP]
from rust-lightning.
Killed by #336 IMO, not sure if bumping engine is in the scope.
from rust-lightning.
Nah, I think its fine as-is. I'd like to get better test coverage on it, but this bug itself is probably done.
from rust-lightning.
Related Issues (20)
- Add error handling for channels which fail to be created in `funding_transaction_generated_intern`
- Change max_value_in_flight based on channel being public HOT 9
- Avoid re-persisting monitors if they are unchanged on startup
- Wipe `get_claimable_balances` for timed-out, closed, inbound channels, with 0 balance HOT 3
- Purge channel-monitor functionality at startup HOT 1
- Add general-purpose HTLC interception interface HOT 1
- Return the funding outpoint `scriptPubKey` in `accept_inbound_channel` HOT 1
- Router: fully utilize first-hop outbound capacity HOT 2
- Need a `funding_transaction_id_generated_unsafe` function HOT 4
- Explore introducing `semgrep` to enforce stricter coding guidelines HOT 3
- Expose method to send node announcement to specfic peer HOT 6
- Make `prevtx` in `TxAddInput` message an `Option` HOT 2
- `<Future as std::..::Future>::poll()` always allocates, growing until we're woken HOT 1
- COmmitment tx broadcasted on `force_close_broadcasting_latest_txn` even if we haven't broadcasted the funding tx
- Rename `payment_parameters_from_zero_amount_invoice` to imply amount-less, not 0-amount HOT 5
- `CustomOnionMessageHandler::handle_message` doesn't expose the `reply_path` HOT 5
- Add method to delete channel state while online
- Prefer non-onion nodes in blinded paths HOT 4
- RUSTSEC-2021-0145: Potential unaligned read HOT 4
- bad reestablish revocation_number: 0 vs 3 HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from rust-lightning.