script: Change SignatureHash input index check to an assert. #11411

jimpo commented at 2:54 AM on September 28, 2017: contributor

In the SignatureHash function, the input index must refer to a valid index. This is not enforced equally in the segwit/non-segwit branches and should be an assertion rather than returning a error hash.

script: Change SignatureHash input index check to an assert.

In the SignatureHash function, the input index must refer to a valid
index. This is not enforced equally in the segwit/non-segwit branches
and should be an assertion rather than returning a error hash.

5ddf56045a

fanquake added the label Validation on Sep 28, 2017

laanwj added the label Consensus on Sep 28, 2017

TheBlueMatt commented at 9:57 PM on September 28, 2017: member

utACK 5ddf56045ad65162c7cd5c757c81d9446299a5aa

sipa commented at 11:55 PM on September 28, 2017: member

utACK 5ddf56045ad65162c7cd5c757c81d9446299a5aa

Perhaps useful for other reviewers: the SignatureHash code contains two such checks. One comparing nIn with vin.size() (addressed by this PR), which should never occur.

There is another test, comparing nIn with vout.size() in the SIGHASH_SINGLE case. That one can trigger normally in the network, and is not touched by this PR.

jl2012 approved

jl2012 commented at 9:14 AM on September 29, 2017: contributor

utACK 5ddf560

laanwj commented at 9:28 AM on September 29, 2017: member

There is another test, comparing nIn with vout.size() in the SIGHASH_SINGLE case. That one can trigger normally in the network, and is not touched by this PR.

I admit I was worried about this scenario, thanks for clearing it up.

jl2012 commented at 10:15 AM on September 29, 2017: contributor

should we all use vin.at(i) instead of vin[i], so we don't need such assert at all?

practicalswift commented at 12:18 PM on September 29, 2017: contributor

utACK, but please remove this line too:

static const uint256 one(uint256S("0000000000000000000000000000000000000000000000000000000000000001"));

TheBlueMatt commented at 12:27 PM on September 29, 2017: member

@practicalswift sadly, the SIGHASH_SINGLE bug uses the one constant to validate poorly-constructed (but valid) SIGHASH_SINGLE signatures.

On September 29, 2017 8:18:58 AM EDT, practicalswift notifications@github.com wrote:

practicalswift commented on this pull request.

@@ -1221,10 +1223,6 @@ uint256 SignatureHash(const CScript& scriptCode, const CTransaction& txTo, unsig }

static const uint256 one(uint256S("0000000000000000000000000000000000000000000000000000000000000001"));

Please remove :-)

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11411#pullrequestreview-66134985

practicalswift commented at 12:29 PM on September 29, 2017: contributor

@TheBlueMatt Oh, sorry, missed the second use :-)

jl2012 commented at 5:34 PM on September 29, 2017: contributor

not sure if we should softfork away the SIGHASH_SINGLE bug. Such signatures are not safe, anyway

laanwj commented at 1:09 PM on October 2, 2017: member

should we all use vin.at(i) instead of vin[i], so we don't need such assert at all?

AFAIK that throws a std::runtime exception, which can be caught instead of crashing the application instantly which is the bevavior intended here.

not sure if we should softfork away the SIGHASH_SINGLE bug. Such signatures are not safe, anyway

Indeed. It's outside the scope here, in any case. If it's something worth tracking at all, please open a new issue.

laanwj commented at 1:10 PM on October 2, 2017: member

utACK 5ddf560

laanwj merged this on Oct 2, 2017

laanwj closed this on Oct 2, 2017

laanwj referenced this in commit 339da9ca41 on Oct 2, 2017

jimpo deleted the branch on Nov 28, 2018

jasonbcox referenced this in commit 2e257a9218 on Oct 11, 2019

PastaPastaPasta referenced this in commit ad6474a1de on Dec 22, 2019

jonspock referenced this in commit 147e3bd344 on Dec 27, 2019

jonspock referenced this in commit 734b761b53 on Dec 29, 2019

PastaPastaPasta referenced this in commit b25e2e26c2 on Jan 2, 2020

PastaPastaPasta referenced this in commit 4d9175f242 on Jan 4, 2020

PastaPastaPasta referenced this in commit 4446c30580 on Jan 12, 2020

PastaPastaPasta referenced this in commit 4b29092146 on Jan 12, 2020

PastaPastaPasta referenced this in commit df9965c574 on Jan 12, 2020

PastaPastaPasta referenced this in commit 82ef6540d2 on Jan 12, 2020

PastaPastaPasta referenced this in commit 68659b4be2 on Jan 12, 2020

PastaPastaPasta referenced this in commit f6f94ee47e on Jan 12, 2020

ckti referenced this in commit 3be7116ae4 on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021