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

[jtimon]

...is created by individual transactions to 2 places (but call only once in each):

• ConnectBlock ( before calculated fees per txs twice )
• AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated
  fees per tx one extra time )

Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2)

For more motivation:

https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1493
jtimon/bitcoin@0.13-consensus-inputs...jtimon:0.13-consensus-inputs-comments

EDIT: partially replaces #6445

Near-Bugfix as pointed out in #8498 (comment)

[dcousens]

nit: weird spacing here?

[jtimon]

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

[jtimon]

Needed rebase. Besides, the previous version contained a bug.

[jtimon]

Needed rebase after renaming main.o, see #9260 (although needed, the rebase was clean in this case)

[sipa]

Concept ACK

[sipa]

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

[jtimon]

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.

[dcousens]

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.

[jtimon]

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.

[dcousens]

utACK, but see comment RE suggested change

[jtimon]

Added a commit that may make @dcousens happier or not, thanks for making me remember.

EDIT: I would be also be happy to take the function out of the consensus namespace "for free" here. That was a mistake on my part.

[ryanofsky]

utACK 1590c19

PR was initially confusing to me, but here are my notes on what it does:

• Changes CheckTxInputs to return DoS level 100 and code REJECT_INVALID instead of 0 and 0.
• Adds CheckTxInputs nFees in/out argument
• Removes CheckTxInputs call from CheckInputs. Avoids changing behavior by having the two callers of CheckInputs (AcceptToMemoryPoolWorker and ConnectBlock) call CheckTxInputs themselves.
• Changes AcceptToMemoryPoolWorker and ConnectBlock to use fees returned by CheckTxInputs instead of computing fees themselves.
• Updates calls to CheckTxInputs in CTxMemPool::check. No impact there because mempool code ignores returned validation state and fees.

[ryanofsky]

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.

[ryanofsky]

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?

[jtimon]

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.

[jtimon]

Needed rebase, hopefully fixed all nits.

[ryanofsky]

Tests for this PR seem to be failing currently, maybe due to the SpendHeight change?

Only changes since my previous review were rebasing post-#8329 and the nFees and nSpendHeight changes commented on above.

[jtimon]

Yes, the problem was in that change but not on stop using GetSpendHeight, just added +1 to the height by mistake. Updated.

[jtimon]

Needed rebase after #10195, it also needs more review now (AcceptToMemoryPoolWorker may return different DoS pnctuation [100 instead of 0] now, happy to fix if that's a problem).

[ryanofsky]

Some discussion of this in IRC https://botbot.me/freenode/bitcoin-core-dev/msg/87002756/

[ryanofsky]

utACK 4d9e9a5. Only changes since last review were removing a stray comment and fixing the spendheight failures.

[ryanofsky]

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.

[jtimon]

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

[ryanofsky]

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.

[jtimon]

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

[sdaftuar]

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!

[ryanofsky]

Not sure if this needs more review. This PR has two utACKs from dcousens and me, and a concept ack from sipa.

[ryanofsky]

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.

[sdaftuar]

Concept ACK. I think this will slightly speed up validation time; I'm doing a benchmark to see if this is observable. Left one request and some style nits below.

[sdaftuar]

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.

[jtimon]

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

[sipa]

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

[jtimon]

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.

[sipa]

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.

[jtimon]

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

[sdaftuar]

style nit: braces for one-line if

[sdaftuar]

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!

[sdaftuar]

style nit: curly braces for one-line if

[sdaftuar]

style nit: braces for one-line if

[jtimon]

Hopefully fixed @sdaftuar 's nits.
Also add a third commit to properly indent CheckTxInputs and other minor style fixes like braces for one line ifs while at it.

[jtimon]

Added a couple of commits to be squashed. I wouldn't personally squash the latter but it seems @sdaftuar and @sipa would like it more this way and I don't care. Before squashing I will also move the indentation commit to the first thing.

EDIT: already squashed one, updated OP.

[TheBlueMatt]

Totally agree with @sipa and @sdaftuar, thanks for making it a non-accumulator. Generally looks good, obviously needs squash.

[TheBlueMatt]

nit: extra space at EOL here.

[TheBlueMatt]

Need to change var name here, too.

[TheBlueMatt]

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

[TheBlueMatt]

Specifically, commit 8d7849b broke the check at 8d7849b#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.

[jtimon]

Squashed the non-accumulator commit.
Hopefully fixed all @TheBlueMatt 's nits, good catch.
Also s/nTxFee/tx_fees/ to comply with the new style.

[TheBlueMatt]

utACK 3161a7c

[TheBlueMatt]

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

[jtimon]

No reason, will change

[jtimon]

Fixed latest nit.