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.

Sign up for GitHub

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

Jump to bottom

Implement BIP66 #5713

Merged
merged 5 commits into from
Feb 3, 2015
Merged

Implement BIP66 #5713

merged 5 commits into from
Feb 3, 2015

Conversation

sipa
Copy link
Member

@sipa sipa commented Jan 27, 2015

See the corresponding BIP text (see bitcoin/bips#135) and mailing list discussion for details.

@laanwj
Copy link
Member

laanwj commented Jan 27, 2015

Cool, looks good to me.
I'll have a try at filing in the TODOs in the RPC test.

@sipa
Copy link
Member Author

sipa commented Jan 27, 2015

@laanwj That'd be awesome, but I don't see how to easily do that without adding functionality to the RPC framework to build its own blocks. Bitcoin Core won't mine blocks with transactions that violate the rules it wants. I guess there could be a -overridemempoolvalidationflags or something, but that gets really ugly.

@laanwj
Copy link
Member

laanwj commented Jan 27, 2015

@sipa Bleh, yes, I just noticed; added python code to fuzz a transaction into non-canonical-DER, but it's impossible to get it into the mempool.
Building blocks from the test framework would be neat, of course.

laanwj
laanwj reviewed Jan 27, 2015
View reviewed changes
qa/rpc-tests/bipdersig.py
self.nodes[1].setgenerate(True, 1)
self.sync_all()
if (self.nodes[0].getblockcount() != cnt + 1050):
raise AssertionFailure("Failed to mine a version=2 block after 949 version=3 blocks")
Copy link
Member

Choose a reason for hiding this comment

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

s/AssertionFailure/AssertionError/

@laanwj
Copy link
Member

laanwj commented Jan 28, 2015

tested ACK. Verified that the deployment behavior as mentioned in BIP66 is indeed what is enforced.

@sipa
Copy link
Member Author

sipa commented Feb 1, 2015

Addressed comments and rebased.

@luke-jr
Copy link
Member

luke-jr commented Feb 1, 2015

utACK: read over non-test commits (80ad135 and 5a47811), looks reasonable (I did not compare to DER spec).

@fanquake fanquake mentioned this pull request Feb 2, 2015
Merged
@sdaftuar
Copy link
Member

sdaftuar commented Feb 2, 2015

I was able to test deployment behavior, submitting regtest blocks with a couple kinds of non-DER signatures over the p2p network to a node running the new code -- worked as expected.

@laanwj laanwj merged commit bf6cdeb into bitcoin:master Feb 3, 2015
laanwj added a commit that referenced this pull request Feb 3, 2015
@laanwj
Merge pull request #5713
41e6e4c 
bf6cdeb Increase coverage of DERSIG edge cases (Pieter Wuille)
819bcf9 Add RPC test for DERSIG BIP switchover logic (Pieter Wuille)
5a47811 BIP66 changeover logic (Pieter Wuille)
092e9fe Example unit tests from BIP66 (Pieter Wuille)
80ad135 Change IsDERSignature to BIP66 implementation (Pieter Wuille)
@sdaftuar sdaftuar mentioned this pull request Feb 6, 2015
Merged
@TheBlueMatt TheBlueMatt mentioned this pull request Sep 25, 2015
Closed
@fanatid
Copy link
Contributor

fanatid commented Mar 17, 2016

Why BIP66 not checked that r or s less more than 33 bytes?
related: bitcoinjs/bip66#2

Invalid DER signature for secp256k1 (but still valid by BIP66):

3044021458a2f39bd87f0000000506030000000000050603022c402dde9afe7f0000010000000100000004000000040000000000000000000000000000000a00000000000000

@Stouse49 Stouse49 mentioned this pull request Jan 11, 2018
Closed
@vietlq
Copy link

vietlq commented Jul 14, 2018

@fanatid the example you posted should be invalidated as an invalid signature even before coming to BIP66 validation code. Layered filters I'd say, so no worries.

@elichai elichai mentioned this pull request May 26, 2020
Merged
fanquake added a commit that referenced this pull request May 27, 2020
@fanquake
Merge #19073: Remove outdated comment about DER encoding
cffbf1e 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: #3843
  But in #5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 27, 2020
@fanquake
Merge bitcoin#19073: Remove outdated comment about DER encoding
bf5ca92 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19073: Remove outdated comment about DER encoding
eb91540 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19073: Remove outdated comment about DER encoding
aaa9915 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19073: Remove outdated comment about DER encoding
7e867e9 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19073: Remove outdated comment about DER encoding
e1dc813 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19073: Remove outdated comment about DER encoding
c333a6f 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 14, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19073: Remove outdated comment about DER encoding
f3f00f4 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
@fanquake @PastaPastaPasta
Merge bitcoin#19073: Remove outdated comment about DER encoding
7ac32ac 
4c82579 Remove outdated comment about DER encoding (Elichai Turkel)

Pull request description:

  This comment got me confused about the status of BIP66 (Thanks jnewbery for explaining)
  The comment was added in: bitcoin#3843
  But in bitcoin#5713 strict DER encoding was enforced in consensus,
  and is now it's buried and enforced by the height of the block here: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1889

  P.S. This is also quite confusing: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1560-L1563 But seems to be intentional: https://github.com/bitcoin/bitcoin/blob/4af01b37d40246cd1fdb54719855927e36a36b46/src/validation.cpp#L1510-L1511

ACKs for top commit:
  laanwj:
    ACK 4c82579

Tree-SHA512: 7afbbae84ed4ecfaa0a273ae024b14f2b7ffe65307f078086fe0b5b645c57722bc2952fb15d167d9e4fa5b052d1d0ac6e5e33f57e8fc881c0ea611d352bccc1e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sipa @laanwj @luke-jr @sdaftuar @fanatid @vietlq