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

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I can't apply the patch.. :/

Then review it.

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

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
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.

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
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).

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.