9b8dcb25b5 [net processing] Rename PoissonNextSendInbound to NextInvToInbounds (John Newbery)
ea99f5d01e [net processing] Move PoissonNextSendInbound to PeerManager (John Newbery)
bb060746df scripted-diff: replace PoissonNextSend with GetExponentialRand (John Newbery)
03cfa1b603 [refactor] Use uint64_t and std namespace in PoissonNextSend (John Newbery)
9e64d69bf7 [move] Move PoissonNextSend to src/random and update comment (John Newbery)
Pull request description:
`PoissonNextSend` and `PoissonNextSendInbound` are used in the p2p code to obfuscate various regularly occurring processes, in order to make it harder for others to get timing-based information deterministically.
The naming of these functions has been confusing to several people (including myself, see also #23347) because the resulting random timestamps don't follow a Poisson distribution but an exponential distribution (related to events in a Poisson process, hence the name). This PR
- moves `PoissonNextSend()` out of `net` to `random` and renames it to `GetExponentialRand()`
- moves `PoissonNextSendInbound()` out of `CConnman` to `PeerManager` and renames it to `NextInvToInbounds()`
- adds documentation for these functions
This is work by jnewbery - due to him being less active currently, I opened the PR and will address feedback.
ACKs for top commit:
jnewbery:
ACK 9b8dcb25b5
hebasto:
ACK 9b8dcb25b5, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
ACK 9b8dcb25b5📊
Tree-SHA512: 85c366c994e7147f9981fe863fb9838502643fa61ffd32d55a43feef96a38b79a5daa2c4d38ce01074897cc95fa40c76779816edad53f5265b81b05c3a1f4f50
36012ef143 qa: test descriptors with mixed xpubs and const pubkeys (Antoine Poinsot)
Pull request description:
Writing unit tests for Miniscript descriptors i noticed that `test/descriptor_tests`'s `DoCheck()` assumes that a descriptor would either contain only extended keys or only const pubkeys: if it detects an xpub in the descriptor it would assert the number of cached keys is equal to the number of keys in the descriptor, which does not hold if the descriptor also contains const (raw?) public keys since we only cache parent xpubs.
ACKs for top commit:
achow101:
ACK 36012ef143
Tree-SHA512: 2ede67a6dff726bcad3e260f3deb25c9b77542ed1880eb4ad136730b741014ce950396c69c7027225de1ef27108d609bafd055188b88538ace0beb13c7e34b0b
5e7e4c9f6e refactor: replace RecursiveMutex g_maplocalhost_mutex with Mutex (w0xlt)
a7da1409bc scripted-diff: rename cs_mapLocalHost -> g_maplocalhost_mutex (w0xlt)
Pull request description:
This PR is related to #19303 and gets rid of the `RecursiveMutex cs_mapLocalHost`.
ACKs for top commit:
shaavan:
ACK 5e7e4c9f6e
theStack:
ACK 5e7e4c9f6e
hebasto:
ACK 5e7e4c9f6e, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 961171e346fe385e16db9830115a8096f4ca2499bbea11a08c02ca808638dfb63c434ab9d66392c71e85be6352c8a2b6a0054b5a61aaabd28d71581fed5beae7
e2ab9f83f8 build: disable external signer on Windows (fanquake)
Pull request description:
This change explicitly disables support for external signing when targeting Windows and OpenBSD. The driver for this is that Boost Process uses boost::filesystem internally, when targeting Windows, which gets in the way of removing our usage of it (#20744). While we could adjust #20744 to still link against the Boost libs when building for Windows, that would be disappointing, as we wouldn't have cleanly removed the Boost usage we're trying too (including the build infrastructure), and, we'd be in a position where we would be building releases differently depending on the platform, which is something I want to avoid.
After discussion with Sjors, Achow and Hebasto, this seemed like a reasonable step to move #20744 forward (as-is). Note that support for external signing ([while already being experimental](https://github.com/bitcoin/bitcoin/blob/master/doc/external-signer.md#example-usage)), could be considered even more experimental on Windows. Also, oddly, we have external-signing [explicitly disabled in our Windows (cross-compile) CI](807169e10b/ci/test/00_setup_env_win64.sh (L16)), it's not clear why this is the case, as, if it's a feature being built into releases, it should be being built and tested in the CI which is most-like the release process.
There is an [issue open upstream](https://github.com/boostorg/process/issues/207), in regards to migrating Boost Process to std::filesystem, or having an option to use it. However there hasn't been much discussion since it was opened ~9 months ago. There is another related issue here: https://github.com/klemens-morgenstern/boost-process/issues/164.
Resolves#24036.
ACKs for top commit:
Sjors:
utACK e2ab9f8
achow101:
ACK e2ab9f83f8
kallewoof:
utACK e2ab9f83f8
hebasto:
ACK e2ab9f83f8, tested on Linux Mint 20.2 (x86_64).
Tree-SHA512: 36fcfc0e1a008a8271dc76b8e12e93d3e1d1e528bf668e95a559e9f6fd7d5f031bd7a6a6bc8b9fa9d057b2cd56f9ec8838c7f74e87899bf9a6aeb787afbd112c
dc5d6b0d47 fs: Make compatible with boost 1.78 (Andrew Chow)
Pull request description:
Boost 1.78 removed `operator+` in a way that breaks our usage of it in a subclass. A [proposed workaround](https://github.com/boostorg/filesystem/issues/223#issuecomment-1000230207) for this is to cast the argument to `boost::filesystem::path`, and this is backwards compatible with older versions of boost.
Additionally, it appears that `fs::canonical` no longer removes trailing slashes. This was causing a test to fail. The solution is to explicitly remove the trailing separator in the one place that `fs::canonical` is used.
Lastly, `fs::create_directories` now has an error message saying `create_directories` instead of `create_directory`. This caused wallet_multiwallet.py to fail. The error message check has been updated to be able accept either string.
Fixes#23846
ACKs for top commit:
ryanofsky:
Code review ACK dc5d6b0d47
vincenzopalazzo:
ACK dc5d6b0d47
Tree-SHA512: d4d8e7b49b8dfbf0ced9bfe9a2b3827841227fc755fc799f19159076b0ccf882432cc8b6ad93cdeda98fb58b942b9ba50a9e0a6b4f6b1e0097e80f1074ae5682
e177fcab38 Replace `struct update_lock_points` with lambda (glozow)
c7cd98c717 document and clean up MaybeUpdateMempoolForReorg (glozow)
Pull request description:
followup to #23683, addressing https://github.com/bitcoin/bitcoin/pull/23683#issuecomment-989741186
ACKs for top commit:
hebasto:
ACK e177fcab38, I have reviewed the code and it looks OK, I agree it can be merged.
instagibbs:
ACK e177fcab38
MarcoFalke:
Approach ACK e177fcab38😶
Tree-SHA512: 8c2709dd5cab73cde41f3e5655d5f237bacfb341f78eac026169be579528695ca628c8777b7d89760d8677a4e6786913293681cfe16ab702b30c909703e1824c
-BEGIN VERIFY SCRIPT-
s() { sed -i 's/cs_mapLocalHost/g_maplocalhost_mutex/g' $1; }
s src/net.cpp
s src/net.h
s src/rpc/net.cpp
s src/test/net_tests.cpp
-END VERIFY SCRIPT-
No behavior change.
This code was introduced in 5add7a7 before we required C++11, which is
why the struct was needed. As we are now using more modern C++ and this
is the only place where lockpoints are updated for mempool entries, it
is more idiomatic to call `modify` with a lambda.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Behavior change: don't quit right after LimitMempoolSize() when a
package is partially submitted. We should still send
TransactionAddedToMempool notifications for
transactions that were submitted.
Not behavior change: add a new package validation result for mempool logic errors.
The previous interface required callers to guess that the tx had been
swapped and look up the tx again by txid to find a `MEMPOOL_ENTRY`
result. This is a confusing interface.
Instead, explicitly tell the caller that this transaction was
`DIFFERENT_WITNESS` in the result linked to the mempool entry's wtxid.
This gives the caller all the information they need in 1 lookup, and
they can query the mempool for the other transaction if needed.
7f122a4188 fuzz: non-addrman fuzz tests: override-able check ratio (Vasil Dimov)
3bd83e273d fuzz: addrman fuzz tests: override-able check ratio (Vasil Dimov)
46b0fe7829 test: non-addrman unit tests: override-able check ratio (Vasil Dimov)
81e4d54d3a test: addrman unit tests: override-able check ratio (Vasil Dimov)
6dff6214be bench: put addrman check ratio in a variable (Vasil Dimov)
6f7c7567c5 fuzz: parse the command line arguments in fuzz tests (Vasil Dimov)
92a0f7e58d test: parse the command line arguments in unit tests (Vasil Dimov)
Pull request description:
Previously command line arguments passed to unit and fuzz tests would be ignored by the tests themselves. They would be used by the boost test framework (e.g. `--run_test="addrman_tests/*"`) or by the fuzzer (e.g. `-runs=1`). However both provide ways to pass down the extra arguments to the test itself. Use that, parse the arguments and make them available to the tests via `gArgs`.
This makes the tests more flexible as they can be run with any bitcoind config option specified on the command line.
When creating `AddrMan` objects in tests, use `-checkaddrman=` (if provided) instead of hardcoding the check ratio in many different places. See https://github.com/bitcoin/bitcoin/pull/20233#issuecomment-889813074 for further motivation for this.
ACKs for top commit:
mzumsande:
re-ACK 7f122a4188
josibake:
reACK 7f122a4188
Tree-SHA512: 3a05e61e4d70a0569bb67594bcce3aad8fdef63cdcc54e2823a3bc9f18679571985004412b6c332a210f67849bab32d8467b4115fbff8f5fac9834982e60dcf3
5574e6ed52 refactor: replace RecursiveMutex `m_callbacks_mutex` with Mutex (Sebastian Falbesoner)
3aa258109e scripted-diff: rename `m_cs_callbacks_pending` -> `m_callbacks_mutex` (Sebastian Falbesoner)
Pull request description:
This PR is related to #19303 and gets rid of the RecursiveMutex `m_cs_callbacks_pending`. All of the critical sections (6 in total) only directly access the guarded elements, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex:
807169e10b/src/scheduler.cpp (L138-L145)807169e10b/src/scheduler.cpp (L152-L160)807169e10b/src/scheduler.cpp (L169-L172)807169e10b/src/scheduler.cpp (L184-L187)807169e10b/src/scheduler.cpp (L197-L199)807169e10b/src/scheduler.cpp (L203-L206)
Also, it is renamed to adapt to the (unwritten) naming convention to use the `_mutex` suffix rather than the `cs_` prefix.
ACKs for top commit:
hebasto:
ACK 5574e6ed52, I have reviewed the code and it looks OK, I agree it can be merged.
w0xlt:
crACK 5574e6e
Tree-SHA512: ba4b151d956582f4c7183a1d51702b269158fc5e2902c51e6a242aaeb1c72cfcdf398f9ffa42e3072f5aba21a8c493086a5fe7c450c271322da69bd54c37ed1f
30927cb530 refactor: replace RecursiveMutex `m_subver_mutex` with Mutex (Sebastian Falbesoner)
0639aba42a scripted-diff: rename `cs_SubVer` -> `m_subver_mutex` (Sebastian Falbesoner)
Pull request description:
This PR is related to #19303 and gets rid of the RecursiveMutex `cs_SubVer`. Both of the critical sections only directly access the guarded variable, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex.
ACKs for top commit:
hebasto:
ACK 30927cb530, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2faead792ea0b2f79f9f7fe99acde5cf2bfcd2f15c51fbb6cb1099d4f81276001a492f7d46a5139055f4366c2d58a36a8ba19f21d56df20e0ed93af3141dbe11
fa99e108e7 Fix implicit-integer-sign-change in arith_uint256 (MarcoFalke)
Pull request description:
This refactor doesn't change behaviour, but clarifies that the numbers being dealt with aren't supposed to be negative. This helps when reading the code and allows to remove a sanitizer suppression for the whole file.
ACKs for top commit:
PastaPastaPasta:
utACK fa99e108e7
shaavan:
ACK fa99e108e7
Tree-SHA512: f227e2fd22021e39f0445ec041f4a299d13477c23cef0fc06c53fb3313cbe550cec329336224a7e8775d9045b8009423052b394e83d42a1e40772085dfcdd471
fac22fd36b log: Remove GetAdjustedTime from IBD header progress estimation (MarcoFalke)
Pull request description:
This is a "refactor" that shouldn't change behaviour, because the two times are most likely equal. A minimum of 5 outbound peers are needed to adjust the time. And if the time is adjusted, it will be by at most 70 minutes (`DEFAULT_MAX_TIME_ADJUSTMENT`). Thus, the progress estimate should differ by at most 7 blocks.
ACKs for top commit:
laanwj:
Code review ACK fac22fd36b
vincenzopalazzo:
ACK fac22fd36b
Tree-SHA512: bf9f5eef66db0110dd268cf6dbfab64b9c11ba776924f5b386ceae3f2d005272cceb87ebcc96e0c8b854c051514854a2a5af39ae43bad008fac685b5aafaabd0
fa7238300c fuzz: Limit fuzzed time to years 2000-2100 (MarcoFalke)
Pull request description:
It doesn't make sense to fuzz times in the past, as Bitcoin Core will refuse to start in the past.
Fix that and also remove a sanitizer suppression, which would be hit in net_processing in `ProcessMessage`:
```cpp
if (addr.nTime <= 100000000 || addr.nTime > nNow + 10 * 60)
addr.nTime = nNow - 5 * 24 * 60 * 60; // <-- Here
```
This changes the format of fuzz inputs. Previously a time value was (de)serialized as 40 bytes, now it is 32 bytes.
ACKs for top commit:
mzumsande:
Code Review ACK fa7238300c
Tree-SHA512: ca6e7233beec2d9ef9fd481d8f1331942a4d2c8fe518b857629bebcc53a4f42ae123b994cf5d359384a0a8022098ff5a9c146600bc2593c6d88734e25bc240ad
In each of the critical sections, only the the guarded variable is
accessed, without any chance that within one section another one is
called. Hence, we can use an ordinary Mutex instead of RecursiveMutex.
-BEGIN VERIFY SCRIPT-
sed -i 's/std::string m_command;/std::string m_type;/g' ./src/net.h
sed -i 's/* command and size./* type and size./g' ./src/net.h
sed -i 's/msg.m_command/msg.m_type/g' ./src/net.cpp ./src/net_processing.cpp ./src/test/fuzz/p2p_transport_serialization.cpp
-END VERIFY SCRIPT-
On a period boundary, getdeploymentinfo (and previously getblockchaininfo)
would report the status and statistics for the next block rather than
the current block. Change this to always report the status/statistics
of the current block, but add status-next to report the status for the
next block.
In each of the critical sections, only the the guarded variables are
accessed, without any chance that within one section another one is
called. Hence, we can use an ordinary Mutex instead of RecursiveMutex.
c62d763fc3 Necessary improvements to make configure work without libevent installed (Perlover)
091ccc38c2 The evhttp_connection_get_peer function from libevent changes the type of the second parameter. Fixing the problem. (Perlover)
Pull request description:
The second parameter of evhttp_connection_get_peer in libevent already has type as `const char **`
The compilation of bitcoind with the fresh libevent occurs errors
Details: https://github.com/bitcoin/bitcoin/issues/23606
ACKs for top commit:
laanwj:
Code review ACK c62d763fc3
luke-jr:
tACK c62d763fc3
Tree-SHA512: d1c8062d90bd0d55c582dae2c3a7e5ee1b6c7ca872bf4aa7fe6f45a52ac4a8f59464215759d961f8efde0efbeeade31b08daf9387d7d50d7622baa1c06992d83
This distribution is used for more than just the next inv send, so make
the name more generic.
Also rename to "exponential" to avoid the confusion that this is a
poisson distribution.
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1" ./src) ; }
ren PoissonNextSend GetExponentialRand
ren "a poisson timer" "an exponential timer"
-END VERIFY SCRIPT-
b5c9bb5cb9 util: Restore GetIntArg saturating behavior (James O'Beirne)
Pull request description:
The new locale-independent atoi64 method introduced in #20452 parses large integer values higher than maximum representable value as 0 instead of the maximum value, which breaks backwards compatibility. This commit restores compatibility and adds test coverage for this case in terms of the related GetIntArg and strtoll functions.
Specifically, command line or bitcoin.conf integer values greater than `9223372036854775807` (`2**63-1`) used to be parsed as `9223372036854775807` before #20452. Then #20452 caused them to be parsed as `0`. And after this PR they will be parsed as `9223372036854775807` again.
This change is a stripped-down alternative version of #23841 by jamesob
ACKs for top commit:
jamesob:
Github ACK b5c9bb5cb9
vincenzopalazzo:
ACK b5c9bb5cb9
MarcoFalke:
review ACK b5c9bb5cb9🌘
Tree-SHA512: 4e8abdbabf3cf4713cf5a7c5169539159f359ab4109a4e7e644cc2e9b2b0c3c532fad9f6b772daf015e1c5340ce59280cd9a41f2730afda6099cbf636b7d23ae
3c4f5d2a20 scripted-diff: Rename functions to drop mention of boost (Hennadii Stepanov)
Pull request description:
Nothing specific to Boost is in `GUIUtil::qstringToBoostPath` and `GUIUtil::boostPathToQString` functions.
Related to bitcoin/bitcoin#20744.
ACKs for top commit:
laanwj:
Concept and code review ACK 3c4f5d2a20
promag:
Code review ACK 3c4f5d2a20👋
Tree-SHA512: 0c8790979783a067811f7699b4ce4c204f6e0818d5f32986ec24b2c71583b4496d7a0e0c0361dd77c7e641a75d983fee35cd51ef722bbca9a5f13194efb3b4c0
The new locale-independent atoi64 method introduced in #20452 parses
large integer values higher than maximum representable value as 0
instead of the maximum value, which breaks backwards compatibility.
This commit restores compatibility and adds test coverage for this case
in terms of the related GetIntArg and strtoll functions.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
Make it possible to override from the command line (without recompiling)
the addrman check ratio in non-addrman fuzz tests (connman and
deserialize) instead of hardcoding it to 0:
```
FUZZ=connman ./src/test/fuzz/fuzz --checkaddrman=5
```
Make it possible to override from the command line (without recompiling)
the addrman check ratio in addrman fuzz tests instead of hardcoding it
to 0:
```
FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5
```
Make it possible to override from the command line (without recompiling)
the addrman check ratio in the common `TestingSetup::m_node::addrman`
(used by all unit tests) instead of hardcoding it to 0:
```
test_bitcoin --run_test="transaction_tests/tx_valid" -- -checkaddrman=1
```
In addrman unit tests, make it possible to override the check ratio from
the command line, without recompiling:
```
test_bitcoin --run_test="addrman_tests/*" -- -checkaddrman=1
```
Also, make the arguments of the constructor of `AddrManTest` the
same as the arguments of `AddrMan`.
Retrieve the command line arguments from the fuzzer and save them for
later retrieval by `BasicTestingSetup` so that we gain extra flexibility
of passing any config options on the test command line, e.g.:
```
FUZZ=addrman ./src/test/fuzz/fuzz --checkaddrman=5
```
A fuzz test should call `MakeNoLogFileContext<>()` in its initialize
function in order to invoke the constructor of `BasicTestingSetup`,
which sets `gArgs`.
Retrieve the command line arguments from boost and pass them to
`BasicTestingSetup` so that we gain extra flexibility of passing any
config options on the test command line, e.g.:
```
test_bitcoin -- -printtoconsole=1 -checkaddrman=5
```
BlockManager is a large data structure, and cs_main is not required to
take its address or access every part of it. Individual BlockManager
fields and methods which do require cs_main like m_block_index and
LookupBlockIndex are already annotated separately, and these other
annotations describe locking requirements more accurately and do a
better job enforcing thread safety.
Since cs_main is not needed to access the address of the m_block object,
this commit drops cs_main LOCK calls which were added pointlessly to
satisfy this annotation in the past.
Co-authored-by: Carl Dong <contact@carldong.me>
e5b6aef612 Move CBlockFileInfo::ToString method where class is declared (Russell Yanofsky)
f7086fd8ff Add src/wallet/* code to wallet:: namespace (Russell Yanofsky)
90fc8b089d Add src/node/* code to node:: namespace (Russell Yanofsky)
Pull request description:
There are no code changes, this is just adding `namespace` and `using` declarations and `node::` or `wallet::` qualifiers in some places.
Motivations for this change are:
- To make it easier to see when node and wallet code is being accessed places where it shouldn't be. For example if GUI code is accessing node and wallet internals or if wallet and node code are referencing each other.
- To make source code organization clearer ([#15732](https://github.com/bitcoin/bitcoin/issues/15732)), being able to know that `wallet::` code is in `src/wallet/`, `node::` code is in `src/node/`, `init::` code is in `src/init/`, `util::` code is in `src/util/`, etc.
Reviewing with `git log -p -n1 -U0 --word-diff-regex=.` can be helpful to verify this is only updating declarations, not changing code.
ACKs for top commit:
achow101:
ACK e5b6aef612
MarcoFalke:
Concept ACK e5b6aef612🍨
Tree-SHA512: 3797745c90246794e2d55a2ee6e8b0ad5c811e4e03a242d3fdfeb68032f8787f0d48ed4097f6b7730f540220c0af99ef423cd9dbe7f76b2ec12e769a757a2c8d
3a45dc36a6 Change type of `backup_file` parameter in RestoreWallet/restoreWallet (Hennadii Stepanov)
213172c734 refactor: Block unsafe std::string fs::path conversion copy_file calls (Hennadii Stepanov)
Pull request description:
This PR is an optional prerequisite for bitcoin/bitcoin#20744 "Use std::filesystem. Remove Boost Filesystem & System" which:
- makes further code changes safer
- prevents [some](https://cirrus-ci.com/task/6525835388649472) test failures on native Windows
ACKs for top commit:
ryanofsky:
Code review ACK 3a45dc36a6. Looks great! Thanks for debugging and fixing this and making #20744 smaller!
Tree-SHA512: c6dfaef6b45b9c268bc9ee9b943b9d679152c9d565ca4f86da8d33f8eb9b3cdbe9ba6df7b7578eacc0d00db6551048beff97419f86eb4b1d3182c43e2b4eb9a5
`fs::path` looks more native than `std::string` for a parameter which
represents a backup file. This change eliminates back-and-forth type
conversions.
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding
copy_file calls that will be unsafe after the transition to
std::filesystem to due lack of a boost::filesystem::path::imbue
equivalent and inability to set a predictable locale.
fa8e01a5b8 doc: Remove outdated scriptChange TODO comment (MarcoFalke)
Pull request description:
This was added in commit bf798734db (raw multisig). Raw multisig isn't a thing, so remove the TODO.
ACKs for top commit:
S3RK:
ACK fa8e01a5b8
achow101:
ACK fa8e01a5b8
Tree-SHA512: 01d521ca3605ab130c43531da4922ea85461ca5e7436267a34fb5df348009e086b3c66d85532c62255d9a0ba43db56424884808e773d0ef0177035dfb25d6a6c
efab28b06b Add FastRange32 function and use it throughout the codebase (Pieter Wuille)
96ecd6fa3e scripted-diff: rename MapIntoRange to FastRange64 (Pieter Wuille)
c6d15c45d9 [moveonly] Move MapIntoRange() to separate util/fastrange.h (Pieter Wuille)
Pull request description:
Several places in the codebase use the fast range mapping technique described in https://lemire.me/blog/2016/06/27/a-fast-alternative-to-the-modulo-reduction/, some for 32-bit ranges, some for 64-bit ones.
Move all of these to `util/fastrange.h`, and give them a consistent name.
ACKs for top commit:
Sjors:
ACK efab28b06b
shaavan:
reACK efab28b06b
MarcoFalke:
review ACK efab28b06b🍸
Tree-SHA512: 3190a25ef21d17f0ab2afcd9b8d5a1813fdfac0d93996878017e84ff62eee412c823d6149ae8e92cfc3214458641e83ace4b022b4a0fe0679f78dbaee21c6227
742918c8ef qt: hide Create Unsigned button behind an expert mode option (Andrew Chow)
5c3b800acd qt: Add Create Unsigned button to SendConfirmationDialog (Andrew Chow)
Pull request description:
Instead of having different buttons or changing button behavior for making a PSBT, just have SendConfirmationDialog return whether the user wants a PSBT or a broadcasted transaction. Since this dialog is used by both the bumpFeeAction and the SendCoinsDialog, changes to both to support the different behavior is needed. They will check the return value of the SendConfirmationDialog for whether a PSBT needs to be created instead of checking whether private keys are disabled.
Strings used in this dialog are being slightly modified to work with both private keys enabled and disabled wallets.
Moved from https://github.com/bitcoin/bitcoin/pull/18789
ACKs for top commit:
jarolrod:
ACK 742918c
ryanofsky:
Code review ACK 742918c8ef. Just suggested changes since last review. Looks great!
hebasto:
ACK 742918c8ef, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: dd29f4364c7b4f15befe8fe63257b26187918786b005e0f8336183270b1a162680b93f6ced60f0285c6e607c084cc0d24950fc68a8f9c056521ede614041be66
The new helper function, BlockManager::WriteBlockIndexDB(),
has a thread safety lock annotation in its declaration but is
missing the corresponding run-time lock assertion in its definition.
Per doc/developer-notes.md: "Combine annotations in function
declarations with run-time asserts in function definitions."
fa68a6c2fc scripted-diff: Rename touched member variables (MarcoFalke)
facd3df21f Make blockstorage globals private members of BlockManager (MarcoFalke)
faa8c2d7d7 doc: Clarify nPruneAfterHeight for signet (MarcoFalke)
fad381b2f8 test: Load genesis block to allow flush (MarcoFalke)
fab262174b Move blockstorage-related unload to BlockManager::Unload (MarcoFalke)
fa467f3913 move-only: Create WriteBlockIndexDB helper (MarcoFalke)
fa88cfd3f9 Move functions to BlockManager (MarcoFalke)
Pull request description:
Globals aren't too nice because they hide dependencies, also they make testing harder.
Fix that by removing some.
ACKs for top commit:
Sjors:
ACK fa68a6c2fc
ryanofsky:
Code review ACK fa68a6c2fc. Nice changes!
Tree-SHA512: 6abc5929a5e43a05e238276721d46a64a44f23dca18c2caa9775437a32351d6815d88b88757254686421531d0df13861bbd3a202e13a3192798d87a96abef65d
172096e9dd scripted-diff: Rename libbitcoin_server.a to libbitcoin_node.a (Russell Yanofsky)
Pull request description:
Goal along with namespacing PR #23497 is to make code organization more obvious and have `src/node/` code in `node::` namespace in `libbitcoin_node.a` library
ACKs for top commit:
MarcoFalke:
cr ACK 172096e9dd
Tree-SHA512: a2e787eeaa3ab769b0f5376473072cae584d237aa8b67b677bea833bb36b0134a0eca17eb01722389639473b8463f4953bc3a5e4801a6b2c8965ac1075cba005
e3544c864e init: Use clang-tidy named args syntax (Carl Dong)
3401630417 style-only: Rename *Chainstate return values (Carl Dong)
1dd582782d docs: Make LoadChainstate comment more accurate (Carl Dong)
6b83576388 node/chainstate: Use MAX_FUTURE_BLOCK_TIME (Carl Dong)
Pull request description:
There are 2 proposed fixups in discussions in #23280 which I have not implemented:
1. An overhaul to return types and an option type for the two `*Chainstate` functions: https://github.com/bitcoin/bitcoin/pull/23280#issuecomment-984149564
- The change reintroduces stringy return types and is quite involved. It could be discussed in a separate PR.
2. Passing in the unix time to `VerifyChainstate` instead of a callback to get the time: https://github.com/bitcoin/bitcoin/pull/23280#discussion_r765051533
- I'm not sure it matters much whether it's a callback or just the actual unix time. Also, I think `VerifyDB` can take quite a while, and I don't want to impose that the function have to "run quickly" in order to have it be correct.
If reviewers feel strongly about either of the two fixups listed above, please feel free to open a PR based on mine and I'll close this one!
ACKs for top commit:
ryanofsky:
Code review ACK e3544c864e
MarcoFalke:
ACK e3544c864e🐸
Tree-SHA512: dd1de0265b6785eef306e724b678ce03d7c54ea9f4b5ea0ccd7af59cce2ea3aba73fd4af0c15e2dca9265807dc4075f9afa2ec103672677b6638b1a4fc090904
fe86eb50c9 Refactor: Uses c++ init convention for time variables (Shashwat)
6111b0d7fa Refactor: Changes remaining time variable type from int to chrono (Shashwat)
Pull request description:
This PR is a follow-up to #23801.
This PR aims to make the following changes to all the time variables in **net_processing.cpp** wherever possible.
- Convert all time variables to `std::chrono.`
- Use `chorno::literals` wherever possible.
- Use `auto` keywords wherever possible.
- Use `var{val}` convention of initialization.
This PR also minimizes the number of times, serialization of time `count_seconds(..)` occurs.
ACKs for top commit:
MarcoFalke:
re-ACK fe86eb50c9 🏕
Tree-SHA512: c8684c0c60a11140027e36b6e9706a45ecdeae6b5ba0bf267e50655835daee5e5410e34096a8c4eca005f327caae1ac258cc7b8ba663eab58abf131f6d2f4d42
fa9f4554ca refactor: Remove pointless and confusing shift in RelayAddress (MarcoFalke)
Pull request description:
The second argument written to the siphash is already quantized to 24 hours, so it seems confusing to quantize the first argument to 32 bits (out of 64 bits).
> The shifting is pointless, we should get rid of it. It seems to be a silly evolution of this 2010 Satoshi code: 5cbf753 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones).
(Copied from https://github.com/bitcoin/bitcoin/pull/18642#issuecomment-613773120)
(The original code was `uint256 hashRand = hashSalt ^ (((int64)addr.ip)<<32) ^ ((GetTime()+addr.ip)/(24*60*60));`)
This also allows to remove a integer sanitizer suppression for the whole file.
ACKs for top commit:
laanwj:
Code review ACK fa9f4554ca
sipa:
utACK fa9f4554ca
promag:
Code review ACK fa9f4554ca.
Tree-SHA512: f5fd107464ccd839d6749aed6914b4935e39ab42906546b3f3810a7339fc4633fef931a1783a287572af5ec64525626fa91d147d8ff52eb076740465bf5cf839
ac617cc141 wallettool: Check that the dumpfile checksum is the correct size (Andrew Chow)
Pull request description:
After parsing the checksum, make sure that it is the size that we expect it to be.
This issue was reported by Pedro Baptista.
ACKs for top commit:
laanwj:
Code review ACK ac617cc141
Tree-SHA512: 8135b3fb1f4f6b6c91cfbac7d1d3421f1f6c664a742c92940f68eae857f92ce49d042cc3aa5c2df6ef182825271483d65efc7543ec7a8ff047fd7c08666c8899
8bd34dc774 test: check that bitcoin-tx detects missing input amount for segwit transactions (Sebastian Falbesoner)
c337b27d7c Require that input amount is provided for bitcoin-tx witness transactions (Ben Woosley)
Pull request description:
This PR picks up the obviously abandoned PR #13608 (last activity was three and a half years ago) by rebasing it on master and adding missing tests. Original PR description: "_Applies fix from #12458 / #13547 to bitcoin-tx._"
The private key is the compressed version of the one used in most other util tests (5HpHagT65TZzG1PH3CSu63k8DbpvD8s5ip4nEB3kEsreAnchuDf, corresponds to the scalar value k=1 in big endian), since segwit signing refuses uncompressed keys.
The error message from the picked up PR is changed to not include the amount, as showing any value would be just confusing.
ACKs for top commit:
josibake:
ACK 8bd34dc774
Tree-SHA512: 334b418f89527363ad7e3326b4126e86a05fd64876c49a8280de38e64cfac52cb62c4b24b83603dd68b6bcebbe57c64161832edffb1cac7e9c68426f6b6eae1f
This is needed to turn globals into member variables. Otherwise, this
will lead to issues:
runtime error: reference binding to null pointer of type 'CBlockFileInfo'
#0 in std::vector<CBlockFileInfo, std::allocator<CBlockFileInfo> >::operator[](unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:1046:2
#1 in BlockManager::FlushBlockFile(bool, bool) src/node/blockstorage.cpp:540:47
#2 in CChainState::FlushStateToDisk(BlockValidationState&, FlushStateMode, int) src/validation.cpp:2262:28
#3 in CChainState::ResizeCoinsCaches(unsigned long, unsigned long) src/validation.cpp:4414:15
#4 in validation_chainstate_tests::validation_chainstate_resize_caches::test_method() src/test/validation_chainstate_tests.cpp:66:12
This is a refactor and safe to do because:
* UnloadBlockIndex calls ChainstateManager::Unload, which calls
BlockManager::Unload
* Only unit tests call Unload directly
fab16415ba doc: Fix typo in getmempoolinfo (MarcoFalke)
Pull request description:
Also, remove whitespace. Can be reviewed with `--ignore-all-space --word-diff-regex=.`.
ACKs for top commit:
laanwj:
Good catch. ACK fab16415ba
shaavan:
ACK fab16415ba
Tree-SHA512: 9d51ef4a4eccfcf437c99a9f84f48e4f090d75715332ad2b4cf10ad77c3691de03255b4817e9fd203fad9baf338066982304dc62fab93dd605e735943f8ca346
6bf6e9fd9d net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85a net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)
Pull request description:
_This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._
Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.
ACKs for top commit:
laanwj:
Code review ACK 6bf6e9fd9d
jamesob:
ACK 6bf6e9fd9d ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
jonatack:
ACK 6bf6e9fd9d per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
w0xlt:
tACK 6bf6e9f
Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
fa996c58e8 refactor: Avoid integer overflow in ApplyStats when activating snapshot (MarcoFalke)
fac01888d1 Move AdditionOverflow to util, Add CheckedAdd with unit tests (MarcoFalke)
fa526d8fb6 Add dev doc to CCoinsStats::m_hash_type and make it const (MarcoFalke)
faff051560 style: Remove unused whitespace (MarcoFalke)
Pull request description:
A snapshot contains the utxo set, including the out value. To activate the snapshot, the hash needs to be calculated. As a side-effect, the total amount in the snapshot is calculated (as the sum of all out values), but never used. Instead of running into an integer overflow in an unused result, don't calculate the result in the first place.
Other code paths (using the active utxo set) can not run into an integer overflow, since the active utxo set is valid.
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39716
ACKs for top commit:
shaavan:
reACK fa996c58e8
vasild:
ACK fa996c58e8
Tree-SHA512: 4f207f634841f6f634fd02ae1e5907e343fd767524fd0e8149aa99fa9a1834fe50167d14874834d45236e9c325d567925f28129bacb7d80be29cf22277a16a14
faa51a6aa9 doc: Mark proprietary array optional (MarcoFalke)
Pull request description:
The global one is returned all the time, but the input/output array is returned optionally
ACKs for top commit:
fanquake:
ACK faa51a6aa9
Tree-SHA512: db987c13d59e0ccc633032707438d506fe4f8fbf7569a03b99d899cb1309de94f99c343840107fc51a9f904bcf55e00049808b6cdf732fc16c6e9e818b480936
ea4c9fd4ab test: Cover eviction by timeout in addrman_evictionworks (Martin Zumsande)
4f1bb467b5 test: Add test for multiplicity in addrman new tables (Martin Zumsande)
e880bb7836 test: Add test for updating addrman entries (Martin Zumsande)
f02eee8c87 test: introduce utility function to retrieve an addrman (Martin Zumsande)
f0e5efb824 test: Remove unused AddrManTest class (Martin Zumsande)
b696d7870b test: Remove tests for internal helper functions (Martin Zumsande)
0538520091 test: use AddrMan instead of AddrManTest where possible (Martin Zumsande)
1c65d427bb test: Inline SimConnFail function (Martin Zumsande)
5b7aac34f2 test: delete unused GetBucketAndEntry function (Amiti Uttarwar)
2ba1e74e59 test: Update addrman_serialization unit test to use AddrMan's interface (Amiti Uttarwar)
dad5f76021 addrman: Introduce a test-only function to lookup addresses (Amiti Uttarwar)
Pull request description:
This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public `AddrMan` interface:
This has the advantage that the tests are less implementation-dependent, i.e. it would be possible to rewrite the internal addrman implementation (as drafted [here](https://github.com/sipa/bitcoin/tree/202106_multiindex_addrman) for using a multiindex) without having to adjust the tests.
This includes the following steps:
* Adding a test-only function `FindAddressEntry()` to the public addrman interface which returns info about an address in addrman (e.g. bucket, position, whethe the address is in new or tried). Obviously we want to do this sparingly, but I think a single test-only function is ok (which could also be useful elsewhere, e.g. in fuzz tests).
* Removal of the `AddrManTest` subclass which would reach into AddrMan's internals, using `AddrMan` instead
* Removal of tests for internal helper functions that are not publicly exposed (these are still tested indirectly via the public functions calling them).
* Additional tests for previously untested features such as multiplicity in the new tables, that can be tested with the help of `FindAddressEntry()`.
All in all, this PR increases the unit test coverage of AddrMan by a bit.
ACKs for top commit:
jnewbery:
ACK ea4c9fd4ab
josibake:
reACK ea4c9fd4ab
Tree-SHA512: c2d4ec8bdc62ffd6055ddcd37dea85ec08c76889e9e417e8d7c62a96cf68a8bcbe8c67bec3344d91fa7d3c499f6d9f810962da1dddd38e70966186b10b8ab447
fa5bb37611 rpc: Use EnsureAnyArgsman in rpc/blockchain (MarcoFalke)
fa98b6f644 rpc: Add EnsureArgsman helper (MarcoFalke)
Pull request description:
This refactor doesn't change anything for the rpc layer, but it helps a bit with removing gArgs (See #21005)
ACKs for top commit:
shaavan:
Code Review ACK fa5bb37611
Tree-SHA512: 18f9cc90830a168acb94452b541b6e97ba9a50a0f4834698a16994c3884c833dc606e36a5538a16352e5e5cc43d3ac77cb498f877f8f5bc5dd52fa84f8bf2056
059f88b6a9 Add RPC help for getblock verbosity level 3 (Kiminuo)
1bdd5f6322 Address review comments from #22918 (Kiminuo)
Pull request description:
This is a follow-up PR to #22918 which addresses review comments (first commit). The second commit adds missing RPC help for verbosity level 3.
ACKs for top commit:
pg156:
ACK 059f88b6a9
laanwj:
re-ACK 059f88b6a9
Tree-SHA512: f27d53ac34b93a304ef5668701ed2b5c986a926bc8ad0df4de89695fc9e1df26acb008611451319ea897658acd9c56c6a0555d60359960c9cd28238ebefa2d50
This commit does following changes to time variables in net_processing.h:
- Used {} initialization.
- Uses universal initializer auto.
- Uses chrono::literals
The reason for these changes is to make code simpler, and easier to
understand and rationalize.
fa7efc915b Fixup style of moved code (MarcoFalke)
fade2a44f4 Move BlockManager to node/blockstorage (MarcoFalke)
Pull request description:
`BlockManager` is responsible for reading and writing block(headers). So move it to the existing `blockstorage` module in `node`. Also, move validation code unrelated to block-storage out from `BlockManager`.
ACKs for top commit:
ryanofsky:
Code review obvious ACK fa7efc915b
Tree-SHA512: 0197943d818e5f59e743b07fbb92e7661bff90081127a41e35e5692ce49d6f6a7872448670b0da282f7714580a45c8d93e571a67177c8b5f785ce9edefe834c5
29e1794ba5 build, qt: No need to set inapplicable QPA backend for Android (Hennadii Stepanov)
Pull request description:
The current workflow looks weird. At first, the inapplicable `xcb` QPA backend is set in Qt `configure` options. Then the correct `android` QPA backend is forced via the `QT_QPA_PLATFORM` environment variable.
Using the default QPA backend, which is `android` for Android devices, is just fine.
ACKs for top commit:
icota:
re-tACK 29e1794ba5
Tree-SHA512: 08ed7d05209c1bedc1a71de5ea3be5d86b40319a164dceb9191f7a4dfe78f2f951778b90421335e73e71a798a57bdf046aa96876762d338b600037bd7ee27b52
b4adc5ad67 [bugfix] update lockpoints correctly during reorg (glozow)
b6002b07a3 MOVEONLY: update_lock_points to txmempool.h (glozow)
Pull request description:
I introduced a bug in #22677 (sorry! 😅)
Mempool entries cache `LockPoints`, containing the first height/blockhash/`CBlockIndex*` at which the transaction becomes valid. During a reorg, we re-check timelocks on all mempool entries using `CheckSequenceLocks(useExistingLockPoints=false)` and remove any now-invalid entries. `CheckSequenceLocks()` also mutates the `LockPoints` passed in, and we update valid entries' `LockPoints` using `update_lock_points`. Thus, `update_lock_points(lp)` needs to be called right after `CheckSequenceLocks(lp)`, otherwise we lose the data in `lp`. I incorrectly assumed they could be called in separate loops.
The incorrect behavior introduced is: if we have a reorg in which a timelocked mempool transaction is still valid but becomes valid at a different block, the cached `LockPoints` will be incorrect.
This PR fixes the bug, adds a test, and adds an assertion at the end of `removeForReorg()` to check that all mempool entries' lockpoints are valid. You can reproduce the bug by running the test added in the [test] commit on the code before the [bugfix] commit.
ACKs for top commit:
jnewbery:
ACK b4adc5ad67
vasild:
ACK b4adc5ad67
mzumsande:
Code Review ACK b4adc5ad67
hebasto:
ACK b4adc5ad67
MarcoFalke:
re-ACK b4adc5ad67🏁
Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401
fadd73037e refactor: Remove implicit-integer-sign-change suppressions in validation.cpp (MarcoFalke)
Pull request description:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
ACKs for top commit:
shaavan:
ACK fadd73037e
theStack:
Code-review ACK fadd73037e
Tree-SHA512: a8a05613be35382b92d7970f958a4e8f4332432056eaa9d72f6719495134b93aaaeea692899d9035654d0e0cf56bcd759671eeeacfd0535582c0ea048ab58a56
4523d28b6b [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge)
3a2464f216 [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge)
064abd14a5 [rest] add a more verbose error message for invalid header counts (Niklas Gögge)
83b8f3a896 [refactor] various style fix-ups (Niklas Gögge)
Pull request description:
This PR addresses unresolved review comments from [#17631](https://github.com/bitcoin/bitcoin/pull/17631).
This includes:
* various style fix-ups
* returning a more verbose error message for invalid header counts
* removing superfluous rpc serializations flags for block filters
* improving the test to include comparing the block filters returned from the rest with the ones returned from the `getblockfilter` RPC.
ACKs for top commit:
jnewbery:
ACK 4523d28b6b
brunoerg:
tACK 4523d28b6b
Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c
fa1a51cbc1 doc: testnet3 was not reset and is doing BIP30 checks again (MarcoFalke)
Pull request description:
ACKs for top commit:
theStack:
ACK fa1a51cbc1
Tree-SHA512: 793eccda583a3edb056b142c36a09a5c867f61d90b96e15e6643417d62eb651eb2f3429c5f245bdb062d18ab9bb05b5048c0888aa5a492cb7bb21a2f3f52324e
The logic of these functions is already covered by existing unit tests
using publicly exposed functions of the interface.
Therefore, removing them does not decrease test coverage.
Switches to AddrMan for tests that use no features of AddrManTest.
Also removes unusued AddrManTest variables
Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>
fa50d8b66e doc: Remove TODO comment in tx_verify (MarcoFalke)
Pull request description:
The comment has no clear motivation, so it seems better to remove it and fix it when there is a reason.
An alternative (if a fix isn't possible when there is a clear motivation) would be to create an issue thread for easier discussion.
ACKs for top commit:
fanquake:
ACK fa50d8b66e
Tree-SHA512: e9c25bab46a73b7c2db288c62ed9838a5e794b3b72db494173f4502da60b58dec4383064964c0842932cd30e4251fc01ad0c28681e2ef6cb442482eea2bad595
fa993d0e7e doc: Fix -changetype help text (MarcoFalke)
Pull request description:
This was forgotten in commit 3ac38058ce
ACKs for top commit:
shaavan:
ACK fa993d0e7e
w0xlt:
ACK fa993d0
josibake:
ACK fa993d0e7e
Tree-SHA512: 9f84b1168e3b3ab06e5c1f4915a1874598b273099eb5878ed28c3a66f1484e34c836fd3c68c4131bee541f3428052f6b66e02b192170752d1082de689d44cd4d
fa562fdd5e doc: Remove fixed TODO from wallet/feebumper (MarcoFalke)
Pull request description:
Fixed in commit 9522b53a91
ACKs for top commit:
shaavan:
ACK fa562fdd5e
Tree-SHA512: 968fda0994020c369b7acfc01db109d0f50d4c137fadf533ae55d0e14a353ebbde4320e798cf89e5489f1020c459712631b3967976c1f73d99db8a2d1cbad982
7b481f015a Fix Racy ParseOpCode function initialization (Jeremy Rubin)
Pull request description:
If multiple callers call ParseOpCode concurrently it will cause a race condition. We can either move the static to it's own area and require init be called explicitly, or just allow concurrent first callers to race to fill in an atomic variable that never changes thereafter. The second approach is taken here.
Static initialization *is* threadsafe, but only insofar as definining the variable being guaranteed to be called once. This is used incorrectly here.
practicalswift --> are there tools we can deploy to catch usage like this?
ACKs for top commit:
MarcoFalke:
re-ACK 7b481f015a 🗣
Tree-SHA512: cbf9dc3af26d7335305026f32ce8472a018309b89b3d81a67357e59fbeed72c37b5b8a6e30325ea68145c3b2403867be82de01f22decefb6e6717cf0c0045633
ff5f6dea53 scripted-diff: Rename interfaces::WalletClient to interfaces::WalletLoader (Russell Yanofsky)
Pull request description:
Name has been confusing since it was introduced, and it was pointed in recent review club https://bitcoincore.reviews/10102 that it was particularly unclear how `interfaces::WalletClient` was different from `interfaces::Wallet`.
ACKs for top commit:
w0xlt:
ACK ff5f6de
Tree-SHA512: 26fa10baa457e76da1933adab187e9be61b8d76cff1cf2c73ad4320461c7e31fb9db07b7c2486998294826beb4a1aca255c14903920b443db6213e653c5f7e0a
826e12b010 test: call VerifyLoadedChainstate during ChainTestingSetup (James O'Beirne)
Pull request description:
for additional coverage and similarity to actual init process.
Followup to #23280.
ACKs for top commit:
dongcarl:
Code Review ACK 826e12b010
ryanofsky:
Code review ACK 826e12b010
Tree-SHA512: a4e7fd25e5d7a08b1e154ae6daf67c3048260a2684b0e569b544dd826693b7b969db9923b191e499cb8d8d0a2a73eb9330ff45909313145a9abb6052eb8c3ad9
Name has been confusing since it was introduced, and it was pointed in
recent review club as https://bitcoincore.reviews/10102 that it was
particularly unclear how interfaces::WalletClient was different from
interfaces::Wallet.
-BEGIN VERIFY SCRIPT-
ren() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
ren WalletClient WalletLoader
ren walletClient walletLoader
ren wallet_client wallet_loader
ren "wallet clients release the wallet" "wallet pointer owners release the wallet"
ren "wallet client" "wallet loader"
ren "Wallet client" "Wallet loader"
-END VERIFY SCRIPT-
a2a92317ad rpc: Add warning to user about newkeypool command (Samuel Dobson)
Pull request description:
This PR prevents `newkeypool` from being run on non-HD wallets, because this would require a new backup every time, so it isn't very safe.
David Harding also suggested [here](https://github.com/bitcoin/bitcoin/pull/23093#issuecomment-945350003) that the RPC help text should include a warning to the users about the interaction between newkeypool.
ACKs for top commit:
achow101:
ACK a2a92317ad
Tree-SHA512: 0aa497900f1d179764bce13ffce0bb081ba2ca354492bf2e04b21d0212e960b3ed13a386fbf65602b6b50461f4056a0285752ef707d312da28e82449cd8ea048
fada6c65d2 wallet: Strictly match tx change type to improve privacy (MarcoFalke)
Pull request description:
Currently the change type will only match a destination by accident, making it easier to determine the change.
Fix that by strictly matching one of the destinations.
ACKs for top commit:
S3RK:
Concept & Approach ACK fada6c6. Also did light code review .
achow101:
ACK fada6c65d2
prayank23:
tACK fada6c65d2
w0xlt:
tACK fada6c6
Tree-SHA512: 2b072c3c32debac7b0bef07a6df9a8f1a631e0f7d556b859973f18894ca490225582dc13e4588b29fa205ffbcd30fb632d5313b304d10ad17a26adc3f7684471
92082ea0bb Change time variable type to std::chrono::seconds in src/net_processing.cpp (Shashwat)
Pull request description:
- This is a follow-up to PR #23758
- This changes the remaining time variable in `net_processing.cpp` from **int64_t** to **std::chrono::seconds**
ACKs for top commit:
naumenkogs:
ACK 92082ea0bb
hebasto:
re-ACK 92082ea0bb
Tree-SHA512: 559e351d9046d4ba2b842ae38da13b4befc7feee71f0762f97907812471e2840b0d43c90c92222d15619fe40cc21f11d40900500ca07b470f7ac8b0046cc1d68
bf4f817135 refactor: addrman_select test (josibake)
5a64dc018c refactor: addrman_evictionworks test (josibake)
e281fccd8a refactor: addrman_noevict test (josibake)
8bdd9240d4 refactor: addrman_selecttriedcollisions test (josibake)
Pull request description:
As a follow-up to #23713 , this PR refactors the remaining tests in `src/tests/addrman_tests.cpp` to use the output from `AddrMan::Good()` where appropriate.
ACKs for top commit:
naumenkogs:
ACK bf4f817135
mzumsande:
Code Review ACK bf4f817135
Tree-SHA512: 93cc127aecff42c1c174daa04911af7e3460a5c40ddf96952fe4a6ab86fa1ff22d66724326abb709008d7f9f79c26c55c6d62753c40059c9ac60f869507ec913
2b64fa3251 Update REST docs with new accessors (Matt Corallo)
ef7c8228fd Expose block filters over REST. (Matt Corallo)
Pull request description:
This adds a new rest endpoint:
/rest/blockfilter/filtertype/requesttype/blockhash (eg
/rest/blockfilter/basic/header/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.hex)
which exposes either the filter "header" or the filter data itself.
Most of the code is cribbed from the equivalent RPC.
You can test it at 000000005b.hex
ACKs for top commit:
dergoegge:
ACK 2b64fa3251 - Adding blockfilters to the REST interface is analogous to serving other public data such as transactions or blocks.
Tree-SHA512: d487bc694266375c94d6fcf2e9d788a8a42a3b94e8d3290e46335a64cbcde55084ce5ea6119b79a4065888d94d7c3ae25a59a901fa46e3711f0eb296add12696
c44c20108f p2p, refactor: drop unused DNSLookupFn param in LookupSubnet() (Vasil Dimov)
f0c9e68080 p2p, refactor: tidy up LookupSubNet() (Jon Atack)
Pull request description:
This pull originally resolved a code `TO-DO`, as well as fixing different param names between the function declaration and definition, updating the function to current style standards, clearer variable naming, and improving the Doxygen documentation.
Following the merge of #17160, it now does the non-`TODO` changes and also now drops an unused param to simplify the function.
ACKs for top commit:
dunxen:
ACK c44c201
vasild:
ACK c44c20108f
shaavan:
crACK c44c20108f
Tree-SHA512: 55f64c7f403819dec84f4da06e63db50f7c0601a2d9a1ec196fda667c220ec6f5ad2a3c95e0e02275da9f6da6b984275d1dc10e19ed82005c5e13da5c5ecab02
edd0313ae7 test: Improve "invalid_command" subtest in system_tests for Windows (Hennadii Stepanov)
fb1b0590af test: Fix "non-zero exit code" subtest in system_tests for Windows (Hennadii Stepanov)
0aad33db64 test: Fix "false" subtest in system_tests for Windows (Hennadii Stepanov)
507c009c1e test: Fix "echo" subtest in the system_tests for Windows (Hennadii Stepanov)
Pull request description:
An attempt to fixbitcoin/bitcoin#23775.
With this PR on Windows 10 Pro 21H1 (build 19043.1348):
```
C:\Users\hebasto\bitcoin>src\test_bitcoin.exe --run_test=system_tests/run_command
Running 1 test case...
*** No errors detected
C:\Users\hebasto\bitcoin>src\test_bitcoin.exe
Running 482 test cases...
*** No errors detected
```
ACKs for top commit:
sipsorcery:
tACK edd0313ae7
Tru3Nrg:
tACK edd0313ae7
Tree-SHA512: 66a4f2372858011ff862b71c6530bedb8bc731b18595636fac9affc9189d9320f212c68b62498f2b57ee7a07f59e842dbec085b76a7419791d1a06c8e80e7744
8f79831ab5 Refactor the chacha20 differential fuzz test (stratospher)
Pull request description:
This PR addresses [comments from #22704](https://github.com/bitcoin/bitcoin/pull/22704/files#discussion_r771510963) to make the following changes in `src/test/fuzz/crypto_diff_fuzz_chacha20.cpp`:
- replace `memcmp()` with ==
- add a missing assert statement to compare the encrypted bytes
Top commit has no ACKs.
Tree-SHA512: 02338460fb3a89e732558bf00f3aebf8f04daba194e03ae0e3339bb2ff6ba35d06841452585b739047a29f8ec64f36b1b4ce2dfa39a08f6ad44a6a937e7b3acb
314195c8be Remove unnecessary cast in CKey::SignSchnorr (Pieter Wuille)
a1f76cdb22 Remove --disable-openssl-tests for libsecp256k1 configure (Pieter Wuille)
86dbc4d075 Squashed 'src/secp256k1/' changes from be8d9c262f..0559fc6e41 (Pieter Wuille)
Pull request description:
The motivation for this bump is getting rid of a cast in `CKey::SignSchnorr`; the `aux_rand` argument isn't modified by the `secp256k1_schnorrsig_sign` function, but was marked as non-`const` anyway. This is fixed now (bitcoin-core/secp256k1#966), and the cast is removed in this PR.
There are a few other relevant changes:
* (bitcoin-core/secp256k1#956): replaces a runtime-computed table with a precomputed one; this adds arouns 1 MiB to the binary size, but is a step towards significantly simplifying the API. If 1 MiB is too much, it can be reduced by 2 or 4 (or more) for a slight verification performance reduction.
* (bitcoin-core/secp256k1#983): removes (test/bench only) OpenSSL support entirely, removing the need to pass `--disable-openssl-tests` (see #23314).
* (bitcoin-core/secp256k1#810): mild performance increase for 64-bit non-x86 platforms.
* (bitcoin-core/secp256k1#1002): Make aux_rnd32==NULL behave identical to 0x0000..00 (which impacts BIP341/BIP342 signing in Bitcoin Core, making it more strictly BIP340 compliant, though not in a manner that affects security).
ACKs for top commit:
fanquake:
ACK 314195c8be - this includes a nice simplification to the lilbsecp build system (and thus our build system), and fixes issues like #22854. Did a Guix build on x86 (above), as well as a build on arm64 (except for the arm64 host):
Tree-SHA512: 0e048390fc148fbbdf5b98d9cce8c71067564e7d69d97b68347808a9bc45a04f4fc653c392c880d79d5d8b9cf282195520955581ac4f1595f6a948080cf5949d
fa1dc9b36a p2p: Always serialize local timestamp for version msg (MarcoFalke)
Pull request description:
Currently we serialize the local time when connecting to outbound connections and the "adjusted network" time when someone connects to us.
I presume the reason is to avoid a fingerprint in case the local time is misconfigured. However, the fingerprint still exits when:
* The local time goes out-of-sync after timedata is filled up, in which case the adjusted time is *not* adjusted. See comment in `src/timedata.cpp`. (In practise I expect no adjustment to happen after timedata is filled up by one entry more than half its size).
* The local time is off by more than 70 minutes. See `DEFAULT_MAX_TIME_ADJUSTMENT`. While there is a warning in this case, the warning might be missed by the node operator.
* The adjusted time is poisoned by an attacker. This is only a theoretical concern after commit e457513eb1.
Using the adjusted time does help in a the case where the local time is off by a constant less than 70 minutes and the node quickly connects to 5 outbound peers to retrieve the adjusted time.
Still, I think using `GetAdjustedTime` here gives a false sense of security. It will be better for node operators to instead set the correct time.
ACKs for top commit:
naumenkogs:
ACK fa1dc9b36a
laanwj:
Code review ACK fa1dc9b36a
w0xlt:
crACK fa1dc9b
Tree-SHA512: 70a0f4ab3500e6ddcde291620e35273018cefd1d9e94b91ad333e360139ed18862718bb1a9854af2bf79990bf74b05d95492f77d0747c7b9bdd276c020116dcb
4d0ac72f3a [fuzz] Add fuzzing harness to compare both implementations of ChaCha20 (stratospher)
65ef93203c [fuzz] Add D. J. Bernstein's implementation of ChaCha20 (stratospher)
Pull request description:
This PR compares Bitcoin Core's implementation of ChaCha20 with D. J. Bernstein's in order to find implementation discrepancies if any.
ACKs for top commit:
laanwj:
Code review ACK 4d0ac72f3a
Tree-SHA512: f826144b4db61b9cbdd7efaaca8fa9cbb899953065bc8a26820a566303b2ab6a17431e7c114635789f0a63fbe3b65cb0bf2ab85baf882803a5ee172af4881544
