-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Newsletters: add 108 (2020-07-29) #442
Conversation
allow two peers with an existing channel between them to negotiate a | ||
new commitment transaction using a different format. Commitment | ||
transactions are used to allow LN nodes to put the current channel | ||
state onchain, but the existing protocol only allows nodes to upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we use 'onchain' as an adjective, but its use here as a preposition seems a bit unusual.
state onchain, but the existing protocol only allows nodes to upgrade | ||
to new commitment formats by opening new channels. Osuntokun's | ||
proposal would allow a node to signal to its peer that it wants to | ||
switch formats; if the peer agrees, the two nodes would port their |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/; if/. If/
Development Meeting in relation to taproot activation. It was noted | ||
that the version of taproot merged into Bitcoin Core (see PR | ||
[#17977][Bitcoin Core #17977]) will likely be based on wtxid relay. | ||
This will make it more complicated to backport taproot to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To summarise what I think the problem is:
- nodes that are unaware of segwit v1 (taproot) rules will reject taproot transactions that spend segwit v1 outputs since they're non-standard. See https://github.com/bitcoin/bitcoin/blob/b62fbf9e1c193464bca443076906b7ea47d56532/src/script/interpreter.cpp#L1552-L1554. (Note that we can send to a segwit v1 address since [POLICY] Make sending to future native witness outputs standard bitcoin/bitcoin#15846)
- if using txid relay, that node can't add the txid to the
recentRejects
filter, since the problem could just have been that the witness was mutated. See https://github.com/bitcoin/bitcoin/blob/0.20/src/net_processing.cpp#L2599-L2604 in v0.20. - that means that taproot-aware nodes will announce taproot-spending transactions to their taproot-unaware peers, those peers will download and reject the transaction, but will then redownload the transaction (and rereject it) every time another peer announces it to them.
- with wtxid relay, this isn't a problem. The taproot-unaware node will store the wtxid in its recentRejects, and never try to redownload it.
- therefore we should only announce taproot transactions using wtxids.
I'm not sure how difficult it actually is to backport wtxid relay. It seems to me that it shouldn't be too hard. v0.20 was only released a few weeks ago, and the wtxid PR was open long before that. I think we should check on how difficult it actually is before running with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @jnewbery that it's not /that/ hard to add wtxidrelay to 0.20 -- it's just not a matter of cherry-picking the patch from master; the code will need to be adapted and re-reviewed. For most backports it /is/ just a mater of cherry-picking a patch, so this would just be "hard" in comparison to that, in my opinion.
The "only announce taproot transactions using wtxids" is an additional change needed (that's not done yet, afaik) to actually avoid 0.20 and earlier nodes from spamming themselves by re-requesting taproot spends that they don't understand by txid from every peer.
The drawback of releasing taproot in a 0.20.x release but only for block consensus but not tx relay is that it would both reduce the ability to actually use taproot when it activated (if you can't spend a taproot tx via p2p and get it reliably to the miners, taproot's not really usable), and, if people did use taproot, would reduce the effectiveness of compact block relay (since nodes wouldn't already have the taproot spending transactions). All of which is to say this approach still means waiting for 0.21.y before taproot's usable, so doesn't really speed anything up compared to just releasing taproot in 0.21.y.
I think it would be super-useful to expand on the explanation in the newsletter a bit; there isn't any other canonical reference of how taproot/wtxid interacts anywhere else to point people to apart from the not very explicit description in the wtxid PR description, and having one would be helpful :)
transactions for [anchor outputs][topic anchor outputs]. Not yet | ||
added is higher-level support that will allow Eclair to negotiate | ||
the use of anchor outputs with peers, but this early step does pass | ||
all [proposed test vectors][BOLTs #688]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should link directly to the comment: lightning/bolts#688 (comment)?
Bitcoin Core #19473 summary added. Also added a todo item for the River Financial field report as it looks that is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments and corrections. The one I see to tighten up is the wtxid's impact on taproot activation paragraph. Thanks @harding!
All discussion participants seemed to support the basic idea. In | ||
discussion, Bastien Teinturier [suggested][teinturier simple] that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cut wordiness and reduce repetition
All discussion participants seemed to support the basic idea. In | |
discussion, Bastien Teinturier [suggested][teinturier simple] that | |
All discussion participants seemed to support the basic idea. Bastien Teinturier [suggested][teinturier simple] that |
All discussion participants seemed to support the basic idea. In | ||
discussion, Bastien Teinturier [suggested][teinturier simple] that | ||
it would be simplest to only allow switching commitment formats when | ||
channels had no pending payments (HTLCs); this implies nodes would |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
channels had no pending payments (HTLCs); this implies nodes would | |
channels had no pending payments (HTLCs); this that implies nodes would |
|
||
ZmnSCPxj [noted][zmnscpxj re-funding] that the same basic idea could | ||
be used to essentially update the funding transaction offchain, for | ||
example in the case where proposals such as [taproot][topic taproot] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example in the case where proposals such as [taproot][topic taproot] | |
example, in the case where proposals such as [taproot][topic taproot] |
current 0.20.x branch of Bitcoin Core and so possibly ensure that | ||
the earliest release of taproot activation logic will be in the | ||
Bitcoin Core 0.21.x branch, whose major release is roughly | ||
[expected][Bitcoin Core #18947] in December 2020. A proposed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at @jnewbery's comments, maybe there is a larger re-write here, but noting that this last sentence is a little hard to follow. I'd suggest breaking into two sentences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Waiting to rewrite based on @jnewbery's comments until I see confirmation that John's summary is basically what AJ was thinking (John asked about it in the ##taproot-activation channel).
all [proposed test vectors][BOLTs #688]. | ||
|
||
- [LND #4455][] enables safe PSBT-based batched channel opens. Previously, each | ||
successful channel negotation in the batch would prematurely broadcast the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
successful channel negotation in the batch would prematurely broadcast the | |
successful channel negotiation in the batch would prematurely broadcast the |
whole transaction with all channel funding outputs. This meant that it was | ||
possible for subsequent channel negotiation failures to result in stuck funds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Slightly simpler
whole transaction with all channel funding outputs. This meant that it was | |
possible for subsequent channel negotiation failures to result in stuck funds. | |
whole transaction with all channel funding outputs. This meant | |
subsequent channel negotiation failures could result in stuck funds. |
Added commits for the field report and stack exchange. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits on the River report, the B.SE section, and Mike and Carl's PRs. Everything looks basically good to me; I'll make any edits tomorrow which still need to be addressed. Thanks everyone!
possible for subsequent channel negotiation failures to result in stuck funds. | ||
This merged PR introduces a `--no_publish` flag to the `openchannel` command, | ||
which can be used to delay transaction broadcast until the very last channel | ||
in the batch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the batch. | |
in the batch has been created. |
|
||
- **Upgrading channel commitment formats:** Olaoluwa Osuntokun | ||
[posted][osuntokun upgrade] to the Lightning-Dev mailing list this | ||
week with a suggested extension to the LN specification that will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
µ-nits in this paragraph:
-
s/will/would/ allow (there is also "Osuntokun’s proposal would allow" and "would port" in the same paragraph)
-
"between them" seems unneeded
-
maybe s/upgrade to new/change/ commitment formats
-
s/on chain/on-chain/? ("put something on chain" seems awkward)
(started reviewing, but I'll be offline for half an hour, so posting this in case GH loses it)
Addressed feedback from @harding and @adamjonas , thanks guys! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great newsletter. Thanks to all the authors!
I have a couple of small comments inline. Otherwise looks really good.
|
||
The solution implemented in this merged PR is to announce | ||
transactions by their wtxid---which includes a commitment to the | ||
witness data for segwit transactions. It is expected that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'expected' is maybe a bit strong. When I asked Pieter about whether we'd only relay taproot using wtxid relay, he said that he hadn't really thought about it. Perhaps you could soften this to something like "A taproot implementation in Bitcoin Core could then only relay transactions by their wtxid ..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit more than halfway through, but since I'm reviewing somewhat late in the cycle I'm putting up this feedback now to hopefully allow time to use it.
would be paid to a new funding transaction that is kept offchain. If | ||
the channel terminates with a mutual close, the original funding | ||
transaction output is paid to the final channel balances; otherwise, | ||
the offchain secondary funding transaction can be published onchain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: here there is "published onchain"; two paragraphs above there is "publish...on chain"
2. **Creating and importing new wallets is easy because the descriptor language | ||
is able to define desired scripts.** River is able to maintain many wallets | ||
with different scripts and as a result have separate security models for each | ||
wallet. A P2WSH multi-signature descriptor is used for the cold wallet and a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 previous uses of "multi-signature" in this report are not hyphenated; maybe settle on one of the two spellings.
in the feature set. Descriptors is a language for describing scripts that was | ||
authored by Pieter Wuille and used in Bitcoin Core. In River’s wallet software, | ||
the descriptor language is leveraged in several places from wallet creation to | ||
address generation. Before descriptors, there had been no interoperable way for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"interop*" (interoperability, interoperate, interoperable, and interoperability) is used four times in the report. Perhaps vary the form ("universal", "industry-wide", "compatible", etc.) or clarify the messages if they are not saying the same thing.
greatly reduced the complexity in wallet operations and has improved flexibility | ||
in the feature set. Descriptors is a language for describing scripts that was | ||
authored by Pieter Wuille and used in Bitcoin Core. In River’s wallet software, | ||
the descriptor language is leveraged in several places from wallet creation to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the descriptor language is leveraged in several places": "leverage" is already used in the second paragraph. Perhaps "deployed", "used", or something else if a more precise meaning is intended.
|
||
The decision to implement Output Script Descriptors in the wallet software has | ||
greatly reduced the complexity in wallet operations and has improved flexibility | ||
in the feature set. Descriptors is a language for describing scripts that was |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"flexibility" is already stated in the second and in the last paragraph and it's somewhat unclear what "flexibility in the feature set" means here before reading the bullet points below...perhaps ease/speed of new/custom feature development, expanded the possible feature set, or allowed a wider range of features.
|
||
- [What are the different upgradability features in the BIP-Taproot (BIP341) proposal?]({{bse}}96951) | ||
Michael Folkson answers a question from Twitter regarding the different ways | ||
in which taproot can enable upgradability including leaf versions for script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: a comma seems to be missing after "upgradability"
(fwiw it seems both versions are acceptable but I do prefer "upgradeability" with an "e", as otherwise it seems like "grad" would be pronounced like "graduate" or "Stalingrad" :-))
for taproot. Pieter also answers a [similar question][stack exchange miner | ||
signaling] about mining signaling support. | ||
|
||
- [Could we skip the Taproot soft fork and instead and use Simplicity to write the equivalent of Taproot scripts?]({{bse}}97049) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/and instead and use/and instead use/
|
||
- [Could we skip the Taproot soft fork and instead and use Simplicity to write the equivalent of Taproot scripts?]({{bse}}97049) | ||
Michael Folkson, quoting Pieter Wuille, outlines the current state of | ||
[Simplicity][news96 simplicity] noting also that integrating Simplicity in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ISTM there is a missing comma before "noting", or alternatively s/noting also/and also notes/
[Hardware Wallet Interface (HWI)][hwi], [Bitcoin Improvement Proposals | ||
(BIPs)][bips repo], and [Lightning BOLTs][bolts repo].* | ||
|
||
- [Bitcoin Core #19473][] adds support for `networkactive` as both a command line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest placing this PR after 18044 wtxid relay (which to me is the headlining change of the week's news, but I digress)
(BIPs)][bips repo], and [Lightning BOLTs][bolts repo].* | ||
|
||
- [Bitcoin Core #19473][] adds support for `networkactive` as both a command line | ||
start-up and configuration file option. Setting this option enables or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not sure "support for", "both" and "start-up" are needed; maybe just "adds networkactive
as a command line and configuration file option."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments. All the links seem good. Great work on the wtxid relay discussion! This for me is the highlight of the week. Apologies for the late review. Who said summer was quiet?
transactions to their peers by the transaction's txid. However, txids | ||
don't commit to the witness data in segwit transactions, so a node who | ||
downloaded an invalid or unwanted segwit transaction can't safely | ||
assume that any transaction with that same txid is also invalid or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/that/the/
announce transactions they wouldn't accept themselves, so only a | ||
disruptive peer that wanted to waste its own upload bandwidth would | ||
advertise invalid or unwanted transactions. However, one type of | ||
unwanted transaction today are spends of v1 segwit UTXOs---the types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after "one type of..." maybe s/are spends of/is the spending of/
|
||
However, after this PR was merged into Bitcoin Core's master | ||
development branch, it was [discussed][meeting xscript] during the | ||
weekly Bitcoin Core Development Meeting whether taproot's soft |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe "weekly Bitcoin Core development meeting"
|
||
1. **Backport wtxid:** both wtxid relay and taproot will be | ||
backported if there's a 0.20.x taproot release. John Newbery has | ||
already created a quick [wtxid relay backport][]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe omit "quick"
also arguably unclear what it means -- quick to make, quick to review, (quick in performance ;), etc.
|
||
4. **Don't backport anything:** don't backport wtxid relay or | ||
taproot---let taproot wait until after the release of Bitcoin | ||
Core 0.21, roughly [expected][Bitcoin Core #18947] in December |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be sure no one reading this will confuse it with saying that taproot is expected in December, maybe drop the phrase about the release timing.
Core 0.21, roughly [expected][Bitcoin Core #18947] in December | ||
2020. | ||
|
||
No clear conclusion on which of these options to follow has been |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe: "At the time of this writing, no consensus was reached yet on which option to follow."
to this merge, all Bitcoin nodes announced new unconfirmed | ||
transactions to their peers by the transaction's txid. However, txids | ||
don't commit to the witness data in segwit transactions, so a node who | ||
downloaded an invalid or unwanted segwit transaction can't safely |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/who/that/
s/downloaded/downloads/ or s/can't/couldn't/
weekly Bitcoin Core Development Meeting whether taproot's soft | ||
dependency on wtxid relay will make it more complicated to backport | ||
taproot to the current 0.20.x branch of Bitcoin Core. Four options | ||
were mentioned during the meeting and in subsequent discussions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latest suhas/sipa irc discussion at http://www.erisian.com.au/bitcoin-core-dev/log-2020-07-28.html#l-511 and http://www.erisian.com.au/bitcoin-core-dev/log-2020-07-29.html#l-3 may be relevant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are good, but I think they basically resolve into "backporting wtxid is best", so I'm going to leave readers just the meeting link.
4d990ae
to
7ce3d3b
Compare
@philipglazman Thank you for your contribution on the field report from River! (dessert?) |
(And @bitschmidty with the gravy!) |
Bitcoin Core #19473
@bitschmidtyLND #4455
@dongcarlRiver Field Report
@bitschmidty