Implement BIP-143-like sighash #1598

svarogg · 2021-03-11T14:47:12Z

This PR resolves #1341

Complete spec: https://github.com/kaspanet/docs/blob/main/Specs/BIP143-like%20SigHashes.md

… hashes

…test code

codecov · 2021-03-11T14:56:58Z

Codecov Report

Merging #1598 (8010e9d) into v0.10.0-dev (caf251b) will decrease coverage by 0.02%.
The diff coverage is 81.81%.

❗ Current head 8010e9d differs from pull request most recent head bf4a242. Consider uploading reports for the commit bf4a242 to get more accurate results

@@               Coverage Diff               @@
##           v0.10.0-dev    #1598      +/-   ##
===============================================
- Coverage        60.01%   59.98%   -0.03%     
===============================================
  Files              515      516       +1     
  Lines            20454    20494      +40     
===============================================
+ Hits             12275    12293      +18     
- Misses            6229     6247      +18     
- Partials          1950     1954       +4

Impacted Files	Coverage Δ
cmd/wallet/send.go	`0.00% <0.00%> (ø)`
domain/consensus/utils/txscript/error.go	`88.88% <ø> (ø)`
domain/consensus/utils/txscript/script.go	`70.80% <ø> (-8.04%)`	⬇️
domain/consensus/utils/txscript/standard.go	`58.38% <0.00%> (ø)`
domain/consensus/utils/txscript/opcode.go	`87.89% <37.50%> (-0.32%)`	⬇️
domain/consensus/utils/serialization/common.go	`40.27% <53.84%> (+5.92%)`	⬆️
...utils/consensushashing/calculate_signature_hash.go	`91.46% <91.46%> (ø)`
domain/consensus/model/externalapi/hash.go	`85.71% <100.00%> (+0.42%)`	⬆️
...ses/transactionvalidator/transaction_in_context.go	`68.98% <100.00%> (ø)`
domain/consensus/utils/txscript/engine.go	`85.92% <100.00%> (-0.77%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update caf251b...bf4a242. Read the comment docs.

stasatdaglabs · 2021-03-15T11:05:00Z

domain/consensus/utils/txscript/sign_test.go

+				address.EncodeAddress(): key,
+			}), mkGetScript(map[string][]byte{
+				scriptAddr.EncodeAddress(): scriptPubKey.Script,
+			})); err != nil {


Please reformat the above five lines for readability's sake.

stasatdaglabs · 2021-03-15T11:08:15Z

domain/consensus/utils/consensushashing/calculate_signature_hash.go

+const (
+	SigHashAll          SigHashType = 0x1
+	SigHashNone         SigHashType = 0x2
+	SigHashSingle       SigHashType = 0x3


I think it'll be a tiny bit more clear that this is a bit field if you write it as 0x01.

we could even go full literal: 0b00000001 which makes it easier to see the bits and masks with the naked eye (ie SigHashMask = 0x1f becomes: 0b00011111)

stasatdaglabs · 2021-03-15T11:18:57Z

domain/consensus/utils/consensushashing/calculate_signature_hash.go

+		}
+		hashWriter := hashes.NewTransactionSigningHashWriter()
+		hashTxOut(hashWriter, tx.Outputs[inputIndex])
+		return hashWriter.Finalize()


I think you should update reusedValues.outputsHash here and only return later.

this outputsHash isn't cached, because if it's single then each input will have its own outputsHash, you can't cache them between inputs

stasatdaglabs

.

domain/consensus/utils/consensushashing/calculate_signature_hash.go

domain/consensus/utils/serialization/common.go

domain/consensus/utils/consensushashing/calculate_signature_hash.go

elichai · 2021-03-16T09:36:06Z

domain/consensus/utils/consensushashing/calculate_signature_hash.go

+	if hashType.isSigHashNone() {
+		return externalapi.NewZeroHash()
+	}


I think there's a meaningful difference between this and the bitcoin approach.
the difference is how do we handle unknown sighashes, in bitcoin unknown hash types will be all zeros here, and in this code SigHashAll will be the default.
I'm not sure which is better and what kinds of validation happen to the sighash before this function (maybe only valid known sighashes can enter this function)

Same for getSequencesHash

I've added a validation in the beggining of CalculateSignatureHash that makes sure that the SigHashType passed is a standard one, so we are not going to have a default (it will error out if you pass 0)

domain/consensus/utils/txscript/engine.go

domain/consensus/utils/consensushashing/calculate_signature_hash_test.go

stasatdaglabs · 2021-03-16T16:07:34Z

domain/consensus/utils/txscript/sign_test.go

-				}), mkGetScript(nil), nil); err != nil {
+			if err := signAndCheck(msg, tx, i, scriptPubKey, hashType, mkGetKey(map[string]*secp256k1.SchnorrKeyPair{
+				address.EncodeAddress(): key,
+			}), mkGetScript(nil)); err != nil {


Please reformat these three lines as well.

elichai · 2021-03-16T18:05:35Z

domain/consensus/utils/consensushashing/calculate_signature_hash.go

-	SigHashAnyOneCanPay SigHashType = 0x80
+	SigHashAll          SigHashType = 0b00000001
+	SigHashNone         SigHashType = 0b00000010
+	SigHashSingle       SigHashType = 0b00000100


That's 4, it should be 3

This is deliberate.
@stasatdaglabs asked me to do this so that SigHashType is a true bitfield.

LMK if you disagree

domain/consensus/utils/consensushashing/calculate_signature_hash.go

elichai · 2021-03-16T18:08:17Z

domain/consensus/utils/consensushashing/calculate_signature_hash.go

+func (sht SigHashType) isStandardSigHashType() bool {
+	switch sht {
+	case SigHashAll, SigHashNone, SigHashSingle,
+		SigHashAll | SigHashAnyOneCanPay, SigHashNone | SigHashAnyOneCanPay, SigHashSingle | SigHashAnyOneCanPay:
+		return true
+	default:
+		return false
+	}
+}
+


Are we sure this is good and correct?

As sure as one can be.
Many things inside the code and tests of CalculateSignatureHash assume the above is correct.
If it is not - we should fix both it and the code.

elichai · 2021-03-16T18:08:54Z

domain/consensus/utils/txscript/sign_test.go

-		hashType:           0x04,
+		hashType:           0b00000011,


you meant 0b00000100?

domain/consensus/utils/serialization/common.go

…shType

…o 1341-bip-143-sighash

svarogg added 13 commits March 10, 2021 12:59

Move CalculateSignatureHash to consensushashing

b6f1968

Added CalcSignatureHash_BIP143 with all parameters except the re-used…

bed42bd

… hashes

Add handling of outputHash

40686cf

Add sequencesHash to the mix

fe43bbe

Add previousOutputsHash to the mix

3935a62

Replace legacy CalculateSigHash with new one, and re-wire to all non-…

301723d

…test code

Add missing types to WriteElement

eaed317

Fix tests in txscript

b96262d

Fix tests in rest of code

0f82961

Add missing comments

a562738

Add SubnetworkID and Gas to sigHash

fdb85ff

Add TestCalculateSignatureHash

614c118

Invert condition in SigHashSingle getOutputsHash

cb90855

svarogg linked an issue Mar 11, 2021 that may be closed by this pull request

Replace sighash algorithm with BIP143 sighash #1341

Closed

svarogg added the breaking-consensus label Mar 11, 2021

Explicitly define that payloadHash for native transactions is 0

db5dc05

svarogg force-pushed the 1341-bip-143-sighash branch from 1a4847d to db5dc05 Compare March 11, 2021 16:10

svarogg added 2 commits March 11, 2021 18:11

Merge branch 'v0.10.0-dev' into 1341-bip-143-sighash

845c601

Merge branch 'v0.10.0-dev' into 1341-bip-143-sighash

fed458d

stasatdaglabs reviewed Mar 15, 2021

View reviewed changes

stasatdaglabs requested changes Mar 15, 2021

View reviewed changes

elichai reviewed Mar 16, 2021

View reviewed changes

added benchmark to CalculateSignatureHash

fd15c11

svarogg changed the title ~~Implement BIP143-like sighash~~ Implement BIP-143-like sighash Mar 16, 2021

svarogg added 4 commits March 16, 2021 13:29

Reformat call for signAndCheck

ae3d779

Change SigHashes to be true bit-fields

416cb46

Add check for transaction version

71c6ea7

Write length of byte array in WriteElement

8adf8fe

elichai reviewed Mar 16, 2021

View reviewed changes

domain/consensus/utils/consensushashing/calculate_signature_hash_test.go Outdated Show resolved Hide resolved

svarogg added 5 commits March 16, 2021 14:28

hashOutpoint should get outpoint, not txIn

a89cadb

Use inputIndex instead of i to determine SigHashType

e1c2a38

Use correct transaction version + fix some typos

4e04a71

Fix hashes in test

d9df2ac

Merge branch 'v0.10.0-dev' into 1341-bip-143-sighash

a7163b8

stasatdaglabs requested changes Mar 16, 2021

View reviewed changes

Reformat an overly-long line

21eee80

stasatdaglabs previously approved these changes Mar 16, 2021

View reviewed changes

elichai reviewed Mar 16, 2021

View reviewed changes

domain/consensus/utils/consensushashing/calculate_signature_hash.go Show resolved Hide resolved

elichai reviewed Mar 16, 2021

View reviewed changes

domain/consensus/utils/txscript/sign_test.go Outdated

hashType: 0x04,

hashType: 0b00000011,

Copy link

Member

elichai Mar 16, 2021

Choose a reason for hiding this comment

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

you meant 0b00000100?

elichai reviewed Mar 16, 2021

View reviewed changes

domain/consensus/utils/serialization/common.go Outdated Show resolved Hide resolved

svarogg added 4 commits March 17, 2021 09:43

Merge branch 'v0.10.0-dev' into 1341-bip-143-sighash

0da0ffb

Replace checkHashTypeEncoding with caalls to hashType.IsStandardSigHa…

75300e1

…shType

Merge branch 'v0.10.0-dev' into 1341-bip-143-sighash

6315d8c

Merge branch '1341-bip-143-sighash' of github.com:kaspanet/kaspad int…

d6cefc0

…o 1341-bip-143-sighash

svarogg dismissed stasatdaglabs’s stale review via d6cefc0 March 17, 2021 11:54

svarogg added 2 commits March 17, 2021 14:30

Convert hashType to uint8

d9bd812

Add comment

bf4a242

elichai approved these changes Mar 17, 2021

View reviewed changes

stasatdaglabs approved these changes Mar 17, 2021

View reviewed changes

stasatdaglabs merged commit cebcab7 into v0.10.0-dev Mar 17, 2021

stasatdaglabs deleted the 1341-bip-143-sighash branch March 17, 2021 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement BIP-143-like sighash #1598

Implement BIP-143-like sighash #1598

svarogg commented Mar 11, 2021 •

edited

codecov bot commented Mar 11, 2021 •

edited

stasatdaglabs Mar 15, 2021

stasatdaglabs Mar 15, 2021

elichai Mar 16, 2021

stasatdaglabs Mar 15, 2021

elichai Mar 16, 2021

stasatdaglabs left a comment

elichai Mar 16, 2021

elichai Mar 16, 2021

svarogg Mar 16, 2021

stasatdaglabs Mar 16, 2021

elichai Mar 16, 2021

svarogg Mar 17, 2021

elichai Mar 16, 2021

svarogg Mar 17, 2021

elichai Mar 16, 2021

Implement BIP-143-like sighash #1598

Implement BIP-143-like sighash #1598

Conversation

svarogg commented Mar 11, 2021 • edited

codecov bot commented Mar 11, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasatdaglabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svarogg commented Mar 11, 2021 •

edited

codecov bot commented Mar 11, 2021 •

edited