Store the UTXO set on a per output basis rather than a per transaction basis
ClosedPublic
Actions

Authored by deadalnix on Aug 30 2017, 21:59.

Details

Reviewers

freetrader
CCulianu
schancel

Group Reviewers

Restricted Project

Commits

rSTAGING611284f473fb: Store the UTXO set on a per output basis rather than a per transaction basis
rABC611284f473fb: Store the UTXO set on a per output basis rather than a per transaction basis

Summary

This is the main chunk of the work started with D342 .

It come with an update on the database format and an upgrade routine to do the migration at startup if it is required.

Test Plan

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

Also run a node on testnet from a folder that has been sync before this patch to test it upgrades properly.

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

deadalnix created this revision.Aug 30 2017, 21:59

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 30 2017, 21:59

deadalnix retitled this revision from Store the ITXO set on a per output basis rather than a per transaction basis to Store the UTXO set on a per output basis rather than a per transaction basis.Aug 30 2017, 22:21

deadalnix added a child revision: D515: Move the CCoins abstraction in txdb.cpp.Aug 31 2017, 13:49

deadalnix mentioned this in D516: Add estimate size to dbwrapper.Aug 31 2017, 20:11

Fix disk space requirement estimation.

Harbormaster completed remote builds in B814: Diff 1299.Aug 31 2017, 20:33

deadalnix mentioned this in rABC2eb72fb63edf: Add estimate size to dbwrapper.Sep 6 2017, 16:36

rerebase

Harbormaster completed remote builds in B844: Diff 1336.Sep 6 2017, 16:36

Honestly, some of this is over my head. Put in some comments, hope they are useful. Going to take a look at the other PR's now.

src/rpc/blockchain.cpp
884 ↗	(On Diff #1336)	Should we use sizeof rather than hard coding sizes for each item?
903 ↗	(On Diff #1336)	Can we use COutPoint instead of uint32 here?
1056 ↗	(On Diff #1336)	+1
src/test/coins_tests.cpp
677 ↗	(On Diff #1336)	Should there be equivalent asserts included?
src/txdb.cpp
90 ↗	(On Diff #1336)	NIT: preferred Committing
315 ↗	(On Diff #1336)	Seems the batch gets written even if the loop above aborts early. Is this intended?

This looks good based on the review we did.

src/coins.cpp
48 ↗	(On Diff #1299)	Is this used in testing, or should this assert that it's unimplemented?

This revision is now accepted and ready to land.Sep 6 2017, 18:01

hanchon added a subscriber: hanchon.Sep 6 2017, 18:38

hanchon added inline comments.

src/txdb.cpp
97 ↗	(On Diff #1336)	This change is breaking the blockchain test (python3 rpc-tests.py blockchain). It makes the estimate size return 0. Reverting the change make the test pass with a size of 16780. I'll keep checking the code to see if the test is wrong or if some values need to be modified. I also need to apply D515 to see if It was solved there

deadalnix added inline comments.Sep 6 2017, 19:08

src/rpc/blockchain.cpp
884 ↗	(On Diff #1336)	I'm good with using sizeof. Maybe a bit out of the scope of this diff.
903 ↗	(On Diff #1336)	This would just duplicate the outpoint many times and make hashing more expensive.
src/test/coins_tests.cpp
677 ↗	(On Diff #1336)	A coin is only one output, so the assert doesn't make sense anymore.
src/txdb.cpp
90 ↗	(On Diff #1336)	I do not care much either way. I changed it because sipa did as well.
315 ↗	(On Diff #1336)	The loop abort when we get at the last item, so that's expected. When there is an error, it returns so this doesn't run.