Page MenuHomePhabricator
Differential D975

Cleaned up last usages of nHashType
ClosedPublic
Actions

Authored by jasonbcox on Jan 17 2018, 19:31.

Details

Reviewers
deadalnix
schancel
Group Reviewers
Restricted Project
Commits
rSTAGING34326d640836: Cleaned up last usages of nHashType
rABC34326d640836: Cleaned up last usages of nHashType
Summary

Final cleanup work to complete T61.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1649
Build 1649: arc lint + arc unit

Event Timeline

jasonbcox created this revision.Jan 17 2018, 19:31
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 17 2018, 19:31
Harbormaster completed remote builds in B1646: Diff 2566.Jan 17 2018, 19:31
deadalnix requested changes to this revision.Jan 17 2018, 21:47
deadalnix added inline comments.
src/test/sighash_tests.cpp
145 ↗(On Diff #2566)

Keep the raw integer in here. You don't want SigHashType involved at all on this codepath. This ensures no information is lost in the process.

This revision now requires changes to proceed.Jan 17 2018, 21:47
jasonbcox marked an inline comment as done.Jan 17 2018, 22:05
jasonbcox added inline comments.
src/test/sighash_tests.cpp
145 ↗(On Diff #2566)

Fair point. I reverted all changes in this function, since it didn't make sense to keep sigHashType and nHashType side-by-side. Not to mention it wouldn't make sense to call RandomTransaction() using sigHashType for the same reason as you stated for SignatureHashOld().

jasonbcox marked 2 inline comments as done.Jan 17 2018, 22:05
jasonbcox updated this revision to Diff 2569.Jan 17 2018, 22:06

reverted changes around SignatureHashOld() to ensure no information loss

Harbormaster completed remote builds in B1649: Diff 2569.Jan 17 2018, 22:06
deadalnix requested changes to this revision.Jan 17 2018, 23:48
deadalnix added inline comments.
src/test/sighash_tests.cpp
134

Arguably, you should add

SigHashType sigHashType(nHashType);
137

sigHashType = sigHashType.withForkId(true);

146

Remove constructor.

This revision now requires changes to proceed.Jan 17 2018, 23:48
jasonbcox updated this revision to Diff 2572.Jan 18 2018, 00:05
jasonbcox marked 3 inline comments as done.

Added back SigHashType ops

Harbormaster completed remote builds in B1651: Diff 2572.Jan 18 2018, 00:05
deadalnix accepted this revision.Jan 18 2018, 02:08
This revision is now accepted and ready to land.Jan 18 2018, 02:08
Closed by commit rABC34326d640836: Cleaned up last usages of nHashType (authored by jasonbcox). · Explain WhyJan 18 2018, 02:27
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
test/
8 lines

Diff 2569

View Options

src/test/sighash_tests.cpp

Loading...