Near-Bugfix: Optimization: Minimize the number of times it is checked that no money... #8498

nit: weird spacing here?

Oh, that's how the editor indented by default. That's actually what we have in some other places, but having a glance searching for strprintf in main.cpp I see we're not being consistent with indentation of function calls broken into several lines

Example from the opening parenthesis position: https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1354 Example 4 spaces from the beginning: https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1110 Example freestyle: https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1436

I'm happy to change it for something else, this maybe?

        return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false,
            strprintf("%s: inputs missing/spent", __func__));

EDIT: Actually the indentation is the same as from the place it's taken from: https://github.com/bitcoin/bitcoin/pull/8498/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaL2417

I think you can change the view.HaveInputs(tx) below into a Consensus::CheckTxInputs call, doing some of the checks slightly earlier.

Perhaps the new comment could be clearer, but the view.HaveInputs(tx) check below sets state.DoS(0, ...) instead of state.DoS(100, ...) like Consensus::CheckTxInputs would do. Feel more than free to rephrase it in a way that would have been more clear to you.

Rather than forcing a bogus nFees (or unused) everywhere, just wrap this function with an overload containing an unused nFees stack variable.

Simpler. Less diff.

Also means anyone reviewing at some later date won't get confused as to whether nFees is actually used or not.

The only place where this makes sense is in txmempool.cpp. There you can do a much nicer wrapper there. The diff won't be smaller in total but there will be more lines in red (which people tend to like). I considered this before and I rembember @sipa said it would be fine to just remove the check in txmempool.cpp but I can't find the comment and he changed his mind about it. I will rewrite this for you to see and I'm happy to squash it.

In commit "Optimization: Minimize the number of times it is checked"

Would be good to declare nFees below close to where it is actually used.

In commit "Optimization: Minimize the number of times it is checked"

Can you add a comment explaining this line? Why do transactions in a block with vtx.size() of >= 2 have a different spend height than transactions in a block with vtx.size() == 1? Is nSpendHeight even used in a block with vtx.size() == 1?

If the block only has the coinbase tx before GetSpendHeight(view) would have not been called at all and starting doing so seems unnecessarily costly, see https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L1306 I was actually hoping that someone suggested to just remove this local variable directly and just use nHeight directly for the coinbase maturity check, assuming that is safe. I think so but would like others to confirm before trying it.

In commit "Optimization: Minimize the number of times"

If I understand correctly, it would be more correct to pass SpendHeight(view) instead of pindex->nHeight as the nSpendHeight argument, but in this case it doesn't matter because the value is only used for coinbase transactions, and this isn't a coinbase transaction? Or maybe there are other reasons why pindex->nHeight is ok to pass?

Either way, I think this is confusing, and that there should be a comment here explaining the pindex->nHeight value. Or maybe you could delete the nSpendHeight argument and have CheckTxInputs call GetSpendHeight(inputs) when needed. This seems like it would be a simplification and I don't think there should be a performance cost because CheckTxInputs seems to be skipped for coinbase transactions in all cases except for one call that invokes GetSpendHeight anyway.

I don't think it would be more correct (but I'm glad to be corrected) and performance would sufffer. I think pindex->nHeight because that's actually what we want to validate here. In acceptToMemPool and CTxMemPool::check you don't know the height. Here, nSpendHeight should always be lower or equal to pindex->nHeight.

I'm happy to add a comment but not sure what it should be. Peharps the comment should be in GetSpendHeight instead of ConnectBlock().

I'm happy to add a comment but not sure what it should be.

Comment could just say why it is better to pass pindex->nHeight as nSpendHeight here instead of GetSpendHeight(view). If the two values are always equal, or have some other relation, comment could just say what that relationship is. If this is too in the weeds, definitely feel free to skip this. I'm just suggesting what I think would make the code clearer for me.

Alternately, I don't know what you think of my suggestion to eliminate the nSpendHeight argument and just call GetSpendHeight in CheckTxInputs. It does look to me like this would be logically equivalent and not effect performance, though maybe there's another reason not to do it.

Part of the optimization here is not calling GetSpendHeight for every tx in the block, and your suggestion would eliminate that part of the optimization. For the comment, what about "We know the height, so we don't need to GetSpendHeight"? it if more people agree. I'm really not sure that comment would add that much value, but I don't mind adding

I agree with @jtimon that it makes more sense for ConnectBlock to pass in pindex->nHeight, rather than get the spend height from the view, even though those are the same. We could add an assert that the view's best block has the same hash as pindex->pprev if that makes the code clearer -- but we have bigger problems than just coinbase maturity if the view is out of sync with our chain!

