-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Implement LOW_S and NULLDUMMY softfork (BIP146) #8533
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
Conversation
|
utACK 24c398f |
d3189b0 to
068e3e3
Compare
|
needs 0.13.1 milestone tag? |
qa/rpc-tests/bip146-p2p.py
Outdated
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 we get constants for these long hex numbers?
src/main.cpp
Outdated
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.
A little worried this could allow some prankster to reorg testnet a huge amount. Side-effect of bundling the mainnet activation 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.
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.
was exactly my thought. Kind of makes me cry for the 2 line SF, but oh well
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.
There is currently no violating txs on testnet and we could start enforcing this on testnet now.
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.
@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.
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.
@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.
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.
Ok the issue is moot now as rules are now enforced on testnet3.
|
tACK 26df793 on my testnet pool |
|
utACK |
|
Tested ACK 26df793 |
|
utACK 26df793 |
|
Added script tests for some special S values |
|
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 You could find the examples here: https://github.com/jl2012/bips/blob/bip146outofrange/bip-0146.mediawiki#LOW_S |
|
The alternative plan is to do NULLDUMMY only : #8636 |
|
@jl2012 to clarify, the 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"... |
|
@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:
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 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 We could fix that by, for example, making a simple byte-to-byte comparison with |
|
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. |
|
Thanks for the great explanation @jl2012, excellent point about implementation replication. |
|
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 |
This will enforce SCRIPT_VERIFY_LOW_S and SCRIPT_VERIFY_NULLDUMMY on all transactions when segwit is activated with BIP9
This PR replaces #8514