Page MenuHomePhabricator

Start refactoring txid -> txhash

Authored by tomtomtom7 on Nov 13 2017, 13:47.


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

Introduces the TxHash type, and starts refactoring txid to txhash. This will be spread over multiple commits.

This is the basis of MalFix with naming txhash/txid.

It is quite tricky to ensure no txid is kept where txhash is meant.
As txid will be rare, I propose for safety to fully rename txid to
txhash, eliminating txid usage before reintroducing txid to COutpoint.

Test Plan

Refactoring only

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 1097
Build 1097: arc lint + arc unit

Event Timeline

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

I would like to note that I still have reservations to this solution compared to the SIGHASH solution. The risk of changing the meaning of txid must be carefully considered.

deadalnix added inline comments.

It is not correct to use the TxHash in the outpoint. It should be an TxId , to identify the transaction we are spending from. The whole problem we are trying to solve here is that the TxHash is mutable. We should have TxId and TxHash to be the same thing at first, but if we ensure that the right one is used at the right place with the help of the type system, then it'll be easy to differentiate them in the future.

deadalnix requested changes to this revision.Dec 5 2017, 16:48

because this seemed to be stuck, I took the initiative to unstuck it via D755 .

This revision now requires changes to proceed.Dec 5 2017, 16:48
tomtomtom7 added inline comments.

Yes. But see comment: It is not going to be easy to do the full refactoring carefully. There is quite a risk of leaving txid's that should be txhashes.

Hence I propose, to first fully refactor txid to txhash. (which corresponds to the current rules). Afterwards change txhash to txid for Outpoints.

This is my assessment from initial investigation for the full refactor at D614. Is that acceptable?