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

jtimon · 2016-08-11T03:44:21Z

...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 · 2016-08-11T04:48:27Z

src/main.cpp

+    // are the actual inputs available?
+    if (!inputs.HaveInputs(tx))
+        return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false,
+                         strprintf("%s: inputs missing/spent", __func__));


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

jtimon · 2016-09-01T16:47:45Z

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

jtimon · 2016-12-03T06:26:39Z

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

sipa

Concept ACK

sipa · 2017-04-09T15:07:38Z

src/validation.cpp

@@ -646,18 +646,20 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
            }
        }

-        // are the actual inputs available?
+        // This redundant check doesn't trigger the DoS code on purpose; if it did, it would make it easier
+        // for an attacker to attempt to split the network (Consensus::CheckTxInputs also checks this).


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.

dcousens · 2017-04-09T23:12:57Z

src/validation.cpp

@@ -1318,16 +1318,14 @@ int GetSpendHeight(const CCoinsViewCache& inputs)
    return pindexPrev->nHeight + 1;
 }

-namespace Consensus {
-bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight)
+bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& nFees)


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.

dcousens

utACK, but see comment RE suggested change

jtimon · 2017-04-18T20:51:11Z

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 1590c19d3b4fa5d406cf2f91772960e11d0d2cb8

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 · 2017-05-08T20:03:16Z

src/validation.cpp

@@ -624,7 +624,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
        CCoinsView dummy;
        CCoinsViewCache view(&dummy);

-        CAmount nValueIn = 0;
+        CAmount nFees = 0;


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 · 2017-05-08T21:08:40Z

src/validation.cpp

@@ -1820,6 +1814,8 @@ static bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockInd
    blockundo.vtxundo.reserve(block.vtx.size() - 1);
    std::vector<PrecomputedTransactionData> txdata;
    txdata.reserve(block.vtx.size()); // Required so that pointers to individual PrecomputedTransactionData don't get invalidated
+    const int nHeight = pindex->pprev == NULL ? 0 : pindex->pprev->nHeight + 1;
+    const int64_t nSpendHeight = block.vtx.size() > 1 ? GetSpendHeight(view) : nHeight;


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.

jtimon · 2017-05-19T04:36:47Z

Needed rebase, hopefully fixed all nits.

ryanofsky · 2017-05-23T19:24:12Z

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 · 2017-05-30T18:11:10Z

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

jtimon · 2017-06-02T01:06:50Z

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 · 2017-06-08T21:20:46Z

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

ryanofsky

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

ryanofsky · 2017-06-14T18:54:58Z

src/validation.cpp

-            if (!view.HaveInputs(tx))
-                return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
-                                 REJECT_INVALID, "bad-txns-inputs-missingorspent");
+            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, nFees))


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!

ryanofsky

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

ryanofsky · 2017-06-21T18:14:10Z

src/validation.cpp

-            if (!view.HaveInputs(tx))
-                return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
-                                 REJECT_INVALID, "bad-txns-inputs-missingorspent");
+            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, nFees))


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 · 2017-06-23T13:14:00Z

src/consensus/tx_verify.h

@@ -24,7 +26,7 @@ namespace Consensus {
 * This does not modify the UTXO set. This does not check scripts and sigs.
 * Preconditions: tx.IsCoinBase() is false.
 */
-bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight);
+bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& nFees);


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

sdaftuar · 2017-06-23T13:16:25Z

src/validation.cpp

+        } // end LOCK(pool.cs)
+
+        CAmount nFees = 0;
+        if (!Consensus::CheckTxInputs(tx, state, view, GetSpendHeight(view), nFees))


style nit: braces for one-line if

sdaftuar · 2017-06-23T15:26:16Z

src/consensus/tx_verify.cpp

-        if (!inputs.HaveInputs(tx))
-            return state.Invalid(false, 0, "", "Inputs unavailable");
+    // are the actual inputs available?
+    if (!inputs.HaveInputs(tx))


style nit: braces for one-line if

sdaftuar · 2017-06-23T15:34:20Z

src/validation.cpp

-            if (!view.HaveInputs(tx))
-                return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
-                                 REJECT_INVALID, "bad-txns-inputs-missingorspent");
+            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, nFees))


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 · 2017-06-23T15:34:40Z

src/validation.cpp

-            if (!view.HaveInputs(tx))
-                return state.DoS(100, error("ConnectBlock(): inputs missing/spent"),
-                                 REJECT_INVALID, "bad-txns-inputs-missingorspent");
+            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, nFees))


style nit: curly braces for one-line if

jtimon · 2017-06-25T02:19:18Z

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 · 2017-06-27T06:05:41Z

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 · 2017-06-26T17:26:51Z

src/consensus/tx_verify.h

