Continuing work on T61 to migrate to SigHashType.
Details
Details
- Reviewers
deadalnix schancel - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rSTAGING556a2f1f1013: Replaced nHashType with SigHashType in interpreter's SignatureHash and…
rABC556a2f1f1013: Replaced nHashType with SigHashType in interpreter's SignatureHash and…
Existing tests pass, which cover both valid and invalid base sig hash types. Note that although base type '0' has no valid use, it occurs in both the tests and in the current longest chain, so this implementation was tweaked to support it, yet still support strongly typed base types.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/script/interpreter.cpp | ||
|---|---|---|
| 1399 ↗ | (On Diff #2313) | Makes sense to do, but there doesn't seem to be a drop-in replacement for this part of the function, so I think it falls outside the scope of this diff. I've created a task to look into this later: https://reviews.bitcoinabc.org/T157 |
| 1452 ↗ | (On Diff #2313) | I could, but the diff at this spot will still look half-complete:
+ SigHashType sigHashType = GetHashType(vchSig); vchSig.pop_back();
+ uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType.getRawSigHashType(), amount, I'm fine doing either one first. I don't think it will make much difference. |
| src/script/interpreter.cpp | ||
|---|---|---|
| 1399 ↗ | (On Diff #2313) | When you introduce wrong way to do thing, they tend to spread. |
| 1425 ↗ | (On Diff #2313) | dito |
| 1452 ↗ | (On Diff #2313) | It's ok to have half finished stuff. You get the same problem when you do it the other way around as you have a constructor call added. The problem with doing it in that order is that later on you end up in a position where this is a copy constructor, so you won't even know you are doing this for nothing. You should really have these construction from raw value as input, not in the middle of a pipeline. In that case it's not too bad because the copy is cheap, but still we should do it the right way :) |
| src/script/sighashtype.h | ||
| 64 ↗ | (On Diff #2313) | I don't think you need this. You should just return BaseSigHashType(sigHash & 0x1f) so you don't lose information. |
Comment Actions
Added SigHashType refactor to TransactionSignatureChecker functions to prevent landing half-completed code
| src/script/interpreter.cpp | ||
|---|---|---|
| 1399 ↗ | (On Diff #2313) | I think I'm not understanding, as I didn't introduce the code above. Are you asking me to write a serialization function for SigHashType only, or for the entire block of code above? I only realized that you might be asking for a SigHashType serialization function once you flagged the 'dito' below. |
| 1452 ↗ | (On Diff #2313) | Got it, makes sense. I actually poked around some more and was able to finish up this section of code without bloating this diff a lot. It removes the half-completed code completely, so I guess we don't have to worry about it. :) I'll keep those comments in mind for the future though. |
| src/script/sighashtype.h | ||
| 64 ↗ | (On Diff #2313) | That will require callers to check the bounds on the return value. It seems more concise and easier to read to just check sigHash.getBaseSigHashType() != UNSUPPORTED. Alternatively I could write isValidBaseSigHash() as part of this class which checks the bounds. Is that preferable? |
| src/script/sighashtype.h | ||
|---|---|---|
| 64 ↗ | (On Diff #2313) | See line 211 of interpreter.cpp in this diff for how UNSUPPORTED allows for simpler code. |
Comment Actions
Added serialization to SigHashType, added hasSupportedBaseSigHashType, and fixed a bug from merging the previous changes.
| src/script/sighashtype.h | ||
|---|---|---|
| 64 ↗ | (On Diff #2382) | This should be added to the unit test. |
| 80 ↗ | (On Diff #2382) | This as well. |
| src/script/sign.cpp | ||
| 33 ↗ | (On Diff #2382) | I guess TransactionSignatureCreator is also good for a SigHasTypeization. |
| src/test/script_tests.cpp | ||
| 316 ↗ | (On Diff #2382) | dito |
| 1436 ↗ | (On Diff #2382) | It seems like SigHashType(BaseSigHashType::NONE) should be possible, but that may be out of the scope of this diff. |
| src/script/interpreter.cpp | ||
|---|---|---|
| 190 ↗ | (On Diff #2381) | Still working on automation, but saw this: |
| src/script/interpreter.cpp | ||
|---|---|---|
| 190 ↗ | (On Diff #2381) | You're looking at an old version of this diff. See latest. :) |