Page MenuHomePhabricator

Add the API to add/spend coin on a per UTXO basis.
ClosedPublic

Authored by deadalnix on Aug 14 2017, 20:36.

Details

Reviewers
freetrader
CCulianu
Group Reviewers
Restricted Project
Commits
Restricted Diffusion Commit
rABCacd289859064: Add the API to add/spend coin on a per UTXO basis.
Summary

This ensure we can add/spend coin per utxo. It remove the ModifyNewCoins new API and makes the ModifyCoins on obsolete. Once the API is fully migrated, the storage can move to a work on a per UTXO basis.

This is a continuation of the work started with D342 .

Test Plan

Updated relevent tests.

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

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Aug 14 2017, 20:36
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 14 2017, 20:36
CCulianu edited edge metadata.Aug 14 2017, 20:49

I can't apply the patch.. :/

Then review it.

Then review it.

You like being sadistic, don't you? ;)

CCulianu accepted this revision.Aug 19 2017, 00:29

Overall this looks good and I agree with this entire class and with the usage and code. See my in-line comments.

I am all-for moving towards more and better abstractions, especially for things like maintaining UTXO sets. The design here is solid and decent and makes use of best practices for the most part.

Good comments, too.

src/coins.cpp
162 ↗(On Diff #1174)

FYI -- This will trigger UBSAN underflow warning on first call to AddCoin. size_t is unsigned.

180 ↗(On Diff #1174)

Suggest: removing lines 162 above and just doing:

if (inserted) cachedCoinsUsage += it->second.coins.DynamicMemoryUsage();

or even clearer:

cachedCoinsUsage += inserted ? it->second.coins.DynamicMemoryUsage() : 0;
189 ↗(On Diff #1174)

I like this comment. THANK YOU.

This revision is now accepted and ready to land.Aug 19 2017, 00:29
CCulianu requested changes to this revision.Aug 19 2017, 00:58
This revision now requires changes to proceed.Aug 19 2017, 00:58
deadalnix added inline comments.Aug 19 2017, 22:14
src/coins.cpp
162 ↗(On Diff #1174)

unsigned underflow is defined in C++ .

189 ↗(On Diff #1174)

You got to thank sipa, this is a backport of his work.

CCulianu accepted this revision.Aug 19 2017, 22:22

Ok, this looks exceptionally well crafted to me. Approved.

src/coins.cpp
162 ↗(On Diff #1174)

I did not know that. I learn something new every day. Good to know.

This revision is now accepted and ready to land.Aug 19 2017, 22:22
deadalnix added inline comments.Aug 19 2017, 22:32
src/coins.cpp
162 ↗(On Diff #1174)

It is UB for signed, and defined as wrapping around for unsigned. Don't ask why, it's C++ :) (that's the kind of crap only people who worked on compiler knows about).

CCulianu added inline comments.Aug 19 2017, 22:44
src/coins.cpp
162 ↗(On Diff #1174)

Ha ha ha, as you did. :)

I am guessing here but now that I think about it perhaps it's UB for signed due to possibly varying ways different CPU architectures handle that or have handled that in the past historically. I know nowadays all CPU's are 2's complement for signed ints -- but IIRC that was not always the case. I seem to remember about sign-bit+number implementations being a thing.

With unsigned I think all CPU architectures pretty much do the same thing or at least did at the time C was being standardized (from which C++ inherits this).. namely FFFFFFFF -> 00000000

Just my guess.

This revision was automatically updated to reflect the committed changes.