@@ -22,9 +24,12 @@ namespace Consensus {
 /**
 * Check whether all inputs of this transaction are valid (no double spends and amounts)
 * This does not modify the UTXO set. This does not check scripts and sigs.
+ * @param[in,out] accumulated_fees this serves to get the fees of the tx as output. 


nit: extra space at EOL here.

TheBlueMatt · 2017-06-27T16:51:21Z

src/consensus/tx_verify.h

 * Preconditions: tx.IsCoinBase() is false.
 */
-bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight);
+bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& accumulated_fees);


Need to change var name here, too.

TheBlueMatt · 2017-06-27T17:44:14Z

src/validation.cpp

+            if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, tx_fees)) {
+                return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state));
+            }
+            nFees += tx_fees;


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 8d7849b broke the check at 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.

…y 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)

in 8d7849b This can potentially prevent an overflow that could at least in theory allow the creation of money.

jtimon · 2017-09-20T21:28:07Z

Fixed 1 commit by @danra

danra · 2017-09-21T20:56:57Z

utACK

TheBlueMatt · 2017-10-03T23:41:47Z

re-utACK 4e955c5

…it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...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) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346

Rename assert_raises_jsonrpc to assert_raises_rpc_error in Namecoin tests for upstream refactoring. Adapt verification of names for bitcoin/bitcoin#8498. Conflicts: src/consensus/tx_verify.cpp src/consensus/tx_verify.h src/txmempool.cpp src/validation.cpp src/wallet/rpcdump.cpp test/functional/bumpfee.py test/util/data/txcreatemultisig1.json

… times it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...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 bitcoin#6445 Near-Bugfix as pointed out in bitcoin#8498 (comment) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346

…umber of times it is checked that no money..." This reverts commit 56d28c5.

… times it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...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 bitcoin#6445 Near-Bugfix as pointed out in bitcoin#8498 (comment) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346

…it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...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 bitcoin/bitcoin#8498 (comment) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346

… times it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...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 bitcoin#6445 Near-Bugfix as pointed out in bitcoin#8498 (comment) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346

b56607a Remove CCoinsViewCache::GetValueIn(...) (practicalswift) Pull request description: Remove `CCoinsViewCache::GetValueIn(...)`. Fixes #18858. It seems like `GetValueIn` was added in #748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in #8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017). `CCoinsViewCache::GetValueIn(…)` performs money summation like this: ```c++ CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const { if (tx.IsCoinBase()) return 0; CAmount nResult = 0; for (unsigned int i = 0; i < tx.vin.size(); i++) nResult += AccessCoin(tx.vin[i].prevout).out.nValue; return nResult; } ``` Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow. Proof of concept output: ``` coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long' GetValueIn = -9221444073709551616 ``` Proof of concept code: ```c++ CMutableTransaction mutable_transaction; mutable_transaction.vin.resize(4393); Coin coin; coin.out.nValue = MAX_MONEY; assert(MoneyRange(coin.out.nValue)); CCoinsCacheEntry coins_cache_entry; coins_cache_entry.coin = coin; coins_cache_entry.flags = CCoinsCacheEntry::DIRTY; CCoinsView backend_coins_view; CCoinsViewCache coins_view_cache{&backend_coins_view}; CCoinsMap coins_map; coins_map.emplace(COutPoint{}, std::move(coins_cache_entry)); coins_view_cache.BatchWrite(coins_map, {}); const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction}); std::cout << "GetValueIn = " << total_value_in << std::endl; ``` ACKs for top commit: MarcoFalke: ACK b56607a promag: Code review ACK b56607a. jb55: ACK b56607a hebasto: ACK b56607a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6

b56607a Remove CCoinsViewCache::GetValueIn(...) (practicalswift) Pull request description: Remove `CCoinsViewCache::GetValueIn(...)`. Fixes bitcoin#18858. It seems like `GetValueIn` was added in bitcoin#748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in bitcoin#8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017). `CCoinsViewCache::GetValueIn(…)` performs money summation like this: ```c++ CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const { if (tx.IsCoinBase()) return 0; CAmount nResult = 0; for (unsigned int i = 0; i < tx.vin.size(); i++) nResult += AccessCoin(tx.vin[i].prevout).out.nValue; return nResult; } ``` Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow. Proof of concept output: ``` coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long' GetValueIn = -9221444073709551616 ``` Proof of concept code: ```c++ CMutableTransaction mutable_transaction; mutable_transaction.vin.resize(4393); Coin coin; coin.out.nValue = MAX_MONEY; assert(MoneyRange(coin.out.nValue)); CCoinsCacheEntry coins_cache_entry; coins_cache_entry.coin = coin; coins_cache_entry.flags = CCoinsCacheEntry::DIRTY; CCoinsView backend_coins_view; CCoinsViewCache coins_view_cache{&backend_coins_view}; CCoinsMap coins_map; coins_map.emplace(COutPoint{}, std::move(coins_cache_entry)); coins_view_cache.BatchWrite(coins_map, {}); const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction}); std::cout << "GetValueIn = " << total_value_in << std::endl; ``` ACKs for top commit: MarcoFalke: ACK b56607a promag: Code review ACK b56607a. jb55: ACK b56607a hebasto: ACK b56607a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6

