Page MenuHomePhabricator

Refactoring GetId() to GetHash()
AbandonedPublic

Authored by tomtomtom7 on Nov 13 2017, 16:22.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

Removed redundant CTransaction::GetHash() and refactored GetId() to GetHash().

This is part of refactoring txid to txhash.

Depends on D665

Test Plan

Refactoring only

Diff Detail

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

Event Timeline

Owners added a reviewer: Restricted Owners Package.Nov 13 2017, 16:22

There should be aa GetID *and* a GetHash. They can both do the same thing internally for now, but we need both to begin with. Then gradually move from using uint256 to TxId or TxHash which should flag the pieces of code that use the wrong one.

There should be aa GetID *and* a GetHash. They can both do the same thing internally for now, but we need both to begin with. Then gradually move from using uint256 to TxId or TxHash which should flag the pieces of code that use the wrong one.

I understand that this is the goal. But we can't chunk the diffs that way.

There are >700 references to txids in the code/comments/vars/tests. Almost all of those will be txhashes, (txid will be usually referred to via Outpoint). This means that if we start the way you suggest we will have intermediate diffs which are rather messy, referring wrongly to txid where txhash is meant in hundreds of places. This makes it also rather impossible to review the completeness of the operation.

Hence I think, the only way to do this properly is to first refactor txid to txhash, then introduce txid for the few cases it will be used.

That said; as you may have noticed, I am also really hesitant about whether there is broad enough support for the change. I know you like it, but I am not sure that is enough, and I don't really like the idea of sneaking it in to avoid controversy. Maybe the least we should do is preannounce it?

Most of this code takes a uint256, so there is no problem as long as both TxId and TxHash convert back to uint256. You can add both function and then convert pieces of code one by one to use the right one with the right type. Converting all use of function A to use of function B doesn't really helps.

To clarify, if you move everything to TxHash, then every time you'll introduce TxId somewhere you'll get a tons of errors. That'ss pretty much force you to do the TxId thing in one shot and guarantee that you'll miss it somewhere. Just convert pieces of code one by one to what they need to take. This doesn't get you any closer to the goal.