d92204c900 build: add warning to detect hidden copies in range-for loops (Cory Fields)
466e16e0e8 cleanup: avoid hidden copies in range-for loops (Cory Fields)
Pull request description:
Following-up on #13241, which was itself a follow-up of #12169.
See title. Fixing these would otherwise be a continuous process, adding the warning should keep them from cropping up.
Note that the warning seems to be Clang-only for now.
Tree-SHA512: ccfb769c3128b3f92c95715abcf21ee2496fe2aa384f80efead1529a28eeb56b98995b531b49a089f8142601389e63f7bb935963d724eacde4f5e1b4a024934b
f74894480 Only set fNewBlock to true in AcceptBlock when we write to disk (Matt Corallo)
fa6e49731 rpc: Avoid "duplicate" return value for invalid submitblock (MarcoFalke)
Pull request description:
This is #13395 with one more commit tacked on. MarcoFalke got tired of dealing with the stupidity of fixing a return code with too many rounds of review (not that I blame him). Honestly we should probably have no return whatsoever, but for now, this fixes it (as well as nLastBlockTime for eviction purposes).
Original description:
When `submitblock` of an invalid block, the return value should not be `"duplicate"`.
This is only seen when the header was previously found (denoted by the incorrectly named boolean `fBlockPresent`). Fix this bug by removing `fBlockPresent`.
Tree-SHA512: 0ce3092655d5d904b4c8c5ff7479f73ce387144a738f20472b8af132564005c6db5594ae366e589508f6258506ee7a28b1c7995a83a8328b334f99316006bf2d
fafa270328 Make ReceivedBlockTransactions return void (MarcoFalke)
Pull request description:
Instead of always returning `bool{true}` and forcing the caller to handle the return code, make it void and remove "a bunch" of dead code at the call sites.
Tree-SHA512: 10e41461c0516c0441d8b8eedcf6385874355c224b9e9d65e89addb142b4cf3e3be2d4ca0a7f2bd95c76aecdaa8537b6bd2d25631bf804bc42863ad5e84fa271
9b72c988a0 scripted-diff: Avoid temporary copies when looping over std::map (Ben Woosley)
Pull request description:
The ::value_type of the std::map/std::multimap/std::unordered_map containers is
std::pair<const Key, T>. Dropping the const results in an unnecessary copy,
for example in C++11 range-based loops.
For this I started with a more general scripted diff, then narrowed it down
based on the inspection showing that all actual map/multimap/unordered_map
variables used in loops start with m or have map in the name.
Tree-SHA512: b656d66b69ffa1eb954124aa8ae2bc5436ca50262abefa93bdda55cfcdaffc5ff90cd40539051a2bd06355ba69ddf245265cc8764eebff66d761b3aec06155a9
25bc9615b7 Document validationinterace callback blocking deadlock potential. (Matt Corallo)
Pull request description:
From the branches-I've-had-lying-around-and-forgot-to-PR department...
This is a comment-only PR, but the comments point out an API quirk that isn't exactly trivial. None of our use-cases right now hit this, but if we were to call SyncWithValidationInterfaceQueue (eg to limit queue depth) in ATMP, I'm pretty sure we'd hit a deadlock there.
Tree-SHA512: 889dd8fc9eb15d1f2aa5ca467e783bc8f07bc543b166b032741795b0db7a0df11a2846d3cb7c69bafa8d1acf970021001b742f52be06725a932813230c5b4a7b
f6f8026e40 validation: check the specified number of blocks (off-by-one) (Karl-Johan Alm)
Pull request description:
```
echeveria | 2018-06-11 02:03:03.384975 Verifying last 3 blocks at level 3
echeveria | 2018-06-11 02:03:23.676793 No coin database inconsistencies in last 4 blocks (6564 transactions)
echeveria | off by one?
sipa | echeveria: possibly!
kallewoof | Looks like it checks one more block than suggested. `if (pindex->nHeight < chainActive.Height()-nCheckDepth) break;` should probably be `<=`.
sipa | kallewoof: agree
```
Post-commit:
```
2018-06-11T05:24:02Z Verifying last 6 blocks at level 3
2018-06-11T05:24:02Z [0%]...[16%]...[33%]...[50%]...[66%]...[83%]...[99%]...[DONE].
2018-06-11T05:25:07Z No coin database inconsistencies in last 6 blocks (7258 transactions)
```
Pre-commit:
```
2018-06-11T05:27:11Z Verifying last 6 blocks at level 3
2018-06-11T05:27:11Z [0%]...[16%]...[33%]...[50%]...[66%]...[83%]...[99%]...[DONE].
2018-06-11T05:27:12Z No coin database inconsistencies in last 7 blocks (9832 transactions)
```
Tree-SHA512: 6e68dc4ba74232518c2ba8ea624d65893534f3619d43ccdf0b9c65992f25b68cb52cf54fa35e6e3d092d1eee5c9a8887057828895f1acdafc0ebb48f683fffdc
The only affect this should have is fixing the return code in
submitblock in cases where a block fails ContextualCheckBlock and
not setting nLastBlockTime on peers that provide blocks which fail
ContextualCheckBlock (which is only used in eviction and cosmetic).
The ::value_type of the std::map/std::multimap/std::unordered_map containers is
std::pair<const Key, T>. Dropping the const results in an unnecessary copy,
for example in C++11 range-based loops.
For this I started with a more general scripted diff, then narrowed it down
based on the inspection showing that all actual map/multimap/unordered_map
variables used in loops start with m or have map in the name.
-BEGIN VERIFY SCRIPT-
sed -i -E 's/for \(([^<]*)std::pair<([^c])(.+) : m/for (\1std::pair<const \2\3 : m/' src/*.cpp src/**/*.cpp
sed -i -E 's/for \(([^<]*)std::pair<([^c])(.+) : (.*)map/for (\1std::pair<const \2\3 : \4map/' src/*.cpp src/**/*.cpp
-END VERIFY SCRIPT-
0bf431870e net: Serve blocks directly from disk when possible (Wladimir J. van der Laan)
Pull request description:
In `ProcessGetBlockData`, send the block data directly from disk if type MSG_WITNESS_BLOCK is requested. This is a valid shortcut as the on-disk format matches the network format.
This is expected to increase performance because a deserialization and subsequent serialization roundtrip is avoided.
Tree-SHA512: 9a9500b4c1354eaae1a6f1c6ef2416c1c1985029852589266f3a70e808f6c7482c135e9ab251a527566935378ab7c32dba4ed43ba5451e802d8e72b77d1ba472
dd435ad Add unit tests for signals generated by ProcessNewBlock() (Jesse Cohen)
a3ae8e6 Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen)
ecc3c4a Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo)
Pull request description:
Originally this PR was just to add tests around concurrency in block validation - those tests seem to have uncovered another bug in ActivateBestChain - this now fixes that bug and adds tests.
ActivateBestChain (invoked after a new block is validated) proceeds in steps - acquiring and releasing cs_main while incrementally disconnecting and connecting blocks to sync to the most work chain known (FindMostWorkChain()). Every time cs_main is released the result of FindMostWorkChain() can change - but currently that value is cached across acquisitions of cs_main and only refreshed when an invalid chain is explored. It needs to be refreshed every time cs_main is reacquired. The test added in 6094ce7304 will occasionally fail without the commit fixing this issue 26bfdbaddb
Original description below
--
After a bug discovered where UpdatedBlockTip() notifications could be triggered out of order (#12978), these unit tests check certain invariants about these signals.
The scheduler test asserts that a SingleThreadedSchedulerClient processes callbacks fully and sequentially.
The block validation test generates a random chain and calls ProcessNewBlock from multiple threads at random and in parallel. ValidationInterface callbacks verify that the ordering of BlockConnected BlockDisconnected and UpdatedBlockTip events occur as expected.
Tree-SHA512: 4102423a03d2ea28580c7a70add8a6bdb22ef9e33b107c3aadef80d5af02644cdfaae516c44933924717599c81701e0b96fbf9cf38696e9e41372401a5ee1f3c
In `ProcessGetBlockData`, send the block data directly from disk if
type MSG_WITNESS_BLOCK is requested. This is a valid shortcut as the
on-disk format matches the network format.
This is expected to increase performance because a deserialization and
subsequent serialization roundtrip is avoided.
11fa6bb66e Bugfix: ensure consistency of m_failed_blocks after reconsiderblock (Suhas Daftuar)
Pull request description:
This was introduced in 015a5258ad and could cause a node to crash (due to assertion failure) when using the `reconsiderblock` rpc.
Tree-SHA512: 820dcd761bf983e36f5d0f16777ed75c833daaf62a6b3a4dbd17f6caaf9287223e3a202d06540ac62f8ba72926b73b0873bb76c6273ddcb19d9408f4c1cd325e
If multiple threads are invoking ActivateBestChain, it was possible to have
them working towards different tips, and we could arrive at a less work tip
than we should. Fix this by introducing a ChainState lock which must
be held for the entire duration of ActivateBestChain to enforce
exclusion in ABC.
Technically, some internal datastructures may be in an inconsistent
state if we do this, though there are no known bugs there. Still,
for future safety, its much better to only unlock cs_main if we've
made progress (not just tried a reorg which may make progress).
364bae5 qa: Pad scriptPubKeys to get minimum sized txs (MarcoFalke)
7485488 Policy to reject extremely small transactions (Johnson Lau)
0f8719b Add transaction tests for constant scriptCode (Johnson Lau)
9dabfe4 Add constant scriptCode policy in non-segwit scripts (Johnson Lau)
Pull request description:
This disables `OP_CODESEPARATOR` in non-segwit scripts (even in an unexecuted branch), and makes a positive `FindAndDelete` result invalid. This ensures that the `scriptCode` serialized in `SignatureHash` is always the same as the script passing to the `EvalScript`.
Tree-SHA512: a0552cb920294d130251c48053fa2ff1fbdd26332e62b52147d918837852750f0ce35ce2cd1cbdb86588943312f8154ccb4925e850dbb7c2254bc353070cd5f8
Introduced in 4e0eed88ac
When an invalid block is found during a reorg, we know the last of the blocks in
the was-to-be-connected chain is invalid, but not necessarily the first. As
vpIndexToConnect is ordered in decreasing height, the end of the reorg is the
front of the vector, and not the back.
This only affected the warning system.
A transaction with 1 segwit input and 1 P2WPHK output has non-witness size of 82 bytes. Anything smaller than this have unnecessary malloc overhead and are not relayed/mined.
Previously, ChainStateFlushed would fire either if a full flush
completed (which can happen due to memory limits, forced flush, or
on its own DATABASE_WRITE_INTERVAL timer) *or* on a
ChainStateFlushed-specific DATABASE_WRITE_INTERVAL timer. This is
both less clear for clients (as there are no guarantees about a
flush having actually happened prior to the call), and reults in
extra flushes not clearly intended by the code. We drop the second
case, providing a strong guarantee without removing the periodit
timer-based flushing.
This much more accurately captures the meaning of the callback.
-BEGIN VERIFY SCRIPT-
sed -i 's/SetBestChain/ChainStateFlushed/g' src/validationinterface.h src/validationinterface.cpp src/wallet/wallet.h src/wallet/wallet.cpp src/validation.cpp src/index/txindex.h src/index/txindex.cpp
-END VERIFY SCRIPT-
1accfbc Output values for "min relay fee not met" error (Kristaps Kaupe)
Pull request description:
It is already done this way for "mempool min fee not met" error.
Tree-SHA512: 829db78ecc066cf93b8e93ff1aeb4e7b98883cf45f341d5be6e6b4dff4135f3f54fa49b3a6f12eb43f676a9ba54f981143c9887f786881e584370434a9566cfd
18326ae [doc] Add comments for chainparams.h, validation.cpp (James O'Beirne)
Pull request description:
Added a few comments during a leisurely read through some of the validation code. If this kind of thing seems useful, I can add similar documentation for most of the `CChainState` interface.
Tree-SHA512: a4d9db60383a8ff02e74ac326ed88902eec1ee441e8cd4e1845bcf257072673c15974225288cebf0a633e76a3410f99e2206616b4694725a2a5b0d19c78327d6
e4d0b44 Consistently log CValidationState on failure (Ben Woosley)
Pull request description:
This replaces potential silent failures and partial logging with full logging. Seems providing at least minimal visibility to the failure is a good practice. E.g. `FlushStateToDisk` can return a rare but meaningful out of disk space error that would be better to note than leave out.
Note many of these are related to `ActivateBestChain` or `FlushStateToDisk`. Only a few cases of ignored state remain, e.g. LoadExternalBlockFile and RelayWalletTransaction, where I expect logging would likely be spammy.
Tree-SHA512: fb0e521039e5a5250cd9c82e7a8676423b5e3899d495649c0e71752059d1984e5175f556386ade048f51a7d59f5c8e467df7fe91d746076f97d24c000ccf7891
cf02779 Add logging and error handling for file syncing (Wladimir J. van der Laan)
Pull request description:
Add logging and error handling inside, and outside of FileCommit.
Functions such as fsync, fdatasync will return error in case of hardware I/O errors, and ignoring this means it can silently continue through data corruption.
(c.f. https://lwn.net/SubscriberLink/752063/12b232ab5039efbe/)
EINVAL is handled specially to avoid crashing out on (network, fuse) filesystems that don't handle `f[data]sync`.
I checked that the syncing inside leveldb is already generating an I/O error as appropriate.
Tree-SHA512: 64cc9bbedca3ecc97ff4bac0a7b7ac6526a7ed763c66f6786d03ca4f2e9e366e42b152cb908299c060448d98ca39ff03395280bffaca51d592e728aa2516f5dd
Add logging and error handling inside, and outside of FileCommit.
Functions such as fsync, fdatasync will return error in case of hardware
I/O errors, and ignoring this means it can silently continue through
data corruption. (c.f.
https://lwn.net/SubscriberLink/752063/12b232ab5039efbe/)
3cc9094 scripted-diff: Rename CChainState::g_failed_blocks to m_failed_blocks (Ben Woosley)
Pull request description:
To reflect its actual status as a member rather than a global value.
g_failed_blocks was previously global: 2862aca40f
Tree-SHA512: a0e679a151e0fb70d245a7a1821449d0a4738f5ba503abca9f19d9cfbcbb0e72a1598e3364e29775b0c203acd6d04d882d2788208f685edc57aaba5e946fde3b
8b56fc0b91 [qa] Test that v0 segwit outputs can't be spent pre-activation (Suhas Daftuar)
ccb8ca42a4 Always enforce SCRIPT_VERIFY_WITNESS with P2SH (Suhas Daftuar)
5c31b20a35 [qa] Remove some pre-activation segwit tests (Suhas Daftuar)
95749a5836 Separate NULLDUMMY enforcement from SEGWIT enforcement (Suhas Daftuar)
ce650182f4 Use P2SH consensus rules for all blocks (Suhas Daftuar)
Pull request description:
As discussed at the IRC meeting back in October (https://botbot.me/freenode/bitcoin-core-dev/2017-10-12/?msg=92231929&page=2), I had looked into the feasibility of enforcing P2SH and SCRIPT_VERIFY_WITNESS back to the genesis block.
The P2SH change is pretty straightforward -- there was only one historical block on mainnet that violated the rule, so I carved out an exception to it, similar to the way we have exceptions for the BIP30 violators.
The segwit change is not entirely as clear. The code changes themselves are relatively straightforward: we can just always turn on SCRIPT_VERIFY_WITNESS whenever P2SH is active. However conceptually, this amounts to splitting up BIP141 into two parts, the part that implements new script rules, and the part that handles witness commitments in blocks.
Arguably though the script rules are really defined in BIP 143 anyway, and so this really amounts to backdating BIP 143 -- script rules for v0 segwit outputs -- back to genesis. So maybe conceptually this isn't so bad...
I don't feel strongly about this change in either direction; I started working on it because I was searching for a way to simplify the way we understand and implement the consensus rules around segwit, but I'm not yet sure whether I think this achieves anything toward that goal.
ping @TheBlueMatt
Tree-SHA512: 73551d4a983eb9792c7ac67f56005822528ac4d1fd52c27cee6d305ebee953f69687ef4ddee8bdc0fec77f77e6b5a9d669750793efee54c076533a095e233042
Seems providing at least minimal visibility to the failure is a good practice.
The only remaining ignored state is in LoadExternalBlockFile, where logging
would likely be spammy.
To reflect its actual status as a member rather than a global value.
g_failed_blocks was previously global: 2862aca40f
-BEGIN VERIFY SCRIPT-
sed -i 's/g_failed_blocks/m_failed_blocks/g' src/validation.cpp
-END VERIFY SCRIPT-
0ef7b40 [doc] Fix comment in FindForkInGlobalIndex (James O'Beirne)
Pull request description:
The comment erroneously implies that we're searching `chainActive` for the
first block common to `locator`, but we're using the parameter `chain`.
Tree-SHA512: 42ba0fb378597820bdf1eaff1e3e284097baa312e7dd8448421c8c71aa91c353ea6c840860afcb7725f392431f3134d4feb271b96ab7058a62f84f48e468e714
This commit moves P2SH activation back to the genesis block, with
a hardcoded exception for the one historical block in the chain that
violated this rule.
4a6c0e3dcf Modernize best block mutex/cv/hash variable naming (Pieter Wuille)
45dd135039 Fix csBestBlock/cvBlockChange waiting in rpc/mining (Pieter Wuille)
Pull request description:
This is an alternative to #11694.
It reintroduces a uint256 variable with the best block hash, protected by csBestBlock, and only updated while holding it.
Also rename the involved variable to modern guidelines, as there are very few uses.
Tree-SHA512: 826a86c7d3cee7fe49f99f4398ae99e81cb0563197eaeba77306a3ca6072b67cdb932bc35720fc0f99c2a57b218efa029d0b8bdfb240591a629b2e90efa3199d
FlushStateToDisk(...) won't do anything besides check if we need to prune if
FLUSH_STATE_NONE is given. We avoid reading the variable fCheckForPruning
which is guarded by the mutex cs_LastBlockFile.
0e7c52d Shut down if trying to connect a corrupted block (Suhas Daftuar)
Pull request description:
(Updated OP after reworking the approach)
Shut down if a corrupted block is found in ConnectBlock(). This prevents an infinite loop trying to connect such a block, and alerts the node operator that there may be potential hardware failure.
Tree-SHA512: f20d56aa9d36d6eeff4c3d13c0fbd14f06a57701bd13c2416d36f0cc4235f81f752139e336a073617e8e803782c5096c960108af122b19a51227de512e9095ee
