* Handle attempts to read non-existent records from isdb properly
* Do not reject blocks that conflict with islocks while still syncing
Otherwise you can stuck with no new blocks/headers which means you won't be able to verify new chainlocks that might override stored islocks
* Handle duplicates/conflicting islocks better
* More constness
789e9dd3aa727176797529c35b2848f994630a82 validation: use std::chrono in IsCurrentForFeeEstimation() (fanquake)
47be28c8bc475eafeebd4fc58ea92f0d3df0d8c6 validation: use std::chrono in CChainState::FlushStateToDisk() (fanquake)
Pull request description:
Probably up for debate as to which type is used for the constants. Personally, swapping these to hours is more readable.
ACKs for top commit:
MarcoFalke:
ACK 789e9dd3aa727176797529c35b2848f994630a82
jonatack:
ACK 789e9dd3aa727176797529c35b2848f994630a82
Tree-SHA512: f4a25cbd00a49a54b7783a1f588be83706dd2a475cecb5c2e8b97b2d4b27c0955a7454d7486f2454e96351c44f233b300c4f4b9ca62fc7336277f10da34dd5c3
62d50ef308 Add LOCKS_EXCLUDED(cs_main) to LimitValidationInterfaceQueue(...) which does AssertLockNotHeld(cs_main) (practicalswift)
Pull request description:
This PR adds compile-time checking for negative locking requirements that follow from the run-time locking requirement `AssertLockNotHeld(cs_main)` in `LimitValidationInterfaceQueue(...)`.
Changes:
* Add `LOCKS_EXCLUDED(cs_main)` to `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
* Add `LOCKS_EXCLUDED(cs_main)` to `CChainState::ActivateBestChain(…)`, `CChainState:: InvalidateBlock(…)` and `CChainState::RewindBlockIndex(…)` which all call `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
* Add `LOCKS_EXCLUDED(cs_main)` to `InvalidateBlock(…)` which calls `CChainState::InvalidateBlock(...)` which in turn calls `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
* Add `LOCKS_EXCLUDED(cs_main)` to `RewindBlockIndex(…)` which calls `CChainState::RewindBlockIndex(...)` which in turn calls `LimitValidationInterfaceQueue(...)` which does `AssertLockNotHeld(cs_main)`
ACKs for commit 62d50e:
MarcoFalke:
utACK 62d50ef308
Tree-SHA512: 73d092ccd08c851ae3c5d60370c369fc030c5793f5507e2faccb6f91c851ddc0ce059fbea3899f2856330d7a8c78f2ac6a2988e8268b03154f946be9e60e3be1
2a4e60b48261d3f0ec3d85f97af998ef989134e0 Fix block index inconsistency in InvalidateBlock() (Suhas Daftuar)
Pull request description:
Previously, we could release `cs_main` while leaving the block index in a state
that would fail `CheckBlockIndex()`, because `setBlockIndexCandidates` was not being
fully populated before releasing `cs_main`.
ACKs for top commit:
TheBlueMatt:
utACK 2a4e60b48261d3f0ec3d85f97af998ef989134e0. I also discovered another issue in InvalidateBlock while reviewing, see #16856.
Sjors:
ACK 2a4e60b. Tested on top of #16899. Also tested `invalidateblock` with `-checkblockindex=1`.
fjahr:
ACK 2a4e60b. Ran tests, reviewed code, inspected behavior while manually testing `invalidateblock`.
Tree-SHA512: ced12f9dfff0d413258c709921543fb154789898165590b30d1ee0cdc72863382f189744f7669a7c924d3689a1cc623efdf4e5ae3efc60054572c1e6826de612
519b0bc5dc5155b6f7e2362c2105552bb7618ad0 Make last disconnected block BLOCK_FAILED_VALID, even when aborted (Pieter Wuille)
8d220417cd7bc34464e28a4861a885193ec091c2 Optimization: don't add txn back to mempool after 10 invalidates (Pieter Wuille)
9ce9c37004440d6a329874dbf66b51666d497dcb Prevent callback overruns in InvalidateBlock and RewindBlockIndex (Pieter Wuille)
9bb32eb571a846b66ed3bac493f55cee11a3a1b9 Release cs_main during InvalidateBlock iterations (Pieter Wuille)
9b1ff5c742dec0a6e0d6aab29b0bb771ad6d8135 Call InvalidateBlock without cs_main held (Pieter Wuille)
241b2c74ac8c4c3000e778554da1271e3f293e5d Make RewindBlockIndex interruptible (Pieter Wuille)
880ce7d46b51835c00d77a366ec28f54a05239df Call RewindBlockIndex without cs_main held (Pieter Wuille)
436f7d735f1c37e77d42ff59d4cbb1bd76d5fcfb Release cs_main during RewindBlockIndex operation (Pieter Wuille)
1d342875c21b5d0a17cf4d176063bb14b35b657e Merge the disconnection and erasing loops in RewindBlockIndex (Pieter Wuille)
32b2696ab4b079db736074b57bbc24deaee0b3d9 Move erasure of non-active blocks to a separate loop in RewindBlockIndex (Pieter Wuille)
9d6dcc52c6cb0cdcda220fddccaabb0ffd40068d 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
418d3230f8 Resolve the checkpoints <-> validation CD. (251)
Pull request description:
This pull request attempts to resolve the `checkpoints -> validation -> checkpoints` circular dependency.
The circular dependency is resolved by moving the `CheckPoints::GetLastCheckpoint(const CCheckpointData& data)` function to `validation.cpp` where it used exclusively by the private function `ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, const CChainParams& params, const CBlockIndex* pindexPrev, int64_t nAdjustedTime)`.
ACKs for commit 418d32:
promag:
utACK 418d323, only `GetLastCheckpoint` usage is in `validation.cpp` and so makes sense to move it there.
practicalswift:
utACK 418d3230f86f77dde6e817f502baff8a54b707fa
MarcoFalke:
utACK 418d3230f86f77dde6e817f502baff8a54b707fa
sipa:
utACK 418d3230f86f77dde6e817f502baff8a54b707fa
Tree-SHA512: 03c3556bc192e65f5e3fa76fd545d4ee7d63d3fb06b132f7a1fa6131aa21ddd2e5b2d19e2222dfe524f422daaca30efde219bed188db8c74ff4b088876b5bc16
* merge bitcoin#15855: Add missing LockAnnotation for cs_main
* mutex: update cs_main locks, assertions and annotations
This commit is a squash between 8c98823 and 90d0535
ce9dd45422e1f4ecce6df68da086b8bfc2100756 Add [[nodiscard]] to RenameOver(...) (practicalswift)
9429a398e291a1b5edcfc657b94fcaf52cd1d8f9 Handle rename failure in DumpMempool(...) by using RenameOver(...) return value (practicalswift)
Pull request description:
Handle rename failure in `DumpMempool(...)` by using the `RenameOver(...)` return value.
Add `[[nodiscard]]` to `RenameOver(...)` to reduce the risk of similar rename issues in the future.
ACKs for top commit:
vasild:
ACK ce9dd454
theStack:
ACK ce9dd45422e1f4ecce6df68da086b8bfc2100756 🏷️
Tree-SHA512: 1e63d7f3061e1f6ea2df5750dbc1547a39bd50b6c529812a0c8a0c11d3100c241afdf14094e69b69a38bade7e54a12b2a42888545874398eaf5d02421b57e874
ac94141af0c16161afa68de1c3720f254ae4e12c validation: delay flushing undo files in syncing node case (Karl-Johan Alm)
Pull request description:
Fixes#17890. Replaces #17892.
Data files (`{blk|rev}<number>.dat`) pre-allocate space as they are written, and then trims down to the final size once they move on to the next sequence ("finalized flush"). The code currently assumes (incorrectly) that blk and rev files finish at the same time, but because blk files are written as blocks come in, and rev files are written in block height order, rev files end up being written to for awhile after moving on to the next block file, resulting in pre-allocation and waste of up to 1 MB of space per rev file.
The exact point at which rev file writing finishes is the highest height block found inside the corresponding block file, which is already available in the CBlockFileInfo vector. This PR moves finalized flushing of undo files to to directly after the undo data for the previous block file has been written.
There is a branch with annotation that demonstrates how this is handling flushing here: https://github.com/kallewoof/bitcoin/tree/200124-rev-files-annotated
ACKs for top commit:
vasild:
ACK ac94141af (no changes in the code since ed34e00da).
fjahr:
Code review re-ACK ac94141af0c16161afa68de1c3720f254ae4e12c
jonatack:
Code review ACK ac94141af0c16
Tree-SHA512: 1d4e3b3d1d99bd7ebe7a2f632b1231146dd4f9f993c54db3a4090d9c086d95d2e4c327fd936066392b3afc6277b8f3a908d5c5993d4c8e49f72b92a417716dd2
21fa0a44abe8c1b5c452e097eab20cf0ae988805 [docs] use consistent naming for possible_overwrite (John Newbery)
2685c214cce4b07695273503e60350e3f05fe3e2 [tests] small whitespace fixup (John Newbery)
e9936966c08bd8a6ac02828131f619ddaa1ced13 scripted-diff: Rename PRUNED to SPENT in coins tests (John Newbery)
c205979031ff4e8e32a5f05bae813405f233fccd [docs] Improve commenting in coins.cpp|h (John Newbery)
Pull request description:
- Add full commenting for spentness / DIRTYness / FRESHness and which combinations are valid
- Remove the 'pruned' terminology, which doesn't make sense since per-txout chainstate db was merged (#10195).
- Rename `potential_overwrite` to `possible_overwrite` to standardize terminology (there were previously examples of both, which made searching the codebase difficult).
- Make other minor improvements to the comments
ACKs for top commit:
jonatack:
Re-ACK 21fa0a4 per `git diff 98bee55 21fa0a4` the only change since my previous review is the following code commenting diff in `src/coins.cpp::L177-179`; rebuilt/ran unit tests anyway as a sanity check on the unit test changes.
Tree-SHA512: 391e01588ef5edb417250080cec17361f982c4454bc5f8c6d78bbd528c68a2bb94373297760691295c24660ce1022ad3ef7599762f736c8eed772ce096d38c3d
* refactor: Add `const` qualifier to various llmq related functions
* refactor: Add thread safety annotations to various llmq related functions
And around them where it's required
c5ed6e73d Move CheckBlock() call to critical section (Hennadii Stepanov)
Pull request description:
This is an alternative to #14803.
Refs:
- #14058
- #14072
- https://github.com/bitcoin/bitcoin/pull/14803#issuecomment-442233211 by @gmaxwell
> It doesn't support multithreaded validation and there are lot of things that prevent that, which is why I was concerned. Why doesn't the lock on the block index or even cs main prevent concurrency here?
- https://github.com/bitcoin/bitcoin/pull/14803#issuecomment-442237566 by @MarcoFalke
Tree-SHA512: 2152e97106e11da5763b2748234ecd2982daadab13a0da04215f4db60af802a44ab5700f32249137d122eb13fc2a02e0f2d561d364607d727d8c6ab879339afb
ef362f2773 rpc/gui: Remove 'Unknown block versions being mined' warning (Wladimir J. van der Laan)
Pull request description:
Due to miners inserting garbage into the version numbers causing false positives, 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.
Tree-SHA512: 51407ccd24a571462465d9c7180f0f28307c50b82a03284abe783e181d8ab7e0638dbb710698d883f28de8a609db70763e39be2470d956e67c833da0768e43e9
* Merge #13311: Don't edit Chainparams after initialization
6fa901fb47 Don't edit Chainparams after initialization (Jorge Timón)
980b38f8a1 MOVEONLY: Move versionbits info out of versionbits.o (Jorge Timón)
Pull request description:
This encapsulates the "-vbparams" option, which is only meant for regtest, directly on CRegTestParams.
This is a refactor and doesn't change functionality.
Related to https://github.com/bitcoin/bitcoin/pull/8994
Tree-SHA512: 79771d729a63a720e743a9c77d5e2d80369f072d66202a43c1304e83a7d0ef7c6103d4968a03aea9666cc89a7203c618da972124a677b38cfe62ddaeb28f9f5d
* Resolve Merge with #13311
* Incorporated review changes
* Apply suggestions from code review
* Update src/chainparams.cpp
* Update src/chainparams.cpp
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
* Apply suggestions from code review
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
* Merge #17519: rpc: Remove unused COINBASE_FLAGS
e9a27cf338dc618b8ecab8984abc54d588de8a05 refactor: Remove unused COINBASE_FLAGS (Neha Narula)
Pull request description:
Commit d449772cf6 stopped setting
COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.
Following up on #17489, remove COINBASE_FLAGS which is unused. I verified that removing this did not change the contents of the coinbase's scriptSig.
ACKs for top commit:
laanwj:
ACK e9a27cf338dc618b8ecab8984abc54d588de8a05
MarcoFalke:
ACK e9a27cf338dc618b8ecab8984abc54d588de8a05 💻
Tree-SHA512: f9dac124ce7e3edcae974137764bb5039387b1b123b86af44486e398aa4a8d91a9ecf640e207b364ae303acbbaee7cca300d303ea3d6869ba9cae2bf555a6334
* Update src/rpc/mining.cpp
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: Wladimir J. van der Laan <laanwj@protonmail.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
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
* Merge #13743: refactor: Replace boost::bind with std::bind
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
* Replace boost::bind with std::bind and remove Boost.Bind includes
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fa4fc8856b239059421a8e507b3e3e7f4b379a72 validation: Add and use HaveTxsDownloaded where appropriate (MarcoFalke)
Pull request description:
`nChainTx` is an implementation detail that shouldn't be exposed without a wrapper that comes with appropriate documentation.
Tree-SHA512: 56ab7378c2ce97794498724c271f861de982de69099e90ec09632a26230ae6fded3c59668adb378bd64dcb8ef714769b970210977b88a53fc7550774ddba3d59
faa1a749428a195af784633eb78e1df5d6a0e875 tx pool: Use class methods to hide raw map iterator impl details (MarcoFalke)
Pull request description:
ATMP et al would often use map iterator implementation details such as
`end()` or `find()`, which is acceptable in current code.
However, this not only makes it impossible to turn the maps into
private members in the future but also makes it harder to replace the
maps with different data structures.
This is required for and split off of #13804
Tree-SHA512: 4f9017fd1d98d9df49d25bba92655a4a97755eea161fd1cbb565ceb81bbc2b4924129d214f8a29563a77e3d8eef85a67c81245ecdc9a9e5292d419922a93cb88
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
