Add support for Testnet4
#4148
Add support for Testnet4
#4148
Conversation
|
👋 Thanks for assigning @benthecarman as a reviewer! |
lightning-invoice/src/lib.rs
Outdated
| match currency { | ||
| Currency::Bitcoin => Network::Bitcoin, | ||
| Currency::BitcoinTestnet => Network::Testnet, | ||
| Currency::BitcoinTestnet4 => Network::Testnet4, |
There was a problem hiding this comment.
We could also consider not adding a new Currency variant, but if we just kept Currency::BitcoinTestnet we wouldn't be able to make this mapping back to Testnet4.
There was a problem hiding this comment.
We already kinda cant, though? If someone reads an invoice with the tb prefix they'll get a Currency/Network of testnet, which won't map to tn4.
There was a problem hiding this comment.
We already kinda cant, though? If someone reads an invoice with the tb prefix they'll get a
Currency/Networkof testnet, which won't map to tn4.
Okay, so what resolution do you suggest then? Opening a spec PR for a new BOLT11 prefix for Testnet4? As I mentioned below I was close to do this, but refrained so far as I thought the idea was that Testnet3 eventually is just superseded by Testnet4 and goes away. But maybe it's the easiest solution here, though we'll need to double-check that other impls are onboard with it ofc.
There was a problem hiding this comment.
Yea, I dunno, its definitely awk either way. We should at least explore it at the spec level, I imagine, but we can also map Testnet4 to Currency::BitcoinTestnet for now so that you can build an invoice from Network::Testnet4?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4148 +/- ##
========================================
Coverage 89.32% 89.33%
========================================
Files 180 180
Lines 138329 138480 +151
Branches 138329 138480 +151
========================================
+ Hits 123566 123710 +144
+ Misses 12157 12148 -9
- Partials 2606 2622 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning-invoice/src/ser.rs
Outdated
| let currency_code = match *self { | ||
| Currency::Bitcoin => "bc", | ||
| Currency::BitcoinTestnet => "tb", | ||
| Currency::BitcoinTestnet4 => "tb", |
There was a problem hiding this comment.
is this what other implementations are doing? would be a shame to lose the one-to-one mapping
There was a problem hiding this comment.
AFAICT yes. BOLT11 speaks about 'testnet' overall, without a specific version. I considered a PR to introduce a change specific to Testnet4 (e.g. tb4), but given that Testnet3 is supposed to eventually go away it could make sense to reuse the same prefix?
Not sure how this was handled during previous testnet upgrades though.
There was a problem hiding this comment.
lightning postdates tn3
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @jkczyz! This PR has been waiting for your review. |
tnull
force-pushed
the
2025-10-testnet4-support
branch
from
a0b4c2e to
c49c982
Compare
|
Since the other Lightning implementations seem to just map @TheBlueMatt @benthecarman Any opinions how to do better if the community doesn't seem to be enthused about introducing a separate prefix for testnet4? |
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
Hmm, yea, this is fine. Annoying, but fine. Maybe the docs for Currency::Testnet should be updated to reference that its both?
|
It seemed like people on the call were just viewing testnet4 LN as DoA, so not much reason to worry about supporting it well IMO. |
We simply add support for `bitcoin::Network::Testnet4`, which was added with release v0.32.4, which we hence deem the new minimal requirement for `lightning` and `lightning-invoice`. As other Lightning implementations seem to simply map `Testnet4` coins to be `Testnet` (also reusing the `tb` prefix), we here follow their lead and choose not to introduce a separate `Currecy` variant, even though that could lead to checks failing if someone validated the returned `fallback_addresses` for the given `Network`.
tnull
force-pushed
the
2025-10-testnet4-support
branch
from
c49c982 to
743f43f
Compare
Amended with some more docs: > git diff-tree -U2 c49c982bb 743f43fcf
diff --git a/lightning-invoice/src/lib.rs b/lightning-invoice/src/lib.rs
index 2bc6e53d9..0dbb3bdb0 100644
--- a/lightning-invoice/src/lib.rs
+++ b/lightning-invoice/src/lib.rs
@@ -439,4 +439,6 @@ pub enum Currency {
/// Bitcoin testnet
+ ///
+ /// Note this will also be used for invoices on [`Network::Testnet4`].
BitcoinTestnet,
@@ -1623,4 +1625,7 @@ impl Bolt11Invoice {
/// Returns the network for which the invoice was issued
///
+ /// **Caution**: On Testnet4, this method will return [`Network::Testnet`]` rather than
+ /// [`Network::Testnet4`].
+ ///
/// This is not exported to bindings users, see [`Self::currency`] instead.
pub fn network(&self) -> Network { |
benthecarman
left a comment
benthecarman
left a comment
There was a problem hiding this comment.
mutinynet stock rises more
@TheBlueMatt @benthecarman Any opinions how to do better if the community doesn't seem to be enthused about introducing a separate prefix for testnet4?
i think the caution docs are probably sufficient for now, it is only testnet 3 vs 4 so not like people can lose money or anything on this
|
CI failure looks like a flake. |
4 participants
We simply add support for
bitcoin::Network::Testnet4, which was added with release v0.32.4, which we hence deem the new minimal requirement forlightningandlightning-invoice.