style nit: curly braces for one-line if

I think it's somewhat confusing that CheckTxInputs() will be doing calculations on accumulated fee values, and not just the particular transaction passed in.

Can we just have CheckTxInputs return the fee of the transaction passed in, and then the caller can decide what additional checks should happen? It would make more sense to me that the MoneyRange() check on total block fees happen in ConnectBlock(), rather than here (though I do think conceptually that is a reasonable check for us to add).

Alternatively, if we decide to go with the accumulated fees being checked here, I'd strongly prefer to rename this variable to something like "accumulated_fees" to make it clear what this is, and update the documentation to explain it. But I'd prefer to move the accumulation to the caller, and leave this function as operating on a single transaction.

If the fee is returned, then we can't also return a bool, and we would need to duplicate the check in ConnectBlock, AcceptToMemeoryPool and CTxMemPool::check. And at that point the additional complexity wouldn't be worth it IMO.

We could also pass a fees input/output parameter that doesn't accumulate (although you can already enjoy that functionality by simply passing 0 to the variable). In that case, the duplication would only need to duplicateMoneyrange check in ConnectBlock, but I don't really see it as an improvement over this.

Agreed, accumulated_fees would be more readable with only 2 more lines needing to change

@jtimon You can 'return' the fee in a tuple, or as a pass-by-reference value. So for example CheckTxInputs would have CAmount& return_fee field, and return_fee would just be assigned that transaction's fee. The caller can then do the accumulated_fee += return_fee logic. This involves no duplication, and still makes CheckTxInputs purely operate on data for a single transaction.

If the concern is that the addition with other fees is not done within the function, as documented the caller can set the in/out argument to zero before calling. I could also always return only the value for the single transaction without accumulating anything (although I don't see the gain, it's just more code), at that point the argument would be out only, not in/out.

From there, to return the output argument as a tuple or pair I think it's just making things uglier and unnecessarily complicated.

instead of:

CAmount tx_fee = 0; // You will initialize the variable even if the function starts setting it to zero
if (!CheckTxInputs(..., tx_fee))
   return false;
accumulated_fee += tx_fee;

You would have something like:

struct CheckTxInputsReturn
{
   bool fValid;
   CAmount tx_fee;
}

CheckTxInputsReturn ret = CheckTxInputs(...);
if (!ret.fValid)
   return false;
accumulated_fee += ret.tx_fee;

I think that's incredibly ugly. Specially since I don't see the problem with how it is currently used when you don't want to accumulate any fees:

CAmount tx_fee = 0;
if (!CheckTxInputs(..., tx_fee))
   return false;

Please, I invite you to write your suggestions on top of this and see for yourselves that they are not an improvement but actually the contrary. If people agree that those IMO much uglier solutions are better, I will squash your suggestions, no problem.

I was just clarifying that @sdaftuar's suggestion does not imply duplication of logic. I personally think that it's a bit cleaner to do the summing of fees outside of the validation of a single function, but it's an unimportant nit for sure.

Oh, sorry, the duplication comment was a mistake. Before I was checking the moneyRange for the accumulated fees and not just for the single tx fees, and I thought that check would need to be duplicated outside, but that was a change in functionality that I shouldn't be doing (and btw was missed in review). Pedantically adding that check would be a softfork, but it should be impossible that the MoneyRange check on the accumulated fees fails anyway.

I can not accumulate anything if you guys prefer that, that's a simple change (even though I still can't understand why would you prefer that), reading your previous comment again better, you were also considering the pass by reference (what is done now), not necessarily the tuple (which is what I would be against in principle because of its ugliness).

style nit: braces for one-line if

style nit: braces for one-line if

nit: extra space at EOL here.

Need to change var name here, too.

While you're at it, can you please re-add the MoneyRange checks here that Gavin removed years ago (and maybe update the PR title to indicate that you're fixing Gavin's near-bug while also decreasing the number of CCoinsView map lookups).

Specifically, commit 8d7849b6db5f54dc32fe4f8c6c7283068473cd21 broke the check at https://github.com/bitcoin/bitcoin/commit/8d7849b6db#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR1110 and made it useless, which very narrowly didn't cause a major consensus failure and re-introduction of the original bitcoin-printing overflow bug.

Why not use the normal formatting here of return state.DoS(100, error(), ...)?

No reason, will change

Isn't this redundant given the check on line 239?

