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)
Details
Details
- Reviewers
jasonbcox Fabien deadalnix - Group Reviewers
Restricted Project - Commits
- rSTAGINGa79d870634fc: Merge #8498: Near-Bugfix: Optimization: Minimize the number of times it is…
rABCa79d870634fc: Merge #8498: Near-Bugfix: Optimization: Minimize the number of times it is…
make check
test_runner.py
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- PR8498
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 6404 Build 10855: Bitcoin ABC Buildbot (legacy) Build 10854: arc lint + arc unit
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. |
| 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 ? 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). |
| 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. |
| src/consensus/tx_verify.h | ||
|---|---|---|
| 8 | Oh great idea -- I love forward declarations :) | |
| 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 |
Comment Actions
A small change, but otherwise, LGTM.
| src/consensus/tx_verify.h | ||
|---|---|---|
| 8 | This is not required, remove. | |
| src/txmempool.cpp | ||
| 693 | What does that 1000000 refers to ? | |
| src/validation.cpp | ||
| 1186 | 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. | |
Comment Actions
@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 | 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 | |
| src/validation.cpp | ||
|---|---|---|
| 579 ↗ | (On Diff #9550) | It clearly is not the end of the LOCK(pool.cs). |
| 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. |
| 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. |
Comment Actions
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.