Page MenuHomePhabricator

MalFix: Type separation of txids to txhash and unspentid

Authored by tomtomtom7 on Oct 18 2017, 08:18.


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

Unspentid is used as a temporary term to allow properly reviewing the split.
This will be refactored back to txid.

The purpose of the split is to prepare for changing unspentid
to be calculated without signatures

  • Separate uint256 hashes in types txhash_t and unspentid_t
  • Add index to the mempool to allow lookup by unspentid
  • Replace Outpoint txid field with unspentid

The separation is only through types. Unspentids are still calculated
in the same way as txids. No functional changes.

Test Plan

This is an intermediate refactoring diff. Tests don't pass until next diffs.

Diff Detail

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

Event Timeline

Owners added a reviewer: Restricted Owners Package.Oct 18 2017, 08:18
deadalnix requested changes to this revision.Oct 18 2017, 14:31
deadalnix added a subscriber: deadalnix.

This is essentially unreviewable. It's huge, touches everything and does many things at once.

Here is how you go about doing such refactorings:

  1. Introduce types for txhash and txid . Ensure they both have a different type, and implicitly convert to uint256.
  2. move subsystems, one by one, to use either ones of these types.
  3. When all subsystem are migrated, start introducing feature that rely on the fact that the txhash and txid are different.
This revision now requires changes to proceed.Oct 18 2017, 14:31

Thanks. I understand this is too big as I commented on slack, but I also think it's wrong and needs discussion.

Changing txid => txhash is not only a very large operation but also strange as we cannot use our new txid to identify transactions. It's a misnomer.

For instance, you can't do a gettransaction with a txid or use it for other rpc/rest methods, use a txid in the wallet display, or (as discusses) use it for normal P2P traffic.

Why do we want this? If we follow MalFix and only change the prev_tx_out then IMO we are much better of with a new name for this new field.