| Age | Commit message (Collapse) | Author |
|---|
|
Can be reviewed with --color-moved=dimmed-zebra
|
|
d831e711cab83c70bf2ded62fe33f484844e73dd [validation] RewindBlockIndex no longer needed (Dhruv Mehta)
Pull request description:
Closes #17862
Context from [original comment](https://github.com/bitcoin/bitcoin/issues/17862#issuecomment-744285188) (minor edits):
`RewindBlockIndex()` is a mechanism to allow nodes to be upgraded after segwit activation, while still keeping their chainstate/datadir in a consistent state. It works as follows:
- A pre-segwit (i.e. v0.13.0 or older) node is running.
- Segwit activates. The pre-segwit node remains sync'ed to the tip, but is not enforcing the new segwit rules.
- The user upgrades the node to a segwit-aware version (v0.13.1 or newer).
- On startup, in `AppInitMain()`, `RewindBlockIndex()` is called. This walks the chain backwards from the tip, disconnecting and erasing blocks that from after segwit activation that weren't validated with segwit rules.
- those blocks are then redownloaded (with witness data) and validated with segwit rules.
This logic probably isn't required any more since:
- Segwit activated at height 481824, when the block chain was 130GB and the total number of txs was 250 million. Today, we're at height 667704, the blockchain is over 315GB and the total number of txs is over 600 million. Even if 20% of that added data is witness data (a high estimate), then around 150GB of transactions would need to be rewound to get back to segwit activation height. It'd probably be faster to simply validate from genesis, especially since we won't be validating any scripts before the assumevalid block. It's also unclear whether rewinding 150GB of transactions would even work. It's certainly never been tested.
- Bitcoin Core v0.13 is hardly used any more. https://luke.dashjr.org/programs/bitcoin/files/charts/software.html shows less than 50 nodes running it. The software was EOL on Aug 1st 2018. It's very unlikely that anyone is running 0.13 and will want to upgrade to 0.22.
This PR introduces `NeedsRedownload()` which merely checks for insufficiently validated segwit blocks and requests that the user restarts the node with `-reindex`. Reindexing the block files upon restart will make the node rebuild chain state and block index from the `blk*.dat` files on disk. The node won't be able to index the blocks with `BLOCK_OPT_WITNESS`, so they will be missing from the chain and be re-downloaded, with witness data.
Removing this code allows the following (done in follow-up #21090):
- removal of tests using `segwitheight=-1` in `p2p_segwit.py`.
- in turn, that allows us to drop support for `-segwitheight=-1`, which is only supported for that test.
- that allows us to always set `NODE_WITNESS` in our local services. The only reason we don't do that is to support `-segwitheight=-1`.
- that in turn allows us to drop all of the `GetLocalServices() & NODE_WITNESS` checks inside `net_processing.cpp`, since our local services would always include `NODE_WITNESS`
ACKs for top commit:
jnewbery:
utACK d831e711cab83c70bf2ded62fe33f484844e73dd
jamesob:
ACK https://github.com/bitcoin/bitcoin/pull/21009/commits/d831e711cab83c70bf2ded62fe33f484844e73dd
laanwj:
Cursory code review ACK d831e711cab83c70bf2ded62fe33f484844e73dd. Agree with the direction of the change, thanks for simplifying the logic here.
glozow:
utACK d831e711cab83c70bf2ded62fe33f484844e73dd
Tree-SHA512: 3eddf5121ccd081ad7f15a5c6478ef867083edc8ba0bf1ee759e87bc070ee3d2f0698a3feba8db8dc087987c8452887b6f72cff05b3e178f41cb10a515fb8053
|
|
Removes assumptions of use only on the active chainstate.
|
|
To prepare VerifyDB semantics for multiple
chainstate use.
|
|
Instead of rewinding blocks, we request that the user restarts with
-reindex
|
|
|
|
|
|
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.
Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
|
|
fa73ce6e653d00824eb68f772fd29b7f8fb93d84 Fix assumeutxo crash due to truncated file (MarcoFalke)
Pull request description:
ACKs for top commit:
jamesob:
ACK fa73ce6e653d00824eb68f772fd29b7f8fb93d84
ryanofsky:
Code review ACK fa73ce6e653d00824eb68f772fd29b7f8fb93d84. Easy fix. It seems like this could have been caught in review, though.
Tree-SHA512: 3a98687c386e3995114ddf0ad7194fadd9520989290681ef703b578e3ca21aee51eadfb83aa38a489bac13d12709ea137b9b184b08e5bfa2919cca177aab90be
|
|
fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f doc: Remove irrelevant link to GitHub (MarcoFalke)
fa121b628d51bb0e25eb3fbd716881fa55527dc7 blockstorage: [refactor] Use chainman reference where possible (MarcoFalke)
fa0c7d9ad24d3c9515d3f9c136af4071cbd79055 move-only: Move *Disk functions to blockstorage (MarcoFalke)
fa91b2b2b3447a3645e7958c7dc4e1946a69cb9c move-only: Move AbortNode to shutdown (MarcoFalke)
fa413f07a14744e7d7f7746e861aabd9cf938f61 move-only: Move ThreadImport to blockstorage (MarcoFalke)
faf843c07f99f91603e08ea858f972516f1d669a refactor: Move load block thread into ChainstateManager (MarcoFalke)
Pull request description:
This picks up the closed pull request #21030 and is the first step toward fixing #21220.
The basic idea is to move all disk access into a separate module with benefits:
* Breaking down the massive files init.cpp and validation.cpp into logical units
* Creating a standalone-module to reduce the mental complexity
* Pave the way to fix validation related circular dependencies
* Pave the way to mock disk access for testing, especially where it is performance critical (like fuzzing)
ACKs for top commit:
promag:
Code review ACK fadcd3f78e, checked (almost) moved only changes. This is a nice tidy up change and doesn't change behavior. Easily reviewed commit by commit.
jamesob:
ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f ([`jamesob/ackr/21575.1.MarcoFalke.refactor_create_blocksto`](https://github.com/jamesob/bitcoin/tree/ackr/21575.1.MarcoFalke.refactor_create_blocksto))
ryanofsky:
Code review ACK fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f. New organization makes sense, moves extraneous things outside of validation.cpp. PR is also easy to review with helpfully split up moveonly commits.
Tree-SHA512: 917996592b6d8f9998289d8cb2b1b78b23d1fdb3b07216c9caec1380df33baa09dc2c1e706da669d440b497e79c9c62a01ca20dc202df5ad974a75f3ef7a143b
|
|
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
|
Can be reviewed with the git option
--color-moved=dimmed-zebra
|
|
|
|
|
|
4eca20d6f7d850492d331d89d1cdd77abb3c70c1 [doc] correct comment about ATMPW (glozow)
8fa74aeb5b96419c7d40b40f8e1e1269509278e2 [doc] correct comment in chainparams (glozow)
2f8272c2a4b6fa84c04dfeb4d751bb218f2d4c78 [doc] GetBestBlock() doesn't do nothing (gzhao408)
Pull request description:
Came across a few misleading comments, wanted to fix them
ACKs for top commit:
jnewbery:
ACK 4eca20d6f7
MarcoFalke:
ACK 4eca20d6f7d850492d331d89d1cdd77abb3c70c1
laanwj:
Code review ACK 4eca20d6f7d850492d331d89d1cdd77abb3c70c1
Tree-SHA512: 5bef1f1e7703f304128cf0eb8945e139e031580c99062bbbe15bf4db8443c2ba5a8c65844833132e6646c8980c678fc1d2ab0c63e17105585d583570ee350fd0
|
|
ATMPW stands for AcceptToMemoryPoolWorker, which was removed in #16400.
|
|
This has tripped people up multiple times because it looks like
GetBestBlock is a const function returning the value of hashBlock.
|
|
|
|
In #21415 we decided to return `std::optional` rather than `{}` for
uninitialized values. This PR repalces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
```
|
|
CChainState::InvalidateBlock
787df19b09babf50dd8124b3ac990b29c33cfe93 validation: don't try to invalidate genesis block (Sebastian Falbesoner)
Pull request description:
In the block invalidation method (`CChainState::InvalidateBlock`), the code for creating the candidate block map assumes that the passed block's previous block (`pindex->pprev`) is available and otherwise segfaults due to null-pointer deference in `CBlockIndexWorkComparator()` (see analysis by practicalswift in #20914), i.e. it doesn't work with the genesis block. Rather than analyzing all possible code paths and implications for this corner case, simply fail early if the genesis block is passed.
Fixes #20914.
ACKs for top commit:
sipa:
ACK 787df19b09babf50dd8124b3ac990b29c33cfe93. Tested invalidation of generic on regtest.
practicalswift:
Tested ACK 787df19b09babf50dd8124b3ac990b29c33cfe93
Tree-SHA512: 978be7cf2bd1c1faebfe945d191ac77dea72791bea826459abd308f77c74c5991efee495a38817c306e488ecd5208b5c888df7d9d044132dd9a06bbbdb256b6c
|
|
ebc4ab721b0371c0ef217c0f5bd7d42613e951e6 refactor: post Optional<> removal cleanups (fanquake)
57e980d13ca488031bde6ef197cf34d493d36796 scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6: patch looks correct
jnewbery:
utACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6
laanwj:
Code review ACK ebc4ab721b0371c0ef217c0f5bd7d42613e951e6
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
|
|
|
|
DumpMempool(...)
68afd3eeec27a270765ad26cd62d87cd0935e99f tests: Add fuzzing harness for LoadMempool(...) and DumpMempool(...) (practicalswift)
91af6b97c9197f8ac9766a8559dd50bbc443ad38 validation: Make DumpMempool(...) and LoadMempool(...) easier to test/fuzz/mock (practicalswift)
af322c7494d6bc4b94890c85d16623b082c4fe24 tests: Set errno in FuzzedFileProvider. Implement seek(..., ..., SEEK_END). (practicalswift)
Pull request description:
Add fuzzing harness for `LoadMempool(...)` and `DumpMempool(...)`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
jonatack:
Tested re-ACK 68afd3eeec27a270765ad26cd62d87cd0935e99f
Tree-SHA512: 4b5fcaa87e6eb478611d3b68eb6859645a5e121e7e3b056ad2815699dace0a6123706ff542def371b47f4ab3ce2b8a29782026d84fb505827121e9b4cc7dac31
|
|
-BEGIN VERIFY SCRIPT-
git rm src/optional.h
sed -i -e 's/Optional</std::optional</g' $(git grep -l 'Optional<' src)
sed -i -e 's/{nullopt}/{std::nullopt}/g' $(git grep -l 'nullopt' src)
sed -i -e 's/ nullopt;/ std::nullopt;/g' $(git grep -l 'nullopt' src)
sed -i -e 's/ nullopt)/ std::nullopt)/g' $(git grep -l 'nullopt' src)
sed -i -e 's/(nullopt)/(std::nullopt)/g' $(git grep -l 'nullopt' src)
sed -i -e 's/ nullopt,/ std::nullopt,/g' $(git grep -l 'nullopt' src)
sed -i -e 's/? nullopt :/? std::nullopt :/g' $(git grep -l 'nullopt' src)
sed -i -e 's/: nullopt}/: std::nullopt}/g' $(git grep -l 'nullopt' src)
sed -i -e '/optional.h \\/d' src/Makefile.am
sed -i -e '/#include <optional.h>/d' src/test/fuzz/autofile.cpp src/test/fuzz/buffered_file.cpp src/test/fuzz/node_eviction.cpp
sed -i -e 's/#include <optional.h>/#include <optional>/g' $(git grep -l '#include <optional.h>' src)
-END VERIFY SCRIPT-
|
|
1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff doc: update developer notes for removal of MakeUnique (fanquake)
3ba2840e7ee81341b0748c0121aedc2e9305041a scripted-diff: remove MakeUnique<T>() (fanquake)
Pull request description:
Since requiring C++17, this is just pointless abstraction. I think we should just "tear the band-aid off" and remove it. Similar to the changes happening in #21366.
Also, having a comment saying this is deprecated doesn't prevent it's usage in new code. i.e : https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.
The repository is fairly quiet at the moment, so any potential complaints about having to rebase should be minimal. Might as well get this over and done with.
ACKs for top commit:
jnewbery:
utACK 1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff
practicalswift:
cr ACK 1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff: patch looks correct
ajtowns:
ACK 1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff -- code review only
glozow:
ACK https://github.com/bitcoin/bitcoin/commit/1a6323bdbe20bdb7b1c907d8fa0333ffa88b21ff looks correct
Tree-SHA512: 4a14b9611b60b9b3026b54d6f5a2dce4c5d9b63a7b93d7de1307512df736503ed84bac66e7b93372c76e3117f49bf9f29cd473d3a47cb41fb2775bc10234736f
|
|
|
|
-BEGIN VERIFY SCRIPT-
git rm src/util/memory.h
sed -i -e 's/MakeUnique/std::make_unique/g' $(git grep -l MakeUnique src)
sed -i -e '/#include <util\/memory.h>/d' $(git grep -l '#include <util/memory.h>' src)
sed -i -e '/util\/memory.h \\/d' src/Makefile.am
-END VERIFY SCRIPT-
|
|
|
|
...instead of recursively locking unconditionally
|
|
|
|
|
|
|
|
|
|
This is the only instance where validation reaches for something outside
of it.
|
|
|
|
CChainState needed cuz setBlockIndexCandidates
|
|
- InvalidChainFound
- CheckForkWarningConditions
|
|
|
|
Tip: versionbitscache is currently a global so we didn't need to pass it
in to any of ::VersionBitsTip*'s callers
|
|
|
|
|
|
|
|
|
|
...also update comments to remove mention of ::ChainActive()
From: https://github.com/bitcoin/bitcoin/pull/20750#discussion_r579400663
> Also, what about passing a const reference instead of a pointer? I
> know this is only theoretical, but previously if the tip was nullptr,
> then Height() evaluated to -1, now it evaluates to UB
|
|
25c57d640992255ed67964a44b17afbfd4bed0cf [doc] Add a note about where lock annotations should go. (Amiti Uttarwar)
ad5f01b96045f304b6cf9100879592b835c49c40 [validation] Move the lock annotation from function definition to declaration (Amiti Uttarwar)
Pull request description:
Based on reviewing #21188
the first commit switches the lock annotations on `CheckInputScripts` to be on the function declaration instead of on the function definition. this ensures that all call sites are checked, not just ones that come after the definition.
the second commit adds a note to the developer-notes section to clarify where the annotations should be applied.
ACKs for top commit:
MarcoFalke:
ACK 25c57d640992255ed67964a44b17afbfd4bed0cf 🥘
promag:
Code review ACK 25c57d640992255ed67964a44b17afbfd4bed0cf.
Tree-SHA512: 61b6ef856bf6c6016d535fbdd19daf57b9e59fe54a1f30d47282a071b9b9d60b2466b044ee57929e0320cb1bdef52e7a1687cacaa27031bbc43d058ffffe22ba
|
|
|
|
|
|
|
|
|
|
Several other parameters are now redundant since they can be safely
obtained from the chainstate given that ::cs_main is locked. These are
now removed.
|
