fa4fb8d98b random: Add FastRandomContext::rand_uniform_delay (MarcoFalke)
faa5c62967 Add time helpers for std::chrono::steady_clock (MarcoFalke)
Pull request description:
A steady clock can be used in the future for the scheduler, for example.
A random uniform delay applied to a time point can be used in the future for time points passed to the scheduler, or delays in net processing.
Currently they are unused outside of tests, but if they turn out unused in the future (unlikely), they can trivially be removed again. I am splitting them out, so that several branches/pulls can build on top of them without duplicating the commits.
ACKs for top commit:
ajtowns:
ACK fa4fb8d98b
Tree-SHA512: 2c37174468fe84b1cdf2a032f458706df44b99a5f99062417bb42078b6f69e2f1738d20c21cd9386ca5a35f3bc0583e547ba40168c66f6aa42f700ba35dd95d4
92b35aba22 index, refactor: Change sync variables to use `std::chrono::steady_clock` (w0xlt)
Pull request description:
This PR refactors the sync variables to use `std::chrono::steady_clock` as it is best suitable for measuring intervals.
ACKs for top commit:
jonatack:
utACK 92b35aba22
ajtowns:
ACK 92b35aba22 - code review only
Tree-SHA512: cd4bafde47b30beb88c0aac247e41b4dced2ff2845c67a7043619da058dcff4f84374a7c704a698f3055c888d076d25503c2f38ace8fbc5456f624e0efe1e188
f70ee34c71 qt, refactor: Declare `WalletModel` member functions with `const` (Hennadii Stepanov)
Pull request description:
After bitcoin/bitcoin#12830 the `WalletModel` class has two member functions: be7a5f2fc4/src/qt/walletmodel.h (L81) and be7a5f2fc4/src/qt/walletmodel.h (L154)
This PR drops the former one as redundant, and declares `WalletModel` member functions with the `const` qualifier where appropriate.
ACKs for top commit:
promag:
Code review ACK f70ee34c71.
kristapsk:
cr ACK f70ee34c71
w0xlt:
Code Review ACK f70ee34c71
Tree-SHA512: 43e6661822c667229ea860fb94c2e3154c33773dbd9fca1f6f76cc31c5875a1a0e8caa65ddfc20dec2a43e29e7b2469b3b6fa148fe7ec000ded518b4958b2b38
15069130c6 qt, test: Add tests for `tableView` in `AddressBookPage` dialog (Hennadii Stepanov)
edae3ab699 qt: No need to force Qt::QueuedConnection for NotifyAddressBookChanged (Hennadii Stepanov)
Pull request description:
This PR is a prerequisite for more thorough testing of filtering in the `AddressBookPage` class in context of bitcoin-core/gui#578 and bitcoin-core/gui#585.
Required for bitcoin-core/gui#592.
ACKs for top commit:
promag:
Code review ACK 15069130c6.
Tree-SHA512: 86986d47606cbd54d813436c7afb21894e2200b6d3042a7aa0b5e84821c765bd68b14ad38a445069891ab33f2d7bcd4933b8373e14e9afb0c91f1a6ddf4da740
81c09ee45c Unroll the ChaCha20 inner loop for performance (Pieter Wuille)
Pull request description:
Unrolling the inner ChaCha20 loop gives a ~15% speedup for me in the CHACHA20_* benchmarks. It's a simple change, this performance helps with RNG generation, and will matter more for BIP324.
ACKs for top commit:
martinus:
tested ACK 81c09ee with clang++ 13.0.1, test `CHACHA20_1MB`:
MarcoFalke:
ACK 81c09ee45c🍟
Tree-SHA512: 108bd0ba573bb08de92d611e7be7c09a2c2700f9655f44129b87f9b71f7e101dfc6bd345783e7b4b9b40f0b003913cf59187f422da8cdb5b20887f7855b2611a
308dd2e93e 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 308dd2e93e
Tree-SHA512: 5bfdaab1499607ae2c3cd3e2e9e8c37850bfd0e327e680f4e36c81f9c6d98a543af78ecfac1ab0e06325d264412615a04d52005875780c7db2a4d81bd2d2259a
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
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>
e71c51b27d refactor: rename command -> message type in comments in the src/net* files (Shashwat)
2b09593bdd scripted-diff: Rename message command to message type (Shashwat)
Pull request description:
This PR is a follow-up to #24078.
> a message is not a command, but simply a message of some type
The first commit covers the message_command variable name and comments not addressed in the original PR in `src/net*` files.
The second commit goes beyond the original `src/net*` limit of #24078 and does similar changes in the `src/rpc/net.cpp` file.
ACKs for top commit:
MarcoFalke:
review ACK e71c51b27d💥
Tree-SHA512: 24015d132c00f15239e5d3dc7aedae904ae3103a90920bb09e984ff57723402763f697d886322f78e42a0cb46808cb6bc9d4905561dc6ddee9961168f8324b05
b42643c253 doc: update init.cpp -conf help text (josibake)
970b9987ad doc: update devtools, release-process readmes (josibake)
50635d27b4 build: include bitcoin.conf in build outputs (josibake)
6aac946f49 doc: update bitcoin-conf.md (Josiah Baker)
1c7e820ded script: add script to generate example bitcoin.conf (josibake)
b483084d86 doc: replace bitcoin.conf with placeholder file (josibake)
Pull request description:
create a script for parsing the output from `bitcoind --help` to create an example conf file for new users
## problem
per #10746 , `bitcoin.conf` not being put into the data directory during installation causes some confusion for users when running bitcoin. in the discussion on the issue, one proposed solution was to have an example config file and instruct users to `cp` it into their data directory after startup. in addition to #10746 , there have been other requests for a "skeleton config file" (https://github.com/bitcoin/bitcoin/issues/19641) to help users get started with configuring bitcoind.
the main issue with an example config file is that it creates a second source of truth regarding what options are available for configuring bitcoind. this means any changes to the options (including the addition or removal of options) would have to be updated for the command line and also updated in the example file.
this PR addresses this issue by providing a script to generate an example file directly from the `bitcoind --help` on-demand by running `contrib/devtools/gen-bitcoin-conf.sh`. this solution was originally proposed on #10746 and would also solve #19641 . this guarantees any changes made to the command-line options or the command-line options help would also be reflected in the example file after compiling and running the script.
the main purpose of this script is to generate a config file to be included with releases, same as `gen-manpages.sh`. this ensures every release also includes an up-to-date, full example config file for users to edit. the script is also available for users who compile from source for generating an example config for their compiled binary.
## special considerations
this removes the `bitcoin.conf` example file from the repo as it is now generated by this script. the original example file did contain extra text related to how to use certain options but going forward all option help docs should be moved into `init.cpp`
this also edits `init.cpp` to have the option help indicate that `-conf` is not usable from the config file. this is similar to how `-includeconf` 's help indicates it cannot be used from the command line
ACKs for top commit:
laanwj:
Tested and code review ACK b42643c253
Tree-SHA512: 4546e0cef92aa1398da553294ce4712d02e616dd72dcbe0b921af474e54f24750464ec813661f1283802472d1e8774e634dd1cc26fbf1f13286d3e0406c02c09
e3a06a3c6c test: Add `strerror` to locale-dependence linter (laanwj)
f00fb1265a util: Increase buffer size to 1024 in SysErrorString (laanwj)
718da302c7 util: Refactor SysErrorString logic (laanwj)
e7f2f77756 util: Use strerror_s for SysErrorString on Windows (laanwj)
46971c6dbf util: Replace non-threadsafe strerror (laanwj)
Pull request description:
Some uses of non-threadsafe `strerror` have snuck into the code since they were removed in #4152. Add a wrapper `SysErrorString` for thread-safe strerror alternatives (with code from `NetworkErrorString`) and replace all uses of `strerror` with this.
Edit: I've also added a commit that refactors the code so that buf[] is never read at all if the function fails, making some fragile-looking code unnecessary.
Edit2: from the linux manpage:
```
ATTRIBUTES
For an explanation of the terms used in this section, see attributes(7).
┌───────────────────┬───────────────┬─────────────────────────┐
│Interface │ Attribute │ Value │
├───────────────────┼───────────────┼─────────────────────────┤
│strerror() │ Thread safety │ MT-Unsafe race:strerror │
├───────────────────┼───────────────┼─────────────────────────┤
…
├───────────────────┼───────────────┼─────────────────────────┤
│strerror_r(), │ Thread safety │ MT-Safe │
│strerror_l() │ │ │
└───────────────────┴───────────────┴─────────────────────────┘
```
As the function can be called from any thread at any time, using a non-thread-safe function is unacceptable.
ACKs for top commit:
jonatack:
ACK e3a06a3c6c
Tree-SHA512: 20e71ebb9e979d4e1d8cafbb2e32e20c2a63f09115fe72cdde67c8f80ae98c531d286f935fd8a6e92a18b72607d7bd3e846b2d871d9691a6036b0676de8aaf25
e5d1831517 [netgroup] Use nStartByte as offset for the last byte of the group (dergoegge)
Pull request description:
This addresses my review [comments](https://github.com/bitcoin/bitcoin/pull/22910#discussion_r856095896) I left on #22910.
This has no effect on the current logic as `nStartByte` is only used for internal addresses which only ever add 10 whole bytes to the returned group. However to avoid future bugs, I think we should use `nStartByte` as offset for the last byte as well, in case we ever add a new address type that makes makes use of `nStartByte` and adds fractional bytes to the group.
ACKs for top commit:
jnewbery:
Code review ACK e5d1831517
theStack:
Concept and code-review ACK e5d1831517
Tree-SHA512: 4c08c7d6cb38b553e998798b3e3b790177aaa2141a48e277dfd538e01a7fccadf644329e93c5b0fb5e7e4037494c8dfe061b94eb52c6b31dc21bdf99eb0e311a
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
4cb9d21434 blockstorage: add LIFETIMEBOUND to GetFirstStoredBlock()::start_time (Jon Atack)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/25016#discussion_r862330288, the lifetimebound attribute here indicates that a resource owned by the `start_block` param of `CBlockIndex* BlockManager::GetFirstStoredBlock()` can be retained by the method's return value, which enables detecting the use of out-of-scope stack memory (ASan `stack-use-after-scope`) at compile time.
See https://releases.llvm.org/12.0.0/tools/clang/docs/AttributeReference.html#lifetimebound and #22278 for related discussion, and #25040 for a similar example.
ACKs for top commit:
MarcoFalke:
review ACK 4cb9d21434
Tree-SHA512: a3f5ef83ebb6f08555d7c89f2437a682071b4ad77a7aa3326b6d2282c909bf9fcf4dac6bf05ee1d9931f2102cad4a02df5468bde1cf377d7126e84e8541604dc
fa758f9bc5 scripted-diff: Rename rpc/misc.cpp to rpc/node.cpp (MacroFake)
fa87eb8ce1 rpc: Move output script RPCs to separate file (MacroFake)
Pull request description:
RPCs handling output scripts (addresses, scriptPubKeys, and output script descriptors) should not be placed in a file called `misc.cpp`, so move them out, then rename `misc.cpp`.
ACKs for top commit:
pk-b2:
ACK fa758f9bc5
vincenzopalazzo:
ACK fa758f9bc5
Tree-SHA512: 0cf8b5b8456361015513e93d3e604ea07d998dd578415b1d0e2918fb401fc44547fc1bb80b7c33c2086f6268e7b8f59837d2955f57434f646ea7921f0158b32d
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
Note that `SplitString` doesn't support token compression, but in this case
it does not matter as empty strings are already skipped anyways.
Also removes split.hpp and classification.hpp from expected includes
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
88044a14d9 Guard `#include <config/bitcoin-config.h>` (Hennadii Stepanov)
Pull request description:
A fix for builds when the `HAVE_CONFIG_H` macro is not defined.
ACKs for top commit:
Empact:
Code Review ACK 88044a14d9
Tree-SHA512: f2bf1693c7671d7113dccaf66ae34a84719d86cb3271fa18b36611deab93a48d787b3ccfbd735d3b763017d709971cb1151d8d7f30390720009e6e2a6275b5b0
fa753abd7c rpc: Move fee estimation RPCs to separate file (MacroFake)
Pull request description:
Fee estimation is generally used by wallets when creating txs. It doesn't have anything to do with creating or submitting blocks.
ACKs for top commit:
pk-b2:
ACK fa753abd7c
brunoerg:
crACK fa753abd7c
Tree-SHA512: 81e0edc936198a0baf0f5bfa8cfedc12db51759c7873bb0082dfc5f0040d7f275b35f639c6f5b86fa1ea03397b0d5e757c2ce1b6b16f1029880a39b9c3aaceda
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
e2b954e87f rpc: use GetBlockTime() for getblockchaininfo#time (Jon Atack)
86ce844d3b blockstorage, refactor: pass GetFirstStoredBlock() start_block by reference (Jon Atack)
ed12c0a49d blockstorage, refactor: make GetFirstStoredBlock() a member of BlockManager (Jon Atack)
Pull request description:
Picks up the remaining review feedback in #21726 and #24956.
- make the global function `GetFirstStoredBlock()` a member of the `BlockManager` class
- pass the `start_block` param of `GetFirstStoredBlock()` by reference instead of a pointer
- use `GetBlockTime()` for RPC getblockchaininfo#time
ACKs for top commit:
MarcoFalke:
ACK e2b954e87f
Tree-SHA512: 546e3c2e18245996b5b286829a605ae919eff3510963ec71b7c9ede521b1f501697e5b2f9d35d7a0606a74cbc8907201c58acf1e2cf7daaa86eefe2e3a8e296b
fa2102e239 test: Split MempoolAncestryTests into two (MacroFake)
Pull request description:
The two tests don't share any state, so it seems clearer to put them in separate scopes.
ACKs for top commit:
jnewbery:
Code review ACK fa2102e239
Tree-SHA512: 6669f50f8d5944fed55ecc88aa1bd139bddf6a40e3c2e8f88c3cc7e70cf6d4650c0dd652c7f304813893827c3930d626268655cd9b3f17ff9c9a1a02f0359714
fa60169811 rpc: Move signmessage RPC util to new file (MacroFake)
fa9425177e Remove cs_main from verifymessage (MacroFake)
Pull request description:
The `verifymessage` RPC has several issues:
* It takes `cs_main` for no reason, blocking progress on removing the `cs_main` global mutex.
* It is located in a file called `misc`, which is not a very helpful name.
Fix all issues.
ACKs for top commit:
vincenzopalazzo:
ACK fa60169811
Tree-SHA512: c71a1f481b828e0a544405fecbbc7ca44e66ea46b498d7aed1f1c584d6a99724deb13e89d90b9d5cdeecbce293e6a41e9f7ae299543f6d761bf9e7a839b6c7f3
