| Age | Commit message (Collapse) | Author |
|---|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
fab6d6b2d154893ab422dda87f3535d42c3e06f4 Move pindexBestInvalid to ChainstateManager (MarcoFalke)
facd2137eceacb95e1f71c87ddc704d752b37272 Move m_failed_blocks to ChainstateManager (MarcoFalke)
fa47b5c100f81c65c15b5a6afaf6c91bc0861264 Move AcceptBlockHeader to ChainstateManager (MarcoFalke)
fa3d62cf7b3501a056b34c5458c14d2fe6a55bd7 Move FindForkInGlobalIndex from BlockManager to CChainState (MarcoFalke)
Pull request description:
Move globals or members of the wrong class to the right class.
ACKs for top commit:
naumenkogs:
ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
Sjors:
ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
shaavan:
ACK fab6d6b2d154893ab422dda87f3535d42c3e06f4
Tree-SHA512: 926cbdfa22838517497bacb79ed5f521f64117c2aacf96a0176f62831b4713314a32abc0213df5ee067edf63e4a4300f752a26006d36e5aab415bb91209a271f
|
|
for packages of 1 child + parents
046e8ff264be6b888c0f9a9d822e32aa74e19b78 [unit test] package submission (glozow)
e12fafda2dfbbdf63f125e5af797ecfaa6488f66 [validation] de-duplicate package transactions already in mempool (glozow)
8310d942e046c5a9b6bd90afdcd3af68dd91e081 [packages] add sanity checks for package vs mempool limits (glozow)
be3ff151a1f9665720cdf70d072b098a2f9726a9 [validation] full package accept + mempool submission (glozow)
144a29099a865ac1dc3e5291d9529fbcca9c83a4 [policy] require submitted packages to be child-with-unconfirmed-parents (glozow)
d59ddc5c3d1c035474d7bc9fa9f8a0eeb1c8498c [packages/doc] define and document package rules (glozow)
ba26169f6035c238378a3c9647213328a006fa23 [unit test] context-free package checks (glozow)
9b2fdca7f03911ac40fe0f8a0b5da534bee4554b [packages] add static IsChildWithParents function (glozow)
Pull request description:
This is 1 chunk of [Package Mempool Accept](https://gist.github.com/glozow/dc4e9d5c5b14ade7cdfac40f43adb18a); it restricts packages to 1 child with its parents, doesn't allow conflicts, and doesn't have CPFP (yet). Future PRs (see #22290) will add RBF and CPFP within packages.
ACKs for top commit:
laanwj:
Code review ACK 046e8ff264be6b888c0f9a9d822e32aa74e19b78
Tree-SHA512: 37dbba37d527712f8efef71ee05c90a8308992615af35f5e0cfeafc60d859cc792737d125aac526e37742fe7683ac8c155ac24af562426213904333c01260c95
|
|
A private member is better than a global.
|
|
The member is unrelated to block storage (BlockManager). It is related
to validation.
Fix the confusion by moving it.
Can be reviewed with
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
|
This is needed for the next commit.
|
|
The helper was moved in commit b026e318c39f59a06e29f1b25c7f577e01b25ccb,
which also mentioned that it could be moved to CChainState. So do that,
as the functionality is not block-storage related.
This also allows to drop one function argument.
|
|
snapshot use
2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24 test: add tests for LoadBlockIndex when using multiple chainstates (James O'Beirne)
0fd599a51a700c028d6f7ed8067d2d9f6e6a04a5 validation: have LoadBlockIndex account for snapshot use (James O'Beirne)
d0c6e61f5dd3b6af818459d9d03b7ba316c5a3f7 validation: don't modify genesis during snapshot load (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Currently, `BlockManager::LoadBlockIndex` adds all blocks that have downloaded transactions to the active chain state's `setBlockIndexCandidates` set, ignoring the background chain state.
This PR changes ChainstateManager::LoadBlockIndex to update `setBlockIndexCandidates` in the background chain, not just the active chain. In the active chain, the same blocks are added as before. In the background chain, only blocks that have actually been validated, not blocks marked assumed-valid are added so the background chain will continue to download and validate assumed-valid blocks.
ACKs for top commit:
MarcoFalke:
Concept ACK 2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24 🤽
Sjors:
utACK 2283b9cd1ee0fbd1e8ebc61673b1fe7596199a24
Tree-SHA512: 7c9a80802df4722d85d12b78d2e7f628ac5f11cb8be66913d5c3230339bd1220c6723805509d4460826a17d1dc04b0ae172eb7d09ac0ea5dc5e41d77975cbd5e
|
|
Ensure that blocks past the snapshot base block (i.e. the end of the
assumed-valid region of the chain) are not included in
setBlockIndexCandidates for the background validation chainstate. These
blocks, while fully validated and lacking the BLOCK_ASSUMED_VALID flag,
*rely* on blocks which are assumed-valid, and so shouldn't be added to
the IBD chainstate.
Co-authored-by: Russ Yanofsky <russ@yanofsky.org>
|
|
between {,non-}unittest codepaths
7f15eff2ddd86034e84a19413e1a42883987f660 style-only: Remove redundant scope in *Chainstate (Carl Dong)
89bec827fdea1cedd560be85201f04e0a78aa48d Collapse the 2 cs_main locks in LoadChainstate (Carl Dong)
3b1584b794499158e0df07bd1bee1b803c568614 Remove all #include // for * comments (Carl Dong)
9a5a5a3d08b2c130ab9147914739ff3583b0dc84 test/setup: Use LoadChainstate (Carl Dong)
c541da0d62eaf5e96eca00d7508899f98bdfc1bc node/chainstate: Add options for in-memory DBs (Carl Dong)
ceb979034184345a662be4e3b327a573fbb31c63 node/caches: Remove intermediate variables (Carl Dong)
ac4bf138b849a8544798f3891d6623803040c265 node/caches: Extract cache calculation logic (Carl Dong)
15f2e33bb3d1ad3bc997f6a84956337f46495091 validation: VerifyDB only needs Consensus::Params (Carl Dong)
4da9c076d1cf12728730bb1f7e8906d4e9bfaba5 node/chainstate: Decouple from ShutdownRequested (Carl Dong)
05441c2dc5f60e2025476d8ec94c9025032d118c node/chainstate: Decouple from GetTime (Carl Dong)
2414ebc18b8bebf79c47e58a4293d0fc6420a811 init: Delay RPC block notif until warmup finished (Carl Dong)
8d466a8504bfb81ce8699d650aa72ec9cc8b0a54 Move -checkblocks LogPrintf to AppInitMain (Carl Dong)
aad8d597890c3707ae96fdb2b9fadc270ca574dd node/chainstate: Reduce coupling of LogPrintf (Carl Dong)
b345979a2b03b671c0984edd7e48e0baec2e2f34 node/chainstate: Decouple from concept of uiInterface (Carl Dong)
ca7c0b934db68acdc410e3a82f1ed898382da2e5 Split off VerifyLoadedChainstate (Carl Dong)
adf4912d77496b9a243476c5944528f95641f14d node/chainstate: Remove do/while loop (Carl Dong)
975235ca0a8f5bcf9df880698b3b0d4bbde9f7fb Move init logistics message for BAD_GENESIS_BLOCK to init.cpp (Carl Dong)
8715658983a0a07c56513acd8ded8dfc59c5c169 Move mempool nullptr Assert out of LoadChainstate (Carl Dong)
9162a4f93ef5aeb57fe11a6e09f5881cf431f5e6 node/chainstate: Decouple from concept of NodeContext (Carl Dong)
c7a5c46e6fd6d6ff46ca7a65fc3f0fff3cbdb24e node/chainstate: Decouple from ArgsManager (Carl Dong)
ae9121f958a4124ea6238cad0c3f2acb8b9eb4bb node/chainstate: Decouple from stringy errors (Carl Dong)
cbac28b72f5b831f6f84b7628f73e85627af3d94 node/chainstate: Decouple from GetTimeMillis (Carl Dong)
cb64af9635a9553e335f2dc0b1cca20c6bbd0933 node: Extract chainstate loading sequence (Carl Dong)
Pull request description:
This PR:
1. Coalesce the Chainstate loading sequence between `AppInitMain` and `*TestingSetup` (which makes it more tested)
2. Makes the Chainstate loading sequence reusable in preparation for future work extracting out our consensus engine.
Code-wise, this PR:
1. Extracts `AppInitMain`'s Chainstate loading sequence into a `::LoadChainstateSequence` function
2. Makes this `::LoadChainstateSequence` function reusable by
1. Decoupling it from various concepts (`ArgsManager`, `uiInterface`, etc)
2. Making it report errors using an `enum` rather than by setting a `bilingual_str`
3. Makes `*TestingSetup` use this new `::LoadChainstateSequence`
Reviewers: Aside from commentary, I've also included `git diff` flags of interest in the commit messages which I hope will aid review!
ACKs for top commit:
ryanofsky:
Code review ACK 7f15eff2ddd86034e84a19413e1a42883987f660. Thanks for updates!
MarcoFalke:
review ACK 7f15eff2ddd86034e84a19413e1a42883987f66 💳
Tree-SHA512: fb9a6cbd1c511a52b477c62a5e68e53a8be5dec2fff0e44a279966afb91efbab44bf1fe7c6b1519f8464ecc25f42dd4bae8e1efbf55ee91fc90fa0b92e3a83e2
|
|
f1f10c0514fe81318c8b064f9dad0e2f9a2cd037 Remove CTxMemPool params from ATMP (lsilva01)
Pull request description:
Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in https://github.com/bitcoin/bitcoin/pull/23437#issuecomment-962536149 .
This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned https://github.com/bitcoin/bitcoin/pull/23173#discussion_r731895386. So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`.
Requires #23437.
ACKs for top commit:
jnewbery:
utACK f1f10c0514fe81318c8b064f9dad0e2f9a2cd037
MarcoFalke:
review ACK f1f10c0514fe81318c8b064f9dad0e2f9a2cd037 🔙
Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349
|
|
Co-authored-by: John Newbery <1063656+jnewbery@users.noreply.github.com>
Co-authored-by: Jon Atack <jon@atack.com>
|
|
Previously we were passing in CChainParams, when VerifyDB only needed
the Consensus::Params subset.
|
|
CoinsTip() access requires cs_main and therefore so should this function.
|
|
It is unclear what the goal of the helper is, as the caller already
knows the spend height before calling the helper.
Also, in case the coins view is corrupted, LookupBlockIndex will return
nullptr. Dereferencing a nullptr is UB.
Fix both issues by removing it. Also, add a sanity check, which aborts
if the coins view is corrupted.
|
|
dependency 2/2
a64078e38563ef3ac5e5ec20c07569441c87eeee Break validation <-> txmempool circular dependency (glozow)
64e4963c635ec3a73a5fa3f32f6ec08e70609f60 [mempool] always assert coin spent (glozow)
bb9078ed51159fa162484f16993313ed6cf980e3 [refactor] put finality and maturity checking into a lambda (glozow)
bedf246f1e2497a3641093c6e8fa11fb34dddac4 [mempool] only update lockpoints for non-removed entries (glozow)
1b3a11e126b258fba975ed7c452221608f2c5472 MOVEONLY: TestLockPointValidity to txmempool (glozow)
Pull request description:
Remove 2 circular dependencies: validation - txmempool and validation - policy/rbf - txmempool
Validation should depend on txmempool (e.g. `CChainstateManager` has a mempool and we often need to know what's in our mempool to validate transactions), but txmempool is a data structure that shouldn't really need to know about chain state.
- Changes `removeForReorg()` to be parameterized by a callable that returns true/false (i.e. whether the transaction should be removed due to being now immature or nonfinal) instead of a `CChainState`. The mempool really shouldn't need to know about coinbase maturity or lockpoints, it just needs to know which entries to remove.
ACKs for top commit:
laanwj:
Code review ACK a64078e38563ef3ac5e5ec20c07569441c87eeee
mjdietzx:
reACK a64078e38563ef3ac5e5ec20c07569441c87eeee
theStack:
re-ACK a64078e38563ef3ac5e5ec20c07569441c87eeee
Tree-SHA512: f75995200569c09dfb8ddc09729da66ddb32167ff1e8a7e72f105ec062d2d6a9a390e6b4a2a115e7ad8ad3525f891ee1503f3cd2bed11773abcaf7c3230b1136
|
|
|
|
|
|
As node operators are free to set their mempool policies however they
please, it's possible for package transaction(s) to already be in the
mempool. We definitely don't want to reject the entire package in that
case (as that could be a censorship vector).
We should still return the successful result to the caller, so add
another result type to MempoolAcceptResult.
|
|
|
|
|
|
Central place for putting package-related info. This document or parts
of it can also be easily ported to other places if deemed appropriate.
|
|
0fdb619aaf1d62598263361a6082d182be1af792 [validation] Always call mempool.check() after processing a new transaction (John Newbery)
2c64270bbe523ef87e7225c351464e7c716f0b3e [refactor] Don't call AcceptToMemoryPool() from outside validation.cpp (John Newbery)
92a3aeecf6a82e9cbc9fda11022b0548efd24d05 [validation] Add CChainState::ProcessTransaction() (John Newbery)
36167faea92c97ddea7403280a5074073c8e5f90 [logging/documentation] Remove reference to AcceptToMemoryPool from error string (John Newbery)
4c24142b1ec121623f81ba644d77341bc1bd88dd [validation] Remove comment about AcceptToMemoryPool() (John Newbery)
5759fd12b8d5937e9187fa33489a95b1d8e6d1e5 [test] Don't set bypass_limits to true in txvalidation_tests.cpp (John Newbery)
497c9e29640858bb3beb20089c2d4f9e133c7e42 [test] Don't set bypass_limits to true in txvalidationcache_tests.cpp (John Newbery)
Pull request description:
Similarly to how #18698 added `ProcessNewBlock()` and `ProcessNewBlockHeaders()` methods to the `ChainstateManager` class, this PR adds a new `ProcessTransaction()` method. Code outside validation no longer calls `AcceptToMemoryPool()` directly, but calls through the higher-level `ProcessTransaction()` method. Advantages:
- The interface is simplified. Calling code no longer needs to know about the active chainstate or mempool object, since `AcceptToMemoryPool()` can only ever be called for the active chainstate, and that chainstate knows which mempool it's using. We can also remove the `bypass_limits` argument, since that can only be used internally in validation.
- responsibility for calling `CTxMemPool::check()` is removed from the callers, and run automatically by `ChainstateManager` every time `ProcessTransaction()` is called.
ACKs for top commit:
lsilva01:
tACK 0fdb619 on Ubuntu 20.04
theStack:
Code-review ACK 0fdb619aaf1d62598263361a6082d182be1af792
ryanofsky:
Code review ACK 0fdb619aaf1d62598263361a6082d182be1af792. Only changes since last review: splitting & joining commits, adding more explanations to commit messages, tweaking MEMPOOL_ERROR string, fixing up argument name comments.
Tree-SHA512: 0b395c2e3ef242f0d41d47174b1646b0a73aeece38f1fe29349837e6fb832f4bf8d57e1a1eaed82a97c635cfd59015a7e07f824e0d7c00b2bee4144e80608172
|
|
|
|
This just calls through to AcceptToMemoryPool() internally, and is currently unused.
Also add a new transaction validation failure reason TX_NO_MEMPOOL to
indicate that there is no mempool.
|
|
This is not only cleaner but also helps make sure we are always using
the virtual size measure that includes the sigop weight heuristic (which
is the vsize the mempool would return).
|
|
performance of check() and remove dependency on validation
082c5bf099c64e3d27abe9b68a71ce500b693e7e [refactor] pass coinsview and height to check() (glozow)
ed6115f1eae0eb4669601106a9aaff078a2f3a74 [mempool] simplify some check() logic (glozow)
9e8d7ad5d9cc4b013826daead9cee09aad539401 [validation/mempool] use Spend/AddCoin instead of UpdateCoins (glozow)
09d18916afb0ecae90700d4befd9d5dc52767970 MOVEONLY: remove single-use helper func CheckInputsAndUpdateCoins (glozow)
e8639ec26aaf4de3fae280963434bf1cf2017b6f [mempool] remove now-unnecessary code (glozow)
54c6f3c1da01090aee9691a2c2bee0984a054ce8 [mempool] speed up check() by using coins cache and iterating in topo order (glozow)
30e240f65e69c6dffcd033afc63895345bd51f53 [bench] Benchmark CTxMemPool::check() (glozow)
cb1407196fba648aa75504e3ab3d46aa0181563a [refactor/bench] make mempool_stress bench reusable and parameterizable (glozow)
Pull request description:
Remove the txmempool <-> validation circular dependency by removing txmempool's dependency on validation. There are two functions in txmempool that need validation right now: `check()` and `removeForReorg()`. This PR removes the dependencies in `check()`.
This PR also improves the performance of `CTxMemPool::check()` by walking through the entries exactly once, in ascending ancestorcount order, which guarantees that we see parents before children.
ACKs for top commit:
jnewbery:
reACK 082c5bf099c64e3d27abe9b68a71ce500b693e7e
GeneFerneau:
tACK [082c5bf](https://github.com/bitcoin/bitcoin/pull/23157/commits/082c5bf099c64e3d27abe9b68a71ce500b693e7e)
rajarshimaitra:
tACK https://github.com/bitcoin/bitcoin/pull/23157/commits/082c5bf099c64e3d27abe9b68a71ce500b693e7e
theStack:
Code-review ACK 082c5bf099c64e3d27abe9b68a71ce500b693e7e
Tree-SHA512: 40ac622af1627b5c3e6abb4f0f035d833265a8c5e8dc88faf5354875dfb5137f137825e54bbd2a2668ed37b145c5d02285f776402629f58596e51853a9a79d29
|
|
|
|
UpdateCoins is an unnecessary dependency on validation. All we need to
do is add and remove coins to check inputs. We don't need the extra
logic for checking coinbases and handling TxUndos.
Also remove the wrapper function in validation.h which constructs a
throwaway TxUndo object before calling UpdateCoins because it is now
unused.
|
|
Move amount.h to consensus/amount.h.
Renames, adds missing and removes uneeded includes.
|
|
assumeutxo support
673a5bd3377929a0a6a62eda8b560e47bc2cca0c test: validation: add unittest for UpdateTip behavior (James O'Beirne)
2705570109a2a90ecfd3f4180944498626fc2707 test: refactor: separate CreateBlock in TestChain100Setup (James O'Beirne)
298bf5d563cc740c6ae71750d86942e0278b22d6 test: refactor: declare NoMalleation const auto (James O'Beirne)
071200993f3a9412821ce5387851d659baf85327 move-only: unittest: add test/util/chainstate.h (James O'Beirne)
8f5710fd0ac5173b577e5d00708485170b321bcc validation: fix CheckBlockIndex for multiple chainstates (James O'Beirne)
5a807736dacfc3e6fa57231219336acf08be38fb validation: insert assumed-valid block index entries into candidates (James O'Beirne)
01a9b8fe719efab2c268dc738bc93cfbdf92edb7 validation: set BLOCK_ASSUMED_VALID during snapshot load (James O'Beirne)
42b2520db93fd9feb3df4101654391fa7d3e2140 chain: add BLOCK_ASSUMED_VALID for use with assumeutxo (James O'Beirne)
b217020df78bc981d221fe04497c831120ef969f validation: change UpdateTip for multiple chainstates (James O'Beirne)
665072a36df2e4c88705fedd4ac7c955d7f6a488 doc: add comment for g_best_block (James O'Beirne)
ac4051d891e2d5c8ac130da16b85b9d880b44720 refactor: remove unused assumeutxo methods (James O'Beirne)
9f6bb539359b98d5b39482ab8a28a68608f0c645 validation: add chainman ref to CChainState (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Modify UpdateTip and CheckBlockIndex for use with multiple chainstates. Includes a new unittest verifying `g_best_block` behavior (previously untested at the unit level) and various changes necessary for running and testing `ProcessNewBlock()`-like behavior on the background validation chainstate.
This changeset introduces a new block index `nStatus` flag called `BLOCK_ASSUMED_VALID`, and it is applied to block index entries that are beneath the UTXO snapshot base block upon snapshot load. Once each block is validated (during async background validation), the flag is removed. This allows us to avoid (ab)using `BLOCK_VALID_*` flags for snapshot chain block entries, and preserves the original meaning of those flags.
Note: this PR previously incorporated changes to `LoadBlockIndex()` and `RewindBlockIndex()` as noted in Russ' comments below, but once I generated the changes necessary to test the UpdateTip change, I decided to split this changes out into another PR due to the size of this one.
ACKs for top commit:
achow101:
ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
jonatack:
Code-review re-ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c reviewed diff, rebased to master/debug build/ran unit+functional tests
naumenkogs:
ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
fjahr:
Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
ariard:
utACK 673a5bd3
ryanofsky:
Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c. Just linker fix and split commit changes mentioned https://github.com/bitcoin/bitcoin/pull/21526#issuecomment-921064563 since last review
benthecarman:
ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
Tree-SHA512: 0a6dc23d041b27ed9fd0ee1f3e5971b92fb1d2df2fc9b655d5dc48594235321ab1798d06de2ec55482ac3966a9ed56de8d56e9e29cae75bbe8690bafc2dda383
|
|
|
|
fa20f815a9cb438c5ab61e97a453612ddd8b21b5 Remove txindex migration code (MarcoFalke)
fae878603345854527c211ebb7d1967f12c8bb9d doc: Fix validation typo (MarcoFalke)
fab89006d656261770503e54fdd01ac9167bdd49 Add missing includes and forward declarations, remove unused ones (MarcoFalke)
Pull request description:
No supported version of Bitcoin Core used the legacy txindex, so all relevant nodes can be assumed to have upgraded. Thus, there is no need to keep this code any longer.
As a temporary courtesy, provide a one-time warning on how to free the disk space used by the legacy txindex.
Fixes #22615
ACKs for top commit:
laanwj:
Code review ACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5
hebasto:
ACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5, tested on Linux Mint 20.2 (x86_64).
Zero-1729:
crACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5
theStack:
Approach ACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5
Tree-SHA512: 68aa32d064d1e3932e6e382816a4b5de417bd7e82861fea1ee50660e8c397f4efeb88ae4ed54a8ad1952c3563eb0b8449d7ccf883c353cc4d4dc7e15c53d78e8
|
|
|
|
|
|
|
|
The RecursiveMutex cs_nBlockSequenceId is only used at one place in
CChainState::ReceivedBlockTransactions() to atomically read-and-increment the
nBlockSequenceId member. At this point, the cs_main lock is set, hence we can
use a plain int for the member and mark it as guarded by cs_main.
|
|
|
|
|
|
node/transaction.cpp
f685a13bef0418663015ea6d8f448f075510c0ec doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f0882a1a2bb20474648419979af6e383d refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef0418663015ea6d8f448f075510c0ec
rajarshimaitra:
tACK https://github.com/bitcoin/bitcoin/pull/22528/commits/f685a13bef0418663015ea6d8f448f075510c0ec
mjdietzx:
crACK f685a13bef0418663015ea6d8f448f075510c0ec
LarryRuane:
Code review, test ACK f685a13bef0418663015ea6d8f448f075510c0ec
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
|
|
GetTransaction
78f4c8b98eada337346ffb206339c3ebae4ff43b prefer to use txindex if available for GetTransaction (Jameson Lopp)
Pull request description:
Fixes #22382
Motivation: prevent excessive disk reads if txindex is enabled.
Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?
ACKs for top commit:
jonatack:
ACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
theStack:
Code review ACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
LarryRuane:
tACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
luke-jr:
utACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
jnewbery:
utACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
rajarshimaitra:
Code review ACK https://github.com/bitcoin/bitcoin/commit/78f4c8b98eada337346ffb206339c3ebae4ff43b
lsilva01:
Code Review ACK and Tested ACK https://github.com/bitcoin/bitcoin/pull/22383/commits/78f4c8b98eada337346ffb206339c3ebae4ff43b on Ubuntu 20.04
Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0
|
|
can be reviewed with --color-moved
|
|
After feedback from Russ, I realized that there are some extraneous assumeutxo methods
that are not necessary and probably just overly confusing. These include
- `Validated*()`
- `IsBackgroundIBD()`
and they can be removed.
|
|
Add an upwards reference to chainstate instances to the owning
ChainstateManager. This is necessary because there are a number
of `this_chainstate == chainman.ActiveChainstate()` checks that
will happen (as a result of assumeutxo) in functions that otherwise
don't have an easily-accessible reference to the chainstate's
ChainManager.
|
|
|
|
|
|
Makes sense and saves on arguments.
Co-authored-by: John Newbery <john@johnnewbery.com>
|
|
Unnecessary argument since we can make use of this->m_mempool
Co-authored-by: John Newbery <john@johnnewbery.com>
|
|
Allows fewer arguments and simplification of call sites.
Co-authored-by: John Newbery <john@johnnewbery.com>
|
|
Since we now have multiple chainstate objects, only one of them is active at any given
time. An active chainstate has a mempool, but there's no point to others having one.
This change will simplify proposed assumeutxo semantics. See the discussion here:
https://github.com/bitcoin/bitcoin/pull/15606#pullrequestreview-692965905
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
|
