519b0bc5dc Make last disconnected block BLOCK_FAILED_VALID, even when aborted (Pieter Wuille)
8d220417cd Optimization: don't add txn back to mempool after 10 invalidates (Pieter Wuille)
9ce9c37004 Prevent callback overruns in InvalidateBlock and RewindBlockIndex (Pieter Wuille)
9bb32eb571 Release cs_main during InvalidateBlock iterations (Pieter Wuille)
9b1ff5c742 Call InvalidateBlock without cs_main held (Pieter Wuille)
241b2c74ac Make RewindBlockIndex interruptible (Pieter Wuille)
880ce7d46b Call RewindBlockIndex without cs_main held (Pieter Wuille)
436f7d735f Release cs_main during RewindBlockIndex operation (Pieter Wuille)
1d342875c2 Merge the disconnection and erasing loops in RewindBlockIndex (Pieter Wuille)
32b2696ab4 Move erasure of non-active blocks to a separate loop in RewindBlockIndex (Pieter Wuille)
9d6dcc52c6 Abstract EraseBlockData out of RewindBlockIndex (Pieter Wuille)
Pull request description:
This PR makes a number of improvements to the InvalidateBlock (`invalidateblock` RPC) and RewindBlockIndex functions, primarily around breaking up their long-term cs_main holding. In addition:
* They're made safely interruptible (`bitcoind` can be shutdown, and no progress in either will be lost, though if incomplete, `invalidateblock` won't continue after restart and will need to be called again)
* The validation queue is prevented from overflowing (meaning `invalidateblock` on a very old block will not drive bitcoind OOM) (see #14289).
* `invalidateblock` won't bother to move transactions back into the mempool after 10 blocks (optimization).
This is not an optimal solution, as we're relying on the scheduler call sites to make sure the scheduler doesn't overflow. Ideally, the scheduler would guarantee this directly, but that needs a few further changes (moving the signal emissions out of cs_main) to prevent deadlocks.
I have manually tested the `invalidateblock` changes (including interrupting, and running with -checkblockindex and -checkmempool), but haven't tried the rewinding (which is probably becoming increasingly unnecessary, as very few pre-0.13.1 nodes remain that would care to upgrade).
Tree-SHA512: 692e42758bd3d3efc2eb701984a8cb5db25fbeee32e7575df0183a00d0c2c30fdf72ce64c7625c32ad8c8bdc56313da72a7471658faeb0d39eefe39c4b8b8474
04cca33094 Style cleanup. (Jim Posen)
4c01e4e159 flatfile: Unit tests for FlatFileSeq methods. (Jim Posen)
65a489e93d scripted-diff: Rename CBlockDiskPos to FlatFilePos. (Jim Posen)
d6d8a78f26 Move CDiskBlockPos from chain to flatfile. (Jim Posen)
e0380933e3 validation: Refactor file flush logic into FlatFileSeq. (Jim Posen)
992404b31e validation: Refactor block file pre-allocation into FlatFileSeq. (Jim Posen)
e2d2abb99f validation: Refactor OpenDiskFile into method on FlatFileSeq. (Jim Posen)
9183d6ef65 validation: Extract basic block file logic into FlatFileSeq class. (Jim Posen)
62e7addb63 util: Move CheckDiskSpace to util. (Jim Posen)
Pull request description:
This cleans up and refactors block file helpers so that they may be used by the block filter indexer. Per [design discussion](https://github.com/bitcoin/bitcoin/pull/14121#issuecomment-451252591) about storing BIP 157 block filters, it has been suggested that they are stored in the same way as block and undo data. This refactor is sufficient to simplify file operations for this use case, though in the future perhaps more pruning-related logic ought to be moved into the new classes.
The basic abstraction is a `FlatFileSeq` which manages access to a sequence of numbered files into which raw data is written.
Tree-SHA512: b2108756777f2dad8964a1a2ef2764486e708a4a4a8cfac47b5de8bcb0625388964438eb096b10cfd9ea39212c299b5cb32fa943e768db2333cf49ea7def157e
Due to miners inserting garbage into the version numbers, the current
version signalling has become completely useless. This removes the
"unknown block versions" warning which has the tendency to scare
users unnecessarily (and might get them to "update" to something
bad).
It preserves the warning in the logs. Whether this is desirable can
be a point of discussion.
Note that the former 'else' branch in RewindBlockIndex is now
dealt with more naturally inside the EraseBlockData call (by
checking whether the parent needs to be re-added as candidate
after deleting a child).
04da9f4834 [RPC] Update getrawtransaction interface (Amiti Uttarwar)
Pull request description:
- stop checking unspent UTXOs for a transaction when txindex is not enabled, as per conversation here: https://github.com/bitcoin/bitcoin/issues/3220#issuecomment-377458383
- code contributed by sipa
Tree-SHA512: aa07353bccc14b81b7803992a25d076d6bc06d15ec7c1b85828dc10aea7e0498d9b49f71783e352ab8a14b0bb2010cfb7835de3dfd1bc6f2323f460449348e66
fa5e373365 validation: Add cs_main locking annotations (MarcoFalke)
fa5c346c5a doc: Add comment to cs_main and mempool::cs (MarcoFalke)
fafe941bdd test: Add missing validation locks (MarcoFalke)
fac4558462 sync: Add RecursiveMutex type alias (MarcoFalke)
Pull request description:
Both the chain state and the transaction pool are validation specific, but access to them is protected by two locks. The two locks have the following semantics:
* Writing to the chain state or adding transactions to the transaction pool -> Take both `cs_main` and `mempool::cs`
* Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock
Tree-SHA512: 6f6e612ffc391904c6434a79a4f3f8de1b928bf0a3e3434b73561037b395e2b40a70a5a4bd8472dd230e9eacc8e5d5374c904a3c509910cf3971dd7ff59a626c
e58985c916 Log progress while verifying blocks at level 4. (Daniel Kraft)
Pull request description:
When verifying blocks at startup, the progress is printed in 10% increments to logs. When `-checklevel=4`, however, the second half of the verification (connecting the blocks again) does not log the progress anymore. (It is still computed and shown in the UI, but not printed to logs.)
This change makes the behaviour consistent, by adding the missing progress logging also for level-4 checks.
Tree-SHA512: 6a4c5914726fc1a1337de0c5130b20d4edf4e2feeb0aa0449d2ce422b2d8c41e56ede94163a02044d9a28ac4dc6624b1ad611da93ce5792ff32ad9fb1f0ea1e0
cb53b825c2 scripted-diff: Replace boost::bind with std::bind (Chun Kuan Lee)
2196c51821 refactor: Use boost::scoped_connection in signal/slot, also prefer range-based loop instead of std::transform (Chun Kuan Lee)
Pull request description:
Replace boost::bind with std::bind
- In `src/rpc/server.cpp`, replace `std::transform` with simple loop.
- In `src/validation.cpp`, store the `boost::signals2::connection` object and use it to disconnect.
- In `src/validationinterface.cpp`, use 2 map to store the `boost::signals2::scoped_connection` object.
Tree-SHA512: 6653cbe00036fecfc495340618efcba6d7be0227c752b37b81a27184433330f817e8de9257774e9b35828026cb55f11ee7f17d6c388aebe22c4a3df13b5092f0
66e15e8f97 Explain GetAncestor check for m_failed_blocks in AcceptBlockHeader (Sjors Provoost)
Pull request description:
Salvaged (but slightly modified) from #12138, the comment there was really helpful to wrap my head around that part of the code.
In addition, a naive reader like yours truly will first think `IsValid(BLOCK_VALID_SCRIPTS)` means the previous block was invalid. But IIUC that's not what it means. Instead, it means the block hasn't been checked for validity at the `BLOCK_VALID_SCRIPTS` level yet. So in that case the existing text "previous block index isn't valid" is wrong.
Tree-SHA512: 442a319a83290d94697fdf51376463b70454e0f3909d4a45594ddc2e7c26cd19dc703808385a25e26d6d2dddab0aa35ca41722f2e65ee6fe57bbaf62652d3ec8
fbaaf782ce validation: assert that pindexPrev is non-null when required (Karl-Johan Alm)
Pull request description:
In `ContextualCheckBlock`, we are checking if `pindexPrev == nullptr` conditionally at the start, but then assume it is non-`null` later. This removes the latter assumption.
Tree-SHA512: 95f1e9dc839b2cc0e099d155e6180634ece8c6760d00b53e7d27128762e64c92e82d98a5f4a5786b48a4851b17cdbb4b667d3b6a99adb651256e2032de67d05c
b7df96f456 refactor: Drop boost::this_thread::interruption_point and boost::thread_interrupted in main thread (Chun Kuan Lee)
Pull request description:
This PR drops useless `boost::this_thread::interruption_point` and `boost::thread_interrupted` catch. They are only executed in main thread.
Tree-SHA512: a980d098c1a8238e4f0da9493731d7e69b9ca8e010103f442722d0d4cce471cc40a1fafd5f05535ad0e18899b6cf7563ee20e4025f7c7bc15182a0058c028922
fa511e8dad Pass tx pool reference into CheckSequenceLocks (MarcoFalke)
Pull request description:
`CheckSequenceLocks` is called from ATMP and the member function `CTxMemPool::removeForReorg` without passing in the tx pool object that is used in those function's scope and instead using the global `::mempool` instance.
This fix should be refactoring only, since currently there is only one (global) tx pool in normal operation. Though, it fixes hard to track down issues in future settings where more than one mempool exists at a time. (E.g. for tests, rpc or p2p tx relay purposes)
Tree-SHA512: f0804588c7d29bb6ff05ec14f22a16422b89ab31ae714f38cd07f811d7dc7907bfd14e799c4c1c3121144ff22711019bbe9212b39e2fd4531936a4119950fa49
3ccfa34b32 convert C-style (void) parameter lists to C++ style () (Arvid Norberg)
Pull request description:
In C, an empty parameter list, `()`, means the function takes any arguments, and `(void)` means the function does not take any parameters.
In C++, an empty parameter list means the function does not take any parameters.
So, C++ still supports `(void)` parameter lists with the same semantics, why change to `()`?
1. removing the redundant `void` improves signal-to-noise ratio of the code
2. using `(void)` exposes a rare inconsistency in that a template taking a template `(T)` parameter list, cannot be instantiated with `T=void`
Tree-SHA512: be2897b6c5e474873aa878ed6bac098382cd21866aec33752fe40b089a6331aa6263cae749aba1b4a41e8467f1a47086d32eb74abaf09927fd5a2f44a4b2109a
893628be01 Drop minor GetSerializeSize template (Ben Woosley)
da74db0940 Drop unused GetType() from CSizeComputer (Ben Woosley)
Pull request description:
Based on conversation in #13462, it seems the serialization `GetType` has very narrow use/effect. In every case except for `CAddress`, which specifically relates to a network peer's address, not a wallet address etc., the serialized representation of an object is irrespective of its destination / type.
This removes the unused `GetType` method from `CSizeComputer` as a step to further narrowing that use.
Tree-SHA512: e72b8e9e5160396691e05aeaee3aba5a57935a75bd5005cfcc7fb51c936f3d1728a397f999da5c36696506dd815fafa5c738f3894df8864f25f91f639eba9c3d
