Correctly report high-S violations #7500

Merged
merged 1 commit into from Feb 10, 2016

Conversation

Projects
None yet
4 participants
@sipa
Member

sipa commented Feb 10, 2016

Reported in #6862 (comment)

@laanwj

This comment has been minimized.

Show comment
Hide comment
Member

laanwj commented Feb 10, 2016

utACK sipa@9d95187

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 10, 2016

Contributor

utACK sipa@9d95187
... passively waiting for some High S tx to compare log output ;-)

Contributor

paveljanik commented Feb 10, 2016

utACK sipa@9d95187
... passively waiting for some High S tx to compare log output ;-)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 10, 2016

Member

@paveljanik wrong commit hash :)

Member

laanwj commented Feb 10, 2016

@paveljanik wrong commit hash :)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 10, 2016

Contributor

Yes, testing two PRs at one time 8)

Contributor

paveljanik commented Feb 10, 2016

Yes, testing two PRs at one time 8)

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 10, 2016

Contributor

ACK

Thx for random volunteer who sends a lot of High S now...

Before the change:

2016-02-10 17:28:54 ERROR: AcceptToMemoryPoolWorker: CheckInputs: 9f8604786542fc4decdd8c5e739eb2c57ba9a7bd67b2c21ecdb99abffb8f643c, non-mandatory-script-verify-flag (unknown error) (code 64)

after:

2016-02-10 19:06:42 ERROR: AcceptToMemoryPoolWorker: CheckInputs: 896a30d13dfd36612e7a77de8e085653d904dea9ecf8af0451f0ed6a78f8dd83, non-mandatory-script-verify-flag (Non-canonical signature: S value is unnecessarily high) (code 64)
Contributor

paveljanik commented Feb 10, 2016

ACK

Thx for random volunteer who sends a lot of High S now...

Before the change:

2016-02-10 17:28:54 ERROR: AcceptToMemoryPoolWorker: CheckInputs: 9f8604786542fc4decdd8c5e739eb2c57ba9a7bd67b2c21ecdb99abffb8f643c, non-mandatory-script-verify-flag (unknown error) (code 64)

after:

2016-02-10 19:06:42 ERROR: AcceptToMemoryPoolWorker: CheckInputs: 896a30d13dfd36612e7a77de8e085653d904dea9ecf8af0451f0ed6a78f8dd83, non-mandatory-script-verify-flag (Non-canonical signature: S value is unnecessarily high) (code 64)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 10, 2016

Member

Tested ACK 9d95187
Without patch:

["0x48 0x304502203e4516da7253cf068effec6b95c41221c0cf3a8e6ccb8cbf1725b562e9afde2c022100ab1e3da73d67e32045a20e0b999e049978ea8d6ee5480d485fcf2ce0d03b2ef001","0x21 0x03363d90d447b00c9c99ceac05b6262ee053441c7e55552ffe526bad8f83ff4640 CHECKSIG","LOW_S","P2PK with high S"] 
-> unknown error

With patch:

["0x48 0x304502203e4516da7253cf068effec6b95c41221c0cf3a8e6ccb8cbf1725b562e9afde2c022100ab1e3da73d67e32045a20e0b999e049978ea8d6ee5480d485fcf2ce0d03b2ef001","0x21 0x03363d90d447b00c9c99ceac05b6262ee053441c7e55552ffe526bad8f83ff4640 CHECKSIG","LOW_S","P2PK with high S"] 
-> Non-canonical signature: S value is unnecessarily high

BTW: would it be useful to add a field for the expected script_error to the invalid tx tests, so that this is verified are part of the unit tests? (not in this pull)

Member

laanwj commented Feb 10, 2016

Tested ACK 9d95187
Without patch:

["0x48 0x304502203e4516da7253cf068effec6b95c41221c0cf3a8e6ccb8cbf1725b562e9afde2c022100ab1e3da73d67e32045a20e0b999e049978ea8d6ee5480d485fcf2ce0d03b2ef001","0x21 0x03363d90d447b00c9c99ceac05b6262ee053441c7e55552ffe526bad8f83ff4640 CHECKSIG","LOW_S","P2PK with high S"] 
-> unknown error

With patch:

["0x48 0x304502203e4516da7253cf068effec6b95c41221c0cf3a8e6ccb8cbf1725b562e9afde2c022100ab1e3da73d67e32045a20e0b999e049978ea8d6ee5480d485fcf2ce0d03b2ef001","0x21 0x03363d90d447b00c9c99ceac05b6262ee053441c7e55552ffe526bad8f83ff4640 CHECKSIG","LOW_S","P2PK with high S"] 
-> Non-canonical signature: S value is unnecessarily high

BTW: would it be useful to add a field for the expected script_error to the invalid tx tests, so that this is verified are part of the unit tests? (not in this pull)

@laanwj laanwj merged commit 9d95187 into bitcoin:master Feb 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Feb 10, 2016

Merge #7500: Correctly report high-S violations
9d95187 Correctly report high-S violations (Pieter Wuille)

laanwj added a commit that referenced this pull request Feb 10, 2016

Correctly report high-S violations
Github-Pull: #7500
Rebased-From: 9d95187
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 10, 2016

Member

Cherry-picked to 0.12 as 889e5b3

Member

laanwj commented Feb 10, 2016

Cherry-picked to 0.12 as 889e5b3

@laanwj laanwj removed the Needs backport label Feb 10, 2016

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Feb 11, 2016

Contributor

utACK

Contributor

dcousens commented Feb 11, 2016

utACK

@dgenr8 dgenr8 referenced this pull request in bitcoinxt/bitcoinxt Jul 16, 2017

Merged

Script testing improvements #217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment