Skip to content

Remove outdated comment about DER encoding #19073

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
Merged
merged 1 commit into from
May 27, 2020
Merged

Remove outdated comment about DER encoding #19073

merged 1 commit into from
May 27, 2020

Conversation

elichai
Copy link
Contributor

@elichai elichai commented May 26, 2020 •
edited by fanquake
Loading

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:

bitcoin/src/validation.cpp

Line 1889 in 4af01b3

flags |= SCRIPT_VERIFY_DERSIG;

P.S. This is also quite confusing:

bitcoin/src/validation.cpp

Lines 1560 to 1563 in 4af01b3

// Check whether the failure was caused by a
// non-mandatory script verification check, such as
// non-standard DER encodings or non-null dummy
// arguments; if so, ensure we return NOT_STANDARD
But seems to be intentional:

bitcoin/src/validation.cpp

Lines 1510 to 1511 in 4af01b3

* Note that we may set state.reason to NOT_STANDARD for extra soft-fork flags in flags, block-checking
* callers should probably reset it to CONSENSUS in such cases.

@jnewbery
Copy link
Contributor

ACK

1 similar comment
@Empact
Copy link
Contributor

Empact commented May 26, 2020

ACK

@laanwj
Copy link
Member

laanwj commented May 26, 2020

ACK 4c82579

@fanquake fanquake merged commit cffbf1e into bitcoin:master May 27, 2020
@elichai elichai deleted the 2020-05-remove-der-comment branch May 27, 2020 12:15
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
@furszy furszy mentioned this pull request Sep 7, 2021
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@elichai @jnewbery @Empact @laanwj @fanquake