Most of this commit is pure-move.
After this change:
- kernel/coinstats.h
-> Contains declarations for:
- enum class CoinStatsHashType
- struct CCoinsStats
- GetBogoSize(...)
- TxOutSer(...)
- ComputeUTXOStats(...)
- node/coinstats.h
-> Just GetUTXOStats, which will be removed as we change callers to
directly use the hashing/indexing codepaths in future commits.
As mentioned in a previous commit, the hashing codepath can now be moved
to a separate file. This decouples callers that only rely on the hashing
codepath from the indexing one.
This is key for libbitcoinkernel, which needs to have the CoinsStats
hashing codepath for AssumeUTXO, but does not wish to be coupled with
indexes.
Note that only the .cpp file is split in this commit, the header files
will be split in a subsequent commit and the #includes to
node/coinstats.h will be adjusted to only #include the necessary
headers.
The indexing codepath logic in node/coinstats.cpp is simple enough to be
moved into CoinStatsIndex::LookUpStats, avoiding an additional layer of
function calls. Callers are modified accordingly.
Also, add 2 missed BOOST_CHECKs to the coinstatsindex_initial_sync unit
test.
Split out ComputeUTXOStats and LookupUTXOStatsWithIndex from
GetUTXOStats, since the hashing and indexing codepaths are quite
disparate in practice.
Also allow add a constructor to CCoinsStats for it to be constructed
from a a block height and hash. This is used in both codepaths.
Also add a note in GetUTXOStats documenting a behaviour quirk that
predates this patchset.
[META] This allows the hashing codepath to be moved to a separate file
in a future commit, decoupling callers that only rely on the
hashing codepath from the indexing one. This is key for
libbitcoinkernel, which needs to have the hashing codepath for
AssumeUTXO, but does not wish to be coupled with indexes.
In previous commits in this patchset, we removed all in-param members of
CCoinsStats. Now that that's done, we can modify GetUTXOStats to return
an optional CCoinsStats instead of a status bool. Callers are modified
accordingly.
In rpc/blockchain.cpp, we discover that GetUTXOStats' status bool when
getting UTXO stats for pprev was not checked for error. We fix this as
well.
Currently, CCoinsStats is a struct with both in-params and out-params
where the hash_type and index_requested members are the only in-params.
This change removes CCoinsStats' hash_type in-param member and adds it
to the relevant functions instead.
[META] In subsequent commits, all of CCoinsStats' members which serve as
in-params will be moved out so as to make CCoinsStats a pure
out-param struct.
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
fa1b76aeb0 Do not call global Params() when chainman is in scope (MacroFake)
fa30234be8 Do not pass CChainParams& to PeerManager::make (MacroFake)
fafe5c0ca2 Do not pass CChainParams& to BlockAssembler constructor (MacroFake)
faf012b438 Do not pass Consensus::Params& to Chainstate helpers (MacroFake)
fa4ee53dca Do not pass time getter to Chainstate helpers (MacroFake)
Pull request description:
It seems confusing to pass chain params, consensus params, or a time function around when it is not needed.
Fix this by:
* Inlining the passed time getter function. I don't see a use case why this should be mockable.
* Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible.
ACKs for top commit:
promag:
Code review ACK fa1b76aeb0.
vincenzopalazzo:
ACK fa1b76aeb0
Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
fa9af21878 scripted-diff: Use getInt<T> over get_int/get_int64 (MacroFake)
Pull request description:
Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).
ACKs for top commit:
fanquake:
ACK fa9af21878
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
e4303c337c [unit test] prioritisation in mining (glozow)
7a8d60676b [miner] bug fix: update for parent inclusion using modified fee (glozow)
0f9a44461c MOVEONLY: group miner tests into MinerTestingSetup functions (glozow)
Pull request description:
Came up while reviewing #24364, where some of us incorrectly assumed that we use the same fee deduction in `CTxMemPoolModifiedEntry::nModFeesWithAncestors` when first constructing an entry and in `update_for_parent_inclusion`.
Actually, the behavior is this: when a mempool entry's ancestor is included in the block template, we create a `CTxMemPoolModifiedEntry` for it, subtracting the ancestor's modified fees from `nModFeesWithAncestors`. If another ancestor is included, we update it again, but use the ancestor's _base_ fees instead.
I can't explain why we use `GetFee` in one place and `GetModifiedFee` in the other, but I'm quite certain we should be using the same one for both.
And should it be base or modified fees? Modified, otherwise the child inherits the prioritisation of the parent, but only until the parent gets mined. If we want prioritisation to cascade down to current in-mempool descendants, we should probably document that in the `prioritsetransaction` helpstring and implement it in `CTxMemPool::mapDeltas`, not as a quirk in the mining code?
Wrote a test in which a mempool entry has 2 ancestors, both prioritised, and both included in a block template individually. This test should fail without the s/GetFee/GetModifiedFee commit.
ACKs for top commit:
ccdle12:
tested ACK e4303c3
MarcoFalke:
ACK e4303c337c🚗
Tree-SHA512: 4cd94106fbc9353e9f9b6d5af268ecda5aec7539245298c940ca220606dd0737264505bfaae1f83d94765cc2d9e1a6e913a765048fe6c19292482241761a6762
In previous commits in this patchset, we've made sure that every
Unload/UnloadBlockIndex member function resets its own members, and does
not reach out to globals.
This means that their corresponding classes' default destructors can now
replace them, and do an even more thorough job without the need to be
updated for every new member variable.
Therefore, we can remove them, and also remove UnloadBlockIndex since
that's not used anymore.
Unfortunately, chainstatemanager_loadblockindex relies on
CChainState::UnloadBlockIndex, so that needs to stay for now.
Fixes https://github.com/bitcoin/bitcoin/issues/22964
Previously, we used UnloadBlockIndex() in order to reset node.mempool
and node.chainman. However, that has proven to be fragile (see
https://github.com/bitcoin/bitcoin/issues/22964), and requires
UnloadBlockIndex and its callees to be updated manually for each member
that's introduced to the mempool and chainman classes.
In this commit, we stop using the UnloadBlockIndex function and we
simply reconstruct node.mempool and node.chainman.
Since PeerManager needs a valid reference to both node.mempool and
node.chainman, we also move PeerManager's construction via `::make` to
after the chainstate activation sequence is complete.
There are no more callers to UnloadBlockIndex after this commit, so it
and its sole callees can be pruned.
fa1970f075 Make BlockManager::LoadBlockIndex private (MarcoFalke)
Pull request description:
* After commit fa27f03b49 `BlockManager::LoadBlockIndex` is only called by `BlockManager::LoadBlockIndexDB`. Thus, it can be made `private`.
* After commit c600ee3816 `m_best_invalid` is no longer accessed by `BlockManager::LoadBlockIndex`. Thus, the unused `friend` can be removed.
ACKs for top commit:
mruddy:
ACK fa1970f075 I verified by double checking references, then applying the patch, and running `make check`. LGTM.
Tree-SHA512: 9b36b4c59bf7ad01171764ce61b1be9750fc92d105c4fe939b1a6a70027ab6300d5d2a2fc3e82f981e22c3987f2ca84e092d2e1f8463fa320af9f05048580c0a
This change also introduces an aditional buffer of 10 blocks (PRUNE_LOCK_BUFFER) that will not be pruned before the best block.
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
36f814c0e8 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e63 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3 [net] Move asmap into NetGroupManager (John Newbery)
17c24d4580 [init] Add netgroupman to node.context (John Newbery)
9b3836710b [build] Add netgroup.cpp|h (John Newbery)
Pull request description:
The asmap data is currently owned by addrman, but is used by both addrman and connman. #22791 made the data const and private (so that it can't be updated by other components), but it is still passed out of addrman as a reference to const, and used by `CNetAddress` to calculate the group and AS of the net address.
This RFC PR proposes to move all asmap data and logic into a new `NetGroupManager` component. This is initialized at startup, and the client components addrman and connman simply call `NetGroupManager::GetGroup(const CAddress&)` and `NetGroupManager::GetMappedAS(const CAddress&)` to get the net group and AS of an address.
ACKs for top commit:
mzumsande:
Code Review ACK 36f814c0e8
jnewbery:
CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed 36f814c0e8, so I've reverted to that.
dergoegge:
Code review ACK 36f814c0e8
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
-----
Code Reviewer Notes
Call graph of relevant functions:
UnloadBlockIndex() <-- Moved from
calls ChainstateManager::Unload()
which calls BlockManager::Unload() <-- Moved to
So calling UnloadBlockIndex() would still run this moved code. The code
will also now run when ~BlockManager gets called, which makes sense.
This is constructed before addrman and connman, and destructed afterwards.
netgroupman does not currently do anything, but will have functionality added in future commits.
Now BlockManager::LoadBlockIndex() will ACTUALLY only load BlockMan
members.
[META] In a later commit, pindexBestHeader will be moved to ChainMan as
a member
-----
Code Reviewer Notes
Call graph of relevant functions:
ChainstateManager::LoadBlockIndex() <-- Moved to
calls BlockManager::LoadBlockIndexDB()
which calls BlockManager::LoadBlockIndex() <-- Moved from
There is only one call to each of inner functions, meaning that no
behavior is changing.
fabdf9f870 Remove gui-only syscalls (MarcoFalke)
fa0c2aa826 init: Disable syscall sandbox in the bitcoin-qt process (MarcoFalke)
Pull request description:
It is basically impossible (and a bit out of scope) for us to maintain a sandbox for the qt library. I am not sure if it is possible to only sandbox a few threads in a process, but I doubt this will add no practical benefit anyway, so I am disabling the sandbox for the whole bitcoin-qt process.
See also https://github.com/bitcoin/bitcoin/pull/24690#issuecomment-1084372400
ACKs for top commit:
laanwj:
Code review ACK fabdf9f870
Tree-SHA512: 944ded03ee25f7dfd0bfeea9c3f97f575f2d470aa03b387b07f3e3bec5cb886e4aaa17e4a9fb359d3e670e6da69adc9111673d13e6561ec55b3161bb67dfe760
cccc4e879a Remove nHeightEnd and nHeight in generateBlocks helper (MarcoFalke)
fa38b1c8bd Remove buggy and confusing IncrementExtraNonce (MarcoFalke)
Pull request description:
IncrementExtraNonce has many issues:
* It is test-only code, but part of bitcoind
* It is using the block height of the tip, as opposed to the block's previous block as reference for the new height. See https://github.com/bitcoin/bitcoin/issues/24730#issuecomment-1085586193
* It has no use case in regtest testing. With a low difficulty the extra nonce won't be incremented. With a high difficulty the test-only functions are clumsy to handle anyway. For example, the generate* RPCs will return an empty array once they reached `maxtries`, as opposed to an error. Also the calls can't be aborted early unless the node shuts down completely. So I think it is fine to just remove the extra nonce functionality and leave it to the outside to implement, if needed. For example, a wrapper script can call the `generate*` RPCs once every second, to use the timestamp as extra nonce.
ACKs for top commit:
ajtowns:
ACK cccc4e879a
Tree-SHA512: d8a3989ad280ebd4b1b574159b3a396b8a42134347e6be3c88445162d86624d221c416456f45ae75aea62ed8c8a1a9bb3a2532924abca2ef7a879cb8e6b15654
fa9112aac0 Remove utxo db upgrade code (MarcoFalke)
Pull request description:
It is not possible to upgrade Bitcoin Core pre-segwit (pre-0.13.1) to a recent version without a full IBD from scratch after commit 19a56d1519 (released in version 22.0).
Any Bitcoin Core version with the new database format after commit 1088b02f0c (released in version 0.15), can upgrade to any version that is supported as of today.
This leaves the versions 0.13.1-0.14.x. Even though those versions are unsupported, some users with an existing datadir may want to upgrade to a recent version. However, it seems reasonable to simply ask them to `-reindex` to run a full IBD from scratch. This allows us to remove the utxo db upgrade code.
ACKs for top commit:
Sjors:
re-ACK fa9112aac0
laanwj:
Code review ACK fa9112aac0
Tree-SHA512: 4243bb35df9ac4892f9fad30fe486d338745952bcff4160bcb0937c772d57b13b800647da14695e21e3655e85ee0d95fa3dc7789ee309d59ad84f422297fecb8
f865cf8ded Add and use BlockManager::GetAllBlockIndices (Carl Dong)
28ba0313ea Add and use CBlockIndexHeightOnlyComparator (Carl Dong)
12eb05df63 move-only: Move CBlockIndexWorkComparator to blockstorage (Carl Dong)
c600ee3816 Only load BlockMan in BlockMan member functions (Carl Dong)
42e56d9b18 style-only: No need for std::pair for vSortedByHeight (Carl Dong)
3bbb6fea05 style-only: Various blockstorage.cpp cleanups (Carl Dong)
5be9ee3c54 refactor: more const annotations for uses of CBlockIndex* (Anthony Towns)
Pull request description:
The only important commit is "Only load BlockMan in BlockMan member functions", everything else is all just small style changes.
Here's the commit message, reproduced:
```
This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.
This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
```
ACKs for top commit:
ajtowns:
ACK f865cf8ded ; code review only
MarcoFalke:
review ACK f865cf8ded 🗂
Tree-SHA512: 7b204d782834e06fd7329d022e2ae860181b4e8105c33bfb928539a4ec24161dc7438a9c4d4ee279dcad77de310c160b997bb8aa18923243d0fd55ccf4ad7c3a
This commit effectively splits the "load block index itself" logic from
"derive Chainstate variables from loaded block index" logic.
This means that BlockManager::LoadBlockIndex{,DB} will only load what's
relevant to the BlockManager.
I strongly recommend reviewing with the following git-diff flags:
--color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change
