Open enigbe opened 1 month ago
Thanks! I think beyond checking for the node alias and listening addresses before broadcasting, we should also modify the
default_user_config
method inConfig
to, if any of the two are unconfigured:a) set
UserConfig::accept_forwards_to_priv_channels
tofalse
b) setChannelHandshakeConfig::announced_channel
tofalse
c) setChannelHandshakeLimits::force_announced_channel_preference
totrue
Additionally, we'll want to return an error from
connect_open_channel
if any of the two are unconfigured and we try to open an announced channel. To this end, it might be bit cleaner if we'd split the method in two (open_channel
andopen_announced_channel
) rather than having it take a simplebool
.TLDR: We should use the configured listening addresses and node alias as indicators whether we are a forwarding node or not. If we're not, we should disallow announced channels.
Let me know if you'd be up for these additional changes, otherwise I'm happy to pick them up as a follow-up.
Thanks for the review. I am happy to keep working on this until it is ready.
Thanks for the review. I am happy to keep working on this until it is ready.
Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up?
Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.
Thanks for the review. I am happy to keep working on this until it is ready.
Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up?
Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.
Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.
Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.
No worries! Cool, thank you!
Thanks for the review. I am happy to keep working on this until it is ready.
Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up? Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.
Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.
Hi @tnull, I have rebased and pushed recommended changes. Unfortunately, the integration tests are flaky and I am uncertain about why. Currently investigating.
Thanks for the review. I am happy to keep working on this until it is ready.
Thanks again! Do you still intend to do this in this PR, or would you prefer we land this and do additional changes in a follow up? Btw. this also needs a rebase already, and there will be larger code changes upcoming in the LDK/BDK/rust-bitcoin upgrade PR, which will likely also lead to further rebase conflicts.
Apologies for the delay. I am almost done and will be pushing changes today. I'll rebase and get it ready for another look before the end of the day.
Hi @tnull, I have rebased and pushed recommended changes. Unfortunately, the integration tests are flaky and I am uncertain about why. Currently investigating.
Hi @tnull, thanks for your review so far. Have you had some time to check out the recent changes I have made? I have addressed the comments raised and would be grateful for any further feedback or recommendation.
What this PR does:
Related issue(s):
304