fac6cfc50f 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 fac6cfc50f
jonatack:
ACK fac6cfc50f
Tree-SHA512: d84296b030bd4fa2709e5adbfe43a5f8377d218957d844af69a819893252af671df7f00004f5ba601a0bd70f3c1c2e58c4f00e75684da663f28432bb5c89fb86
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
f849e63bad fuzz: SplitString with multiple separators (Martin Leitner-Ankerl)
d1a9850102 http: replace boost::split with SplitString (Martin Leitner-Ankerl)
0d7efcdf75 core_read: Replace boost::split with SplitString (Martin Leitner-Ankerl)
b7ab9db545 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 f849e63bad
Tree-SHA512: f37d4dbe11cab2046e646045b0f018a75f978d521443a2c5001512737a1370e22b09247d5db0e5c9e4153229a4e2d66731903c1bba3713711c4cae8cedcc775d
fa4652ce59 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 fa4652ce59 rebased to master, debug build, unit tests
vincenzopalazzo:
ACK fa4652ce59
Tree-SHA512: cd7ec77347e195d659b8892d34c1e9644d4f88552a4d5fa310dc1756eb27050a99d3098b0b0d27f8474230f82c178fd9e22e7018d8248d5e47a7f4caad395e25
f64aa9c411 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 f64aa9c411
Tree-SHA512: 944cce49ed51537ee7a35ea4ea7f5feaf0c8fff2fa67ee81ec5adebfd3dcbaf41b73eb35e49973d5f852620367f13506fd12a7a9b5ae3a7a0007414d5c9df50f
e5485e8e4b test, bench: make prevector and checkqueue swap member functions noexcept (Jon Atack)
abc1ee5090 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
778343a379 scripted-diff: Rename PeerManagerImpl members (dergoegge)
91c339243e [net processing] Move nHighestFastAnnounce into PeerManagerImpl (dergoegge)
10b83e2aa3 [net processing] Move block cache state into PeerManagerImpl (dergoegge)
a4c55a93ef [net processing] Inline and simplify UpdatePreferredDownload (dergoegge)
490c08f96a [net processing] Move nPreferredDownload into PeerManagerImpl (dergoegge)
a292df283a [net processing] Move mapNodeState into PeerManagerImpl (dergoegge)
37ecaf3e7a [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 778343a379
MarcoFalke:
ACK 778343a379 🗒
Tree-SHA512: 4f22105d1de37b94c3ef349f38784a30cf8d450d394a6a7849e5bd78940a71e3edbffa3d25e8efb35d7f698fd255f199de7bd4c33e23af5621a6e4e67ed43cb5
fad35e9afd 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 fad35e9afd
Tree-SHA512: 3df789a222b407d61ad549adc4bbded00705d7c3db07472c31ce0e82216fe3ae27724b7f0ee3e85084bdf405cc28185e85487c9a7001620d6654fda77bab8eb3
7ab07e0332 validation: Prune UnloadBlockIndex and callees (Carl Dong)
7d99d725cd validation: No mempool clearing in UnloadBlockIndex (Carl Dong)
572d831927 Clear {versionbits,warning}cache in ~Chainstatemanager (Carl Dong)
eca4ca4d60 style-only: Use std::clamp for check_ratio, rename (Carl Dong)
fe96a2e4bd style-only: Use for instead of when loading Chainstate (Carl Dong)
5921b863e3 init: Reset mempool and chainman via reconstruction (Carl Dong)
6e747e80e7 validation: default initialize and guard chainman members (Anthony Towns)
98f4bdae81 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 972c5166ee:
```
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 7ab07e0332👘
ajtowns:
ACK 7ab07e0332
ryanofsky:
Code review ACK 7ab07e0332. 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.
Base32/base64 are mechanisms for encoding binary data. That they'd
decode to a string is just bizarre. The fact that they'd do that
based on the type of input arguments even more so.
a62e84438d fuzz: add `SplitString` fuzz target (MarcoFalke)
4fad7e46d9 test: add unit tests for `SplitString` helper (Kiminuo)
9cc8e876e4 refactor: introduce single-separator split helper `SplitString` (Sebastian Falbesoner)
Pull request description:
This PR adds a simple string split helper `SplitString` that takes use of the spanparsing `Split` function that was first introduced in #13697 (commit fe8a7dcd78). This enables to replace most calls to `boost::split`, in the cases where only a single separator character is used. Note that while previous attempts to replace `boost::split` were controversial (e.g. #13751), this one has a trivial implementation: it merely uses an internal helper (that is unit tested and in regular use with output descriptiors) and converts its result from spans to strings. As a drawback though, not all `boost::split` instances can be tackled.
As a possible optimization, one could return a vector of `std::string_view`s (available since C++17) instead of strings, to avoid copies. This would need more carefulness on the caller sites though, to avoid potential lifetime issues, and it's probably not worth it, considering that none of the places where strings are split are really performance-critical.
ACKs for top commit:
martinus:
Code review ACK a62e84438d. Ran all tests. I also like that with `boost::split` it was not obvious that the resulting container was cleared, and with `SplitString` API that's obvious.
Tree-SHA512: 10cb22619ebe46831b1f8e83584a89381a036b54c88701484ac00743e2a62cfe52c9f3ecdbb2d0815e536c99034558277cc263600ec3f3588b291c07eef8ed24
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
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 altoghther, 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.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
fad6d4f952 Remove not needed ArithToUint256 roundtrips in tests (MarcoFalke)
fa456ccb22 Remove duplicate static_asserts (MarcoFalke)
Pull request description:
No need to go from `arith_uint256`->`uint256` when a `uint256` can be constructed right away.
ACKs for top commit:
laanwj:
Code review ACK fad6d4f952
Tree-SHA512: bea901ea5904bf61a0dadf7168c6b126f7e62ff1180d4aa72063c28930a01a8baa57ab0d324226bd4de72fb59559455c29c049d90061f888044198aae1426dcb
3ae7791bca refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make 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>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bca
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
7417594187 miniscript: the 'd:' wrapper must not be 'u' (Antoine Poinsot)
Pull request description:
The type system was incorrectly relying on a standardness rule to be sound.
This bug was found and reported by Andrew Poelstra [based on a question from Aman Kumar Kashyap](https://github.com/rust-bitcoin/rust-miniscript/discussions/341).
ACKs for top commit:
sipa:
ACK 7417594187
apoelstra:
utACK 7417594187
achow101:
ACK 7417594187
Tree-SHA512: af68c1df1c40e40dd105ef54544c226f560524dd8e35248fa0305dbef966e96ec1fa6ff2fe50fb8f2792ac310761a29c55ea81dd7b6d122a0de0a68b135e5aaa
a2c4a7acd1 net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay() (Vasil Dimov)
d65b6c3fb9 net: use Sock::SetSockOpt() instead of setsockopt() (Vasil Dimov)
184e56d668 net: add new method Sock::SetSockOpt() that wraps setsockopt() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Add a `virtual` (thus mockable) method `Sock::SetSockOpt()` that wraps the system `setsockopt()`.
Convert the standalone `SetSocketNoDelay()` function to a `virtual` (thus mockable) method `Sock::SetNoDelay()`.
This will help avoid syscalls during testing and to mock them to return whatever is suitable for the tests.
ACKs for top commit:
laanwj:
Code review ACK a2c4a7acd1
jonatack:
ACK a2c4a7acd1 change since last review is folding `Sock::SetNoDelay()` into the callers
Tree-SHA512: 3e2b016c1e4128317a28c17dc9b30472949e1ac3b071b2697c6d30cbcc830df1ee4392a4e23b2ea1ab4e3fb0f59ef450e2a4f3c1df3d8c803dd081652b6c7387
The value it leaves on the stack depends on the last element on the
stack. However, we can't make sure this element is OP_1 (which would
give us the 'u' property) without the MINIMALIF rule.
MINIMALIF is only policy for P2WSH, therefore giving 'd:' the 'u'
property breaks consensus soundness: it makes it possible (by consensus
but not policy) for instance to satisfy a thresh() without satisfying
at least k of its subs.
This bug was found and reported by Andrew Poelstra.
c848a45101 test: fix connman UB by calling derived constructor (chinggg)
Pull request description:
Hopefully closes #24373 by calling `ConnmanTestMsg` test-constructor to avoid undefined behavior in process_message.cpp after casting `g_setup->m_node.connman`.
Top commit has no ACKs.
Tree-SHA512: c3dce9dcce33614c7b739edf28e416b600ab3d38d16cdb0430490e8ffc9b64aff9292006ae6fe7c636ab0627893bb21f69435893bdfb129a9a865be92baa6f17
This helper uses spanparsing::Split internally and enables to replace
all calls to boost::split where only a single separator is passed.
Co-authored-by: Martin Ankerl <Martin.Ankerl@gmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
4394733331 Add DEBUG_LOCKCONTENTION documentation to the developer notes (Jon Atack)
39a34b6877 Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive (Jon Atack)
Pull request description:
This is a more minimal, no-frills version of #24734 for backport. The other fixes and improvements in that pull can be done after.
*Copy of the PR 24734 description:*
PRs #22736, #22904 and #23223 changed lock contention logging from a `DEBUG_LOCKCONTENTION` compile-time preprocessor directive to a runtime `lock` log category and improved the logging output. This changed the locking from using `lock()` to `try_lock()`:
- `void Mutex::UniqueLock::lock()` acquires the mutex and blocks until it gains access to it
- `bool Mutex::UniqueLock::try_lock()` doesn't block but instead immediately returns whether it acquired the mutex; it may be used by `lock()` internally as part of the deadlock-avoidance algorithm
In theory the cost of `try_lock` might be essentially the [same](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-697) relative to `lock`. The test-and-set logic of these calls is purported to be ~ constant time, optimised and light/quick if used carefully (i.e. no mutex convoying), compared to system calls, memory/cache coherency and fences, wait queues, and (particularly) lock contentions. See the discussion around https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-902851054 and after with respect to performance/cost aspects. However, there are reasonable concerns (see [here](https://github.com/bitcoin/bitcoin/pull/22736#discussion_r691277896) and [here](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-620)) that `Base::try_lock()` may be potentially [costly](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-700) or [risky](https://github.com/bitcoin/bitcoin/pull/22904#issuecomment-930484001) compared to `Base::lock()` in this very frequently called code.
One alternative to keep the run-time lock logging would be to gate the `try_lock` call behind the logging conditional, for example as proposed in ccd73de1dd and ACKed [here](https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-901980815). However, this would add the [cost](https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-910102353) of `if (LogAcceptCategory(BCLog::LOCK))` to the hotspot, instead of replacing `lock` with `try_lock`, for the most frequent happy path (non-contention).
It turns out we can keep the advantages of the runtime lock contention logging (the ability to turn it on/off at runtime) while out of prudence putting the `try_lock()` call and `lock` logging category behind a `DEBUG_LOCKCONTENTION` compile-time preprocessor directive, and also still retain the lock logging enhancements of the mentioned PRs, as suggested in https://github.com/bitcoin/bitcoin/pull/24734#issuecomment-1085785480 by W. J. van der Laan, in https://github.com/bitcoin/bitcoin/pull/22736#discussion_r691280693, and in the linked IRC discussion.
Proposed here and for backport to v23.
ACKs for top commit:
laanwj:
Code review ACK 4394733331
Tree-SHA512: 89b1271cae1dca0eb251914b1a60fc5b68320aab4a3939c57eec3a33a3c8f01688f05d95dfc31f91d71a6ed80cfe2d67b77ff14742611cc206175e47b2e5d3b1