Yes, I think so, unless I'm missing something about some weird overflow. Besides it is also included in MoneyRange, so actually both checks are redundant, but at some point IIRC someone asked to not unify in bad-txns-fee-outofrange. But I think we can choose to ditch either bad-txns-in-belowout or bad-txns-fee-negative. I don't have a strong preference.

* [@params](/bitcoin-bitcoin/contributor/params/)[out] tx_fees Set to the transaction fee if successful.

Why was/is plural?

no good reason, let's make it singular

Use snake_case on new code.

tx_fees or just fees?

That would be more extra disruption only for style. We can do that at any time. Including potentially more rebase work fixing conflicts with other things touching AcceptToMemoryPoolWorker. Can we leave that for later? Note we're not creating a new variable but just moving its declaration here.

++i.

Why? Update only if successfully?

And if not set to to 0? I think this was discussed already...

Update before return true;.

if (!MoneyRange(nValueIn - value_out)) {

Mhmm, right, that's what we're saying in the doxygen docs now...ok, I guess.

Suggest using const auto& to iterate over tx.vin. (This is already the case in other functions in this file)

This line wasn't touched besides indentation until @promag asked s/i++/++i/ Not sure it's worth it to make that change here, to be honest...

@jtimon IMHO it would fit nicely in the minor changes commit

The current code has an explicit comment about not triggering the DoS code on purpose, did you remove it (and invoke the DoS code) intentionally?

The comment was moved to the separated HaveInputs call in AcceptToMemoryPoolWorker. Perhaps lost in rebase, I'll look at it. Perhaps it's fine since we now have: https://github.com/bitcoin/bitcoin/pull/8498/commits/c93ceb2dbde1c2474ee04946fb4594b668613dbe#diff-24efdb00bfbe56b140fb006b562cc70bR514

It's not just the comment, as you can see in the diff previously the code used state.Invalid (and stated it does so on purpose), and now both the comment is gone and the code uses state.DoS

I don't know to say on my own whether that change is valid, but it definitely requires explanation due to the fact that previously there was a specific comment suggesting using state.DoS there is bad.

It should be OK, the DoS from ConnectBlock is important, but the DoS shouldn't be trigger'able in ATMP - we should have already returned false int he pfMissingInputs branch further up.

@TheBlueMatt

It should be OK, the DoS from ConnectBlock is important

Which DoS? The one previously returned from CheckTxInputs in case of missing inputs? If so, IIUC it wouldn't trigger after this change, there would just be an error returned, since it replaced the previous DoS in ConnectBlock https://github.com/bitcoin/bitcoin/commit/c93ceb2dbde1c2474ee04946fb4594b668613dbe#diff-24efdb00bfbe56b140fb006b562cc70bR1638

DoS values persist through the state object (and should only be set once through the entire call tree). Thus, when ConnectBlock calls CheckTxInputs, which sets DoS, ConnectBlock should return only error() and not call DoS again, but the DoS points will still be returned through the state object.

@TheBlueMatt Ah, I see. Thanks!

Local includes should come before system includes

Please dont add needless auto& unless you have to. If you're saving 5 chars of typing it just makes things less readable.

@TheBlueMatt I disagree, and AFAIK so do most of the C++ gurus. The (character) typing isn't the issue here, rather code flexibility. See https://softwareengineering.stackexchange.com/questions/180216/does-auto-make-c-code-harder-to-understand for an example of a discussion on the topic.

auto, here, AFAIU, is going to be slightly more likely to force you to change the line if you change AccessCoin's return type (at least because its a const-ref type, so conversion should only be an inheritance thing). In consensus code, making sure you're forced to change all the places something is used if you change a type strikes me as a very, very good idea. If you want to take Herb's stance on it, I'd also be ok with his recommended auto coin = const Coin&{inputs.AccessCoin(prevout)}; to force it, though I find that somewhat unreadable (which is bias towards what I'm used to, I admit).

Consensus code seems hard enough to change as it is without adding extra hurdles.

In consensus code, making sure you're forced to change all the places something is used if you change a type strikes me as a very, very good idea.

By that logic, instead of const COutPoint &prevout = tx.vin[i].prevout;

it would be better to write

const CTxIn& txin = tx.vin[i];
const COutPoint &prevout = txin.prevout;

Since in the first version, tx.vin[i] is used without specifying its type :)

Let's please leave this for another PR

OK, deleting this comment.

Please dont delete comments. Just because a PR author decided against taking a suggestion doesnt mean review content should be deleted. Others may wish to agree with the suggestion or otherwise just view the history of what was discussed. Unless a comment is updated immediately afterwards, generally I'd prefer they not be edited.