Remove version from UTXO and undo datastructures.
ClosedPublic
Actions

Authored by deadalnix on Jul 21 2017, 14:45.

Details

Reviewers

freetrader
kyuupichan
awemany
sickpig
CCulianu

Group Reviewers

Restricted Project

Commits

rSTAGING80df982ab2f6: Remove version from UTXO and undo datastructures.
rABC80df982ab2f6: Remove version from UTXO and undo datastructures.

Summary

This is a continuation of the work started with D342 . As this info is not very useful, we don't want to be saving it for every single UTXO so we drop it.

This require to add a specific way to verify the hash for on disk data as some of the legacy data do contain a version number that we do not parse anymore, and, as a result, checksum do not match the old way.

Test Plan

make check
../../qa/pull-tester/rpc-tests.py

Start a node on a legacy .bitcoin folder, verify that it pick it up and can sync from it.
Perform a reorg and verify that it behaves properly.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

deadalnix created this revision.Jul 21 2017, 14:45

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 21 2017, 14:45

Harbormaster completed remote builds in B558: Diff 902.Jul 21 2017, 14:45

CCulianu added a subscriber: CCulianu.Jul 21 2017, 16:04

CCulianu added inline comments.

src/coins.h
259 ↗	(On Diff #902)	Small it: I suggest we consider using prefix operators over postfix operators when they are equivalent. Things like --nMaskCode look a little like amateur hour. I may be showing my age here but the prevalence of VAR++/VAR-- in the bitcoin sources always made bitcoin look a little amateurish. Real men always ++VAR unless they really need the post-decrement in an expression. Rationale: post-decrement creates a temporary rvalue. Compiler may optimize it out (or not), but it can produce slower code at least in theory. And.. it just looks strange to us that have been around long enough. Small nitpick, really -- figured this is a good a place as any to bring this up.

CCulianu added inline comments.Jul 21 2017, 16:07

src/coins.h
259 ↗	(On Diff #902)	The above should read: things like nMaskCode-- look a little like amateur hour.

Rest of code changes look good, but introducing a new class without a unit test?

src/hash.h
171 ↗	(On Diff #902)	This new template class has no unit test as far as I saw.

This revision now requires changes to proceed.Jul 22 2017, 13:01

Test plan requires some legacy data. Could you add note how to obtain such legacy data, so the test becomes reproducible.

In D362#6150, @freetrader wrote:

Rest of code changes look good, but introducing a new class without a unit test?

I agree -- a test case seems reasonable..

rebase

Pending test case for CHashVerifier class

This revision now requires changes to proceed.Jul 22 2017, 20:32

This is not using that class at all.

Could you explain how validation.cpp:1460 ff. are not using the new template class?

This revision now requires changes to proceed.Jul 23 2017, 00:55

In D362#6214, @freetrader wrote:

Could you explain how validation.cpp:1460 ff. are not using the new template class?

It appears to me that he's using it. The class looks benign enough, like it hardly can fail. But I do agree it deserves a test be written for it, in case code changes in the future such that its own assumptions about the world are no longer valid.

Add test for CHashVerifier

Good job on writing the test suite. I know it seems redundant since the CHashVerifier is trivial almost -- but I guess we should all enforce decent habits such as these.

Note that I like the test in that it actually captures the essence of why the CHashVerifier is needed (namely the dummyfication of the nVersion field).

The code looks good to me. I tested this on my system.

I will go ahead and approve on my end.

I would have liked to see some more info on how to obtain the legacy data that is referred to in the test plan, but the code changes here look alright to me now.

This revision is now accepted and ready to land.Jul 26 2017, 20:52

awemany requested changes to this revision.Jul 27 2017, 09:33

awemany added inline comments.

src/test/hash_tests.cpp
153 ↗	(On Diff #945)	Any reason the bidirectional macros that are used everywhere else to factor out common de/ser code are not used here?

This revision now requires changes to proceed.Jul 27 2017, 09:33

deadalnix added inline comments.Jul 27 2017, 09:36

src/test/hash_tests.cpp
153 ↗	(On Diff #945)	This thing skip over data on putpose, it doesn't use standard de/ser.

deadalnix requested review of this revision.Jul 27 2017, 09:37

deadalnix edited edge metadata.

deadalnix added inline comments.Jul 27 2017, 09:40

src/coins.h
259 ↗	(On Diff #902)	Real men always ~~~++VAR~~~ Real men check the output assembly to know if their optimization from the 90s still make sense. Hint it doesn't. Source: I'm a LLVM contributor.

awemany accepted this revision.Jul 27 2017, 09:47

awemany added inline comments.

src/test/hash_tests.cpp
153 ↗	(On Diff #945)	accepting as per override of @deadalnix

This revision is now accepted and ready to land.Jul 27 2017, 09:47

If this changes the on-disk file-format then you will break compatibility of the on-disk structures with Classic and maybe others. The result being that users can no longer switch clients without doing a full re-sync.

Thumb math gives me a number of less than a percent of data saved.

My request is that users should be able to sym-link their rev files. They should be able to switch from a Classic maintained UTXO to a ABC one and vice versa.

Closed by commit rABC80df982ab2f6: Remove version from UTXO and undo datastructures. (authored by deadalnix, committed by deadalnix). · Explain WhyAug 29 2017, 16:42

This revision was automatically updated to reflect the committed changes.