Page MenuHomePhabricator

[WIP] Refactor priority calculation slightly for clarity
AbandonedPublic

Authored by schancel on Jan 15 2018, 06:36.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

As per title. Refactor for using cdd in tx acceptance later.

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
refactor-priority
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1610
Build 1610: arc lint + arc unit

Event Timeline

src/coins.cpp
311

The IsSpent() seems unnecessary. The inputs have to be validated before this point. The height requirement is also odd as it's always called with the chainTip and coins shouldn't be in the cache unless they've been confirmed.

This function should be able to just be gotten rid of in favor of GetValueIn.?

Need to dig into out chained unconfirmed transactions interacts with this given that unconfirmed transactions don't seem to be in the coinsviewcache.

schancel retitled this revision from Refactor priority calculation slightly for clarity to [WIP] Refactor priority calculation slightly for clarity.Jan 15 2018, 08:19
deadalnix requested changes to this revision.Jan 15 2018, 14:01
deadalnix added inline comments.
src/coins.cpp
306 ↗(On Diff #2507)

I don't think that returning the variable betting on the fact it is initialized in a certain way make the code very clear. Just return a constant here.

311 ↗(On Diff #2507)

When does this check run ? Not passing it would mean we either are seeing a double spend or a transaction that's confirmed in the future ?

325 ↗(On Diff #2507)

dito

src/coins.h
272 ↗(On Diff #2507)

Comment is now wrong.

src/primitives/transaction.cpp
105 ↗(On Diff #2507)

It seems rather bizarre to use this function as a member of transaction when it's unable to source its data properly. Why not pass a CCoinsView as a argument to it and do the computation based on it ?

This revision now requires changes to proceed.Jan 15 2018, 14:01
src/coins.cpp
311 ↗(On Diff #2507)

It runs here: https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/validation.cpp;484b730e5a2fb08bfdd032f2a11c73dd8a6b3857$783

Pretty sure both of these checks are already done when the transaction is validated in validation.cpp. (And height is always the last block):

https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/validation.cpp;484b730e5a2fb08bfdd032f2a11c73dd8a6b3857$709

I think this entire function should be replaced with just GetValueIn, which is already called above @
https://reviews.bitcoinabc.org/source/bitcoin-abc/browse/master/src/validation.cpp;484b730e5a2fb08bfdd032f2a11c73dd8a6b3857$749

My question is in regards to spends chained, and double spends.

src/primitives/transaction.cpp
105 ↗(On Diff #2507)

I agree, I had another branch where I was working on that.

src/primitives/transaction.cpp
105 ↗(On Diff #2507)

They probably did it this way because of the include order. Most other things depend on CTxOut, so transaction.h can't include much of anything.

Prepare a flag for charging for excessive utxo creation

Prepare a flag for charging for excessive utxo creation.

src/coins.cpp
319 ↗(On Diff #2528)

Regarding the previous comment, a refactor to put this on CTransaction, where it makes sense, is pretty large.

src/validation.cpp
785 ↗(On Diff #2528)

I really don't understand where this is going. This is objectively making the API worse because the caller have to fetch some data and then feed it. The whole logic should be either in the view or in the transaction. The coinage really is not up to the caller to chose, it is a function of the transaction and the coinview.

src/validation.cpp
785 ↗(On Diff #2528)

I don't agree that it is making the API objectively worse, especially when knowing the coinblocks will be useful in the future. Piping the output of one function into another is a pretty standard practice. These are two different APIs and should be split up.

You're correct that these should both live on tx, and ComputePriorty should get the blocks destroyed internally, but the way the code is structured that's not currently possible.

src/validation.cpp
785 ↗(On Diff #2528)

Composition of function is all well and good when they actually compose. There is no composition going on here. ComputePriority has absolutely zero meaning with values other than the coinblock of this very transaction. There is no composition because there is nothing else that can be meaningfully composed with.

src/validation.cpp
785 ↗(On Diff #2528)

Yes, however CoinBlocks does have other uses. There's no other place in the code to get the age of a transaction.

src/coins.cpp
319 ↗(On Diff #2528)

It seems like this function should return uint64_t and the cast-to-double done in ComputePriority instead. Returning double here implies that fractional blocks can be destroyed.