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

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.

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

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.

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

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.

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.

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.

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

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.