Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs #8346

jtimon · 2016-07-16T19:11:21Z

The mempool doesn't need to CheckInputs with fCheckScripts=false, it can call Consensus::CheckTxInputs() directly.
We could also remove the parameter from CheckInputs now, since all the remaining callers always pass fCheckScripts=true. But I have left for later, when/if we separate the function in two: one for libconsensus, the other for the multi-threaded validation (both called from ConnectBlock, the former also called from AcceptToMemeoryPool see jtimon@24a0999 ).

I've tried to do this several times, but usually combined with moveonly commits and more changes.
One of those times, @sipa suggested to remove the checks here instead, but I can't remember his reasoning or find a reference. I believe he changed his mind later.

Ping @NicolasDorier @kanzure

paveljanik · 2016-07-16T20:03:40Z

src/txmempool.cpp

Hmm, does CheckInputs change state in assert in the original code (i.e. is it against our Assertions should not have side-effects rule)?

I didn't know we had just a rule, is there a place where those "rules" are documented?
If we do, I suggest we change that rule to at least not apply to CValidationState.
In any case, this is not a change in this PR: there's no change in functionality here, it was doing the same before.

https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#general-c

Thanks.
So, yes, it seems these asserts were not complying with that rule. Neither before not after my change.
If there's an easy fix to add here, I'm all for it.
Should I launch an exception instead ?
It doesn't seem right the way CTxMemPool::check is written...
Maybe it's enough to rename the offending state local variables to dummyState ?

Assign the result to a local scope bool and assert the value of the bool?

Oh, yeah, is as simple as that. Happy to code and squash that.

NicolasDorier · 2016-07-17T03:12:42Z

CheckTxInputs should return true; if it is a coinbase. (agree that in theory it should not happen, but I prefer that we don't change the semantic by mistake)

jtimon · 2016-07-17T09:18:54Z

@NicolasDorier I'm doing that later in https://github.com/jtimon/bitcoin/commits/0.12.99-consensus , but since you refuse to review that branch...Little steps first, preparations, remember?

jtimon · 2016-07-17T11:44:37Z

Updated solving @paveljanik 's nit as suggested by @MarcoFalke . Didn't squashed the commit yet though.

maflcko · 2016-07-17T11:47:17Z

src/txmempool.cpp

Would be better to move this into the smaller else {} scopes

Sorry, I actually moved the state out. I thought this would be clearer. I will change it back and declare the bool twice in their smaller scopes. Reusing the bool actually takes one extra line.

NicolasDorier · 2016-07-17T15:35:59Z

@jtimon just saying the returns true if coinbase should be part of this commit, not later.
Anyway, not blocking for me ACK fa3e82563db63c6e601ccb440a11ad4ba66c9a5b

EDIT: oh I think you were responding to the first comment I made and deleted shortly before you replied. Don't take note to what I said before, my only complain on this commit is that CheckTxInputs should returns true if it is a coinbase.

jtimon · 2016-07-17T15:40:35Z

I'm not sure I understand. Which return true if coinbase?

NicolasDorier · 2016-07-17T15:41:17Z

CheckTxInputs should returns true if it is a coinbase.

NicolasDorier · 2016-07-17T15:41:33Z

as the main::CheckInputs was doing.

NicolasDorier · 2016-07-17T15:49:59Z

src/txmempool.cpp

here, either you should add

fCheckResult = Consensus::CheckTxInputs(tx, state, mempoolDuplicate, nSpendHeight) || tx.IsCoinbase;

Or better, CheckTxInputs should do it internally.

Oh, yeah again lost in re-write. it's even stated in the Preconditions of Consensus::CheckTxInputs in the doc I'm adding just now!

jtimon · 2016-07-18T14:27:13Z

Updated with more fixes and after squashing.

instagibbs · 2016-07-18T14:32:00Z

utACK 00b4943

NicolasDorier · 2016-07-21T12:56:52Z

utACK 00b4943

btcdrak · 2016-07-21T12:56:55Z

utACK 00b4943

Nice proof of work commit there :-p

maflcko · 2016-07-21T13:17:54Z

utACK 00b4943c2794fecf93c21d3db814ea7b2e362087

NicolasDorier · 2016-07-28T20:52:06Z

I confirm this one can get merged before #8259. I'll need to backport to 0.13 #8259 which does not have this PR, but that's trivial change.

sipa · 2016-07-28T21:30:04Z

I'm not a fan of having the code in main.cpp while the header is in consensus.h. What is the plan afterwards? Will CheckTxInputs move to consesus/consensus.cpp as well? Will that mean that CCoinsViewCache (on which it depends) also moves to consensus?

NicolasDorier · 2016-07-28T21:35:43Z

@sipa most likely in the future for libconsensus, we'll need to make an interface and replace CCoinsViewCache dependencies by such interface. CCoinsViewCache would stay out of consensuslib.

The implementation of the interface in libconsensus will use user provided callbacks, while bitcoin core's implementation would use CCoinViewCache under the hood.

sipa · 2016-07-28T21:36:38Z

@NicolasDorier I mean in the immediate future. It's not a nice state that the definition for a function is inside a module where the code can't actually move yet.

NicolasDorier · 2016-07-28T21:40:42Z

@sipa ah yes, I also think so.
I'm not against moving before, but I would prefer moving the declaration after as well.

sipa · 2016-07-28T21:43:36Z

I'd rather not move things until they can actually cleanly moved.

jtimon · 2016-07-28T22:02:26Z

@sipa My plan was to move it later to tx_verify as shown in #8329, but there's many other options.
I'm happy moving the declaration to main.h (although I strongly believe the declaration doesn't belong there) or the definition to whatever place outside of main, even if it means moving the declaration outside of consensus/consensus.h.
Whatever looks easier to merge in the short term.

jtimon · 2016-07-28T22:52:01Z

Updated with the new declaration in main.
Please, the sooner you nick or nack-or-at-least-nack-for-this-pr nit/NACK the new doc, the better.

NicolasDorier · 2016-07-28T23:04:20Z

utACK a6cc299

sipa · 2016-07-28T23:04:22Z

utACK a6cc299

paveljanik · 2016-07-31T19:51:32Z

nit: commit message typo: direclty -> directly

paveljanik · 2016-07-31T19:55:37Z

utACK a6cc299

…::CheckInputs a6cc299 Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs (Jorge Timón)

…er main::CheckInputs a6cc299 Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs (Jorge Timón)

Overwinter SignatureHash Implements zcash/zips#129. Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7276 - bitcoin/bitcoin#7976 - bitcoin/bitcoin#8118 - bitcoin/bitcoin#8149 - Only amount validation and SignatureHash commits. - bitcoin/bitcoin#6915 - Only the rework of `mempool.check()` calls that the next PR depends on. - bitcoin/bitcoin#8346 - bitcoin/bitcoin#8524 Part of #2254. Closes #1408 and #2584.

Overwinter SignatureHash Implements zcash/zips#129. Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7276 - bitcoin/bitcoin#7976 - bitcoin/bitcoin#8118 - bitcoin/bitcoin#8149 - Only amount validation and SignatureHash commits. - bitcoin/bitcoin#6915 - Only the rework of `mempool.check()` calls that the next PR depends on. - bitcoin/bitcoin#8346 - bitcoin/bitcoin#8524 Part of #2074 and #2254. Closes #1408 and #2584.

Overwinter SignatureHash Implements ZIP 143. Includes code cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7276 - bitcoin/bitcoin#7976 - bitcoin/bitcoin#8118 - bitcoin/bitcoin#8149 - Only amount validation and SignatureHash commits. - bitcoin/bitcoin#8346 - bitcoin/bitcoin#8524 Part of #2074 and #2254. Closes #1408 and #2584.

…er main::CheckInputs a6cc299 Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs (Jorge Timón)

paveljanik reviewed Jul 16, 2016
View reviewed changes

maflcko reviewed Jul 17, 2016
View reviewed changes

jonasschnelli added the Refactoring label Jul 17, 2016

NicolasDorier reviewed Jul 17, 2016
View reviewed changes

jtimon force-pushed the 0.12.99-consensus-mempool-checks branch from 008114f to 00b4943 Compare July 18, 2016 14:27

Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs

a6cc299

jtimon force-pushed the 0.12.99-consensus-mempool-checks branch from 00b4943 to a6cc299 Compare July 28, 2016 22:49

sipa merged commit a6cc299 into bitcoin:master Jul 31, 2016

sipa added a commit that referenced this pull request Jul 31, 2016

Merge #8346: Mempool: Use Consensus::CheckTxInputs direclty over main…

f798b89

…::CheckInputs a6cc299 Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs (Jorge Timón)

codablock pushed a commit to codablock/dash that referenced this pull request Jan 8, 2018

Merge bitcoin#8346: Mempool: Use Consensus::CheckTxInputs direclty ov…

60668fb

…er main::CheckInputs a6cc299 Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs (Jorge Timón)

str4d mentioned this pull request Jan 25, 2018

Overwinter SignatureHash zcash/zcash#2903

Merged

dagurval mentioned this pull request Mar 19, 2018

Precompute sighashes bitcoinxt/bitcoinxt#366

Merged

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

Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs #8346

Mempool: Use Consensus::CheckTxInputs direclty over main::CheckInputs #8346

Uh oh!

Conversation

jtimon commented Jul 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 17, 2016 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Jul 17, 2016

Uh oh!

jtimon commented Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Jul 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Jul 17, 2016

Uh oh!

NicolasDorier commented Jul 17, 2016

Uh oh!

NicolasDorier commented Jul 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jul 18, 2016

Uh oh!

instagibbs commented Jul 18, 2016

Uh oh!

NicolasDorier commented Jul 21, 2016

Uh oh!

btcdrak commented Jul 21, 2016

Uh oh!

maflcko commented Jul 21, 2016

Uh oh!

NicolasDorier commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jul 28, 2016

Uh oh!

NicolasDorier commented Jul 28, 2016

Uh oh!

sipa commented Jul 28, 2016

Uh oh!

jtimon commented Jul 28, 2016

Uh oh!

jtimon commented Jul 28, 2016

Uh oh!

NicolasDorier commented Jul 28, 2016

Uh oh!

sipa commented Jul 28, 2016

Uh oh!

paveljanik commented Jul 31, 2016

Uh oh!

jtimon commented Jul 16, 2016 •

edited

Loading

NicolasDorier commented Jul 17, 2016 •

edited

Loading

jtimon commented Jul 17, 2016 •

edited

Loading

NicolasDorier commented Jul 17, 2016 •

edited

Loading

NicolasDorier commented Jul 28, 2016 •

edited

Loading

sipa commented Jul 28, 2016 •

edited

Loading

NicolasDorier commented Jul 28, 2016 •

edited

Loading