Summary: > Fixes #18858. > > It seems like `GetValueIn` was added in PR748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in [[bitcoin/bitcoin#8498 | PR8498]] ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017). This is a backport of Core [[bitcoin/bitcoin#18859 | PR18859]] Test Plan: nja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9071

dcousens reviewed Aug 11, 2016
View reviewed changes

laanwj added the Consensus label Aug 11, 2016

jtimon force-pushed the 0.13-consensus-inputs branch from 03d0f76 to 2569f80 Compare September 1, 2016 16:45

jtimon mentioned this pull request Sep 1, 2016

[WIP] "Lockfree" Checkqueue Implementation #8464

Closed

5 tasks

jtimon force-pushed the 0.13-consensus-inputs branch from 2569f80 to c93d1cf Compare December 3, 2016 06:25

sipa reviewed Apr 9, 2017

View reviewed changes

dcousens reviewed Apr 9, 2017

View reviewed changes

dcousens approved these changes Apr 9, 2017

View reviewed changes

jtimon force-pushed the 0.13-consensus-inputs branch 2 times, most recently from 7a17ce7 to 1590c19 Compare April 19, 2017 15:52

ryanofsky reviewed May 8, 2017

View reviewed changes

jtimon force-pushed the 0.13-consensus-inputs branch from 1590c19 to edbb3f7 Compare May 19, 2017 04:36

jtimon force-pushed the 0.13-consensus-inputs branch from edbb3f7 to 7650b8a Compare May 30, 2017 18:09

jtimon force-pushed the 0.13-consensus-inputs branch from 7650b8a to 4d9e9a5 Compare June 2, 2017 01:03

ryanofsky reviewed Jun 14, 2017

View reviewed changes

ryanofsky reviewed Jun 21, 2017

View reviewed changes

sdaftuar suggested changes Jun 23, 2017

View reviewed changes

jtimon force-pushed the 0.13-consensus-inputs branch from 4d9e9a5 to 4731626 Compare June 25, 2017 02:17

jtimon force-pushed the 0.13-consensus-inputs branch from c154f59 to 7b78d8b Compare June 27, 2017 06:46

TheBlueMatt reviewed Jun 27, 2017

View reviewed changes

jtimon added 3 commits September 20, 2017 23:25

Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp

3e8c916

Near-Bugfix: Reestablish consensus check removed in 8d7849b

4e955c5

in 8d7849b This can potentially prevent an overflow that could at least in theory allow the creation of money.

jtimon force-pushed the 0.13-consensus-inputs branch from b989b70 to 4e955c5 Compare September 20, 2017 21:27

danra approved these changes Sep 21, 2017

View reviewed changes

jtimon mentioned this pull request Oct 4, 2017

BIP90: Make buried deployments slightly more easily extensible #11426

Closed

laanwj merged commit 4e955c5 into bitcoin:master Oct 11, 2017

codablock mentioned this pull request Sep 30, 2019

Backport orphan TX tests and dependent PRs dashpay/dash#3127

Merged

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Oct 2, 2019

Revert "Merge bitcoin#8498: Near-Bugfix: Optimization: Minimize the n…

ee08585

…umber of times it is checked that no money..." This reverts commit 56d28c5.

This was referenced May 3, 2020

Signed integer overflow in CCoinsViewCache::GetValueIn(…) #18858

Closed

Remove CCoinsViewCache::GetValueIn(...) #18859

Merged

random-zebra mentioned this pull request Oct 28, 2020

[Validation] Introducing Sapling transaction network connection and validations. PIVX-Project/PIVX#1940

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Conversation

jtimon commented Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

jtimon Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

jtimon commented Sep 1, 2016 • edited Loading

jtimon commented Dec 3, 2016

sipa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

dcousens Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens left a comment

Choose a reason for hiding this comment

jtimon commented Apr 18, 2017 • edited Loading

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented May 19, 2017

ryanofsky commented May 23, 2017

jtimon commented May 30, 2017

jtimon commented Jun 2, 2017

ryanofsky commented Jun 8, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdaftuar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipa Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Jun 25, 2017

jtimon commented Jun 27, 2017 • edited Loading

TheBlueMatt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Sep 20, 2017

danra commented Sep 21, 2017

TheBlueMatt commented Oct 3, 2017

jtimon commented Aug 11, 2016 •

edited

Loading

jtimon Aug 11, 2016 •

edited

Loading

jtimon commented Sep 1, 2016 •

edited

Loading

jtimon Apr 18, 2017 •

edited

Loading

dcousens Apr 9, 2017 •

edited

Loading

jtimon commented Apr 18, 2017 •

edited

Loading

sipa Jun 27, 2017 •

edited

Loading

jtimon commented Jun 27, 2017 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading