To avoid issues with test data leaking across tests cases, the global
vCoins and testWallet are removed from coinselector_tests and all of the
relevant functions reworked to not need them.
6544ea5035 refactor: Block unsafe fs::path std::string conversion calls (Russell Yanofsky)
b39a477ec6 refactor: Add fs::PathToString, fs::PathFromString, u8string, u8path functions (Russell Yanofsky)
Pull request description:
The `fs::path` class has a `std::string` constructor which will implicitly convert from strings. Implicit conversions like this are not great in general because they can hide complexity and inefficiencies in the code, but this case is especially bad, because after the transition from `boost::filesystem` to `std::filesystem` in #20744 the behavior of this constructor on windows will be more complicated and can mangle path strings. The `fs::path` class also has a `.string()` method which is inverse of the constructor and has the same problems.
Fix this by replacing the unsafe method calls with `PathToString` and `PathFromString` function calls, and by forbidding unsafe method calls in the future.
ACKs for top commit:
kiminuo:
ACK 6544ea5035
laanwj:
Code review ACK 6544ea5035
hebasto:
re-ACK 6544ea5035, only added `fsbridge_stem` test case, updated comment, and rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22937#pullrequestreview-765503126) review. Verified with the following command:
Tree-SHA512: c36324740eb4ee55151146626166c00d5ccc4b6f3df777e75c112bcb4d1db436c1d9cc8c29a1e7fb96051457d317961ab42e6c380c3be2771d135771b2b49fa0
Presumably these stubs indicate to packagers that external leveldb is meant to
be supported in some way. It is not. Remove the stubs to avoid sending any
mixed messages.
6531599f42 test: Add check that newkeypool flushes change addresses too (Samuel Dobson)
84fa19c77a Add release notes for keypool flush changes (Samuel Dobson)
f9603ee4e0 Add test for flushing keypool with newkeypool (Samuel Dobson)
6f6f7bb36c Make legacy wallet upgrades from non-HD to HD always flush the keypool (Samuel Dobson)
2434b10781 Fix outdated keypool size default (Samuel Dobson)
22cc797ca5 Add newkeypool RPC to flush the keypool (Samuel Dobson)
Pull request description:
This PR makes two main changes:
1) Adds a new RPC `newkeypool` which will entirely flush and refill the keypool.
2) When upgradewallet is called on old, non-HD wallets upgrading them to HD, we now always flush the keypool and generate a new one, to immediately start using the HD generated keys.
This PR is motivated by a number of users with old, pre-compressed-key wallets upgrading them and being confused about why they still can't generate p2sh-segwit or bech32 addresses -- this is due to uncompressed keys remaining in the keypool post-upgrade and being illegal in these newer address formats. There is currently no easy way to flush the keypool other than to call `getnewaddress` a hundred/thousand times or an ugly hack of using a `sethdseed` call.
ACKs for top commit:
laanwj:
re-ACK 6531599f42
meshcollider:
Added new commit 6531599f42 to avoid invalidating previous ACKs.
instagibbs:
ACK 6531599f42
Tree-SHA512: 50c79c5d42dd27ab0ecdbfdc4071fdaa1b2dbb2f9195ed325b007106ff19226419ce57fe5b1539c0c24101b12f5e034bbcfb7bbb0451b766cb1071295383d774
b5950dd59c validation: put coins cache write log into bench debug log (Anthony Towns)
31b2b802b5 blockstorage: use debug log category (Anthony Towns)
da94ebc2fa validation: move header validation error logging to VALIDATION debug category (Anthony Towns)
1d7d835ec3 validation: include block hash when reporting prev block not found errors (Anthony Towns)
Pull request description:
Moves the following log messages into debug log categories:
* "AcceptBlockHeader: ..." to validation
* "Prune: deleted blk/rev" to new blockstorage log category
* "Leaving block file" moves from validation to blockstorage
* "write coins cache to disk" to bench
Also adds the hash of the block to the log message when AcceptBlockHeader is rejecting because of problems with the prev block.
ACKs for top commit:
practicalswift:
cr ACK b5950dd59c
Empact:
Code review ACK b5950dd59c
laanwj:
Code review ACK b5950dd59c
promag:
Code review ACK b5950dd59c.
meshcollider:
Code review ACK b5950dd59c
Tree-SHA512: a73fdbfe8d36da48a3e89c2d5e0b6a3c5045d280c1a57f61c38d0d21f4f198aece4bd85155be3439e179d5dabdb523bf15fa0395e0e3ceff19c878ba3112c840
ce69e18947 scripts: remove pixie.py (fanquake)
00b85d0b13 scripts: only parse the binary once in security-check.py (fanquake)
cad40a5b16 scripts: use LIEF for ELF checks in security-check.py (fanquake)
8242ae230e scripts: only parse the binary once in symbol-check.py (fanquake)
309eac9019 scripts: use LIEF for ELF checks in symbol-check.py (fanquake)
610a8a8e39 test-*-check: Pass in *FLAGS and compile with them (Carl Dong)
Pull request description:
This finishes the transition to using LIEF for the ELF symbol and security checks.
Note that there's currently a work around used for identifying RISCV binaries (just checking the interpreter). I've sent a PR upstream, https://github.com/lief-project/LIEF/pull/562, and we should be able to drop that when using LIEF 0.12.0 and onwards.
ACKs for top commit:
dongcarl:
Code Review ACK ce69e18947
laanwj:
Code review ACK ce69e18947
Tree-SHA512: 911ba693cd9777ad1fc1f66dff6c4d3630a907351215380cbde5b14a4bbf5cf7eebf52eafa7e86b27deabd2d93d1b403f34aabd356b5ceaab3cc6e9941a01dd4
fa6f29de51 bitcoin-tx: Reject non-integral and out of range multisig numbers (MarcoFalke)
fafab8ea5e bitcoin-tx: Reject non-integral and out of range sequence ids (MarcoFalke)
fa53d3d826 test: Check that bitcoin-tx accepts whitespace around sequence id and multisig numbers (MarcoFalke)
Pull request description:
Seems odd to silently accept arbitrary strings that don't even represent integral values.
Fix that.
ACKs for top commit:
practicalswift:
cr ACK fa6f29de51
laanwj:
Code review ACK fa6f29de51
Empact:
Code review ACK fa6f29de51
promag:
Code review ACK fa6f29de51.
Tree-SHA512: e31f7f21fe55ac069e755557bdbcae8d5d29e20ff82e441ebdfc65153e3a31a4edd46ad3e6dea5190ecbd1b8ea5a8f94daa5d59a3b7558e46e794e30db0e6c79
7e88f61b28 multiprocess: Make interfaces::Chain::isTaprootActive non-const (Russell Yanofsky)
Pull request description:
`interfaces::Chain` is an abstract class, so declaring the method const would be exposing internal implementation details of subclasses to interface callers. And specifically this doesn't work because the multiprocess implementation of the `interfaces::Chain::isTaprootActive` method can't be const because IPC connection state and request state is not constant during the call.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
jamesob:
ACK 7e88f61b28
Tree-SHA512: 1c5ed89870aeb7170b9048c41299ab650dfa3d0978088e08c4c866fa0babb292722710b16f25540f26667220cb4747b1c256c4bd42893c552291eccc155346a3
fa4d0aacf2 test: * -> & (MarcoFalke)
Pull request description:
This changes background_cs from being a pointer to a reference to work
around a gcc false warning. Also, this makes the test easier to read.
Fixes bitcoin#23101
Can be reviewed with --ignore-all-space.
ACKs for top commit:
practicalswift:
cr ACK fa4d0aacf2
jamesob:
ACK fa4d0aacf2
hebasto:
ACK fa4d0aacf2, tested on Linux Mint 20.2 (x86_64) by merging this PR on top of the current master.
Tree-SHA512: 93a0d8859201f7074bea52fab8f6701409148bc50cfbb142cacfa6c991fc12c07584df04fead645f11703883df99535423d154f9945202e1c5aff49540d9b607
fa43e7c2d9 bitcoin-tx: Avoid treating overflow as OP_0 (MarcoFalke)
fa053c0019 style: Fix whitespace in Parse* functions (MarcoFalke)
fa03dec7e9 refactor: Use C++11 range based for loop in ParseScript (MarcoFalke)
fad55e79ca doc: Fixup ToIntegral docs (MarcoFalke)
Pull request description:
Seems odd to treat integer overflow as `OP_0`, so fix that.
ACKs for top commit:
theStack:
re-ACK fa43e7c2d9
shaavan:
ACK fa43e7c2d9
Tree-SHA512: 1bbe2de62d853badc18d57d169c6e78ddcdff037e5a85357995dead11c8e67a4fe35087e08a181c60753f8ce91058b7fcc06f5b7901afedc78fbacea8bc3ef4f
This addresses issues like the one in #12467, where some of our compiler flags
end up being dropped during the subconfigure of Univalue. Specifically, we're
still using the compiler-default c++ version rather than forcing c++17.
We can drop the need subconfigure completely in favor of a tighter build
integration, where the sources are listed separately from the build recipes,
so that they may be included directly by upstream projects. This is
similar to the way leveldb build integration works in Core.
Core benefits of this approach include:
- Better caching (for ex. ccache and autoconf)
- No need for a slow subconfigure
- Faster autoconf
- No more missing compile flags
- Compile only the objects needed
There are no benefits to Univalue itself that I can think of. These changes
should be a no-op there, and to downstreams as well until they take advantage
of the new sources.mk.
This also removes the option to use an external univalue to avoid similar ABI
issues with mystery binaries.
Co-authored-by: fanquake <fanquake@gmail.com>
7fc487afd1 refactor: use `{Read,Write}BE32` helpers for BIP32 nChild (de)serialization (Sebastian Falbesoner)
Pull request description:
This small refactoring PR replaces manual bit-fiddling (de)serialization of the BIP32 child number (nChild) by the helpers `ReadBE32`/`WriteBE32`. Note that those were first introduced in #4100, almost one year _after_ the BIP32 derivation implementation has been merged (#2829, eb2c9990).
ACKs for top commit:
sipa:
utACK 7fc487afd1
laanwj:
Code review ACK 7fc487afd1
Tree-SHA512: bbe3e411fb0429fa74c8a5705a91f4d6ed704dac9d6623ecb633563f22acf8e21f3189a16f1d0cf1aeedfc56a5b695df54ae51e9577e34eb6d7dc335de2da6de
fa165e9545 Replace stoul with ToIntegral in dbwrapper (MarcoFalke)
Pull request description:
The string is created with `%llu`. See: 7fcf53f7b4/src/leveldb/db/db_impl.cc (L1436-L1437)
So it seems odd to silently accept when parsing: whitespace, a sign character, trailing chars, overflow, ....
Fix that by using the stricter ToIntegral.
ACKs for top commit:
laanwj:
Code review ACK fa165e9545
practicalswift:
cr ACK fa165e9545
theStack:
Code-review ACK fa165e9545
Tree-SHA512: b87f01431ca0b971ff84610022da8679d3c33470b88cfc3f4a337e6e176a0455715588aefd40e8e2bbe7459d902dc89d7bfe34e7fd66755f631cc18dc039fa2f
d54ec27bac qt: Add helper to load font (João Barbosa)
Pull request description:
Originally submitted as https://github.com/bitcoin-core/gui-qml/pull/49.
ACKs for top commit:
hebasto:
re-ACK d54ec27bac
stratospher:
Tested ACK d54ec27. Refactoring the code and defining `loadFont()` in `src/qt/guiutil.cpp` reduces redundant imports of the `QFontDatabase` and is a better design.
shaavan:
ACK d54ec27bac
Tree-SHA512: b156bb6ffb08dd57476f383a29bbb0a1108b62794d430debb77252f7d09df1409a7532b09d17d8836d1c2ab7c126a6618231164b9d0def1b8f361a81ef22d107
When calculating the fee for a given tx size from a fee rate, we should
always round up to the next satoshi. Otherwise, if we round down (via
truncation), the calculated fee may result in a fee with a feerate
slightly less than targeted.
This is particularly important for coin selection as a slightly lower
feerate than expected can result in a variety of issues.
as some of these tests are designed to be heavily contested to trigger race
conditions or other issues. This created very large log files when run with
DEBUG_LOCKCONTENTION defined (up to v22) or with lock logging enabled by default
in current master.
Examples running the following command:
./src/test/test_bitcoin -t checkqueue_tests/test_CheckQueue_Correct_Random -- DEBUG_LOG_OUT > testlog.txt
-rw-r--r-- 87042178 Oct 8 12:41 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run1.txt
-rw-r--r-- 73879896 Oct 8 12:42 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run2.txt
-rw-r--r-- 65150518 Oct 8 12:51 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run1.txt
-rw-r--r-- 65774554 Oct 8 12:52 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run2.txt
-rw-r--r-- 73493309 Oct 8 13:00 testlog-current-master-at-991753e-run1.txt
-rw-r--r-- 65616977 Oct 8 13:01 testlog-current-master-at-991753e-run2.txt
-rw-r--r-- 5093 Oct 8 13:04 testlog-with-this-commit-run1.txt
-rw-r--r-- 5093 Oct 8 13:05 testlog-with-this-commit-run2.txt
44452110f0 [fuzz] Update comment in FillAddrman() (John Newbery)
640476eb0e [fuzz] Make Fill() a free function in fuzz/addrman.cpp (John Newbery)
90ad8ad61a [fuzz] Make RandAddr() a free function in fuzz/addrman.cpp (John Newbery)
491975c596 [fuzz] Pass FuzzedDataProvider& into Fill() in addrman fuzz tests (John Newbery)
56303e382e [fuzz] Create a FastRandomContext in addrman fuzz tests (John Newbery)
Pull request description:
#22974 improved the performance of `CAddrMan::Good()` significantly so that it could be used directly in the fuzz tests, instead of those tests reaching directly into addrman's internal data structures.
This PR continues the work of making the fuzz tests only use `CAddrMan`'s public methods and pulls the `Fill()` and `RandAddr()` methods from `CAddrManDeterministic` into free functions, since they no longer need access to `CAddrMan` internals.
ACKs for top commit:
theuni:
utACK 44452110f0. Agree the failure seems unrelated, looks like some startup race.
mzumsande:
ACK 44452110f0
vasild:
ACK 44452110f0
Tree-SHA512: fcf994e1dedd0012b77f632720b6423d51ceda4eb85c9efe572f2a1150117f9e511114a5206738dd94409137287577f3b01a9998f5237de845410d3d96e7cb7f
43568782c2 External input fund support cleanups (Gregory Sanders)
Pull request description:
Minor cleanups to https://github.com/bitcoin/bitcoin/pull/17211
ACKs for top commit:
achow101:
ACK 43568782c2
meshcollider:
utACK 43568782c2
benthecarman:
ACK 43568782c2
Tree-SHA512: 865f8a3804f8c0027f5393a0539041158166a919378f2c3bc99b936843eee2329372bcc2af888fa62babfa5f6baf4f13d4cfef7b4e26a7265a82a908f9719ad6
ac402e749c util: Conditionalize some syscalls in syscall name table (W. J. van der Laan)
64085b37f8 util: Add __NR_copy_file_range syscall constant for sandbox (W. J. van der Laan)
Pull request description:
Make the new syscall sandbox compilable with kernel 4.4.0.
This defines a further syscall constant `__NR_copy_file_range` to make sure all syscalls used in the profile are available even if not defined in the kernel headers.
Also, make a few syscalls optional in the syscall name table:
- `__NR_pkey_alloc`
- `__NR_pkey_free`
- `__NR_pkey_mprotect`
- `__NR_preadv2`
- `__NR_pwritev2`
ACKs for top commit:
practicalswift:
cr ACK ac402e749c
Tree-SHA512: be6c55bf0a686bcdfad0b80b950d0d7d77a559ac234fc997b47514bdba44865a371c96dd8d34a811ba46424a84f410e23f75485b9b1e69e529b7d40e0b4b91b8
3b613722f6 Add release notes for fee est with replacement txs (Antoine Poinsot)
4556406562 qa: test fee estimation with replacement transactions (Antoine Poinsot)
053415b297 qa: split run_test into smaller parts (Antoine Poinsot)
06c5ce9714 Re-include RBF replacement txs in fee estimation (Antoine Poinsot)
Pull request description:
This effectively reverts #9519.
RBF is now largely in use on the network (signaled for by around 20% of
all transactions on average) and replacement logic is implemented in
most end-user wallets. The rate of replaced transactions is also
expected to rise as fee-bumping techniques are being developed for
pre-signed transaction ("L2") protocols.
ACKs for top commit:
prayank23:
reACK 3b613722f6
Zero-1729:
re-ACK 3b613722f6
benthecarman:
reACK 3b613722f6
glozow:
ACK 3b613722f6
theStack:
re-ACK 3b613722f6🍪
Tree-SHA512: a6146d15c80ff4ba9249314b0ef953a66a15673e61b8f98979642814f1b169b5695e330e3ee069fa9a7e4d1f8aa10e1dcb7f9aa79181cea5a4c4dbcaf5483023
fafff132cf doc: Extract FundTxDoc (MarcoFalke)
Pull request description:
No need to duplicate the documentation for the same field(s) three times.
Fix that by de-duplicating it for the fields: conf_target, estimate_mode, replaceable, and solving_data.
Can be reviewed with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
fanquake:
ACK fafff132cf
Tree-SHA512: 098ddad3904b80b24c9e7b57ca8e807a6ccc3899eac2c9986d71ba3873c2b580bbb95f2fdfbf94b2db02f81c7b0ebf438a90324c23389b7b968ca85ae8475373
fab360aa00 util: Add mremap syscall to AllowAddressSpaceAccess (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/23206
ACKs for top commit:
practicalswift:
cr ACK fab360aa00
laanwj:
Code review ACK fab360aa00
fanquake:
ACK fab360aa00 - confirmed that the GUIX build is working with this change:
Tree-SHA512: 9cf808b3e04830e87bca49b27914993929be3c27eb674d89739b8ea5e5c848c87713d638506c1cd2b80b0129c3dff0c488eb240eef3bbf3d7508ece3c934fb54
01bff8f049 qt: Fix WalletControllerActivity progress dialog title (Shashwat)
Pull request description:
Throughout the GUI, the title of the window, tells about the purpose of the window. This was not true for the title of wallet loading wallet.
This PR fixes this issue by renaming the wallet loading window title to 'Open Wallet'
Changes introduced in this PR (Runned Bitcoin-GUI on signet network)
|Master|PR|
|---|---|
|||
ACKs for top commit:
jarolrod:
ACK 01bff8f
hebasto:
ACK 01bff8f049, tested on Linux Mint 20.2 (Qt 5.12.8).
Tree-SHA512: cd21c40752eb1c0afb5ec61b8a40e900bc3aa05749963f7957ece6024e4957f5bb37e0eb4f95aac488f5e08aea51fe13b023b05d8302a08c88dcc6790410ba64
faa5e171e6 RPCConsole: Throw when overflowing size_t type for array indices (MarcoFalke)
Pull request description:
To test:
-> `getblock(getbestblockhash(), 1)[tx][22222222222222222222222222222]`
Before:
<- `868693731dea69a197c13c2cfaa41c9f78fcdeb8ab8e9f8cdf2c9025147ee7d1` (hash of the coinbase tx)
After:
<- `Error: Invalid result query`
ACKs for top commit:
jarolrod:
ACK faa5e171e6
shaavan:
ACK faa5e17
Tree-SHA512: ddff39aae1c15db45928b110a9f1c42eadc5404cdfa89d67ccffc4c6af24091967d43c068ce9e0c1b863cfc4eb5b4f12373a73756a9474f8294e8a44aabc28d8
The WalletControllerActivity progress dialog had title of "Bitcoin-Qt".
The window title for opening wallet window should be "Open Wallet",
for creating wallet window should be "Create Wallet", and for the window
that is displayed when wallets are being loaded at startup should be
"Load Wallets". This PR fixes that.
Make the watch-only icon in the bottom bar enabled by default for a better user interface.
Currently, it's disabled by default with a 50% opacity which makes it hard to see the icon in dark mode.
Put these in `#ifdef` as they are newer syscalls that might not be
defined on all kernels:
__NR_pkey_alloc
__NR_pkey_free
__NR_pkey_mprotect
__NR_preadv2
__NR_pwritev2
Thanks to jamesob for reporting.
and update the function name to CheckAddrman (drop "Force") for
nicer log output as it is prefixed to each of these log messages:
2021-09-21T18:42:50Z [opencon] CheckAddrman: new 64864, tried 1690, total 66554 started
2021-09-21T18:42:50Z [opencon] CheckAddrman: completed (76.21ms)
The existing Doxygen documentation on the function already makes
clear that it is unaffected by m_consistency_check_ratio.
The original CAddrMan behaviour (before commit
e6b343d880) was to pick a uniformly
random non-empty bucket, and then pick a random element from that
bucket. That commit, which introduced deterministic placement
of entries in buckets, changed this to picking a uniformly random
non-empty bucket position instead.
This commit reverts the original high-level behavior, but in the
deterministic placement model.
Use a (reference) parameter instead of a data member of
CAddrManDeterministic. This will allow us to make Fill() a free function
in a later commit.
Also remove CAddrManDeterministic.m_fuzzed_data_provider since it's no
longer used.
Add interfaces::ExternalSigner to let signer objects be passed between
processes and signer code to run in the original process, without
multiple processes linking and running signer code.
There is no change in behavior. This just helps prepare for the
transition from boost::filesystem to std::filesystem by avoiding calls
to methods which will be unsafe after the transaction to std::filesystem
to due lack of a boost::filesystem::path::imbue equivalent and inability
to set a predictable locale.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
There is no change in behavior. This just helps prepare for the
transition from the boost::filesystem to the std::filesystem path
implementation.
Co-authored-by: Kiminuo <kiminuo@protonmail.com>
2d0279987e util: Make sure syscall numbers used in profile are defined (W. J. van der Laan)
8289d19ea5 util: Define SECCOMP_RET_KILL_PROCESS if not provided by the headers (W. J. van der Laan)
Pull request description:
Looks like we've broke the GUIX build in #20487. This attempts to fix it:
- Define `__NR_statx` `__NR_getrandom` `__NR_membarrier` as some kernel headers lack them, and it's important to have the same profile independent on what kernel is used for building.
- Define `SECCOMP_RET_KILL_PROCESS` as it isn't defined in the headers.
ACKs for top commit:
practicalswift:
cr ACK 2d0279987e
Tree-SHA512: c264c66f90af76bf364150e44d0a31876c2ef99f05777fcdd098a23f1e80efef43028f54bf9b3dad016110056d303320ed9741b0cb4c6266175fa9d5589b4277
021f86953e [style] Run changed files through clang formatter. (Amiti Uttarwar)
375750387e scripted-diff: Rename CAddrInfo to AddrInfo (Amiti Uttarwar)
dd8f7f2500 scripted-diff: Rename CAddrMan to AddrMan (Amiti Uttarwar)
3c263d3f63 [includes] Fix up included files (Amiti Uttarwar)
29727c2aa1 [doc] Update comments (Amiti Uttarwar)
14f9e000d0 [refactor] Update GetAddr_() function signature (Amiti Uttarwar)
40acd6fc9a [move-only] Move constants to test-only header (Amiti Uttarwar)
7cf41bbb38 [addrman] Change CAddrInfo access (Amiti Uttarwar)
e3f1ea659c [move-only] Move CAddrInfo to test-only header file (Amiti Uttarwar)
7cba9d5618 [net, addrman] Remove external dependencies on CAddrInfo objects (Amiti Uttarwar)
8af5b54f97 [addrman] Introduce CAddrMan::Impl to encapsulate addrman implementation. (Amiti Uttarwar)
f2e5f38f09 [move-only] Match ordering of CAddrMan declarations and definitions (Amiti Uttarwar)
5faa7dd6d8 [move-only] Move CAddrMan function definitions to cpp (Amiti Uttarwar)
Pull request description:
Introduce the pimpl pattern for AddrMan to separate the implementation details from the externally used object representation. This reduces compile-time dependencies and conceptually clarifies AddrMan's interface from the implementation specifics.
Since the unit & fuzz tests currently rely on accessing AddrMan internals, this PR introduces addrman_impl.h, which is exclusively imported by addrman.cpp and test files.
ACKs for top commit:
jnewbery:
ACK 021f86953e
GeneFerneau:
utACK [021f869](021f86953e)
mzumsande:
ACK 021f86953e
rajarshimaitra:
Concept + Code Review ACK 021f86953e
theuni:
ACK 021f86953e
Tree-SHA512: aa70cb77927a35c85230163c0cf6d3872382d79048b0fb79341493caa46f8e91498cb787d8b06aba4da17b2f921f2230e73f3d66385519794fff86a831b3a71d
Define the following syscall numbers for x86_64, so that the profile
will be the same no matter what kernel is built against, including
kernels that don't have `__NR_statx`:
```c++
#define __NR_statx 332
#define __NR_getrandom 318
#define __NR_membarrier 324
```
44d77d2213 sandbox: add copy_file_range to allowed filesystem syscalls (fanquake)
ee08741c9c sandbox: add newfstatat to allowed filesystem syscalls (fanquake)
Pull request description:
Similar to #23178, this is a follow up to #20487, which has broken running the unit tests for some developers. Fix this by adding `newfstatat` to the list of allowed filesystem related calls.
ACKs for top commit:
achow101:
ACK 44d77d2213
laanwj:
Code review ACK 44d77d2213
practicalswift:
cr ACK 44d77d2213
Tree-SHA512: ce9d1b441ebf25bd2cf290566e05864223c1418dab315c962e1094ad877db5dd9fcab94ab98a46da8b712a8f5f46675d62ca3349215d8df46ec5b3c4d72dbaa6
Display the prevout in transaction inputs when calling getblock level 3
verbosity.
Co-authored-by: Luke Dashjr <luke_github1@dashjr.org>
Co-authored-by: 0xB10C <19157360+0xB10C@users.noreply.github.com>
9d0379cea6 consensus: use <cstdint> over <stdint.h> in amount.h (fanquake)
863e52fe63 consensus: make COIN & MAX_MONEY constexpr (fanquake)
d09071da5b [MOVEONLY] consensus: move amount.h into consensus (fanquake)
Pull request description:
A first step (of a few) towards some source code reorganization, as well as making libbitcoinconsensus slightly more self contained.
Related to #15732.
ACKs for top commit:
MarcoFalke:
concept ACK 9d0379cea6 🏝
Tree-SHA512: 97fc79262dcb8c00996852a288fee69ddf8398ae2c95700bba5b326f1f38ffcfaf8fa66e29d0cb446d9b3f4e608a96525fae0c2ad9cd531ad98ad2a4a687cd6a
4747da3a5b Add syscall sandboxing (seccomp-bpf) (practicalswift)
Pull request description:
Add experimental syscall sandboxing using seccomp-bpf (Linux secure computing mode).
Enable filtering of system calls using seccomp-bpf: allow only explicitly allowlisted (expected) syscalls to be called.
The syscall sandboxing implemented in this PR is an experimental feature currently available only under Linux x86-64.
To enable the experimental syscall sandbox the `-sandbox=<mode>` option must be passed to `bitcoind`:
```
-sandbox=<mode>
Use the experimental syscall sandbox in the specified mode
(-sandbox=log-and-abort or -sandbox=abort). Allow only expected
syscalls to be used by bitcoind. Note that this is an
experimental new feature that may cause bitcoind to exit or crash
unexpectedly: use with caution. In the "log-and-abort" mode the
invocation of an unexpected syscall results in a debug handler
being invoked which will log the incident and terminate the
program (without executing the unexpected syscall). In the
"abort" mode the invocation of an unexpected syscall results in
the entire process being killed immediately by the kernel without
executing the unexpected syscall.
```
The allowed syscalls are defined on a per thread basis.
I've used this feature since summer 2020 and I find it to be a helpful testing/debugging addition which makes it much easier to reason about the actual capabilities required of each type of thread in Bitcoin Core.
---
Quick start guide:
```
$ ./configure
$ src/bitcoind -regtest -debug=util -sandbox=log-and-abort
…
2021-06-09T12:34:56Z Experimental syscall sandbox enabled (-sandbox=log-and-abort): bitcoind will terminate if an unexpected (not allowlisted) syscall is invoked.
…
2021-06-09T12:34:56Z Syscall filter installed for thread "addcon"
2021-06-09T12:34:56Z Syscall filter installed for thread "dnsseed"
2021-06-09T12:34:56Z Syscall filter installed for thread "net"
2021-06-09T12:34:56Z Syscall filter installed for thread "msghand"
2021-06-09T12:34:56Z Syscall filter installed for thread "opencon"
2021-06-09T12:34:56Z Syscall filter installed for thread "init"
…
# A simulated execve call to show the sandbox in action:
2021-06-09T12:34:56Z ERROR: The syscall "execve" (syscall number 59) is not allowed by the syscall sandbox in thread "msghand". Please report.
…
Aborted (core dumped)
$
```
---
[About seccomp and seccomp-bpf](https://en.wikipedia.org/wiki/Seccomp):
> In computer security, seccomp (short for secure computing mode) is a facility in the Linux kernel. seccomp allows a process to make a one-way transition into a "secure" state where it cannot make any system calls except exit(), sigreturn(), and read() and write() to already-open file descriptors. Should it attempt any other system calls, the kernel will terminate the process with SIGKILL or SIGSYS. In this sense, it does not virtualize the system's resources but isolates the process from them entirely.
>
> […]
>
> seccomp-bpf is an extension to seccomp that allows filtering of system calls using a configurable policy implemented using Berkeley Packet Filter rules. It is used by OpenSSH and vsftpd as well as the Google Chrome/Chromium web browsers on Chrome OS and Linux. (In this regard seccomp-bpf achieves similar functionality, but with more flexibility and higher performance, to the older systrace—which seems to be no longer supported for Linux.)
ACKs for top commit:
laanwj:
Code review and lightly tested ACK 4747da3a5b
Tree-SHA512: e1c28e323eb4409a46157b7cc0fc29a057ba58d1ee2de268962e2ade28ebd4421b5c2536c64a3af6e9bd3f54016600fec88d016adb49864b63edea51ad838e17
No transaction in the mempool should ever be a coinbase.
Since mempoolDuplicate's backend is the chainstate coins view, it should
always contain the coins available.
UpdateCoins is an unnecessary dependency on validation. All we need to
do is add and remove coins to check inputs. We don't need the extra
logic for checking coinbases and handling TxUndos.
Also remove the wrapper function in validation.h which constructs a
throwaway TxUndo object before calling UpdateCoins because it is now
unused.
Remove variables used for keeping track of mempool transactions for
which we haven't processed the parents yet. Since we're iterating in
topological order now, they're always unused.
No behavior changes.
Before, we're always adding transactions to the "check later" queue if
they have any parents in the mempool. But there's no reason to do this
if all of its inputs are already available from mempoolDuplicate.
Instead, check for inputs, and only mark fDependsWait=true if the
parents haven't been processed yet.
Reduce the amount of "check later" transactions by looking at
ancestors before descendants. Do this by iterating through them in
ascending order by ancestor count. This works because a child will
always have more in-mempool ancestors than its parent.
We should never have any entries in the "check later" queue
after this commit.
fa9d72a794 Remove unused ParseDouble and ParsePrechecks (MarcoFalke)
fa3cd28535 refactor: Remove unused ParsePrechecks from ParseIntegral (MarcoFalke)
Pull request description:
All of the `ParsePrechecks` are already done by `ToIntegral`, so remove them from `ParseIntegral`.
Also:
* Remove redundant `{}`. See https://github.com/bitcoin/bitcoin/pull/20457#discussion_r720116866
* Add missing failing c-string test case
* Add missing failing test cases for non-int32_t integral types
ACKs for top commit:
laanwj:
Code review ACK fa9d72a794, good find on ParseDouble not being used at all, and testing for behavior of embedded NULL characters is always a good thing.
practicalswift:
cr ACK fa9d72a794
Tree-SHA512: 3d654dcaebbf312dd57e54241f9aa6d35b1d1d213c37e4c6b8b9a69bcbe8267a397474a8b86b57740fbdd8e3d03b4cdb6a189a9eb8e05cd38035dab195410aa7
928af61cdb allow send rpc take external inputs and solving data (Andrew Chow)
e39b5a5e7a Tests for funding with external inputs (Andrew Chow)
38f5642ccc allow fundtx rpcs to work with external inputs (Andrew Chow)
d5cfb864ae Allow Coin Selection be able to take external inputs (Andrew Chow)
a00eb388e8 Allow CInputCoin to also be constructed with COutPoint and CTxOut (Andrew Chow)
Pull request description:
Currently `fundrawtransaction` and `walletcreatefundedpsbt` both do not allow external inputs as the wallet does not have the information necessary to estimate their fees.
This PR adds an additional argument to both those RPCs which allows the user to specify solving data. This way, the wallet can use that solving data to estimate the size of those inputs. The solving data can be public keys, scripts, or descriptors.
ACKs for top commit:
prayank23:
reACK 928af61cdb
meshcollider:
Re-utACK 928af61cdb
instagibbs:
crACK 928af61cdb
yanmaani:
utACK 928af61.
Tree-SHA512: bc7a6ef8961a7f4971ea5985d75e2d6dc50c2a90b44c664a1c4b0f1be5c1c97823516358fdaab35771a4701dbefc0862127b1d0d4bfd02b4f20d2befa4434700
Also:
* Remove redundant {} from return statement
* Add missing failing c-string test case and "-" and "+" strings
* Add missing failing test cases for non-int32_t integral types
0ab4c3b272 Return false on corrupt tx rather than asserting (Samuel Dobson)
Pull request description:
Takes up #19793
Rather than asserting, we log an error and return CORRUPT so that the user is informed. This type of error isn't critical so it isn't worth `assert`ing.
ACKs for top commit:
achow101:
ACK 0ab4c3b272
laanwj:
Code review ACK 0ab4c3b272
ryanofsky:
Code review ACK 0ab4c3b272. There may be room for more improvements later like better error messages or easier recovery options, but changing from an assert to an error seems like a clear improvement, and this seems to avoid all the pitfalls of the last PR that tried this.
Tree-SHA512: 4a1a412e7c473d176c4e09123b85f390a6b0ea195e78d28ebd50b13814b7852f8225a172511a2efb6affb555b11bd4e667c19eb8c78b060c5444b62f0fae5f7a
18c5b23a0f [test] Test that -blocksonly nodes still serve compact blocks. (Niklas Gögge)
a79ad65fc2 [test] Test that getdata(CMPCT) is still sent on regular low bandwidth connections. (Niklas Gögge)
5e231c116b [test] Test that -blocksonly nodes do not send getdata(CMPCT) on a low bandwidth connection. (Niklas Gögge)
5bf6587457 [test] Test that -blocksonly nodes do not request high bandwidth mode. (Niklas Gögge)
0dc8bf5b92 [net processing] Dont request compact blocks in blocks-only mode (Niklas Gögge)
Pull request description:
A blocks-only node does not participate in transaction relay to reduce its own bandwidth usage and therefore does not have a mempool. The use of compact blocks is not beneficial to such a node since it will always have to download full blocks.
In both high- and low-bandwidth relaying the `cmpctblock` message is sent. This represent a bandwidth overhead for blocks-only nodes because the `cmpctblock` message is several times larger in the average case than the equivalent `headers` or `inv` announcement.
>**Example:**
>A block with 2000 txs results in a `cmpctblock` with 2000*6 bytes in short ids. This is several times larger than the equivalent 82 bytes for a `headers` message or 37 bytes for an `inv`.
## Approach
This PR makes blocks-only nodes always use the legacy relaying to download new blocks.
It does so by making blocks-only nodes never initiate a high-bandwidth block relay connection by disabling the sending of `sendcmpct(1)`. Additionally a blocks-only node will never request a compact block using `getdata(CMPCT)`.
A blocks-only node will continue to serve compact blocks to its peers in both high- and low-bandwidth mode.
ACKs for top commit:
naumenkogs:
ACK 18c5b23a0f
rajarshimaitra:
tACK 18c5b23a0f
jnewbery:
reACK 18c5b23a0f
theStack:
re-ACK 18c5b23a0f🥛
Tree-SHA512: 0c78804aa397513d41f97fe314efb815efcd852d452dd903df9d4749280cd3faaa010fa9b51d7d5168b8a77e08c8ab0a491ecdbdb3202f2e9cd5137cddc74624
dc3ec74d67 Add rescan removal release note (Samuel Dobson)
bccd1d942d Remove -rescan startup parameter (Samuel Dobson)
f963b0fa8c Corrupt wallet tx shouldn't trigger rescan of all wallets (Samuel Dobson)
6c006495ef Remove outdated dummy wallet -salvagewallet arg (Samuel Dobson)
Pull request description:
Remove the `-rescan` startup parameter.
Rescans can be run with the `rescanblockchain` RPC.
Rescans are still done on wallet-load if needed due to corruption, for example.
ACKs for top commit:
achow101:
ACK dc3ec74d67
laanwj:
re-ACK dc3ec74d67
Tree-SHA512: 608360d0e7d73737fd3ef408b01b33d97a75eebccd70c6d1b47a32fecb99b9105b520b111b225beb10611c09aa840a2b6d2b6e6e54be5d0362829e757289de5c
2fe69efbc6 qt, wallet: Drop no longer used WalletController::getOpenWallets() (Hennadii Stepanov)
f6991cb906 qt, wallet: Add LoadWalletsActivity class (Hennadii Stepanov)
4a024fc310 qt, wallet, refactor: Move connection to QObject::deleteLater to ctor (Hennadii Stepanov)
f9b633eeab qt, wallet: Move activity progress dialog from data member to local (Hennadii Stepanov)
Pull request description:
This PR improves the GUI responsiveness during initial wallets loading at startup (especially ones that have tons of txs), and shows a standard progress dialog for long loading:
Fixes#247.
ACKs for top commit:
ryanofsky:
Code review ACK 2fe69efbc6. Just suggested changes since last review: squashing commits and dropping unused method (thanks!)
shaavan:
reACK 2fe69efbc6
promag:
Code review ACK 2fe69efbc6.
Tree-SHA512: 2ac3cb48886e0005fc36b3fd0c2b35abd557186be16db3145d753c34d94188e4f4ff14dc07fb0fb7558944f84498204a3988f8284fd56c6d85b47bc9081e71a6
4747db8761 util: Introduce ToIntegral<T>(const std::string&) for locale independent parsing using std::from_chars(…) (C++17) (practicalswift)
Pull request description:
Make `Parse{Int,UInt}{32,64}` use locale independent `std::from_chars(…)` (C++17) instead of locale dependent `strto{l,ll,ul,ull}`.
[About `std::from_chars`](https://en.cppreference.com/w/cpp/utility/from_chars): _"Unlike other parsing functions in C++ and C libraries, `std::from_chars` is locale-independent, non-allocating, and non-throwing."_
ACKs for top commit:
laanwj:
Code review ACK 4747db8761
Tree-SHA512: 40f2cd582bc19ddcf2c498eca3379167619eff6aa047bbac2f73b8fd8ecaefe5947c66700a189f83848751f9f8c05645e83afd4a44a1679062aee5440dba880a
4446ef0a54 build: remove support for weak linking getauxval() (fanquake)
e56100c5b4 build: remove arm includes from getauxval() check (fanquake)
Pull request description:
It was [pointed out in #23030](https://github.com/bitcoin/bitcoin/pull/23030#issuecomment-922893367) that we might be able to get rid of our weak linking of [`getauxval()`](https://man7.org/linux/man-pages/man3/getauxval.3.html) (`HAVE_WEAK_GETAUXVAL`) entirely, with only Android being a potential holdout:
> I wonder if it's time to get rid of HAVE_WEAK_GETAUXVAL. I think it's confusing. Either we build against a C library that has this functionality, or not. We don't do this weak linking thing for any other symbols and recently got rid of the other glibc backwards compatibility stuff.
> Unless there is still a current platform that really needs it (Android?), I'd prefer to remove it from the build system, it has caused enough issues.
After looking at Android further, it would seem that given we are moving to using `std::filesystem`, which [requires NDK version 22 and later](https://github.com/android/ndk/wiki/Changelog-r22), and `getauxval` has been available in the since [API version 18](https://developer.android.com/ndk/guides/cpu-features#features_using_libcs_getauxval3), that shouldn't really be an issue. Support for API levels < 19 will be dropped with the NDK 24 release, and according to [one website](https://apilevels.com/), supporting API level 18+ will cover ~99% of devices. Note that in the CI we currently build with NDK version 22 and API level 28.
The other change in this PR is removing the include of headers for ARM intrinsics, from the check for strong `getauxval()` support in configure, as they shouldn't be needed. Including these headers also meant that the check would basically only succeed when building for ARM. This would be an issue if we remove weak linking, as we wouldn't detect `getauxval()` as supported on other platforms. Note that we also use `getauxval()` in our RNG when it's available.
I've checked that with these changes we detect support for strong `getauxval()` on Alpine (muslibc). On Linux, previously we'd be detecting support for weak getauxval(), now we detect strong support. Note that we already require glibc 2.17, and `getauxval()` was introduced in `2.16`.
This is an alternative / supersedes #23030.
ACKs for top commit:
laanwj:
Code review and tested ACK 4446ef0a54
Tree-SHA512: 5f2a9e9cc2d63bddab73f0dcb169d4d6beda74622af82bc0439722f1189f81d052e2fc1eaf27056a7a606320d5ddc4c11075f0d051dd93d77c5e1c15337f354a
1d44513f9b Squashed 'src/crc32c/' changes from b5ef9be675..0d624261ef (MarcoFalke)
Pull request description:
Only change is a warning fix for arm.
```
CXX crc32c/src/crc32c_libcrc32c_a-crc32c.o
In file included from crc32c/src/crc32c.cc:11:0:
crc32c/src/./crc32c_arm64_check.h: In function ‘bool crc32c::CanUseArm64Crc32()’:
crc32c/src/./crc32c_arm64_check.h:43:37: warning: the address of ‘long unsigned int getauxval(long unsigned int)’ will never be NULL [-Waddress]
unsigned long hwcap = (&getauxval != nullptr) ? getauxval(AT_HWCAP) : 0;
~~~~~~~~~~~^~~~~~~~~~
ACKs for top commit:
laanwj:
Code review ACK fac1c13ead
fanquake:
ACK fac1c13ead
Tree-SHA512: 22a52caf67dd89092eff1f075fbf5c5d16bdca9146ba042ce5d3fcc10ce1485e950964089f8536c938ebe650676e03a789d3597fe45b19920fd2c5e72f1391ad
8ff3743f5e Revert "doc: Remove outdated comments" (Hennadii Stepanov)
Pull request description:
Unfortunately, in #23094 the assumption that #14336 makes comments outdated is wrong. As pointed in https://github.com/bitcoin/bitcoin/pull/23094#discussion_r717226839, the #14336 just moved the relevant code a few lines down.
This PR reverts commit ee7891a0c4, and moves the comments into the right place.
I apologize about that.
ACKs for top commit:
MarcoFalke:
cr ACK 8ff3743f5e
laanwj:
ACK 8ff3743f5e
Tree-SHA512: 84aca627bb5b49c06fc172778f9b9407482c5a873ccbc3dc40167e6a8ad0bc60475d6a469c843b7b42712e35cf3fc2d3518923e791d5e0c59628e042acc72747
90be29c5b5 wallet: enable SQLite extended result codes (Sebastian Falbesoner)
Pull request description:
With this change, we get more fine-grained error messages if something goes wrong in the course of communicating with the SQLite database. To pick some random examples, the error codes SQLITE_IOERR_NOMEM, SQLITE_IOERR_CORRUPTFS or SQLITE_IOERR_FSYNC are way more specific than just a plain SQLITE_IOERR, and the corresponding error messages generated by sqlite3_errstr() will hence give a better hint to the user (or also to the developers, if an error report is sent) what the cause for a failure is.
See the SQLite documentation
https://www.sqlite.org/c3ref/extended_result_codes.htmlhttps://www.sqlite.org/c3ref/c_abort_rollback.html
> In its default configuration, SQLite API routines return one of 30 integer result codes. However, experience has shown that many of these result codes are too coarse-grained. They do not provide as much information about problems as programmers might like. In an effort to address this, newer versions of SQLite (version 3.3.8 2006-10-09 and later) include support for additional result codes that provide more detailed information about errors.
ACKs for top commit:
Sjors:
utACK 90be29c
achow101:
ACK 90be29c5b5
laanwj:
Code review ACK 90be29c5b5
Tree-SHA512: 2b7a60860c206f2b5f8ff9d4a7698efdee897c9ad024621b8fd165b841c20746d9780da3cf46aaf448a777e229a5b3cdf3a4792e8ef82cda9c5d46e354a9a598
451ca244db qt, refactor: Drop intermediate BitcoinApplication::shutdownResult slot (Hennadii Stepanov)
f3a17bbe5f qt: Do not exit and re-enter main event loop during shutdown (Hennadii Stepanov)
b4e0d2c431 qt, refactor: Allocate SendConfirmationDialog instances on heap (Hennadii Stepanov)
332dea2852 qt, refactor: Keep HelpMessageDialog in the main event loop (Hennadii Stepanov)
c8bae37a7a qt, refactor: Keep PSBTOperationsDialog in the main event loop (Hennadii Stepanov)
7fa91e8312 qt, refactor: Keep AskPassphraseDialog in the main event loop (Hennadii Stepanov)
6f6fde30e7 qt, refactor: Keep EditAddressDialog in the main event loop (Hennadii Stepanov)
59f7ba4fd7 qt, refactor: Keep CoinControlDialog in the main event loop (Hennadii Stepanov)
7830cd0b35 qt, refactor: Keep OptionsDialog in the main event loop (Hennadii Stepanov)
13f618818d qt: Add GUIUtil::ShowModalDialogAndDeleteOnClose (Hennadii Stepanov)
Pull request description:
On master (1ef34ee25e) during shutdown `QApplication` exits the main event loop, then re-enter again.
This PR streamlines shutdown process by removing the need to interrupt the main event loop, that is required for #59.
Also, blocking [`QDialog::exec()`](https://doc.qt.io/qt-5/qdialog.html#exec) calls are replaced with safer [`QDialog::show()`](https://doc.qt.io/qt-5/qwidget.html#show), except for `SendConfirmationDialog` as that change is not trivial (marked as TODO).
The [`QDialog::open()`](https://doc.qt.io/qt-5/qdialog.html#open) was not used because the actual modality mode (application modal or window modal) of a dialog depends on whether it has a parent.
This PR does not change behavior, and all touched dialogs are still application modal.
As a follow up, a design research could suggest to make some dialogs window modal.
NOTE for reviewers: quitting app while a dialog is open (e.g., via systray icon menu) must work fine.
ACKs for top commit:
laanwj:
Code review and lighly tested ACK 451ca244db
promag:
ACK 451ca244db, just changed signal to `quitRequested`.
Tree-SHA512: ef01ab6ed803b202e776019a4e1f592e816f7bc786e00574b25a0bf16be2374ddf9db21f0a26da08700df7ef0ab9e879550df46dcfe3b6d940f5ed02ca5f8447
The helps for RPCs gettransaction, listtransactions, and
listsinceblock returned by TransactionDescriptionString()
state that the "trusted" boolean field is only present if the
transaction is trusted and safe to spend from.
The "trusted" boolean field is in fact returned by
WalletTxToJSON() when the transaction has 0 confirmations,
or negative confirmations, if conflicted, and it can be
true or false.
This commit updates TransactionDescriptionString() to a
more accurate description for "trusted" and updates the
existing line of test coverage to fail more helpfully.
10c6929d55 Include vout when copying transaction ID from coin selection (Samuel Dobson)
Pull request description:
Fixes#432
I think it makes sense to just add the vout to the existing function because I can't imagine a situation where a user in the coin selection dialog would want just the transaction ID rather than the specific outpoint, and they can just delete it from the end anyway.
ACKs for top commit:
kristapsk:
ACK 10c6929d55
hebasto:
ACK 10c6929d55, tested on Linux Mint 20.2 (Qt 5.12.8).
shaavan:
ACK 10c6929
Tree-SHA512: df4d132b6c2fd0b590594e91cf54f82c6c0f77ee9ca06296fb726bc3c52b9ae459ca3b50c48b2bf303ccafe832b6b4dba692a812f439991ca6d807ea0d8df934
This effectively reverts de1ae324bf.
RBF is now largely in use on the network (signaled for by around 20% of
all transactions on average) and replacement logic is implemented in
most end-user wallets. The rate of replaced transactions is also
expected to rise as fee-bumping techniques are being developed for
pre-signed transaction ("L2") protocols.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
a11da75411 bloom: cleanup includes (fanquake)
f1ed1d3194 bloom: use constexpr where appropriate (fanquake)
2ba4ddf31d bloom: use Span instead of std::vector for `insert` and `contains` (William Casarin)
Pull request description:
This is #18985 rebased, with the most recent comments addressed.
> We can avoid many unnecessary std::vector allocations by changing
CBloomFilter to take Spans instead of std::vector's for the `insert`
and `contains` operations.
> CBloomFilter currently converts types such as CDataStream and uint256
to std::vector on `insert` and `contains`. This is unnecessary because
CDataStreams and uint256 are already std::vectors internally. We just
need a way to point to the right data within those types. Span gives
us this ability.
ACKs for top commit:
sipa:
Code review ACK a11da75411
laanwj:
Code review ACK a11da75411
Tree-SHA512: ee9ba02c9588daa1ff51782d1953fd060839dd15aa85861b2633b6ff2398320188ddd00f01d0c99442224485364ede9f8322366de4239fc7831ebfa06bd34659
This changes background_cs from being a pointer to a reference to work
around a gcc false warning. Also, this makes the test easier to read.
Fixes https://github.com/bitcoin/bitcoin/issues/23101
Can be reviewed with --ignore-all-space.
bd5c826a96 gui: add RPC setting (Sjors Provoost)
Pull request description:
RPC access is disabled by default for the GUI.
With the proliferation of third party desktop applications that use the Bitcoin Core RPC (e.g. Specter Desktop, Sparrow and Wasabi), this PR makes them slight easier to configure. It's no longer required to find and edit `bitcoin.conf` to add `server=1` to it.
<img width="447" alt="Schermafbeelding 2021-09-02 om 14 25 58" src="https://user-images.githubusercontent.com/10217/131844201-be3b49a8-ae88-47e6-8992-e95ee6b70f69.png">
ACKs for top commit:
hebasto:
ACK bd5c826a96, tested on Linux Mint 20.2 (Qt 5.12.8):
shaavan:
reACK bd5c826a96
promag:
Code review ACK bd5c826a96. Just minor fixes to the .ui form since last review.
Tree-SHA512: ab377e2358826096b499013bc3a864b7b63dff9859e96041e93ff0897d2319a35e8b3adcfb8df5f83274466c83d040d4ea18c546699421425c835e6f42562ae0
4832737c7d qt: connection type translator comments (Jarol Rodriguez)
Pull request description:
This PR introduces Qt translator comments for `Connection Type` strings in `guiutil.cpp` as well as `rpcconsole.cpp`.
This is an alternate implementation of the idea presented in the last three commits of #289. It is especially inspired by commit 842f4e834dfe5fd2786a5092f78ea28da1b36e4f.
Per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code), it is better to not break up strings when not necessary. This way we preserve the full context for translators.
ACKs for top commit:
jonatack:
Code review re-ACK 4832737c7d per `git diff 371e2b9 4832737`, changes are translator comment edits since my review yesterday (thank you for updating)
hebasto:
ACK 4832737c7d
Tree-SHA512: 67e1741e10a2e30cde6d50d3293eec89f0b7641b34463865dc6909d2926cdcf33a7d8c1dc8055d2f85906ad2002cdaa594d37b184d16e2f06614b6c5ad00c982
We can avoid many unnecessary std::vector allocations by changing
CBloomFilter to take Spans instead of std::vector's for the `insert`
and `contains` operations.
CBloomFilter currently converts types such as CDataStream and uint256
to std::vector on `insert` and `contains`. This is unnecessary because
CDataStreams and uint256 are already std::vectors internally. We just
need a way to point to the right data within those types. Span gives
us this ability.
Signed-off-by: William Casarin <jb55@jb55.com>
fa189621cc doc: Remove un-actionable TODO from chainparams.cpp (MarcoFalke)
Pull request description:
This can't be fixed by writing code, see discussion in https://github.com/bitcoin/bitcoin/pull/23021/files#r717426632
ACKs for top commit:
jarolrod:
ACK fa189621cc
prayank23:
ACK fa189621cc
Tree-SHA512: 3c5c0a0f45d057c9a617797007220837d7dcb29ae5996441e41b3698a67dc3d898f465adc0a958ecef430068cd9c564540bb534bbb3b230a53130ea001629f3e
Since knowledge of CAddrInfo is limited to callsites that import
addrman_impl.h, only objects in addrman.cpp or the tests have access. Thus we
can remove calling them friends and make the members public.
Now that no bitcoind callers require knowledge of the CAddrInfo object, it can
be moved into the test-only header file.
Review hint: use git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-all-space
CAddrInfo objects are an implementation detail of how AddrMan manages and adds
metadata to different records. Encapsulate this logic by updating Select &
SelectTriedCollision to return the additional info that the callers need.
Introduce the pimpl pattern for CAddrMan to separate the implementation details
from the externally used object representation. This reduces compile-time
dependencies and conceptually clarifies AddrMan's interface from the
implementation specifics.
Since the unit & fuzz tests currently rely on accessing CAddrMan internals, this
commit introduces addrman_impl.h, which is exclusively imported by addrman.cpp
and test files.
Review hint: git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-all-space
Also move `Check` and `ForceCheckAddrman` to be after the `FunctionName_` functions.
Review hint: use git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-all-space
240ea294d5 doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter (BitcoinTsunami)
d6eb39af21 test: add functional test to check transaction time determination during block rescanning (BitcoinTsunami)
07b44f16e7 wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning (BitcoinTsunami)
Pull request description:
The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order.
Moreover the 'smarttime' determination algorithm is coded with realtime scenario in mind and not rescanning of old block.
The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination.
In the context of rescanning old block, the only time value that as a meaning is the blocktime.
That's why I've fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process.
This PR Fixes#20181.
To be fair, I don't think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan.
But RPC API provide the possibility to trigger it. I've discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).
ACKs for top commit:
jonatack:
ACK 240ea294d5 per `git diff b92d552 240ea29`, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions
meshcollider:
re-utACK 240ea294d5
Tree-SHA512: 514b02e41d011ddfa325f5e8080b93800e1ea4ed5853fa420670a6ac700e8b463000dbea65f8ced8565cfb950c7f51b69154034dcb111e67aca3b964a0061494
ea31caf6b4 update estimatesmartfee rpc to return max of estimateSmartFee, mempoolMinFee and minRelayTxFee. (pranabp-bit)
Pull request description:
This PR is in response to the issue [#19699](https://github.com/bitcoin/bitcoin/issues/19699).
Based on the discussion in the comments of PR [#22673](https://github.com/bitcoin/bitcoin/pull/22673) changes have been made in the `estimatesmartfee` itself such that it takes into account `mempoolMinFee` and `relayMinFee` . Hence it provides a fee estimate that is most likely to be paid by the user in an actual transaction, preventing issues such as [#16072](https://github.com/bitcoin/bitcoin/issues/16072).
The test file test/functional/feature_fee_estimation.py has also been updated to check this functionality.
ACKs for top commit:
meshcollider:
re-utACK ea31caf6b4
Tree-SHA512: 8f36153a07cbd552c5c13d11d9c6e987a7a555ea4cc83f2573c0c92dd97c706d90c30a7248671437c2f3a836d3272f8fad53d15a5fa6efaa0409ae8009b0a18d
43cd6b8af9 doc: add release notes for removal of the -deprecatedrpc=addresses flag (Michael Dietz)
2b1fdc2c6c refactor: minor styling, prefer snake case and same line if (Michael Dietz)
d64deac7b8 refactor: share logic between ScriptPubKeyToUniv and ScriptToUniv (Michael Dietz)
8721638daa rpc: remove deprecated addresses and reqSigs from rpc outputs (Michael Dietz)
Pull request description:
Resolves#21797 now that we've branched-off to v23 ("addresses" and "reqSigs" deprecated) "ExtractDestinations" should be removed.
`-deprecatedrpc=addresses` was initially added in this PR #20286 (which resolved the original issue #20102).
Some chunks of code and logic are no longer used/necessary with the removal of this, and therefore some minor refactoring is done in this PR as well (separated commits)
ACKs for top commit:
MarcoFalke:
re-ACK 43cd6b8af9🐉
meshcollider:
Code review ACK 43cd6b8af9
jonatack:
ACK 43cd6b8af9 per `git range-diff a9d0cec 92dc5e9 43cd6b8`, also rebased to latest master, debug built + quick re-review of each commit to bring back context, and ran tests locally at the final commit
Tree-SHA512: fba83495e396d3c06f0dcf49292f14f4aa6b68fa758f0503941fade1a6e7271cda8378e2734af1faea550d1b43c85a36c52ebcc9dec0732936f9233b4b97901c
03a5fe06bd qt: Keep InitExecutor in main gui thread (João Barbosa)
Pull request description:
The `InitExecutor` constructor moves the instance to a dedicated thread. This PR changes that by using `GUIUtil::ObjectInvoke` to run the relevant code in that thread.
A possible follow-up is to ditch the dedicated thread and use `QThreadPool` or even `QtConcurrent::run` (if we want to enable that).
ACKs for top commit:
hebasto:
ACK 03a5fe06bd, tested on Linux Mint 20.2 (Qt 5.12.8).
jarolrod:
ACK 03a5fe06bd
Tree-SHA512: 8b40300371d4c04efb9913600a06ba4899af0b5f50fdb26ea23ec751df6d3bd52f8bd693a5e8f6a94ebf3790583dc96c6070e6878d247dece62347aa9bd99031
In preparation for introducing the pimpl pattern to addrman, move all function
bodies out of the header file.
Review hint: use git diff --color-moved=dimmed-zebra
--color-moved-ws=ignore-all-space
This will provide better estimates which would be closer to fee paid in actual
transactions.
The test has also been changed such that when the node is restarted with a
high mempoolMinFee, the estimatesmartfee still returns a feeRate greater
than or equal to the mempoolMinFee, minRelayTxFee.(just like the feeRate of actual transactions)
7e3ee4cdd0 GUI: Ask user to unlock wallet before signing psbt (Samuel Dobson)
0f3acecf33 Add test that walletprocesspsbt requires unlocked wallet when signing (Samuel Dobson)
0e895212bb Ensure wallet is unlocked before signing in walletprocesspsbt (Samuel Dobson)
Pull request description:
If signing a PSBT, we need to ensure the wallet is unlocked.
Fixes#22874, fixesbitcoin-core/gui#312
ACKs for top commit:
achow101:
ACK 7e3ee4cdd0
lsilva01:
Code Review ACK 7e3ee4cdd0
benthecarman:
ACK 7e3ee4cdd0
Tree-SHA512: 6726a873582747900ab454ea21153a92be86808a4c1517dc2856b389876a2da9e8df1ffa9b567b6bd017038342c3544ecf5ca3c97744e7debe0a5ee65563687d
efcaefc7b5 test: Add remaining scenarios of 0 waste (rajarshimaitra)
Pull request description:
As per the [review club](https://bitcoincore.reviews/22009) discussion on #22009 , it was observed that there were other two fee scenarios in which selection waste could be zero.
These are:
- (LTF - Fee) == Change Cost
- (LTF - Fee) == Excess
Even though these are obvious by the definition of waste metric, adding tests for them can be helpful in explaining its behavior
to new readers of the code base, along with pinning the behavior for future.
This PR adds those two cases to waste calculation unit test.
Also let me know if I am missing more scenarios.
ACKs for top commit:
jonatack:
Tested re-ACK efcaefc7b5
achow101:
ACK efcaefc7b5
meshcollider:
ACK efcaefc7b5
Tree-SHA512: 13fe3e2c0ea7bb58d34e16c32908b84705130dec16382ff941e5e60ca5b379f9c5811b33f36c4c72d7a98cfbb6af2f196d0a69e96989afa4b9e49893eaadd7cb
With this change, we get more fine-grained error messages if something
goes wrong in the course of communicating with the SQLite database. To
pick some random examples, the error codes SQLITE_IOERR_NOMEM,
SQLITE_IOERR_CORRUPTFS or SQLITE_IOERR_FSYNC are way more specific than just a
plain SQLITE_IOERR, and the corresponding error messages generated by
sqlite3_errstr() will hence give a better hint to the user (or also to the
developers, if an error report is sent) what the cause for a failure is.
93b9800fec scripted-diff: Rename overloaded int GetArg to GetIntArg (Russell Yanofsky)
Pull request description:
This is meant to improve readability of code and remove guesswork needed to determine argument types and migrate to [typed arguments (#22978)](https://github.com/bitcoin/bitcoin/issues/22978) by having distinctly named `GetArg` `GetArgs` `GetBoolArg` and `GetIntArg` methods.
---
This commit was originally part of #22766 and had some review discussion there. But it was [wisely suggested](https://github.com/bitcoin/bitcoin/pull/22766#issuecomment-910001542) to be split off to make that PR smaller.
ACKs for top commit:
hebasto:
ACK 93b9800fec.
MarcoFalke:
re-ACK 93b9800fec📨
Tree-SHA512: e034bd938b2c8fbadd90bcd52213a61161965dfddf18c2cb0d68816ecf2438cde8afee6fb7e3418f5c6b35c208338da9deb99e4e418dbf68ca34552e0916a625
dc10ca346b net: switch to signet DNS seed (Sjors Provoost)
Pull request description:
I spun up a DNS seed for Signet, source: https://github.com/sipa/bitcoin-seeder/pull/94
If anyone else spins up a DNS seed, let me know in the comment and I'll add it.
Because one DNS seed is not very diverse, this PR leaves two hardcoded nodes just in case (). The one dropped node no longer exists.
Replaces #23000.
ACKs for top commit:
kallewoof:
utACK dc10ca346b
laanwj:
Concept and code review ACK dc10ca346b
jarolrod:
ACK dc10ca346b
shaavan:
ACK dc10ca346b
Tree-SHA512: 534d189becd51974042fddc3efe3df230484f05cd945e756eaf6a4a8a580e2161b3a959eb3d44dea526269eea6f87a033f7cbfe86586782c6ca4ee7c7c4097a9
b8cd2a4292 Add references for the generator/constant used in Bech32(m) (Pieter Wuille)
Pull request description:
I often find myself recreating this, or looking up references for this construction. So instead, this seems like as good a place as any to place a summary.
ACKs for top commit:
Zero-1729:
crACK b8cd2a4292
Tree-SHA512: 9d2001c5016485cea441c28fda093d67a7d4274e4c1e4dd3d357353ce6a52987e38d684d8462bad2d72ba0b6b1db2f809948e228fb02371e64b12146aace89bd
d2eccacd18 doc: Clarify that change_cost cannot be negative in GetSelectionWaste (benthecarman)
Pull request description:
We assert that the `change_cost` must be positive so we should document it in the function's docs
4f5ad43b1e/src/wallet/coinselection.cpp (L361)
ACKs for top commit:
jonatack:
ACK d2eccacd18
jarolrod:
ACK d2eccacd18
shaavan:
ACK d2eccacd18
Tree-SHA512: 089bc41d7e212b811455527ce2ac83301eae0edc03933dcbd5229b22078827395c8e3bcfda0ba029b7c60705db3119b4f80e372842a6bd8dae195dbaa0b52833
Before this patch, the log might be corrupted by other threads logging
at the same time. For example, another RPC thread:
[httpworker.1] [default wallet] keypool reserve 1296
[httpworker.1] SelectCoins() best subset: Received a POST request for / from 127.0.0.1:53732
[httpworker.3] ThreadRPCServer method=getnetworkinfo user=__cookie__
[httpworker.1] 0.78125 0.1953125 0.02441406 0.00610351 0.00305175 0.00152587 total 1.01025417
Improve readability of code, simplify future scripted diff cleanup PRs, and be
more consistent with naming for GetBoolArg.
This will also be useful for replacing runtime settings type checking
with compile time checking.
-BEGIN VERIFY SCRIPT-
git grep -l GetArg | xargs sed -i 's/GetArg(\([^)]*\( [0-9]\+\|-1\|port\|BaseParams().RPCPort()\|Params().GetDefaultPort()\|_TIMEOUT\|Height\|_WORKQUEUE\|_THREADS\|_CONNECTIONS\|LIMIT\|SigOp\|Bytes\|_VERSION\|_AGE\|_CHECKS\|Checks() ? 1 : 0\|_BANTIME\|Cache\|BLOCKS\|LEVEL\|Weight\|Version\|BUFFER\|TARGET\|WEIGHT\|TXN\|TRANSACTIONS\|ADJUSTMENT\|i64\|Size\|nDefault\|_EXPIRY\|HEIGHT\|SIZE\|SNDHWM\|_TIME_MS\)\))/GetIntArg(\1)/g'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
ee7891a0c4 doc: Remove outdated comments (Hennadii Stepanov)
Pull request description:
The first removed comment was introduced in #5288, the second one in #13503.
Both are outdated since #14336.
ACKs for top commit:
duncandean:
crACK ee7891a0
Tree-SHA512: a2d6071919e81c916bfc2178109bbc464417321bcc567ed0644448c5faea8e58cb08a7657afa1b6ffe1fb63e114a2a47b31c893e471839ba9d49a3986e68b2a7
faa9c19a4b doc: Add 23061 release notes (MarcoFalke)
faff17bbde Fix (inverse) meaning of -persistmempool (MarcoFalke)
Pull request description:
Passing `-persistmempool` is currently treated as `-nopersistmempool`
ACKs for top commit:
jnewbery:
reACK faa9c19a4b
hebasto:
ACK faa9c19a4b, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: f34a89a07745dabe340eb845b2a348b79c093e9056f7a21c17e1ba2e278177c9b4cf30e8095791fd645a7f90eb34850b2eee0c869b4f6ec02bf749c73b0e52ee
d96b000e94 Make GUI UTXO lock/unlock persistent (Samuel Dobson)
077154fe69 Add release note for lockunspent change (Samuel Dobson)
719ae927dc Update lockunspent tests for lock persistence (Samuel Dobson)
f13fc16295 Allow lockunspent to store the lock in the wallet DB (Samuel Dobson)
c52789365e Allow locked UTXOs to be store in the wallet database (Samuel Dobson)
Pull request description:
Addresses and closes#22368
As per that issue (and its predecessor #14907), there seems to be some interest in allowing unspent outputs to be locked persistently. This PR does so by adding a flag to lockunspent to store the change in the wallet database. Defaults to false, so there is no change in default behaviour.
Edit: GUI commit changes default behaviour. UTXOs locked/unlocked via the GUI are now persistent.
ACKs for top commit:
achow101:
ACK d96b000e94
kristapsk:
ACK d96b000e94
lsilva01:
Tested ACK d96b000e94 on Ubuntu 20.04
prayank23:
ACK d96b000e94
Tree-SHA512: 957a5bbfe7f763036796906ccb1598feb6c14c5975838be1ba24a198840bf59e83233165cb112cebae909b6b25bf27275a4d7fa425923ef6c788ff671d7a89a8
e148a52332 bench: fixed ubsan implicit conversion (Martin Ankerl)
da4e2f1da0 bench: various args improvements (Jon Atack)
d312fd94a1 bench: clean up includes (Jon Atack)
1f10f1663e bench: add usage description and documentation (Martin Ankerl)
d3c6f8bfa1 bench: introduce -min_time argument (Martin Ankerl)
9fef832932 bench: make EvictionProtection.* work with any number of iterations (Martin Ankerl)
153e6860e8 bench: change AddrManGood to AddrManAddThenGood (Martin Ankerl)
468b232f71 bench: remove unnecessary & incorrect multiplication in MuHashDiv (Martin Ankerl)
eed99cf272 bench: update nanobench from 4.3.4 to 4.3.6 (Martin Ankerl)
Pull request description:
This PR updates the nanobench with the latest release from upstream, v4.3.6. It fixes the missing performance counters.
Due to discussions on #22999 I have done some work that should make the benchmark results more reliable. It introduces a new flag `-min_time` that allows to run a benchmark for much longer then the default. When results are unreliable, choosing a large timeframe here should usually get repeatable results even when frequency scaling cannot be disabled. The default is now 10ms. For this to work I have changed the `AddrManGood` and `EvictionProtection` benchmarks so they work with any number of iterations.
Also, this adds more usage documentation to `bench_bitcoin -h` and I've cherry-picked two changes from #22999 authored by Jon Atack
ACKs for top commit:
jonatack:
re-ACK e148a52332
laanwj:
Code review ACK e148a52332
Tree-SHA512: 2da6de19a5c85ac234b190025e195c727546166dbb75e3f9267e667a73677ba1e29b7765877418a42b1407b65df901e0130763936525e6f1450f18f08837c40c
fa4db8671b test: Activate all regtest softforks at height 1, unless overridden (MarcoFalke)
faad1e5ffd Introduce -testactivationheight=name@height setting (MarcoFalke)
fadb2ef2fa test: Add extra_args argument to TestChain100Setup constructor (MarcoFalke)
faa46986aa test: Remove version argument from build_next_block in p2p_segwit test (MarcoFalke)
fa086ef539 test: Remove unused ~TestChain100Setup (MarcoFalke)
Pull request description:
All softforks that are active at the tip of mainnet, should also be active from genesis in regtest. Otherwise their rules might not be enforced in user testing, thus making their testing less useful.
To still allow tests to check pre-softfork rules, a runtime argument can change the activation height.
ACKs for top commit:
laanwj:
Code review ACK fa4db8671b
theStack:
re-ACK fa4db8671b
Tree-SHA512: 6397d46ff56ebc48c007a4cda633904d6ac085bc76b4ecf83097c546c7eec93ac0c44b88083b2611b9091c8d1fb8ee1e314065de078ef15e922c015de7ade8bf
There are two more cases where waste can be 0, when:
- (Fee - LTF) == -Change Cost
- (Fee - LTF) == -Excess
Adding these two conditions explicitly in the unit test will help
pin the behavior, also demonstrate waste calculation scenarios to new
readers.
It was [pointed out in #23030](https://github.com/bitcoin/bitcoin/pull/23030#issuecomment-922893367) that we might be able to get rid of our weak linking of [`getauxval()`](https://man7.org/linux/man-pages/man3/getauxval.3.html) (`HAVE_WEAK_GETAUXVAL`) entirely, with only Android being a potential holdout:
> I wonder if it's time to get rid of HAVE_WEAK_GETAUXVAL. I think it's confusing. Either we build against a C library that has this functionality, or not. We don't do this weak linking thing for any other symbols and recently got rid of the other glibc backwards compatibility stuff.
> Unless there is still a current platform that really needs it (Android?), I'd prefer to remove it from the build system, it has caused enough issues.
After looking at Android further, it would seem that given we are moving to using `std::filesystem`, which [requires NDK version 22 and later](https://github.com/android/ndk/wiki/Changelog-r22), and `getauxval` has been available in the since [API version 18](https://developer.android.com/ndk/guides/cpu-features#features_using_libcs_getauxval3), that shouldn't really be an issue. Support for API levels < 19 will be dropped with the NDK 24 release, and according to [one website](https://apilevels.com/), supporting API level 18+ will cover ~99% of devices. Note that in the CI we currently build with NDK version 22 and API level 28.
The other change in this PR is removing the include of headers for ARM intrinsics, from the check for strong `getauxval()` support in configure, as they shouldn't be needed. Including these headers also meant that the check would basically only succeed when building for ARM. This would be an issue if we remove weak linking, as we wouldn't detect `getauxval()` as supported on other platforms. Note that we also use `getauxval()` in our RNG when it's available.
I've checked that with these changes we detect support for strong `getauxval()` on Alpine (muslibc). On Linux, previously we'd be detecting support for weak getauxval(), now we detect strong support. Note that we already require glibc 2.17, and `getauxval()` was introduced in `2.16`.
This is an alternative / supersedes #23030.
This ensures that if we're going to add an action to open up
a transaction in a third-party link (block explorer) that it
is seperated into it's own section.
The text for an open third-party tx URL action
is improved by appending the host name with "Show in".
This makes it self-explanatory what the action will do.
673a5bd337 test: validation: add unittest for UpdateTip behavior (James O'Beirne)
2705570109 test: refactor: separate CreateBlock in TestChain100Setup (James O'Beirne)
298bf5d563 test: refactor: declare NoMalleation const auto (James O'Beirne)
071200993f move-only: unittest: add test/util/chainstate.h (James O'Beirne)
8f5710fd0a validation: fix CheckBlockIndex for multiple chainstates (James O'Beirne)
5a807736da validation: insert assumed-valid block index entries into candidates (James O'Beirne)
01a9b8fe71 validation: set BLOCK_ASSUMED_VALID during snapshot load (James O'Beirne)
42b2520db9 chain: add BLOCK_ASSUMED_VALID for use with assumeutxo (James O'Beirne)
b217020df7 validation: change UpdateTip for multiple chainstates (James O'Beirne)
665072a36d doc: add comment for g_best_block (James O'Beirne)
ac4051d891 refactor: remove unused assumeutxo methods (James O'Beirne)
9f6bb53935 validation: add chainman ref to CChainState (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Modify UpdateTip and CheckBlockIndex for use with multiple chainstates. Includes a new unittest verifying `g_best_block` behavior (previously untested at the unit level) and various changes necessary for running and testing `ProcessNewBlock()`-like behavior on the background validation chainstate.
This changeset introduces a new block index `nStatus` flag called `BLOCK_ASSUMED_VALID`, and it is applied to block index entries that are beneath the UTXO snapshot base block upon snapshot load. Once each block is validated (during async background validation), the flag is removed. This allows us to avoid (ab)using `BLOCK_VALID_*` flags for snapshot chain block entries, and preserves the original meaning of those flags.
Note: this PR previously incorporated changes to `LoadBlockIndex()` and `RewindBlockIndex()` as noted in Russ' comments below, but once I generated the changes necessary to test the UpdateTip change, I decided to split this changes out into another PR due to the size of this one.
ACKs for top commit:
achow101:
ACK 673a5bd337
jonatack:
Code-review re-ACK 673a5bd337 reviewed diff, rebased to master/debug build/ran unit+functional tests
naumenkogs:
ACK 673a5bd337
fjahr:
Code review ACK 673a5bd337
ariard:
utACK 673a5bd3
ryanofsky:
Code review ACK 673a5bd337. Just linker fix and split commit changes mentioned https://github.com/bitcoin/bitcoin/pull/21526#issuecomment-921064563 since last review
benthecarman:
ACK 673a5bd337
Tree-SHA512: 0a6dc23d041b27ed9fd0ee1f3e5971b92fb1d2df2fc9b655d5dc48594235321ab1798d06de2ec55482ac3966a9ed56de8d56e9e29cae75bbe8690bafc2dda383
64e1ddd255 log: call LogPrint only once with time data samples (Martin Zumsande)
Pull request description:
When timedata samples are logged, `LogPrint()` is currently invoked multiple times on the same log entry.
This can lead to chaos in the log when other threads log concurrently, as in this example which motivated this PR:
```
2021-09-20T00:28:57Z -48 -26 -11 -8 -6 Addrman checks started: new 37053, tried 83, total 37136
2021-09-20T00:28:57Z -3 -1 -1 -1 -1 +0 | nTimeOffset = -3 (+0 minutes)
```
Fix this by building the log message in a string and logging it one `LogPrint()` call. I also changed the wording slightly so that it becomes understandable what is being logged, example:
```
2021-09-21T21:03:24Z time data samples: -43 -18 -12 -4 -1 -1 +0 +0 +268 | median offset = -1 (+0 minutes)
```
ACKs for top commit:
jnewbery:
Tested ACK 64e1ddd255
laanwj:
Tested ACK 64e1ddd255, new message lgtm
Tree-SHA512: ffb7a93166cc8fd6a39200b9e03a9d1e8e975b7ded822ccddd015f553258b991162a5cb867501f426d3ebcfef4f32f0e06e17b18e6b01486b967595d102f8379
ab27800799 log: Remove unnecessary timing logs for Callbacks bench (Douglas Chimento)
Pull request description:
Logging of Callbacks are no longer needed and records times that are not relevant for performance analysis.
resolves#23071
ACKs for top commit:
laanwj:
Thanks. re-ACK ab27800799
jonatack:
Code review ACK ab27800799
Tree-SHA512: be1ea780c4db9407a8799065a8824b9d3610abac72af5907809ed62d493d5a54e65735de45ec5fdd0edb85ef21ec6036105abe8ca00093942980f6f92e7fec50
0ef08f8bed add missing includes in policy/rbf (glozow)
c6abeb76fb make MAX_BIP125_RBF_SEQUENCE constexpr (glozow)
3cf46f6055 [doc] improve RBF documentation (glozow)
c78eb8651b [policy/refactor] pass in relay fee instead of using global (glozow)
Pull request description:
Followups to #22675 and documentation-only changes intended to clarify the code/logic concerning mempool Replace-by-Fee.
ACKs for top commit:
jnewbery:
utACK 0ef08f8bed
fanquake:
ACK 0ef08f8bed
Tree-SHA512: 6797ae758beca0c9673cb00ce85da48e9a4ac5cb5100074ca93e004cdb31d24d91a1a7721b57fc2f619addfeb4950d8caf45fee0f5b7528defbbd121eb4d271f
fa08d4cfb1 Use C++11 member initializer in CTxMemPoolEntry (MarcoFalke)
Pull request description:
This removes a bunch of boilerplate, makes the code easier to read.
Also, C++11 member initialization avoids accidental uninitialized
members.
Can be reviewed with the git option "--word-diff-regex=." or with "git
difftool --tool=meld".
ACKs for top commit:
jnewbery:
Code review ACK fa08d4cfb1
shaavan:
Code Review ACK fa08d4cfb1
Tree-SHA512: 2424861002fbcef2a3f01845662c115b973a7a5103f359305b5d9237c055eb003aa7646fc1cb30e6eaf90810d662f94cedc6f90795e30b56680f9c81f631d64b
fa45a1338a refactor: Remove unused validation includes (MarcoFalke)
Pull request description:
Unused includes will cause needless recompilation when headers are changed. Also, they pretend there are dependencies that don't exist.
Fix both by removing them.
ACKs for top commit:
laanwj:
Code review ACK fa45a1338a
theStack:
ACK fa45a1338a♻️
Tree-SHA512: 69190fd09184b75bce34ce3f315a1817e09ea32779f9ddc2d4790c89b0887b6cebd88aba66fa054c43c9183fc66202a556d982dd7034fc389a75802d8aaac83a
Can be reviewed with -W --ignore-all-space
Fixes:
* Calling ConsumeRandomLengthByteVector 4 times, when 2 is enough.
* Slow execution speed: Finalize is expensive because it invokes
division. Speed up the target by calling Finalize() at most twice per
fuzz input.
This removes a bunch of boilerplate, makes the code easier to read.
Also, C++11 member initialization avoids accidental uninitialized
members.
Can be reviewed with the git option "--word-diff-regex=." or with "git
difftool --tool=meld".
The benchmarks can now run much longer due to the minimum of 10ms or
directly with -min_time. With -min_time=20000 I could trigger two ubsan
errors in the benchmarks, which are fixed in this commit by using
unsigned type and adding "& 0xFF".
- use ALLOW_BOOL for -list arg instead of ALLOW_ANY
- touch up `-asymptote=<n1,n2,n3...>` help
- pack Args struct a bit more efficiently
- handle args in alphabetical order
When it is not easily possible to stabilize benchmark machine and code
the argument -min_time can be used to specify a minimum duration
that a benchmark should take. E.g. choose -min_time=1000 if you
are willing to wait about 1 second for each benchmark result.
The default is now set to 10ms instead of 0, which should make runs on
fast machines more stable with negligible slowdown.
Moves copying of the setup into the benchmark loop so it is possible
to run the loop for an arbitrary number of times.
The overhead due to copying the candidates inside the loop is about 3%.
Moves some of the setup into the benchmark loop so it is possible to run
the loop for an arbitrary number of times. Due to recent optimizations
in #22974 the benchmark now runs much faster, so the inner loop now calls
Good() 32 times as often to get better numbers.
Renamed the benchmark to AddrManAddThenGood because that's now what is
actually tested. To get the the number of just Good(), one needs to
subtract the benchmark result of AddrManAdd.
Introduced in #19055, MuHashDiv benchmark used to multiply with a loop
based on epochIterations. That does not do what it is supposed to do,
because epochIterations() is determined automatically from nanobench.
Also, multiplication is not needed for the algorithm (as pointed out by
a comment in #19055), so it's better to remove this loop.
Most importantly, this update fixes a bug in nanobench that always
disabled performance counters on linux.
It also adds another sanitizer suppression that is caught in clang++ 12.
This speeds up compilation of the whole program because the included
header file is smaller.
Can be reviewed with --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
cdaab90662 Add test for addrman consistency check on restart with asmap (Jon Atack)
869f136816 Add test for rpc addpeeraddress with "tried" argument (Jon Atack)
ef242f5213 Allow passing "tried" to rpc addpeeraddress to call CAddrMan::Good() (Jon Atack)
Pull request description:
This pull adds a `tried` argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue.
PR #22697 introduced a reproducible bug in commit 181a1207 that fails addrman consistency checks and causes it to significantly lose peer entries when the `-asmap` configuration option is used.
The issue occurs upon bitcoind restart due to an initialization order change in `src/init.cpp` in that commit, whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before.
Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263.
```
addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses
ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted
```
How to reproduce:
- `git checkout 181a1207`, build, and launch bitcoind with the `-asmap` and `-checkaddrman=1` configuration options enabled
- restart bitcoind
- bitcoind aborts on the second call to the addrman consistency checks in `CAddrMan::Check()`
How to test this pull:
- `git checkout 181a1207`, cherry pick the first commit of this branch, build, git checkout this branch, run `test/functional/rpc_net.py`, which should pass, and then run `test/functional/feature_asmap.py`, which should fail with the following output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed.
```
ACKs for top commit:
jnewbery:
utACK cdaab90662
mzumsande:
re-ACK cdaab90662 (based on code review of diff to d586817)
vasild:
ACK cdaab90662
Tree-SHA512: 0251a18fea629b62486fc907d7ab0e96c6df6fadb9e4d62cff018bc681afb6ac31e0e7258809c0a88f91e4a36c4fb0b16ed294ce47ef30585217de89c3342399
c17f554fcc Fix BlockAssembler::AddToBlock, CTxMemPool::PrioritiseTransaction logging (Jon Atack)
Pull request description:
This is a tale of two fees, er, fee rates... indeed, one is misdescribed as a fee, and the other is incorrectly called a fee rate.
From this review discussion: https://github.com/bitcoin/bitcoin/pull/22689#discussion_r695866211 (thanks to John Newbery).
ACKs for top commit:
laanwj:
Code review ACK c17f554fcc
Tree-SHA512: 3d9df3209a72562c5f9bbf815923d5b089d04491b8d19caa2c04158c501b47ef01e47f1c32d89adcbaf3c6357329507f65b4bb2963214c3451bbfa61ac812530
9bdda50151 Enable TLS in links in documentation (Jeremy Rand)
Pull request description:
This PR enables TLS in several documentation links, which improves security.
ACKs for top commit:
fanquake:
ACK 9bdda50151
Tree-SHA512: 9d04d8771a9daf3c3b9914ff324e2eabfdf3ff5ae7f7dc92b84a1f3527010ceb860e73873a8f24d6051763eb472d9ea324ccbd6129a40318a520ca88c05f0586
57ce20307e fuzz: allow lower number of sources (Martin Zumsande)
acf656d540 fuzz: Use public interface to fill addrman tried tables (Martin Zumsande)
eb2e113df1 addrman: Improve performance of Good (Martin Zumsande)
Pull request description:
Currently, `CAddrman::Good()` is rather slow because the process of moving an addr from new to tried involves looping over the new tables twice:
1) In `Good_()`, there is a loop searching for a new bucket the addr is currently in, but this information is never used except for aborting if it is not found anywhere (since [this commit](e6b343d880 (diff-49d1faa58beca1ee1509a247e0331bb91f8604e30a483a7b2dea813e6cea02e2R263)) it is no longer passed to `MakeTried`)
This is unnecessary because in a non-corrupted addrman, an address that is not in New must be either in Tried or not at all in addrman, both cases in which we'd return early in `Good_()` and never get to this point.
I removed this loop (and left a check for `nRefCount` as a belt-and-suspenders check).
2) In `MakeTried()`, which is called from `Good_()`, another loop removes all instances of this address from new. This can be spedup by stopping the search at `nRefCount==0`. Further reductions in `nRefCount` would only lead to an assert anyway.
Moreover, the search can be started at the bucket determined by the source of the addr for which `Good` was called, so that if it is present just once in New, no further buckets need to be checked.
While calls to `Good()` are not that frequent normally, the performance gain is clearly seen in the fuzz target `addman_serdeser`, where, because of the slowness in creating a decently filled addrman, a shortcut was created that would directly populate the tried tables by reaching into addrman's internals, bypassing `Good()` (#21129).
I removed this workaround in the second commit: Using `Good()` is still slower by a factor of 2 (down from a factor of ~60 before), but I think that this compensated by the advantages of not having to reach into the internal structures of addrman (see https://github.com/jnewbery/bitcoin/pull/18#issuecomment-775218676).
[Edit]: For benchmark results see https://github.com/bitcoin/bitcoin/pull/22974#issuecomment-919435266 and https://github.com/bitcoin/bitcoin/pull/22974#issuecomment-920445700 - the benchmark `AddrManGood` shows a significant speedup by a factor >100.
ACKs for top commit:
naumenkogs:
ACK 57ce20307e
jnewbery:
ACK 57ce20307e
laanwj:
Code review ACK 57ce20307e
theStack:
ACK 57ce20307e
vasild:
ACK 57ce20307e
Tree-SHA512: fb6dfc198f2e28bdbb41cef9709828f22d83b4be0e640a3155ca42e771b6f58466de1468f54d773e794f780a79113f9f7d522032e87fdd75bdc4d99330445198
