Skip to content

Newsletters: add 65 #227

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

Merged
merged 2 commits into from
Sep 25, 2019
Merged

Conversation

harding
Copy link
Collaborator

@harding harding commented Sep 19, 2019

No description provided.

Copy link
Collaborator Author

@harding harding left a comment

Choose a reason for hiding this comment

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

A couple concerns about confusion, but otherwise the B.SE section looks great. Thanks, @bitschmidty!

- [Why does this coinbase transaction have two op_return outputs?]({{bse}}90127)
Murch outlines that while there are some restrictions on coinbase transactions,
there are no special script or quantity restrictions regarding coinbase
transaction outputs.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Murch's full answer seems a bit confused to me by noting a place where generation transaction outputs are restricted (the case where the block contains segwit transactions) and then saying that there are no restrictions. Your summary only quotes the later part and so (it seems to me) twists the answer from confusing to wrong. I left a comment for Mark on the SE; maybe this could say something like "....regarding coinbase transaction outputs, save that any blocks containing segwit spends must have an output matching the BIP141 commitment structure."

Copy link
Contributor

Choose a reason for hiding this comment

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

Pushed a change based on Murch's revised answer and your feedback. Feel free to tweak!

- [Why was 550 (MiB) chosen as a minimum storage size for prune mode?]({{bse}}90140)
Behrad Khodayar answers his own question about why 550 (MiB) was chosen
as a minimum storage size for pruned nodes. The storage amount was chosen
to keep 288 blocks, about two days worth, on disk.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe say "...keep 288 blocks worth of state transition data, about two days worth, on disk" as the data included in this calculation is both blocks and undo data (undo data is the UTXOs each block spends and which are then removed from the UTXO set; if the block is later forked off the chain, those UTXO entries need to be re-added to the database).

The original answer doesn't note this and I'm not sure we do either---since the question is about why the original number was chosen---but the two day value no longer stands as the maximum size of blocks was increased by segwit (also the typical size of undo data, although I think the adversarial size remains the same).

Copy link
Contributor

Choose a reason for hiding this comment

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

Took your suggestion and also added "originally" and "pre-segwit". You’re welcome to push a change if there is a better wording.

@harding
Copy link
Collaborator Author

harding commented Sep 24, 2019

Untested bf20f46 SE section LGTM. Thanks!

@jnewbery
Copy link
Contributor

jnewbery commented Sep 24, 2019

I've only had time for a quick skim this week, but everything looks good to me. Thanks @harding! And thanks @bitschmidty for the SE section!

Copy link
Collaborator

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Great newletter! Embarassed apologies for all the comments below--your time is short, feel free to gleefully ignore, my ego isn't tied to them :) Tested the four links at the bottom, and they are all worth re-reading after this review. Thanks again, Dave.

creating taproot inputs by paying P2SH outputs (the way you can
currently use P2WPKH and P2WSH inputs created from P2SH outputs).
Anyone who expects to need P2SH-wrapped taproot should describe their
planned usecase on the mailing list or by contacting the bip-taproot
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/usecase/use case/?

currently use P2WPKH and P2WSH inputs created from P2SH outputs).
Anyone who expects to need P2SH-wrapped taproot should describe their
planned usecase on the mailing list or by contacting the bip-taproot
author privately.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"contacting the author" -> perhaps more details, unless the idea is to not facilitate doing so :)

- **Tapscript resource limits:** the [bip-tapscript][] proposal limits
transactions to one signature-checking operation (sigop) for every 12.5 vbytes
the witness data adds to the size of the transaction (plus one free
sigop per input). Because signatures are expected to be 16.0 vbytes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

sigop or SigOp, perhaps a style guide candidate unless an obvious/pre-established choice

- **Tapscript resource limits:** the [bip-tapscript][] proposal limits
transactions to one signature-checking operation (sigop) for every 12.5 vbytes
the witness data adds to the size of the transaction (plus one free
sigop per input). Because signatures are expected to be 16.0 vbytes,
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: leading because; maybe "Signatures are expected to be 16 vbytes, so/therefore this limit should prevent abuse..." or similar

the witness data adds to the size of the transaction (plus one free
sigop per input). Because signatures are expected to be 16.0 vbytes,
this limit prevents abuse without affecting normal users. Compared to
earlier abuse-prevention solutions, it means any valid taproot
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: can remove "it means"


- [Bitcoin Core #16400][] refactors part of the mempool transaction
acceptance code. We don't usually cover refactors, but this one has a
tantalizing comment---"this is in preparation for re-using these
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps: /---/: /

acceptance code. We don't usually cover refactors, but this one has a
tantalizing comment---"this is in preparation for re-using these
validation components for a new version of `AcceptToMemoryPool()` that
can operate on multiple transactions ("package relay")." Package
Copy link
Collaborator

@jonatack jonatack Sep 24, 2019

Choose a reason for hiding this comment

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

Can we haz avoid nested double quotes? Perhaps with italics or single quotes around package relay. FWIW package relay is the PR review club topic next week!

Copy link
Collaborator

@jonatack jonatack Sep 24, 2019

Choose a reason for hiding this comment

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

maybe it's perfectly legal, and the issue is the programmer in me that can't read them without wanting to \"escape\" the inside ones

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe it's perfectly legal

Different style guides deal with it differently. Many allow it, but the style I personally use doesn't and I knew it when I lazily pasted this quote without editing, so thanks for calling me out on it. :-)

tantalizing comment---"this is in preparation for re-using these
validation components for a new version of `AcceptToMemoryPool()` that
can operate on multiple transactions ("package relay")." Package
relay could allow allow nodes to accept a transaction below the node's
Copy link
Collaborator

Choose a reason for hiding this comment

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

allow allow for extra permissivity

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

allow allow for extra permissivity

Do you have an example? I'd only heard of package relay being used to allow relay of transactions below the BIP133 minimum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I was only mentioning that there is an extra "allow" in the sentence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, :picard_facepalm:

users who create transactions a long time before broadcasting them
(e.g. timelocked transactions or LN commitment transactions) to safely
pay the minimum possible fee. When it came time to broadcast the
transaction, they could use Child-Pays-For-Parent (CPFP) fee bumping
Copy link
Collaborator

Choose a reason for hiding this comment

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

unsure about those hyphens :D

- [BOLTs #557][] updates [BOLT7][] gossip entries with an optional
extended query flag that allows nodes to request additional
information, namely a timestamp and checksum for each channel update
the node relays. The node can then use another new feature in
Copy link
Collaborator

Choose a reason for hiding this comment

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

"another new feature": which one?

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.

@jonatack thanks for the feedback. I pushed a change that made applicable changes to the SE section using my (dangerous!) best judgment.

> * Drop the 201 non-push ops limit per script.
> * Drop the 100 input stack elements standardness limit, and
> replace with a (consensus) 1,000 limit.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the unit of the 1,000 limit?

Copy link
Contributor

Choose a reason for hiding this comment

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

From Pieter's email: "I believe we should keep this 1000 element stack limit for these reasons. " So I think we should state the unit "elements" in the newsletter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That sentence does state the unit "stack elements".

@harding harding mentioned this pull request Sep 24, 2019
@harding
Copy link
Collaborator Author

harding commented Sep 24, 2019

Pushed a commit with edits for @jonatack's feedback minus a few nits I thought were good enough as-is. Thanks!

@@ -46,12 +46,12 @@ popular Bitcoin infrastructure projects.
He describes some of their findings and advocates the following rule
changes:

> * Replace the separate sigops counter with a "executed sigops must
> * Replace the separate sigops counter with a [rule that] "executed sigops must
> not exceed (witness size / [12.5 vbytes]) + 1" rule (already in the BIP).
Copy link
Collaborator

@jonatack jonatack Sep 24, 2019

Choose a reason for hiding this comment

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

@harding should now drop the second instance of "rule" in the sentence?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh. I'm going to revert this to just sipa's text, I think it's good enough and anyone who's confused can just read his entire email.

@bitschmidty bitschmidty merged commit 04ea81c into bitcoinops:master Sep 25, 2019
@bitschmidty
Copy link
Contributor

Should taproot have p2sh-wrapped support?
Publishing this newsletter from the airport

Pieter and Poelstra doing script analyzing
Harding throwing out the word ‘tantalizing’

Great newsletter @harding
Thanks @jonatack for the reviews

@jnewbery jnewbery added the newsletters Publishing/translating/editing newsletters label Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
newsletters Publishing/translating/editing newsletters
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants