Page MenuHomePhabricator

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

Authored by markblundeberg on Jun 19 2019, 03:01.

Details

Summary

PR8498 backport https://github.com/bitcoin/bitcoin/pull/8498/files
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)

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/consensus/tx_verify.cpp
281 ↗(On Diff #9527)

As noted in the PR discussion, this scary comment doesn't make sense anymore since AcceptToMemoryPoolWorker already calls HaveInputs well before it gets to call this... and in ConnectBlock, calling this function replaces the DoS there.

src/txmempool.cpp
693 ↗(On Diff #9527)

Note the order here is swapped, per D1478

src/validation.cpp
587 ↗(On Diff #9527)

Note to the reviewers: please carefully review the position of this, since our ConnectBlock is modified for non-topological ordering.

1189 ↗(On Diff #9527)

I've put this comment back in the original location, but kept the text (cf D2785). I'm happy to move it back though, if anyone objects.

Fabien requested changes to this revision.Jun 19 2019, 08:13
Fabien added inline comments.
src/consensus/tx_verify.h
31 ↗(On Diff #9527)

Missing the Doxygen comment

src/txmempool.cpp
693 ↗(On Diff #9527)

This raises the question: what is the "good" ordering ?
CheckInputsAndUpdateCoins and AddCoins/UpdateCoins tx/view params are now in opposite order.
This makes the ordering inconsistent and discard the improvement from D1478...

Maybe the best solution will be to revert D1478 as it makes backports easier. Not related to this diff anyway :)

src/validation.cpp
1189 ↗(On Diff #9527)

Agree with the move if you restore the previous comment (to match core).

This revision now requires changes to proceed.Jun 19 2019, 08:13
src/validation.cpp
1189 ↗(On Diff #9527)

I just realized that this position doesn't make sense either. The original comment comes from a 2012 commit of Gavin: 4add41a2a6 . At that time, CheckTxInputs content was embedded into this function, so "The first loop above does all the inexpensive checks.".

Shammah had the correct reading, 'this call' refers to the call to CheckTxInputs. So the way I put it here doesn't make sense, actually the comment should just be taken out.

src/validation.cpp
1189 ↗(On Diff #9527)

Yes, the only reason to have it here is because it matches core and will avoid merge conflicts. Not a very strong case.

include missed doxygen comment & remove superfluous comment from CheckInputs

jasonbcox requested changes to this revision.Jun 20 2019, 15:49
jasonbcox added inline comments.
src/consensus/tx_verify.cpp
313 ↗(On Diff #9536)

The include will be needed in this file.

src/consensus/tx_verify.h
8 ↗(On Diff #9536)

This should be a forward declaration instead.

This revision now requires changes to proceed.Jun 20 2019, 15:49
src/consensus/tx_verify.h
8 ↗(On Diff #9536)

Oh great idea -- I love forward declarations :)

move Amount to forward declaration

jasonbcox requested changes to this revision.Jun 20 2019, 20:49
jasonbcox added inline comments.
src/consensus/tx_verify.cpp
314 ↗(On Diff #9550)

const

323 ↗(On Diff #9550)

const

src/validation.cpp
579 ↗(On Diff #9550)

nit: Missing comment // end LOCK(pool.cs)

Not that it actually matters for reading the code, but so that we don't indicate to our future selves that a backport may have been missed.

581 ↗(On Diff #9550)

nit: Amount nFees = Amount::zero(); Functionally equivalent, but looks more like the original PR. Up to you.

1821 ↗(On Diff #9550)

nit ditto

This revision now requires changes to proceed.Jun 20 2019, 20:49
deadalnix requested changes to this revision.Jun 20 2019, 23:53

A small change, but otherwise, LGTM.

src/consensus/tx_verify.h
8 ↗(On Diff #9536)

This is not required, remove.

src/txmempool.cpp
693 ↗(On Diff #9536)

What does that 1000000 refers to ?

src/validation.cpp
1186 ↗(On Diff #9536)

Core is really playing with fire when doing this. This let's change the function and hope we did not miss any callsite style refactoring is always making me uneasy.

This revision now requires changes to proceed.Jun 20 2019, 23:53

@deadalnix looks like your comments showed up late for some reason. Anyway the amount.h is removed as jason asked for as well.

src/txmempool.cpp
693 ↗(On Diff #9536)

Heh, I was wondering the same. Answers here (it turns out it doesn't matter): https://github.com/bitcoin/bitcoin/issues/15080

And, changed to a more sensible number here: https://github.com/bitcoin/bitcoin/pull/16056

deadalnix requested changes to this revision.Jun 21 2019, 02:47
deadalnix added inline comments.
src/validation.cpp
579 ↗(On Diff #9550)

It clearly is not the end of the LOCK(pool.cs).

This revision now requires changes to proceed.Jun 21 2019, 02:47
src/validation.cpp
579 ↗(On Diff #9550)

I can deduce this via a careful reading of the one line of code in that scope statement.

remove comment again

(besides the comment thing, we good on this?)

jasonbcox added inline comments.
src/validation.cpp
579 ↗(On Diff #9550)

It does make you wonder why it was in the original PR in the first place...

src/validation.cpp
579 ↗(On Diff #9550)

1/ It wasn't. Check the PR again.
2/ Even if it was, the it clearly doesn't make sense and therefore shouldn't be ported.

This revision is now accepted and ready to land.Jun 22 2019, 02:10

Self-reviewing again.

Note on the three callsites of CheckInputs.

  • CheckInputsFromMempoolAndCache : this gets called only in AcceptToMemoryPoolWorker, and only after CheckTxInputs and CheckInputs.
  • AcceptToMemoryPoolWorker : calls CheckTxInputs before CheckInputs.
  • ConnectBlock : calls CheckTxInputs before CheckInputs.