Additional sighash cache index bit - #1581
Conversation
|
utACK 0a0dd9b FWIW I think you could change the size of the |
…PROOF keying Backport of bitcoin/bitcoin#32473 as integrated on elements-23.x (eb64d24), reapplied against master's tree. The SIGHASH_RANGEPROOF (0x40) bit changes the sighash preimage: segwit v0 appends hashRangeproofs, and the legacy serializer appends each output's rangeproof and surjectionproof. It is therefore part of the cache key, and the table has 16 entries rather than upstream's 6. Dropping that term is a consensus split with deployed elements-23.x in both directions. Upstream added SigHashCache as a pure addition, so a merge of this file will auto-merge without prompting for the Elements adaptation. Hence the explicit markers.
0a0dd9b to
1484750
Compare
| // because no input can simultaneously use both. | ||
| // ELEMENTS: SIGHASH_RANGEPROOF changes the preimage (segwit v0 appends hashRangeproofs; the | ||
| // legacy serializer appends each output's rangeproof and surjectionproof), so it must be a | ||
| // dimension of the cache key. Removing this term is a consensus split with elements-23.x. |
There was a problem hiding this comment.
In 903be1e:
This comment is a bit confusing. Removing this term would break the caching logic, but it wouldn't be a "consensus split with elements 23" in particular. It'd just be wrong.
| * optionally store a scriptCode which the hash is for, plus a midstate for the SHA256 | ||
| * computation just before adding the hash_type itself. */ | ||
| // ELEMENTS: the SIGHASH_RANGEPROOF (0x40) bit changes the sighash preimage, so it is part of | ||
| // the cache key and the table has 16 entries rather than upstream's 6. Do not drop this when |
There was a problem hiding this comment.
In 903be1e:
Fine to leave as-is, but the old table had 6 elements and we're adding 8 to it, so 14 is the correct number.
There was a problem hiding this comment.
Yes, you are right. I will leave as is though, as it is (again incorrectly) 16 in 23.x.
There was a problem hiding this comment.
Isn't it 12?
3 (sighash types) x 2 (anyonecanpay) x 2 (rangeproof)
There was a problem hiding this comment.
The index is 8 x rp + 3 x acp + 2 x single + 1 x none. The low part 3 x acp + 2 x single + 1 x none spans 0–5, and the rangeproof term adds 8, so the reachable indices are:
0 1 2 3 4 5 (0x40 clear)
8 9 10 11 12 13 (0x40 set)
Twelve reachable values, but a maximum of 13 — so it must be minimum 14. 6 and 7 are unreachable.
| for _ in range(10): | ||
| # Construct a script with 700 checksig operations (7 sighash types, each 100 times), | ||
| # randomly ordered and interleaved with 100 OP_CODESEPARATORS. | ||
| ops = [0, 1, 2, 3, 0x81, 0x82, 0x83, -1] * 100 |
There was a problem hiding this comment.
1484750 test passes even without the code changes in 903be1e, because we don't have any rangeproof sighashes here.
I tried adding one (0x41), but then it fails even without our code changes in 903be1e, because SIGHASH_RANGEPROOF requires dynafed.
Enabling Dynafed in this test then starts failing in gen_test_vectors with invalid-dyna-fed since we're not constructing dynafed blocks.
There was a problem hiding this comment.
I don't know if it's going to be easier (and/or less merge conflicts) to make this test work with dynafed, or to just design a new unit or functional test to make sure this works as required.
4 participants
Additional cache index bit added for sighash combination with SIGHASH_RANGEPROOF