-
Notifications
You must be signed in to change notification settings - Fork 126
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
Topics: updates for December and four new topics #318
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.
@harding thanks for giving us something new each month in the topics section! (and for keeping references up to date each month!) Great work
- tested changes to schema checker catch errors (including topic date)
- tested schema checker outputs failing files
- confirm that default topic date is indeed ridiculous
- went through new topic links(mentions) in December and sanity checked destination url (I noticed several "bad anchor links" and pointed them out individually, not to be annoying but because I was not sure if they were best addressed with a single fix or individually)
- reviewed each of the 4 new topics
Whoops, these all appear to be bugs in the content of Newsletter 78. I'll fix them in a separate PR presently. Great catch, thanks! |
- Makefile: report which files failed - Topics schema: detect non-date strings
7d19e62
to
52b908b
Compare
Forced pushed rebased on master after #321 was merged and verified this fixes all the bad anchors. I'll address @bitschmidty's other feedback tomorrow. Thanks! |
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.
Valuable additions to the topics @harding! Looked at the content without checking the links and jekyll mechanics.
_topics/en/anchor-outputs.md
Outdated
extended_summary: | | ||
Each time the balance changes in an LN channel, a *commitment | ||
transaction* is created and signed by the participating parties. The | ||
transaction is only broadcast if one party decides they want 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.
nit: can optionally remove "they want"
_topics/en/bip70-payment-protocol.md
Outdated
using Bluetooth or NFC to pay even if they didn't currently have | ||
an Internet connection | ||
|
||
It's main disadvantage was that it required spenders support the SSL |
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/It's/Its/
in Bitcoin and a complex Public Key Infrastructure (PKI) system. In | ||
practice, this meant software needed to include a library such as | ||
OpenSSL as a dependency. Bugs in that library could then be used to | ||
exploit Bitcoin programs, such as the OpenSSL [heartbleed][] |
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.
Could be wrong but it seems to me the comma before "such" should be removed 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.
I think it's a non-restrictive appositive and so is properly set off by a comma.
_topics/en/op_codeseparator.md
Outdated
a pre-release version of Bitcoin [may][pre todd1] have [supported][pre | ||
todd2] this capability. | ||
|
||
In [Bitcoin 0.3.7][] (July 2010), a fix to the [1 OP_RETURN][] bug 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.
Awesome history section.
Officially, it seems there were two consecutive CVEs fixed on 31 July and 1 Aug 2010:
- [CRITICAL] Disabled OP_RETURN - v0.3.5 (Jul 28th, 2010)
- [CRITICAL] Separate scriptSig & scriptPubKey eval - v0.3.7 (Aug 1st, 2010)
Did some sleuthing into this last June (with @jnewbery's help) and it turned out the OP_RETURN bug was fixed in bitcoin/bitcoin@a75560d which has the confusing commit message "reverted makefile.unix wx-config -- version 0.3.6".
In that commit, the Bitcoin version appears to have been bumped from 0.3.3 to 0.3.6 here.
I made a presentation on the OP_RETURN issue last June (and would be happy to correct it if needed):
https://drive.google.com/file/d/1nhxsJiSZh_Nhn0Oql9yCfAuryAy8Ed3W/view
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.
Yeah, a75560d
makes an executed OP_RETURN terminate the script in failure and so fixes the specific 1 OP_RETURN
bug, but I don't think it fixes the general class of fault. For example, imagine your scriptPubKey was:
<pubkey> OP_CHECKSIGVERIFY OP_IF ... OP_ENDIF
Now imagine an attacker creates the following scriptSig:
OP_TRUE OP_FALSE OP_IF
If those scripts are concatenated like in Bitcoin 0.1, then the attacker's un-executed IF wraps the CHECKSIG and the other IF statement, allowing the attacker to spend the money without providing a signature. (I think that works, but I'm not exactly sure of Script's block-matching rules within unexecuted blocks.)
So I think we're ok linking to 6ff5f718b
, but I'll see if I can clarify to mention that there were several commits involved in the fix.
In [Bitcoin 0.3.7][] (July 2010), a fix to the [1 OP_RETURN][] bug was | ||
deployed as a stealth [hard fork][lukejr says sipa confirms hf] that | ||
changed how the scripts were evaluated, including discontinuing the | ||
use of the virtual `OP_CODESEPARATOR`. However, the opcode itself |
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.
Separate scriptSig & scriptPubKey eval (OP_CODESEPARATOR) bug fixed in commit bitcoin/bitcoin@73aa262 "fixed segfault in bignum.h, additional security limits, refactoring"
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.
Yeah, that's identical to the commit this PR already links to 6ff5f718b6a67797b2b3bab8905d607ad216ee21
(all, or almost all, the svn commits imported into git have duplicate commits, probably the result of some bug in the import script or someone running the import twice).
part of the proposal may be changed. | ||
|
||
Additionally, Bitcoin Core 0.16.1 and later will not relay or mine any | ||
transaction that uses `OP_CODESEPARATOR` in a legacy 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.
Great section. I'm trying to figure where commit bitcoin/bitcoin@9dabfe4 in PR bitcoin/bitcoin#11423 fits into all this.
Add constant scriptCode policy in non-segwit scripts
This disables OP_CODESEPARATOR in non-segwit scripts (even in an unexecuted branch), and makes
a positive FindAndDelete result invalid. This ensures that the scriptCode serialized in
SignatureHash() is always the same as the script passing to the EvalScript.
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'm not sure I understand your comment, but jl2012's commit basically makes the change proposed in the consensus cleanup section of the topic post and for the same reason, although obviously Lau's commit is only a standardness change and not consensus.
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.
Thanks!
In [Bitcoin 0.3.7][] (July 2010), a fix to the [1 OP_RETURN][] bug was | ||
deployed as a stealth [hard fork][lukejr says sipa confirms hf] that | ||
changed how the scripts were evaluated, including discontinuing the | ||
use of the virtual `OP_CODESEPARATOR`. However, the opcode itself |
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.
Separate scriptSig & scriptPubKey eval (OP_CODESEPARATOR) bug fixed in commit bitcoin/bitcoin@73aa262 "fixed segfault in bignum.h, additional security limits, refactoring"
52b908b
to
e72eefd
Compare
Forced pushed edits for @bitschmidty and @jonatack feedback (thanks!) |
@harding all of my original comments have been marked resolved. thanks for the quick turnaround! I will do another review after any remaining reviews. |
e72eefd
to
d0cf26e
Compare
Force pushed edits for @jnewbery feedback, thanks! |
All my comments are addressed. Thanks Dave! |
Updates and new topics, plus a couple minor QA changes.