Skip to content
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 297 (2024-04-10) #1608

Merged
merged 2 commits into from
Apr 10, 2024

Conversation

harding
Copy link
Collaborator

@harding harding commented Apr 8, 2024

  • Lede, releases/RCs, topic entries
  • Bitcoin PR Review Club @stickies-v

@stickies-v here's the source to last month's review club section for reference: https://raw.githubusercontent.com/bitcoinops/bitcoinops.github.io/master/_posts/en/newsletters/2024-03-13-newsletter.md Please don't hesitate to ask here, on IRC, or wherever if you have any questions. Thanks for writing! (And many thanks to @LarryRuane for his excellent work on this section every month since Aug 2022!)

@stickies-v
Copy link
Collaborator

stickies-v commented Apr 8, 2024

I don't think I have write access here, so I've pushed a commit to add review club summary to my own repo: 1031b8c 8cb85fc

I tried to be mindful of the formatting and scripts used here but I'm not very familiar with this style so please let me know if something's off?

@LarryRuane
Copy link
Collaborator

@stickies-v - Looks pretty good, two small things: a0 needs a close quote, and we tend to write shorter text source lines (up to around 80 columns), just because it makes the diffs easier to read if there are later edits. I didn't review for content but will try to do that in the next day or so. Thanks for taking this on!

Very minor suggestion, this might sound funny, but I've found it useful to "find on page" the question mark character when looking at the review club page (with the logs) in the browser -- the questions stand out that way and I can see if I can incorporate any of them in my writeup. (There are often questions that come up during the discussion that weren't written by the host ahead of time.)

(I had the same write access problem, but Dave fixed it.)

Another tip, in case no one mentioned it yet, don't force-push (either your initial commit or later corrections), because that could back out someone else's commit since we have multiple people pushing to the same branch. (I did that at least once!) Just do normal pushes, and Mike always squashes just before merging.

new BIP editors mentioned in [last week's newsletter][news296 editors]
has been [extended][erhardt editors] to the UTC end of day on Friday,
April 19th. It is hoped that the new editors will receive merge
access by the end of day on the following Monday.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
access by the end of day on the following Monday.
access by the end of the day on the following Monday.

began assigning economic value to testnet coins, resulting in them
becoming hard to acquire for free for people who wanted to perform
actual testing. Lopp provided evidence of that happening again and
also described the well-known problem of block flooding due
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
also described the well-known problem of block flooding due
also described the well-known problem of block flooding due to

- [Bitcoin Core #29130][] adds two new RPCs. The first will generate a
descriptor for a user based on the settings they want and then add
that descriptor to their wallet. For example, the following command
will add support for [taproot][topic taproot] to a old wallet created
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
will add support for [taproot][topic taproot] to a old wallet created
will add support for [taproot][topic taproot] to an old wallet created

When a wallet contains multiple xpubs, the particular one to use can
be indicated when calling `createwalletdescriptor`.

- [LND #8159][] and [#8160][lnd #8160] adds experimental (disabled by
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [LND #8159][] and [#8160][lnd #8160] adds experimental (disabled by
- [LND #8159][] and [#8160][lnd #8160] add experimental (disabled by

forwards to Dan below the average amount he forwards to her,
eventually all of the channel balance will end up on Carol's side,
preventing Dan from being able to forward more payments to her,
reducing both of their revenue potential. To prevent that, Carol
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
reducing both of their revenue potential. To prevent that, Carol
reducing both of their revenue potentials. To prevent that, Carol

high fee for payments arriving inbound from Alice's problematic node
but a lower fee for payments arriving inbound from Bob's highly liquid
node. Initial inbound fees are expected to always be negative to make
them backwards compatible with older nodes that don't understand
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
them backwards compatible with older nodes that don't understand
them backward compatible with older nodes that don't understand

any node that's now aware of inbound fees may be able to receive a
discount.

Inbound routing fees was first [proposed][bolts #835] about three
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads a little weird because "fees" seems to disagree with "was" (subject-verb plurality disagreement). It could be argued that it's okay because "inbound routing fees" is a single feature. But maybe something like:

Suggested change
Inbound routing fees was first [proposed][bolts #835] about three
The inbound routing fees feature was first [proposed][bolts #835] about three

Inbound routing fees was first [proposed][bolts #835] about three
years ago, was [discussed][jager inbound] on the Lightning-Dev mailing
list about two years ago, and was documented in draft [BLIPs #18][]
also about two years ago. Since it's initial proposal, several
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
also about two years ago. Since it's initial proposal, several
also about two years ago. Since its initial proposal, several

Comment on lines 229 to 230
years ago, was [discussed][jager inbound] on the Lightning-Dev mailing
list about two years ago, and was documented in draft [BLIPs #18][]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
years ago, was [discussed][jager inbound] on the Lightning-Dev mailing
list about two years ago, and was documented in draft [BLIPs #18][]
years ago, [discussed][jager inbound] on the Lightning-Dev mailing
list about two years ago, and documented in draft [BLIPs #18][]

advertisement of additional fee details for each channel. An
alternative approach is proposed in draft [BLIPs #22][]. We're only
aware of one maintainer of a non-LND implementation
[indicating][corallo free money] that they'll adopt LND's method---and
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[indicating][corallo free money] that they'll adopt LND's method---and
[indicating][corallo free money] that said they'll adopt LND's method---and

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving as-is. I think "said" doesn't make sense in this sentence that already contains "indicating".

cause issues for regular users. Developers who do want to work with
emulators now need to pass an additional `--emulators` option.

{% assign day_after_posting = page.date | date: "%s" | plus: 86400 | date: "%Y-%m-%d 14:30" %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{% assign day_after_posting = page.date | date: "%s" | plus: 86400 | date: "%Y-%m-%d 14:30" %}
{% assign day_after_posting = page.date | date: "%s" | plus: 86400 | date: "%Y-%m-%d 16:00" %}

description of [HWI #729][] below for details.

- [Core Lightning 24.02.2][] is a maintenance release that fixes "a
[small incompatibility][core lightning #7174]" between Core Lighting's
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[small incompatibility][core lightning #7174]" between Core Lighting's
[small incompatibility][core lightning #7174]" between Core Lightning's

Bitcoin Core.

- [Bitcoin Core #29130][] adds two new RPCs. The first will generate a
descriptor for a user based on the settings they want and then add
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
descriptor for a user based on the settings they want and then add
[descriptor][topic descriptors] for a user based on the settings they want and then add

[news292 lndcs]: /en/newsletters/2024/03/06/#lnd-8378
[news288 libconsensus]: /en/newsletters/2024/02/07/#bitcoin-core-29189
[teinturier bolts835]: https://github.com/lightning/bolts/issues/835#issuecomment-764779287
[corallo free money]: https://github.com/lightningnetwork/lnd/pull/6703#issuecomment-1374694283
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong link

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, good catch, thanks!

@stickies-v
Copy link
Collaborator

Thanks for the review, @LarryRuane and @bitschmidty . I've pushed an updated commit 8cb85fc that:

  • fixes the missing quote issue (verified by actually building the website this time)
  • uses the proper linker mechanism for the BIP and bitcoin/bitcoin/pull
  • wraps to line length 72

@murchandamus
Copy link
Collaborator

Another tip, in case no one mentioned it yet, don't force-push

It’s generally a good idea to always use --force-with-lease unless you know you need to use --force.

@harding
Copy link
Collaborator Author

harding commented Apr 9, 2024

Reviewed @stickies-v's section, looks great, thanks! I made one small edit to pass our tests and one small edit to clarify a remark (since BIP62 was never activated, minimial-size pushes are only required in relay/mempool for legacy transactions, although I believe they're required for both segwit v0 and v1 at the consensus layer).

Made edits (or left a reply) for all review comments to date (thanks @LarryRuane @bitschmidty !), added lede, releases/RCs, and topic entries. This should be ready to go, but I'll check back later to see if there are any additional reviews.

Thanks everyone!


- **Updating BIP2:** Tim Ruffing [posted][ruffing bip2] to the
Bitcoin-Dev mailing list about updating [BIP2][], which specifies the
current process for adding new BIPs and maintaining updates to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
current process for adding new BIPs and maintaining updates to
current process for adding new BIPs and updating

q2="Why was the `fRequireMinimal` flag introduced to `CScriptNum`?"
a2="`CScriptNum` has a variable length encoding. As described in
[BIP62][] (rule 4), this introduces opportunity for malleability. For
example, zero can be encoded as `[]`,<!-- skip-test --> `0x00`, `0x0000`, ... [Bitcoin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps clearer:

Suggested change
example, zero can be encoded as `[]`,<!-- skip-test --> `0x00`, `0x0000`, ... [Bitcoin
example, zero can be encoded as the empty vector,<!-- skip-test --> `0x00`, `0x0000`, ... [Bitcoin

Could also be avoided by choosing a different number.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to OP_0 since that's a synonym for the empty byte array.

Copy link
Collaborator

@LarryRuane LarryRuane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e27ce2a

@harding
Copy link
Collaborator Author

harding commented Apr 9, 2024

Addressed @vostrnad additional comments (thanks! and thanks @LarryRuane for re-reviewing!)

Copy link
Contributor

@bitschmidty bitschmidty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, fixups, lede, topics, PR Review Club

@bitschmidty bitschmidty force-pushed the 2024-04-10-newsletter branch from dfec3fb to df65269 Compare April 10, 2024 07:23
@bitschmidty bitschmidty merged commit 4e1b5ed into bitcoinops:master Apr 10, 2024
2 checks passed
When a wallet contains multiple xpubs, the particular one to use can
be indicated when calling `createwalletdescriptor`.

- [LND #8159][] and [#8160][lnd #8160] add experimental (disabled by
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey opetch! sorry to be this week's "um, actually" but lightningnetwork/lnd#8160 adds forwarding support to LND so that it can be used by other nodes in the network for blinded path creation. It's just disabled because we need lightningnetwork/lnd#8485 to be fully spec-compliant with error handling (NB for blinded path privacy), and it'll be turned on by default once that goes through.

Sending support (on some APIs) was added in lightningnetwork/lnd#7267, though admittedly the PR title is terrible so it's not particularly clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants