Age | Commit message (Collapse) | Author |
|
|
|
fa1970f075292d7312654730a994a68c2ca8bc06 Make BlockManager::LoadBlockIndex private (MarcoFalke)
Pull request description:
* After commit fa27f03b4943540aa2eab283d4cf50ad4a1a01f8 `BlockManager::LoadBlockIndex` is only called by `BlockManager::LoadBlockIndexDB`. Thus, it can be made `private`.
* After commit c600ee38168a460d3026eae0e289c976194aad14 `m_best_invalid` is no longer accessed by `BlockManager::LoadBlockIndex`. Thus, the unused `friend` can be removed.
ACKs for top commit:
mruddy:
ACK fa1970f075292d7312654730a994a68c2ca8bc06 I verified by double checking references, then applying the patch, and running `make check`. LGTM.
Tree-SHA512: 9b36b4c59bf7ad01171764ce61b1be9750fc92d105c4fe939b1a6a70027ab6300d5d2a2fc3e82f981e22c3987f2ca84e092d2e1f8463fa320af9f05048580c0a
|
|
71c3f0356c01521a95c64fba1e7375aea6286bb0 move-only: Rename index + pruning functional test (Fabian Jahr)
de08932efa953e9a237cbf879460488ad8947411 test: Update test for indices on pruned nodes (Fabian Jahr)
825d19839bf71245306d4c8edde040e5941caa46 Index: Allow coinstatsindex with pruning enabled (Fabian Jahr)
f08c9fb0c6a799e3cb75ca5f763a746471625beb Index: Use prune locks for blockfilterindex (Fabian Jahr)
2561823531c25e1510c107eb41de944b00444ce0 blockstorage: Add prune locks to BlockManager (Fabian Jahr)
231fc7b035481f748159968353c1cab81354e843 refactor: Introduce GetFirstStoredBlock helper function (Fabian Jahr)
Pull request description:
# Motivation
The main motivation of this change and only behavior change noticeable by user is to allow running `coinstatsindex` on pruned nodes as has been requested [here for example](https://twitter.com/benthecarman/status/1388170854140452870?s=20).
# Background
`coinstatsindex` on pruned nodes can be enabled in a much simpler than it is done here but it comes with downside. The ability to run `blockfilterindex`on pruned nodes was added in #15946 but it also added the `blockfilterindex` as a dependency to `validation` and it introduced two new circular dependencies. Enabling `coinstatsindex` on pruned nodes in a similar way would add it as a dependency as well and introduce another circular dependency.
Instead, this PR introduces a `m_prune_blockers` map to `BlockManager` as a flexible approach to block pruning. Entities like `blockfilterindex`, for example, can add a key and a height to block pruning over that height. These entities need to update that value to allow more pruning when they are ready.
# Alternative approach
Upon completing the first draft of this PR I found #19463 as an alternative that follows the same but follows a very different approach. I am listing the main differences here as I see them:
- Usage of globals
- Blocks pruning with a start and a stop height
- Can persist blockers across restarts
- Blockers can be set/unset via RPCs
Personally, I don't think any of these are necessary to be added here but if the general approach or specific features are more appealing to reviewers I am happy to change to a solution based on that PR or port over specific parts of it here.
ACKs for top commit:
mzumsande:
Code review ACK 71c3f0356c01521a95c64fba1e7375aea6286bb0
ryanofsky:
Code review ACK 71c3f0356c01521a95c64fba1e7375aea6286bb0. Changes since last review: just tweaking comments and asserts, and rebasing
w0xlt:
tACK https://github.com/bitcoin/bitcoin/pull/21726/commits/71c3f0356c01521a95c64fba1e7375aea6286bb0 on signet.
Tree-SHA512: de7efda08b44aa31013fbebc47a02cd2de32db170b570f9643e1f013fee0e8e7ca3068952d1acc6e5e74a70910735c5f263437981ad73df841ad945b52d36b71
|
|
information to coin selection
ab5af9ca7293239ffc24ea7e23159b8184543f94 test: Add test for coinselection tracepoints (Andrew Chow)
ca02b68e8a7147f80cbe84b0742908b0b0faa04d doc: document coin selection tracepoints (Andrew Chow)
8e3f39e4fa2d8c63bc697c9ebd303965fcccea55 wallet: Add some tracepoints for coin selection (Andrew Chow)
15b58383d0029c4ae7b487e03cd451e1580eb91d wallet: compute waste for SelectionResults of preset inputs (Andrew Chow)
912f1ed181161b0365776cd490b63137aaad708a wallet: track which coin selection algorithm produced a SelectionResult (Andrew Chow)
Pull request description:
Tracepoints can be useful for coin selection as they would allow us to observe what is being selected, selection parameters, and calculation results. So this PR adds 4 new tracepoints:
1. After `SelectCoins` returns in order to observe the `SelectionResult`
2. After the first `CreateTransactionInternal` to observe the created transaction
3. Prior to the second `CreateTransactionInternal` to notify that the optimistic avoid partial spends selection is occurring
4. After the second `CreateTransactionInternal` to observe the created transaction and inform which solution is being used.
This PR also adds an algorithm enum to `SelectionResult` so that the first tracepoint will be able to report which algorithm was used to produce that result.
The primary use case for these tracepoints is in running coin selection simulations. The script I use to run these simulations use these tracepoints in order to gather data on the algorithm used and the calculated waste.
ACKs for top commit:
jb55:
crACK ab5af9ca7293239ffc24ea7e23159b8184543f94
josibake:
crACK https://github.com/bitcoin/bitcoin/pull/24644/commits/ab5af9ca7293239ffc24ea7e23159b8184543f94
0xB10C:
ACK ab5af9ca7293239ffc24ea7e23159b8184543f94. Code reviewed, ran the `interface_usdt_coinselection.py` test, and tested with the above bpftrace script (updated `%d` -> `%ld` where necessary, ty achow101).
Tree-SHA512: a4bf7a910cdf464622f2f3b5d44c15b891f24852df6e7f8c5b177fe3d8aaa4a1164593a24c3960eb22b16544fa7140e5c745345367b9e291b78395084c0ac8ff
|
|
bae4561938f66b31420ffc3f09c9a62932355b8c scripted-diff: rename BytePtr to AsBytePtr (João Barbosa)
Pull request description:
Building with iPhoneOS SDK fails because it also has `BytePtr` defined
in [/usr/include/MacTypes.h](https://opensource.apple.com/source/CarbonHeaders/CarbonHeaders-18.1/MacTypes.h.auto.html):
```cpp
typedef UInt8 * BytePtr;
```
ACKs for top commit:
MarcoFalke:
cr ACK bae4561938f66b31420ffc3f09c9a62932355b8c
laanwj:
Code review ACK bae4561938f66b31420ffc3f09c9a62932355b8c
sipa:
utACK bae4561938f66b31420ffc3f09c9a62932355b8c
prusnak:
Approach ACK bae4561938f66b31420ffc3f09c9a62932355b8c
Tree-SHA512: fb4d4a94c9c2238107952c071bae9bf6bbde6ed6651f6d300f222adf8a6a423f0567cbbcc3102d4167ef2e4e6f9988a2f91d75a5418bf6bcd64eebb4bcd1077f
|
|
4637bbe448ae7370528f40092ce230c32602a6d6 rpc: Explain active and internal in listdescriptors (Andrew Chow)
Pull request description:
The current help text for active and internal in listdescriptors is not particularly helpful. They require the reader to already know what those terms mean. This help text is updated to actually explain the definitions of those words in context of a descriptor wallet.
ACKs for top commit:
S3RK:
ACK 4637bbe448ae7370528f40092ce230c32602a6d6
jarolrod:
ACK 4637bbe448ae7370528f40092ce230c32602a6d6
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/24977/commits/4637bbe448ae7370528f40092ce230c32602a6d6
Tree-SHA512: 0af2c04f3b9920799cf616ad618bde9248eb9f74cc28f443b5b0f6646deba76e9b1415aca0865ad3bcc24aa6af0e9d07ad7b7cd80f0fe80838cf847f1b944426
|
|
9c96f1008b4997aea31f293fed31f98ed3becfcf tidy: enable modernize-use-nullptr (fanquake)
e53274868e0ec35156349826b0995bc444287690 Don't use zero as null pointer constant (-Wzero-as-null-pointer-constant) (practicalswift)
Pull request description:
Alternative to #15112 which uses `clang-tidy` to do perform the checking, rather than `-Wzero-as-null-pointer-constant`, and avoids having to uses pragmas, i.e:
```cpp
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
#if defined(HAVE_W_ZERO_AS_NULL_POINTER_CONSTANT)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wzero-as-null-pointer-constant"
#pragma GCC diagnostic ignored "-Wunknown-pragmas"
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wzero-as-null-pointer-constant"
#endif
```
to suppress warnings coming from upstream code.
Can be tested by dropping the preceding commit. Should produce errors like:
```bash
clang-tidy-14 --use-color -p=/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu /home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/netbase.cpp
/home/ubuntu/ci_scratch/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/netbase.cpp:678:36: error: use nullptr [modernize-use-nullptr,-warnings-as-errors]
if (!Socks5(strDest, port, 0, sock)) {
^
nullptr
```
ACKs for top commit:
laanwj:
Concept and code review ACK 9c96f1008b4997aea31f293fed31f98ed3becfcf
Tree-SHA512: d822a354e44ba8f7fc53da9a4be7de5c25cc4ffc7c57651b76fdd1a030764b0390cfd79fca94685b8a3ff4f4d13054764f12d1f0d8c2a1b9ba519a7524f7f5bf
|
|
fa870e3d4ccd6dfd0a9a8f2c608721a7251114e2 Remove not needed clang-format off comments (MarcoFalke)
Pull request description:
It seems odd to disable clang-format and force manual formatting when there is no need for it. So remove the clang-format comments and other unneeded comments.
Can be reviewed with `--word-diff-regex=. --ignore-all-space`
Looks like this was initially added in commit d9d79576f423cd9c5cef4547c7e3648dbb339460 to accommodate a linter that has since been removed and replaced by a functional test.
ACKs for top commit:
laanwj:
Code review ACK fa870e3d4ccd6dfd0a9a8f2c608721a7251114e2
fanquake:
ACK fa870e3d4ccd6dfd0a9a8f2c608721a7251114e2
Tree-SHA512: 0f8f97c12f5dbe517dd96c10b10ce1b8772d8daed33e6b41f73ea1040e89888cf3b8c0ad7b20319e366fe30c71e8b181c89098ae7f6a3deb8647e1b4731db815
|
|
reindex-chainstate
dac44fc06ff9938d070aa5221d106a7f31da9d81 init: disallow reindex-chainstate with optional indexes (Martin Zumsande)
62e14285f95d4ecb9528813acca975640dd7c598 doc: Add note that -reindex will rebuild optional indexes (Martin Zumsande)
Pull request description:
When started together with `-reindex-chainstate`, currently coinstatsindex gets corrupted and the blockfilterindex flatfiles duplicated. See the OP of #24630 for more a more detailed explanation on why this happens.
This is an alternative to #24630 which does not wipe and rebuild the indexes but returns an `InitError` when they are activated, thus requiring the user to deactivate them temporarily until the `-reindex-chainstate` run is finished.
This also disallows `-reindex-chainstate` in combination with `-txindex`, which is not leading to corruption, but currently still rebuilds the index unnecessarily and unexpectedly.
As a long-term goal, it would be desirable to have the indexes tolerate `reindex-chainstate` by ignoring their `BlockConnected` notifications (there is discussion in #24630 about this) or possibly move `reindex-chainstate` option into a `bitcoin-chainstate` executable, which could also solve the problem. But these would be larger projects - until then, it might be better to disallow the interaction than having corrupted indexes.
The first commit adjusts the `-reindex` doc to mention that this option does rebuild all active indexes.
ACKs for top commit:
ryanofsky:
Code review ACK dac44fc06ff9938d070aa5221d106a7f31da9d81. Just fixed IsArgSet call and edited error messages since last review
Tree-SHA512: c1abf7d350648ae227c3fd6c95d9a54c3bac9de70915275dea1c87cca6d9a76a056c0e306d95ef8cfe4df1f8525b418e0e7a4f52ded3be464041c0dc297f8930
|
|
df08c23f0164b3e4b0df7d43462924f90a245237 Precomputed hashes are note #16 in BIP341 (Gregory Sanders)
Pull request description:
Seems to have drifted one space
ACKs for top commit:
fanquake:
ACK df08c23f0164b3e4b0df7d43462924f90a245237
Tree-SHA512: f0e959743f67ad4b46584f44305d27a89b52874d70091e004ec05dfd2f8c6481e9edceecb0af98f519ad3debb0c0bb26fa27f370545b6e15f366bd0af1158bab
|
|
|
|
|
|
helper (boost::split replacement)
a62e84438d27ee6213219fe2c233e58814fcbb5d fuzz: add `SplitString` fuzz target (MarcoFalke)
4fad7e46d94a0fdee4ff917e81360d7ae6bd8110 test: add unit tests for `SplitString` helper (Kiminuo)
9cc8e876e412056ed22d364538f0da3d5d71946d 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 fe8a7dcd78cfeedc9a7c705e91384f793822912b). 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 a62e84438d27ee6213219fe2c233e58814fcbb5d. 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
|
|
Building with iPhoneOS SDK fails because it also has `BytePtr` defined
in /usr/include/MacTypes.h.
-BEGIN VERIFY SCRIPT-
sed -i 's/BytePtr/AsBytePtr/' $(git grep -l "BytePtr" src)
-END VERIFY SCRIPT-
|
|
with Mutex and rename it
709af67add93f6674fb80e3ae8e3f175653a62f0 p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex (w0xlt)
8be75fd0f0039eeea5f9af7c1eb17c584ed9f507 p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex` (w0xlt)
a237a065cc2c6337e3797cc30a0f84c56c6d2f3b scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex (w0xlt)
Pull request description:
Related to #19303, this PR gets rid of the RecursiveMutex `cs_totalBytesSent` and also adds `AssertLockNotHeld` macros combined with `LOCKS_EXCLUDED` thread safety annotations to avoid recursive locking.
ACKs for top commit:
jonatack:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0 per `git range-diff 7a4ac71 eff7918 709af67`, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative
vasild:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0
hebasto:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0, tested on Ubuntu 22.04.
Tree-SHA512: 560b4e6c92b1511911d69185207df6ee809db09b96d97f96430d8d2595dc05c98cc691aaec8a58ef87cf2ab0a98675c210b8ce0be3dedb81e31114bbbfdfd8be
|
|
|
|
Prior to this change blocks could be pruned up to the last block before the blockfilterindex current best block.
|
|
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>
|
|
|
|
|
|
The current help text for active and internal in listdescriptors is not
particularly helpful. They require the reader to already know what those
terms mean. This help text is updated to actually explain the
definitions of those words in context of a descriptor wallet.
|
|
Can be reviewed with --word-diff-regex=. --ignore-all-space
|
|
|
|
It currently leads to corruption (coinstatsindex) or
data duplication (blockfilterindex), so disable it.
|
|
function and NONFATAL_UNREACHABLE macro
ee02c8bd9aedad8cfd3c2618035fe275da025fb9 util/check: Add CHECK_NONFATAL identity function, NONFATAL_UNREACHABLE AND UNREACHABLE macros (Aurèle Oulès)
Pull request description:
This PR replaces the macro `CHECK_NONFATAL` with an identity function.
I simplified the usage of `CHECK_NONFATAL` where applicable in `src/rpc`.
This function is useful in sanity checks for RPC and command-line interfaces.
Context: https://github.com/bitcoin/bitcoin/pull/24804#discussion_r846182474.
Also adds `UNREACHABLE_NONFATAL` macro.
ACKs for top commit:
jonatack:
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9
MarcoFalke:
ACK ee02c8bd9aedad8cfd3c2618035fe275da025fb9 🍨
Tree-SHA512: 3cba09223cd7b22e62fe5d0b46c4a024c1d9957d4268ba6d3fb07fcc0a5854fc0886bb3266184e6a7df5df91373b3e84edd6adf6999c4e934aeef8c043b01aa2
|
|
`QMetaObject::invokeMethod()`
6958a26aa136e0976870237ccc6ea015d113f7ac Revert "qt: Add ObjectInvoke template function" (Hennadii Stepanov)
249984f4f93fe6fae81391f474e4d64ad9df3d6d qt: Replace `GUIUtil::ObjectInvoke()` with `QMetaObject::invokeMethod()` (Hennadii Stepanov)
Pull request description:
A comment in 5659e73493fcdfb5d0cb9d686c24c4fbe1c217ed states that `GUIUtil::ObjectInvoke`
> can be replaced by a call to the QMetaObject::invokeMethod functor overload after Qt 5.10
ACKs for top commit:
w0xlt:
tACK https://github.com/bitcoin-core/gui/pull/587/commits/6958a26aa136e0976870237ccc6ea015d113f7ac on Ubuntu 21.10, Qt 5.15.2.
promag:
Code review ACK 6958a26aa136e0976870237ccc6ea015d113f7ac.
Tree-SHA512: 6a840289568113cf38df6c1092821d626c2d206768a21d4dc6846b9dcccb4130477adb45ba718bb6bc15a3041871a7df3238983ac03db80406732be597693266
|
|
36f814c0e84d009c0e0aa26981a20ac4cf338a85 [netgroupman] Remove NetGroupManager::GetAsmap() (John Newbery)
4709fc2019e27e74be02dc5fc123b9f6f46d7990 [netgroupman] Move asmap checksum calculation to NetGroupManager (John Newbery)
1b978a7e8c71dcc1501705022e66f6779c8c4528 [netgroupman] Move GetMappedAS() and GetGroup() logic to NetGroupManager (John Newbery)
ddb4101e6377a998b7c598bf52217b47698ddec9 [net] Only use public CNetAddr functions and data in GetMappedAS() and GetGroup() (John Newbery)
6b2268162e96bc4fe1a3ebad454996b1d3d4615c [netgroupman] Add GetMappedAS() and GetGroup() (John Newbery)
19431560e3e1124979c60f39eca9429c4a0df29f [net] Move asmap into NetGroupManager (John Newbery)
17c24d458042229e00dd4e0b75a32e593be29564 [init] Add netgroupman to node.context (John Newbery)
9b3836710b8160d212aacd56154938e5bb4b26b7 [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 36f814c0e84d009c0e0aa26981a20ac4cf338a85
jnewbery:
CI failure seems spurious. I rebased onto latest master to trigger a new CI run, but whilst I was doing that, mzumsande ACKed https://github.com/bitcoin/bitcoin/commit/36f814c0e84d009c0e0aa26981a20ac4cf338a85, so I've reverted to that.
dergoegge:
Code review ACK 36f814c0e84d009c0e0aa26981a20ac4cf338a85
Tree-SHA512: 244a89cdfd720d8cce679eae5b7951e1b46b37835fccb6bdfa362856761bb110e79e263a6eeee8246140890f3bee2850e9baa7bc14a388a588e0e29b9d275175
|
|
|
|
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
tests
fad6d4f952373690ef16ce27b0926c0ab762066a Remove not needed ArithToUint256 roundtrips in tests (MarcoFalke)
fa456ccb2287b2a1a4eb7224b424f12fe59302e9 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 fad6d4f952373690ef16ce27b0926c0ab762066a
Tree-SHA512: bea901ea5904bf61a0dadf7168c6b126f7e62ff1180d4aa72063c28930a01a8baa57ab0d324226bd4de72fb59559455c29c049d90061f888044198aae1426dcb
|
|
3ae7791bcaa88f5c68592673b8926ee807242ce7 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 3ae7791bcaa88f5c68592673b8926ee807242ce7
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
|
|
asmap no longer needs to be exposed anywhere outside NetGroupManager.
|
|
|
|
Reviewer hint: use:
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
|
|
GetGroup()
Also change parameter/variable names. This makes the next commit mostly
move-only.
|
|
These currently call through to the CNetAddr methods. The logic will be moved in a future commit.
|
|
|
|
`fHavePruned`
f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f scripted-diff: Rename pindexBestHeader, fHavePruned (Carl Dong)
a4014021258319941716d6338c18667462a06280 Clear fHavePruned in BlockManager::Unload() (Carl Dong)
3308ecd3fc254ee4ef9f803c09f00ba4dc968520 move-mostly: Make fHavePruned a BlockMan member (Carl Dong)
c96524113c48553c4bbad63077a25494eca8159e Clear pindexBestHeader in ChainstateManager::Unload() (Carl Dong)
73eedaaacc3b5f2dd791997109f2f5312a894336 style-only: Miscellaneous whitespace changes (Carl Dong)
0d567daf23c9fcb2d95b38913ee45a8b0ba3b027 move-mostly: Make pindexBestHeader a ChainMan member (Carl Dong)
5d670173a32ccdcb25d3a6bf97317f0ac774e0ed validation: Load pindexBestHeader in ChainMan (Carl Dong)
Pull request description:
Split off from #22564 per Marco's suggestion: https://github.com/bitcoin/bitcoin/pull/22564#issuecomment-1100011503
This is basically the move-mostly parts of #22564. The overall intent is to move mutable globals manually reset by `::UnloadBlockIndex` into appropriate structs such that they are cleared at the appropriate times. Please read #22564's description for more rationale.
In summary , this PR moves:
1. `pindexBestHeader` -> `ChainstateManager::m_best_header`
2. `fHavePruned` -> `BlockManager::m_have_pruned`
ACKs for top commit:
ajtowns:
ACK f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f -- code review only
MarcoFalke:
kirby ACK f0a2fb3c5dbf3c4bec7faf934baff3e723734b3f 😋
Tree-SHA512: 8d161701af81af1ff42da1b22a6bef2f8626e8642146bc9c3b27f3a7cd24f4d691910a2392b188ae058fec0611a17304dd73f60da695f53832d327f73d2fc963
|
|
...to m_best_header and m_have_pruned
-BEGIN VERIFY SCRIPT-
find_regex="\bpindexBestHeader\b" \
&& git grep -l -E "$find_regex" -- src \
| xargs sed -i -E "s@$find_regex@m_best_header@g"
find_regex="\bfHavePruned\b" \
&& git grep -l -E "$find_regex" -- src \
| xargs sed -i -E "s@$find_regex@m_have_pruned@g"
-END VERIFY SCRIPT-
|
|
-----
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.
|
|
[META] In the next commit, we move the clearing of fHavePruned to
BlockManager::Unload()
|
|
-----
Code Reviewer Notes
Call graph of relevant functions:
UnloadBlockIndex() <-- Moved from
calls ChainstateManager::Unload() <-- Moved to
Safe because ChainstateManager::Unload() is called only by
UnloadBlockIndex() and no other callers.
|
|
...of touched lines and surrounding
|
|
[META] In the next commit, we move the clearing of pindexBestHeader to
ChainstateManager::Unload()
|
|
`qDBusRegisterMetaType` return type
6cf4dc7f64b42cbbff6a2ce7616ee625a87a29f5 qt: Do not assume `qDBusRegisterMetaType` return type (Hennadii Stepanov)
Pull request description:
`qDBusRegisterMetaType` returns:
- [`int`](https://doc.qt.io/qt-5/qdbusargument.html#qDBusRegisterMetaType) in Qt 5
- [`QMetaType`](https://doc.qt.io/qt-6/qdbusargument.html#qDBusRegisterMetaType) in Qt 6
ACKs for top commit:
laanwj:
Anyhow code review ACK 6cf4dc7f64b42cbbff6a2ce7616ee625a87a29f5
w0xlt:
tACK https://github.com/bitcoin-core/gui/pull/584/commits/6cf4dc7f64b42cbbff6a2ce7616ee625a87a29f5 on Ubuntu 21.10, Qt 5.15.2.
Tree-SHA512: 17d43e191d31a6f927d19550c52471ed3b9222f492a23cee2e553f2c679cf37125e00637b00ea9f4ee3e37dfcf5278171be9a5e1e2e899592516291c7b5cd942
|
|
`QKeyEvent` copy constructor
3ec6504a2e5b4afb7a2719a82191e0b96fe23214 qt: Do not use `QKeyEvent` copy constructor (Hennadii Stepanov)
Pull request description:
This PR is preparation for [Qt 6](https://github.com/bitcoin/bitcoin/pull/24798), and it fixes an experimental build with Qt 6.2.4 as copying of `QEvent` has been [disabled](https://github.com/qt/qtbase/commit/19f9b0d5f54379151eb71e98555b203ad6756276) in Qt 6.0.0.
ACKs for top commit:
w0xlt:
tACK https://github.com/bitcoin-core/gui/pull/580/commits/3ec6504a2e5b4afb7a2719a82191e0b96fe23214 on Ubuntu 21.10, Qt 5.15.2
shaavan:
reACK 3ec6504a2e5b4afb7a2719a82191e0b96fe23214
Tree-SHA512: 583a9dad0c621d9f02f77ccaa9f55ee79e12e3c47f418911ef2dfe0de357d772d1928ae3ec19b6f0c0674da858bab9d4542a26cc14b06ed921370dfeabd1c194
|
|
74175941870347458ba8a0074f88b22cb94d0235 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 74175941870347458ba8a0074f88b22cb94d0235
apoelstra:
utACK 74175941870347458ba8a0074f88b22cb94d0235
achow101:
ACK 74175941870347458ba8a0074f88b22cb94d0235
Tree-SHA512: af68c1df1c40e40dd105ef54544c226f560524dd8e35248fa0305dbef966e96ec1fa6ff2fe50fb8f2792ac310761a29c55ea81dd7b6d122a0de0a68b135e5aaa
|
|
SetSocketNoDelay() mockable/testable
a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay() (Vasil Dimov)
d65b6c3fb9cdd41fa53bc76a7b8f49aaa089b0bc net: use Sock::SetSockOpt() instead of setsockopt() (Vasil Dimov)
184e56d6683d05fc84f5153cfff83a2e32883556 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 a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6
jonatack:
ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 change since last review is folding `Sock::SetNoDelay()` into the callers
Tree-SHA512: 3e2b016c1e4128317a28c17dc9b30472949e1ac3b071b2697c6d30cbcc830df1ee4392a4e23b2ea1ab4e3fb0f59ef450e2a4f3c1df3d8c803dd081652b6c7387
|
|
07ddecb84e6097684fa56cfc79c8c2aad76f6604 refactor: Use [[maybe_unused]] attribute (Hennadii Stepanov)
55e0fc8df9c4045453982888732a0dd7c99ea6d1 refactor: Drop unneeded workarounds aimed to silence unused warning (Hennadii Stepanov)
Pull request description:
This change is required for bitcoin/bitcoin#24773 as it prevents MSVC yelling about "warning C4551: function call missing argument list".
But it is useful by itself as it makes code more concise and readable.
ACKs for top commit:
Empact:
Code review ACK 07ddecb84e6097684fa56cfc79c8c2aad76f6604
laanwj:
Code review ACK 07ddecb84e6097684fa56cfc79c8c2aad76f6604
vincenzopalazzo:
ACK https://github.com/bitcoin/bitcoin/pull/24772/commits/07ddecb84e6097684fa56cfc79c8c2aad76f6604
w0xlt:
ACK 07ddecb
Tree-SHA512: 01791855a9ba742202d5718203303af989fcb501b7cf2a24ac8d78e87487acca38f77bef264b8e27e41ad1ccf96e426725cf65bfd96ce2ac71c46b3792bed857
|
|
0000a63689036dc4368d04c0648a55fdf507932f Simplify GetTime (MarcoFalke)
Pull request description:
The implementation of `GetTime` is confusing:
* The value returned by `GetTime` is assumed to be equal to `GetTime<std::chrono::seconds>()`. Both are mockable and the only difference is return type, the value itself is equal. However, the implementation does not support this assumption.
* On some systems, `time_t` might be a signed 32-bit integer (https://en.cppreference.com/w/c/chrono/time), thus breaking in the year 2038, whereas `GetTime<std::chrono::seconds>` does not. Also, `time_t` might be `-1` "on error", where "error" is unspecified.
* `GetTime<std::chrono::seconds>` calls `GetTimeMicros`, which calls `GetSystemTime`, which calls `std::chrono::system_clock::now`, which doesn't have the above issues. See https://en.cppreference.com/w/cpp/chrono/system_clock/now
* `GetTimeMicros` and the internal-only `GetSystemTime` will likely be renamed (to clarify they are the non-mockable non-monotonic system time) or removed in the future to be replaced by appropriate `std::chrono::time_point<Clock>` getters.
Fix all issues by:
* making `GetTime()` an alias for `GetTime<std::chrono::seconds>().count()`.
* inlining the needed parts of `GetSystemTime` directly instead of needlessly increasing the function call stack with functions that are likely to be removed in the future.
ACKs for top commit:
martinus:
Code review, untested ACK 0000a63689036dc4368d04c0648a55fdf507932f. By the way strictly speaking `std::chrono::system_clock` is only guaranteed to be based on the unix epoch starting with C++20: https://en.cppreference.com/w/cpp/chrono/system_clock
theStack:
Code-review ACK 0000a63689036dc4368d04c0648a55fdf507932f
Tree-SHA512: f751ba740e0da65537be800e9414dd02282d9f04c0b0fb986a36546f257d0b888d8688653cdda5d355ec832c0e09d866922d9161b1ccd33485c1c92c5d1e802f
|