Skip to content

Conversation

@jl2012
Copy link
Contributor

@jl2012 jl2012 commented Aug 17, 2016

This will enforce SCRIPT_VERIFY_LOW_S and SCRIPT_VERIFY_NULLDUMMY on all transactions when segwit is activated with BIP9

This PR replaces #8514

@dcousens
Copy link
Contributor

utACK 24c398f

@jl2012 jl2012 force-pushed the bip146 branch 5 times, most recently from d3189b0 to 068e3e3 Compare August 20, 2016 16:31
@instagibbs
Copy link
Member

needs 0.13.1 milestone tag?

Copy link
Member

Choose a reason for hiding this comment

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

Could we get constants for these long hex numbers?

@maflcko maflcko added this to the 0.13.1 milestone Aug 23, 2016
src/main.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

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

A little worried this could allow some prankster to reorg testnet a huge amount. Side-effect of bundling the mainnet activation here.

Copy link
Member

@sipa sipa Aug 23, 2016 via email

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

was exactly my thought. Kind of makes me cry for the 2 line SF, but oh well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is currently no violating txs on testnet and we could start enforcing this on testnet now.

Copy link
Contributor

@btcdrak btcdrak Aug 25, 2016

Choose a reason for hiding this comment

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

@instagibbs testnet is a testnet and it's being reorged massively all the time. In the last 3 months it's had about 3 major reorgs (>10,000 blocks). We should not be adding complexity for testnet.

Copy link
Member

Choose a reason for hiding this comment

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

@btcdrak it should not be policy to not care about massive reorgs and encourage them. That said, if testnet miners want to make sure none are included by softforking now, I wouldn't complain.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok the issue is moot now as rules are now enforced on testnet3.

@jameshilliard
Copy link
Contributor

tACK 26df793 on my testnet pool

@luke-jr
Copy link
Member

luke-jr commented Aug 27, 2016

utACK

@btcdrak
Copy link
Contributor

btcdrak commented Aug 28, 2016

Tested ACK 26df793

@instagibbs
Copy link
Member

utACK 26df793

@jl2012
Copy link
Contributor Author

jl2012 commented Aug 29, 2016

Added script tests for some special S values

@jl2012
Copy link
Contributor Author

jl2012 commented Aug 30, 2016

The newly added tests of 70fbd82 revealed some interesting properties of the current implementation of LOW_S. The LOW_S rule is enforced only if R and S are both below 0xFFFFFFFF FFFFFFFF FFFFFFFF FFFFFFFE BAAEDCE6 AF48A03B BFD25E8C D0364141. Otherwise, the CHECKSIG will return a False to the stack, instead of fail immediately.

You could find the examples here: https://github.com/jl2012/bips/blob/bip146outofrange/bip-0146.mediawiki#LOW_S

@jl2012
Copy link
Contributor Author

jl2012 commented Aug 31, 2016

The alternative plan is to do NULLDUMMY only : #8636

@dcousens
Copy link
Contributor

dcousens commented Sep 1, 2016

@jl2012 to clarify, the CHECKSIG operation can fail before a LOW_S signature is detected? Why is that an issue?

Is it because a LOW_S signature could get through conceivably if an alternative branch subsequently returns true?

Is that a bad thing? Or should we only allow canonical signatures to be allowed to be CHECKSIG'd? Meaning we'd need some kind of "pre-pass"...

@jl2012
Copy link
Contributor Author

jl2012 commented Sep 1, 2016

@dcousens the problem is CHECKSIG won't fail immediately due to LOW_S if either R or S is out-of-range.

I think this is how it actually get processed:

  1. secp256k1_scalar_check_overflow checks if R or S is overflow, if yes
  2. ecdsa_signature_parse_der_lax transforms both R and S to 0
  3. secp256k1_scalar_is_high checks if S is high. Since S has become 0 in step 2, it doesn't think it's high so the LOW_S rule is effectively bypassed
  4. secp256k1_ecdsa_verify failed because R=S=0
  5. CHECKSIG returns a 0 to the top stack; script evaluation continues

So we have a weird condition that "if R overflows, we bypass LOW_S check"

Before this was discovered, we thought the rule was simply 0x01 <= S <= 0x7FFFFFFF FFFFFFFF FFFFFFFF FFFFFFFF 5D576E73 57A4501D DFE92F46 681B20A0. (as described in BIP62)

Now I have to use a page of text and examples to precisely describe it: https://github.com/jl2012/bips/blob/bip146outofrange/bip-0146.mediawiki#LOW_S
and without any real use case. It is difficult to justify the softfork in current form.

We could fix that by, for example, making a simple byte-to-byte comparison with 0x7FFFF...20A0 , or fix the test in libsecp256k1. Or we could simply require a null signature for failed CHECKSIG operation (see #8634).

@jl2012
Copy link
Contributor Author

jl2012 commented Sep 1, 2016

Having said that, there won't be any irreversible harm if the softfork had been done without this being discovered. It was still a softfork and it would work as we thought for 99.99999% of transactions on the blockchain. However, if an alternative implementation did not replicate this precisely, they would fork away due to the 0.00001% of transactions.

@dcousens
Copy link
Contributor

dcousens commented Sep 1, 2016

Thanks for the great explanation @jl2012, excellent point about implementation replication.

@jl2012
Copy link
Contributor Author

jl2012 commented Sep 2, 2016

The plan is to do NULLDUMMY only (#8636), and leave LOW_S later, probably at the same time banning non-zero failing signature.

Please remove 0.13.1 tag

@jl2012 jl2012 closed this Sep 2, 2016
@fanquake fanquake removed this from the 0.13.1 milestone Sep 2, 2016
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants