All uses of CBlockHeaderAndShortTxIDs in the product code are
constructed with fUseWTXID=true, so remove the parameter.
There is one use of the CBlockHeaderAndShortTxIDs constructor with
fUseWTXID=false in the unit tests. This is used to construct a
CBlockHeaderAndShortTxIDs for a block with only the coinbase
transaction, so setting fUseWTXID to true or false makes no difference.
Suggested in https://github.com/bitcoin/bitcoin/pull/20799#pullrequestreview-963480278
bb5c24b120a3ac7df367a1c5d9b075ca564efb5f validation: move g_versionbitscache into ChainstateManager (Anthony Towns)
eca22c726ac48b4216bb68cc0f0bbd655c43ac12 test/versionbits: make versionbitscache a parameter (Anthony Towns)
d603f1d8a7cdc0a158ed80ade8a843b61b6ad08e deploymentstatus: make versionbitscache a parameter (Anthony Towns)
78adef17536edef833a0bfca06b61ce28120e486 refactor: use chainman instead of chainParams for DeploymentActive* (Anthony Towns)
deffe0df6c36225bada18603b5a840139f030f2c deploymentstatus: allow chainman in place of consensusParams (Anthony Towns)
eaa2e3f25cefbd1b9a1214102f88dbfa8109d244 validation: move UpdateUncommittedBlockStructures and GenerateCoinbaseCommitment into ChainstateManager (Anthony Towns)
5c67e84d37d452e9186a6357e5405fabeff241c7 validation: replace ::Params() calls with chainstate/chainman member (Anthony Towns)
38860f93b680f152fc6fc3d9ae574a4c0659e775 validation: remove redundant CChainParams params from ChainstateManager methods (Anthony Towns)
69675ea4e73dcf5e9dd0f94802bd3463e4262081 validation: add CChainParams to ChainstateManager (Anthony Towns)
Pull request description:
Gives `ChainstateManager` a reference to the `CChainParams` its working on, and simplifies some of the functions that would otherwise take that as a parameter. Removes the `g_versionbitscache` global by moving it into `ChainstateManager`.
ACKs for top commit:
dongcarl:
reACK bb5c24b120a3ac7df367a1c5d9b075ca564efb5f
MarcoFalke:
review ACK bb5c24b120a3ac7df367a1c5d9b075ca564efb5f 📙
Tree-SHA512: 3fa74905e5df561e3e74bb0b8fce6085c5311e6633e7d74c0fb0c82a907f5bbb1fd4ebc5d11d4f0b1c019bb51eabb9f6e4bcc4652a696d36a5878c807b85f121
fab9e8a29c2cdeab6cf1ae7c1fc0e0a3af783b17 Remove unused GetTimeSeconds (MacroFake)
Pull request description:
Seems confusing to have this helper when it is possible to get the system time in a type-safe way by simply calling `std::chrono::system_clock::now` (C++11).
This patch replaces `GetTimeSeconds` and removes it:
* in `bitcoin-cli.cpp` by `system_clock`
* in `test/fuzz/fuzz.cpp` by `steady_clock`
ACKs for top commit:
laanwj:
Code review ACK fab9e8a29c2cdeab6cf1ae7c1fc0e0a3af783b17
naumenkogs:
ACK fab9e8a29c2cdeab6cf1ae7c1fc0e0a3af783b17
Tree-SHA512: 517e300b0baf271cfbeebd4a0838871acbea9360f9dd23572a751335c20c1ba261b1b5ee0aec8a36abd20c94fab83ce94f46042745279aca1f0ca2f885a03b6e
ab1ea29ba1b8379a21fabd3dc859552c470a6421 refactor: make GetRand a template, remove GetRandInt (pasta)
Pull request description:
makes GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from GetRand(std::numeric_limits<uint64_t>::max() -> GetRand<uint64_t>()
ACKs for top commit:
laanwj:
Code review ACK ab1ea29ba1b8379a21fabd3dc859552c470a6421
Tree-SHA512: db5082a0e21783389f1be898ae73e097b31ab48cab1a2c0e29348a4adeb545d4098193aa72a547c6baa6e8205699aafec38d6a27b3d65522fb3246f91b4daae9
This fixes a blk file size calculation made during reindex that results in increased blk file malformity.
The fix is to avoid double counting the size of the serialization header during reindex.
This adds a unit test to reproduce the bug before the fix and to ensure that it does not recur.
These changes include a log message change also so as to not be as alarming. This is a common and recoverable
data corruption. These messages can now be filtered by the debug log reindex category.
308dd2e93e92f4cac4e7d75478316af9bb2b77b8 Sanity assert GetAncestor() != nullptr where appropriate (Adam Jonas)
Pull request description:
Re-opening #17232. I have rebased the PR and addressed jonatack's nit suggestions.
Add sanity asserts for return value of `CBlockIndex::GetAncestor()` where appropriate.
In validation.cpp `CheckSequenceLocks`, check the return value of `tip->GetAncestor(maxInputHeight)` stored into `lp->maxInputBlock`. If it ever returns `nullptr` because the ancestor isn't found, it's going to be a bad bug to keep going, since a `LockPoints` object with the `maxInputBlock` member set to `nullptr` signifies no relative lock time.
In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior.
Co-Authored-By: Adam Jonas <jonas@chaincode.com>
Co-Authored-By: danra <danra@users.noreply.github.com>
ACKs for top commit:
jonatack:
ACK 308dd2e93e92f4cac4e7d75478316af9bb2b77b8
Tree-SHA512: 5bfdaab1499607ae2c3cd3e2e9e8c37850bfd0e327e680f4e36c81f9c6d98a543af78ecfac1ab0e06325d264412615a04d52005875780c7db2a4d81bd2d2259a
fac6cfc50f65c610f2df9af3ec2efff5eade6661 refactor: Change * to & in MutableTransactionSignatureCreator (MarcoFalke)
Pull request description:
The `MutableTransactionSignatureCreator` constructor takes in a pointer to a mutable transaction. This is problematic for several reasons:
* It would be undefined behaviour to pass in a nullptr because for signature creation, the memory of the mutable transaction is accessed
* No caller currently passes in a nullptr, so passing a reference as a pointer is confusing
Fix all issues by replacing `*` with `&` in `MutableTransactionSignatureCreator`
ACKs for top commit:
theStack:
Code-review ACK fac6cfc50f65c610f2df9af3ec2efff5eade6661
jonatack:
ACK fac6cfc50f65c610f2df9af3ec2efff5eade6661
Tree-SHA512: d84296b030bd4fa2709e5adbfe43a5f8377d218957d844af69a819893252af671df7f00004f5ba601a0bd70f3c1c2e58c4f00e75684da663f28432bb5c89fb86
e4303c337c8423f21c2c72ee1bcca3aaf46fa1cb [unit test] prioritisation in mining (glozow)
7a8d60676bc0eec289687b2dfd5d2b00b83c0eaa [miner] bug fix: update for parent inclusion using modified fee (glozow)
0f9a44461c294cf21a335e8a8c13e498baac110f 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 e4303c337c8423f21c2c72ee1bcca3aaf46fa1cb 🚗
Tree-SHA512: 4cd94106fbc9353e9f9b6d5af268ecda5aec7539245298c940ca220606dd0737264505bfaae1f83d94765cc2d9e1a6e913a765048fe6c19292482241761a6762
Add sanity asserts for return value of `CBlockIndex::GetAncestor()` where appropriate.
In validation.cpp `CheckSequenceLocks`, check the return value of `tip->GetAncestor(maxInputHeight)` stored into `lp->maxInputBlock`. If it ever returns `nullptr` because the ancestor isn't found, it's going to be a bad bug to keep going, since a `LockPoints` object with the `maxInputBlock` member set to `nullptr` signifies no relative lock time.
In the other places, the added asserts would prevent accidental dereferencing of a null pointer which is undefined behavior.
Co-Authored-By: Aurèle Oulès <aurele@oules.com>
Co-Authored-By: danra <danra@users.noreply.github.com>
We add an RPC to fetch the mempool transactions spending given outpoints.
Without this RPC, application developers would need to first call
`getrawmempool` which returns a long list of `txid`, then fetch each of
these txs individually to check whether they spend the given outpoint(s).
This RPC can later be enriched to also find confirmed transactions instead
of being restricted to mempool transactions.
f849e63bad963b8717d4bc45efdad9b08567a36e fuzz: SplitString with multiple separators (Martin Leitner-Ankerl)
d1a9850102fe572b8a1e00b80c757dd82bf39f9d http: replace boost::split with SplitString (Martin Leitner-Ankerl)
0d7efcdf75607e19fac77bcd146773a03af14492 core_read: Replace boost::split with SplitString (Martin Leitner-Ankerl)
b7ab9db545492927b774912e53aeb834a590621f Extend Split to work with multiple separators (Martin Leitner-Ankerl)
Pull request description:
As a followup of #22953, this removes the remaining occurrences of `boost::split` and replaces them with our own `SplitString`. To be able to do so, this extends the function `spanparsing::Split` to work with multiple separators. Finally this removes 3 more files from `lint-includes.py`.
ACKs for top commit:
theStack:
Code-review ACK f849e63bad963b8717d4bc45efdad9b08567a36e
Tree-SHA512: f37d4dbe11cab2046e646045b0f018a75f978d521443a2c5001512737a1370e22b09247d5db0e5c9e4153229a4e2d66731903c1bba3713711c4cae8cedcc775d
fa4652ce5995ace831b6a4d3125bfcac9563ff6f Pass lifetimebound reference to SingleThreadedSchedulerClient (MacroFake)
Pull request description:
Currently a pointer is passed, which is confusing and requires run-time asserts to avoid nullptr dereference.
All call sites can pass a reference, so do that. Also mark it LIFETIMEBOUND to avoid call sites passing a temporary. Also, unrelated cleanup in touched lines.
ACKs for top commit:
pk-b2:
ACK fa4652ce59
jonatack:
Code review ACK fa4652ce5995ace831b6a4d3125bfcac9563ff6f rebased to master, debug build, unit tests
vincenzopalazzo:
ACK fa4652ce59
Tree-SHA512: cd7ec77347e195d659b8892d34c1e9644d4f88552a4d5fa310dc1756eb27050a99d3098b0b0d27f8474230f82c178fd9e22e7018d8248d5e47a7f4caad395e25
f64aa9c411ad78259756a28756ec1eb8069b5ab4 Disallow more unsafe string->path conversions allowed by path append operators (Ryan Ofsky)
Pull request description:
Add more `fs::path` `operator/` and `operator+` overloads to prevent unsafe string->path conversions on Windows that would cause strings to be decoded according to the current Windows locale & code page instead of the correct string encoding.
Update application code to deal with loss of implicit string->path conversions by calling `fs::u8path` or `fs::PathFromString` explicitly, or by just changing variable types from `std::string` to `fs::path` to avoid conversions altogether, or make them happen earlier.
In all cases, there's no change in behavior either (1) because strings only contained ASCII characters and would be decoded the same regardless of what encoding was used, or (2) because of the 1:1 mapping between paths and strings using the `PathToString` and `PathFromString` functions.
Motivation for this PR was just that I was experimenting with #24469 and noticed that operations like `fs::path / std::string` were allowed, and I thought it would be better not to allow them.
ACKs for top commit:
hebasto:
ACK f64aa9c411ad78259756a28756ec1eb8069b5ab4
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
e5485e8e4be7f2ee0671f58c3dcce35c68ba0ee0 test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack)
abc1ee509025d92db5311c3f5df3b61c09cad24f validation: make CScriptCheck and prevector swap member functions noexcept (Jon Atack)
Pull request description:
along with those seen elsewhere in the codebase (prevector and checkqueue units/fuzz/bench).
A swap must not fail; when a class has a swap member function, it should be declared noexcept.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c84-a-swap-function-must-not-fail
ACKs for top commit:
pk-b2:
ACK e5485e8e4b
w0xlt:
ACK e5485e8e4b
Tree-SHA512: c82359d5e13f9262ce45efdae9baf71e41ed26568e0aff620e2bfb0ab37a62b6d56ae9340a28a0332c902cc1fa87da3fb72d6f6d6f53a8b7e695a5011f71f7f1
778343a379026ef233dffea67f5226565f6d5720 scripted-diff: Rename PeerManagerImpl members (dergoegge)
91c339243e11ec42eeeaca8fe015fc1c3e6338e1 [net processing] Move nHighestFastAnnounce into PeerManagerImpl (dergoegge)
10b83e2aa3393ef2c942fde7ac86e8cf3ea224c1 [net processing] Move block cache state into PeerManagerImpl (dergoegge)
a4c55a93ef9277e1043c286120e2417652ee8bbb [net processing] Inline and simplify UpdatePreferredDownload (dergoegge)
490c08f96a34ed436c3d2cf7b9a3ed72694b6147 [net processing] Move nPreferredDownload into PeerManagerImpl (dergoegge)
a292df283a596efe7e1d40c33a6d614d70ed564d [net processing] Move mapNodeState into PeerManagerImpl (dergoegge)
37ecaf3e7a028486a0a1c9b717e8eb4214215805 [net processing] Move CNodeState declaration above PeerManagerImpl (dergoegge)
Pull request description:
This PR moves the remaining net processing globals into `PeerManagerImpl`. This will make testing the peer manager in isolation easier and also acts as a code clean up.
ACKs for top commit:
jnewbery:
Code review ACK 778343a379026ef233dffea67f5226565f6d5720
MarcoFalke:
ACK 778343a379026ef233dffea67f5226565f6d5720 🗒
Tree-SHA512: 4f22105d1de37b94c3ef349f38784a30cf8d450d394a6a7849e5bd78940a71e3edbffa3d25e8efb35d7f698fd255f199de7bd4c33e23af5621a6e4e67ed43cb5
fad35e9afdd0bb6e8d6bf7f34a31de11aeb2d39b test: Remove boost::split from rpc_tests.cpp (MacroFake)
Pull request description:
No need for boost, as there are no tabs.
Can be tested with:
```diff
diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
index 50b5078110..ad6a888ad0 100644
--- a/src/test/rpc_tests.cpp
+++ b/src/test/rpc_tests.cpp
@@ -29,6 +29,7 @@ public:
UniValue RPCTestingSetup::CallRPC(std::string args)
{
+Assert(args.find('\t')==std::string::npos);
std::vector<std::string> vArgs;
boost::split(vArgs, args, boost::is_any_of(" \t"));
std::string strMethod = vArgs[0];
ACKs for top commit:
fanquake:
utACK fad35e9afdd0bb6e8d6bf7f34a31de11aeb2d39b
Tree-SHA512: 3df789a222b407d61ad549adc4bbded00705d7c3db07472c31ce0e82216fe3ae27724b7f0ee3e85084bdf405cc28185e85487c9a7001620d6654fda77bab8eb3
Parse also key hashes using the Key type. Make this target the first of
the 4 Miniscript fuzz targets in a single `miniscript` file.
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
7ab07e033237d6ea179a6a2c76575ed6bd01a670 validation: Prune UnloadBlockIndex and callees (Carl Dong)
7d99d725cdb5428ed25dc07c2d7fddf420da7786 validation: No mempool clearing in UnloadBlockIndex (Carl Dong)
572d8319272ae84a81d6bfd53dd9685585697f65 Clear {versionbits,warning}cache in ~Chainstatemanager (Carl Dong)
eca4ca4d60599c9dbdd4e03a73beb33e9b44655a style-only: Use std::clamp for check_ratio, rename (Carl Dong)
fe96a2e4bd87768df8001eb4117926a0977d876e style-only: Use for instead of when loading Chainstate (Carl Dong)
5921b863e39e5c3997895ffee1c87159e37a5d6f init: Reset mempool and chainman via reconstruction (Carl Dong)
6e747e80e7094df0b5bee1eed57e57e82015d0ee validation: default initialize and guard chainman members (Anthony Towns)
98f4bdae81804de17f125bd7c2cd8a48e850a6d2 refactor: Convert warningcache to std::array (Carl Dong)
Pull request description:
Fixes#22964
-----
This is a small part of the work to accomplish what I described in 972c5166ee685447a6d4bf5e501b07a0871fba85:
```
Over time, we should probably move these mutable global state variables
into ChainstateManager or CChainState so it's easier to reason about
their lifecycles.
```
`::UnloadBlockIndex` manually resets a subset of our mutable globals in addition to unloading the `ChainstateManager` and clearing the mempool. The need for this manual reset (AFAICT) arises out of the fact that many of these globals are closely related to the block index (hence `::UnloadBlockIndex`), and need to be reset with it.
I've shot this "manual reset" gun at my foot several times while doing the de-globalize chainman work.
Thankfully, now that we have a `BlockManager` class that owns the block index, these globals should be moved under that class so that they can live and die with the block index. These moves, along with making the block index non-heap-based, eliminates:
1. 3585b52139 The need to reason about when we need to manually call `::UnloadBlockIndex` (this decision can at times seem almost arbitrary)
2. f741623c25 The need to have an `::UnloadBlockIndex` or explicit `~ChainstateManager` at all
ACKs for top commit:
MarcoFalke:
ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670 👘
ajtowns:
ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670
ryanofsky:
Code review ACK 7ab07e033237d6ea179a6a2c76575ed6bd01a670. This all looks good and simplifies things nicely. I left some minor suggestions below but feel free to ignore.
Tree-SHA512: a36ee3fc122ce0b4e8d1c432662d7009df06264b724b793252978a1e409dde7a7ef1f78b9ade3f8bfb5388213f10ae2d058d57a7a46ae563e9034d7d33a52b69
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.
