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

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

Details

Reviewers

freetrader
CCulianu

Group Reviewers

Restricted Project

Commits

rSTAGINGacd289859064: Add the API to add/spend coin on a per UTXO basis.
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

Lint Not Applicable

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

I can't apply the patch.. :/

deadalnix added a child revision: D471: Kill ModifyCoin.Aug 14 2017, 23:55

Then review it.

In D470#8257, @deadalnix wrote:

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

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.

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.