-
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 69 (2019-10-23) #239
Conversation
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.
Great job as always @harding!
one hour each week being a group meeting and the other three hours | ||
being your own independent review of the proposals. In addition to | ||
review, developers will be encouraged to optionally implement a | ||
proof-of-concept that either roughly intergrates schnorr or 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.
intergrates -> integrates
documentation. | ||
|
||
The ultimate goal of the review club is to allow participants to | ||
gain enough technical familarity with the proposals to be able 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.
familarity -> familiarity
|
||
## News | ||
|
||
- **Taproot review club:** starting FIXME:date, several Bitcoin |
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 believe this is scheduled for the first week of November
has to claim its particular payment---from 144 blocks to 40 blocks | ||
(see [Newsletter #40][lnd cltv delta]) but older LND nodes and some | ||
other implementations have continued to use 144 as their default. At | ||
a delta 144, the maximum expiry of 2,016 makes the maximum-length |
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.
delta 144 -> delta of 144
either vocally support the proposals, advocate for changes to the | ||
proposals, or lucidly explain why the proposals shouldn't be adopted | ||
into the Bitcoin consensus rules. Changing Bitcoin is something | ||
that shouldn't be done lightly and it's in every user's interest |
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: done lightly or taken lightly?
@adamjonas edits made. Thanks!, and sorry that I apparently forgot to spell check this week. |
Added a |
For a name alternative, maybe call it "Changes to services and client software". The current text LGTM but the section should probably be mentioned as part of the summary in the newsletter's first paragraph. |
[libsecp256k1][libsecp256k1 repo], [Bitcoin Improvement Proposals | ||
(BIPs)][bips repo], and [Lightning BOLTs][bolts repo].* | ||
|
||
- [LND #3696][] raises the default maximum CLTV expiry from 1,008 blocks |
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 think this is LND #3595 instead?
ACK |
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.
Looks good. I've left a bunch of style nits. Feel free to ignore.
that a large number of technical reviewers examine the proposals for | ||
possible flaws before they are implemented and before users are | ||
asked to consider upgrading their full nodes to enforce the new | ||
rules---a change that can't be safely undone for as long as 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.
Either expand "a change that can't be safely undone for as long as the rules affect anyone's bitcoins." into its own sentence, or remove it entirely.
- [LND #3696][] raises the default maximum CLTV expiry from 1,008 blocks | ||
(about 1 week) to 2,016 blocks (about two weeks). This is the maximum | ||
amount of time a new payment will be valid before it can be reclaimed | ||
by its spender if it hasn't been accepted by the receiver. LND |
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 think the use of 'spender' and 'receiver' is slightly confusing here. If I understand this correctly, the CLTV value is for use by any hop in the route to claim the payment from the next hop. That's why it needs to be decremented for each hop.
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.
Right, but the max CLTV value is the value that applies to the first hop, e.g. if you try to send a payment to me and I don't accept it (and nobody along the route rejects it), you'll have to wait the max CLTV blocks before you can spend your money again. Thus its the max value that most concerns the user because it's the maximum amount of time that some of their funds can be stuck.
by its spender if it hasn't been accepted by the receiver. LND | ||
recently tried to keep this at 1,008 blocks by decreasing the CLTV | ||
delta---the amount of time each routing node along the payment path | ||
has to claim its particular payment---from 144 blocks to 40 blocks |
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 think it would be clearer to explicitly say that the delta is the value that the CLTV is decremented in each hop.
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.
That would be a good description of how it works but I feel the current description better communications what it accomplishes---assures that you have a minimum of x blocks in which to claim your payment. The value is important because, if your node is offline for more than x blocks, someone might be able to steal from you.
has to claim its particular payment---from 144 blocks to 40 blocks | ||
(see [Newsletter #40][lnd cltv delta]) but older LND nodes and some | ||
other implementations have continued to use 144 as their default. At | ||
a delta of 144, the maximum expiry of 2,016 makes the maximum-length |
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.
... if all hops in the route use a delta of 144.
other implementations have continued to use 144 as their default. At | ||
a delta of 144, the maximum expiry of 2,016 makes the maximum-length | ||
routing path about 14 hops; at a delta of 40, it's theoretically about | ||
50 (but [BOLT4][] limits this to 20, or slightly more with recent |
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.
It's a little confusing to say that the limit is 50 but that you can't actually reach that limit. I'd omit this.
Pushed edits @bitschmidty feedback and most @jnewbery feedback (replies left for remaining points). Also changed the action items section to point to RCs for both Bitcoin Core and C-Lightning. Note: I implemented this suggestion of John's as a separate move-only commit to hopefully keep re-review easy. |
ACK fb589d1 |
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.
Great job @harding -- sorry for the late review. No worries if it's too late to take it into account.
|
||
## News | ||
|
||
- **Taproot review club:** starting the first week of November, several Bitcoin |
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.
Semantic nit: in the Square Crypto proposal, no mention is made of a club and there could be potential confusion with the PR review club as they can be seen as similar like here.
Suggest: s/Taproot review club/Taproot BIP review/
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 overview has a section entitled "What happens this review club is over". Also, the idea was originally pitched as "BIP-proposal-review-club".
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.
Do you think there's harm in calling it a review club? I didn't think so myself, as any confusion seems likely to me to be a chance to promote both organizations.
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.
Oh ok, we're not looking at the same doc. Yes, I would avoid confusion between the two unless it's expressly desired to extend the concept, but it's not my call.
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.
TIL about aj's doc. Interestingly it does not have any dates for the sessions, apart from the Oct. 30 signup, whereas moneyball's doc launches the sessions on November 5th.
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 pinged the people involved and will follow up on this. Thanks for raising this point! Edit: talking about the naming; not the dates.
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 agree with @jonatack there should be no reference to "club." I've removed it in this PR but will need @ajtowns to merge. ajtowns/taproot-review#1
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 also created a PR to add the starting date ajtowns/taproot-review#2
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.
and finally, a suggested renaming back to the original name ajtowns/taproot-review#3
the current proposals that might be missed by people who only read the | ||
documentation. | ||
|
||
The ultimate goal of the review club is to allow participants 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.
idem, s/review club/taproot BIP review/
that a large number of technical reviewers examine the proposals for | ||
possible flaws before they are implemented and before users are | ||
asked to consider upgrading their full nodes to enforce the new | ||
rules. Whether through the review club 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.
idem about the club
asked to consider upgrading their full nodes to enforce the new | ||
rules. Whether through the review club or | ||
in some other way, Optech strongly encourages all technically | ||
skilled Bitcoin users to dedicate time to reviewing the taproot set |
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.
While discussing this review initiative with other devs because I find it an interesting idea, one recurring question from people has been the following, for which I don't have an answer and which might be helpful to address: "Why not review in the relevant repository, has this been insufficient?"
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.
My understanding is that Pieter Wuille considers the amount of feedback received to date to be insufficient. Since Pieter isn't organizing this effort (though I believe he'll be available to answer questions and receive feedback), it would odd to organize it through his repository. Besides, opening PRs in developer forks is really just for minor stuff; major feedback on the proposals should be emailed to the dev list.
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.
ultimately, any feedback should end up in the repository (as an issue or PR), or on the mailing list, or both; the idea here is this provides an excuse for people who are already comfortable with that to do it sooner rather than later, and to help people who aren't yet comfortable with it to still be able to contribute to the review.
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.
Anyone wanting to participate should [RSVP][trc rsvp] soon so that the | ||
organizers can estimate the total number of participants and start | ||
forming study groups. To register or learn more, please see | ||
the [Taproot Review Club][trc] repository. |
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.
idem
by its spender. LND | ||
recently tried to keep this at 1,008 blocks by decreasing the CLTV | ||
delta---the minimum number of blocks each routing node along the payment path | ||
has to claim its particular payment---from 144 blocks to 40 blocks |
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 minimum number of blocks each routing node along the payment path has to claim its particular payment" -> I think this sentence could be clearer, in the vein of replacing "has" with a better verb or reconstructing the sentence a bit.
- [LND #3597][] reverts the migration policy described in [Newsletter | ||
#64][lnd3485] where LND could only be upgraded from a maximum of one major | ||
release back. The PR for the reversion notes, "the prior stricter | ||
policy created a large burden on applications that package lnd as |
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.
Ah yes, there was some strong blowback from application developers on that prior upgrade change.
Dropped "club" from the taproot review and made some other minor edits, including adding the links. |
ACK d09fd5f |
1 similar comment
ACK d09fd5f |
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.
ack
people through review of the proposed [bip-schnorr][], | ||
[bip-taproot][], and [bip-tapscript][] changes. All developers, | ||
academics, and anyone else with technical experience are welcome. The | ||
minimum expected commitment is four hours a week for seven weeks, with |
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 expected commitment" no need for minimum?
ACK a993736 |
ACK a993736 |
a993736
to
9f3227d
Compare
Todo: