Mostly changes to remove src/univalue exceptions from the various linters,
and the required code changes to make them happy. As well as minor doc
changes.
fafddafc2c refactor: Introduce PeerManagerImpl::RejectIncomingTxs (MacroFake)
Pull request description:
Currently there are some confusions in net_processing:
* There is confusion between `-blocksonly mode` and `block-relay-only`, so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect `block-relay-only` peers with `relay` permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: https://github.com/bitcoin/bitcoin/pull/17167
ACKs for top commit:
MarcoFalke:
Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa2b5fe0c1 fafddafc2c`.
jnewbery:
Code review ACK fafddafc2c
mzumsande:
ACK fafddafc2c
Tree-SHA512: 73bf91afe93be619169cfbf3bf80cb08a5e6f73df4e0318b86817bd4d45f67408ea85998855992281d2decc9d24f7d75cffb83a0518d670090907309df8a3490
018d70b587 scripted-diff: Avoid incompatibility with CMake AUTOUIC feature (Hennadii Stepanov)
Pull request description:
Working on [migration](https://github.com/hebasto/bitcoin/pull/3) from Autotools to CMake build system, I found that our current code base needs to be adjusted.
CMake [allows](https://cmake.org/cmake/help/latest/prop_tgt/AUTOUIC.html) to
> handle the Qt `uic` code generator automatically
When using this feature, statements like `#include "ui_<ui_base>.h"` are processed in a special way.
The `node/ui_interface.h` unintentionally breaks this feature. Of course, it is possible to provide a list of source files to be excluded from `AUTOUIC`. But, unfortunately, this approach does not work for the `qt/sendcoinsdialog.cpp` source file, where there are both b71d37da2c/src/qt/sendcoinsdialog.cpp (L10) and b71d37da2c/src/qt/sendcoinsdialog.cpp (L24)
ACKs for top commit:
MarcoFalke:
cr ACK 018d70b587
ryanofsky:
Code review ACK 018d70b587
furszy:
Code review ACK 018d70b5
Tree-SHA512: 4fc83f2e5a82c8ab15c3c3d68f48b9863c47b96c0a66b6276b9b4dfc6063abffd73a16382acfe116553487b3ac697dbde2d9ada1b92010c5d8f8c6aa06f56428
ecff20db28 logging: use LogPrintfCategory rather than a manual category (Jon Atack)
eb8aab759f logging: add LogPrintfCategory to log unconditionally with category (Jon Atack)
Pull request description:
These are the next two commits from #25203.
- Add `LogPrintfCategory` to log unconditionally while prefixing the output with the passed category name. Add documentation and a unit test, and update the `lint-logs.py` and `lint-format-strings.py` scripts.
- Replace the log messages that manually print a category, with `LogPrintfCategory`. In upcoming commits, it will likely be used in many other cases, such as to replace `LogPrintf` where it makes sense.
ACKs for top commit:
klementtan:
Code Review ACK ecff20db28
laanwj:
Code review ACK ecff20db28
brunoerg:
ACK ecff20db28
Tree-SHA512: ad3a82835254f7606efcd14b88f3d9072f1eb9b25db1321ed38ef6a4ec60efd555d78f5e19d93736f2f8500251d06f8beee9d694a153f24bf5cce3590a2a45a5
1cb42aeda3 util: modify Win32LockedPageAllocator to query windows for limit (Oskar Mendel)
Pull request description:
This PR resolves a todo within the Win32LockedPageAllocator: `// TODO is there a limit on Windows, how to get it?`.
The idea is to use the Windows API to get the limits like the posix based allocator does with `getrlimit`.
I use [GetProcessWorkingSetSize](https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-getprocessworkingsetsize) to perform this task and fallback to `return std::numeric_limits<size_t>::max();` just like the posix implementation does.
ACKs for top commit:
sipsorcery:
tACK 1cb42aeda3.
Tree-SHA512: 7bdd8a57a4e64ee59d752417a519656e03526878462060753be4dce481eff4889fb5edc1bdbd575b707d9b2dfe255c87da9ef67baac97de9ac5e70a04c852081
Currently there are some confusions in net_processing:
* There is confusion between `-blocksonly mode` and `block-relay-only`,
so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented
differently. For example, it seems a bit confusing to disconnect
`block-relay-only` peers with `relay` permission when they send a tx
message, but not when they send an inv message. Also, keeping track of
their inv announcements seems both wasteful and confusing, as it does
nothing. This isn't possible in practice, as outbound connections do
not have permissions assigned, but sees fragile to rely on. Especially
in light of proposed changes to make that possible:
https://github.com/bitcoin/bitcoin/pull/17167
e47c6c7656 Reset settings.json when GUI options are reset (Ryan Ofsky)
99ccc02b65 Add release notes about unified bitcoin-qt and bitcoind persistent settings (Ryan Ofsky)
504b06b1de Migrate -lang setting from QSettings to settings.json (Ryan Ofsky)
9a016a3c07 Migrate -prune setting from QSettings to settings.json (Ryan Ofsky)
f067e19433 Migrate -proxy and -onion settings from QSettings to settings.json (Ryan Ofsky)
a09e3b7cf2 Migrate -listen and -server settings from QSettings to settings.json (Ryan Ofsky)
d2ada6e635 Migrate -upnp and -natpmp settings from QSettings to settings.json (Ryan Ofsky)
1dc4fc29c1 Migrate -spendzeroconfchange and -signer settings from QSettings to settings.json (Ryan Ofsky)
a7ef6d5975 Migrate -par setting from QSettings to settings.json (Ryan Ofsky)
284f339de6 Migrate -dbcache setting from QSettings to settings.json (Ryan Ofsky)
Pull request description:
If a setting like pruning, port mapping, or a network proxy is enabled in the GUI, it will now be stored in the bitcoin persistent setting file in the datadir and shared with bitcoind, instead of being stored as Qt settings which end up in the the windows registry or platform specific config files and are ignored by bitcoind.
This PR has been split off from bitcoin/bitcoin#15936 so some review of these commits previously took place in that PR.
ACKs for top commit:
furszy:
Code review ACK e47c6c76
hebasto:
ACK e47c6c7656
Tree-SHA512: 076ea7c7efe67805b4a357113bfe1643dce364d0032774106de59566a0ed5771d57a5923920085e03d686beb34b98114bd278555dfdf8bb7af0b778b0f35b7d2
1f653dc262 qt, wallet, refactor: Make `WalletModel::sendCoins()` return `void` (Hennadii Stepanov)
Pull request description:
Currently, the `WalletModel::sendCoins()` function always returns the same value.
Also dead and noop (calling `processSendCoinsReturn(OK)`) code has been removed.
The other `return` statements have been removed from the `WalletModel::sendCoins()` function in bitcoin/bitcoin#17154 and bitcoin/bitcoin#17165.
ACKs for top commit:
kristapsk:
cr ACK 1f653dc262
furszy:
Code review ACK 1f653dc2
shaavan:
Code Review ACK 1f653dc262
w0xlt:
Code Review ACK 1f653dc262
Tree-SHA512: 2b59495a7fc10b4de30fcc63fc3af92d50406e16031112eb72494736dce193ac1fbac0802623496cf81edcd16766e1647d9c4f3a607b3eb84cc50e273b999c04
44c2452fd Merge bitcoin-core/secp256k1#1105: Don't export symbols in static libraries
6f6cab998 abi: Don't export symbols in static Windows libraries
485f608fa Merge bitcoin-core/secp256k1#1104: Fix the false positive of `SECP_64BIT_ASM_CHECK`
8b013fce5 Merge bitcoin-core/secp256k1#1056: Save negations in var-time group addition
7efc9835a Fix the false positive of `SECP_64BIT_ASM_CHECK`
2f984ffc4 Save negations in var-time group addition
git-subtree-dir: src/secp256k1
git-subtree-split: 44c2452fd387f7ca604ab42d73746e7d3a44d8a2
Achieve this by adding a MAIN_FUNCTION macro, consolidating the docs, and
introducing the macro across our distributed binaries.
Also update the docs to explain that anyone using binutils < 2.36 is
effected by this issue. Release builds are not, because they use binutils
2.37. Currently LTS Linux distros, like Ubuntu Focal, ship with 2.34.
https://packages.ubuntu.com/focal/binutils
If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.
ce893c0497 doc: Update developer notes (Anthony Towns)
d2852917ee sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37 net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497🐦
hebasto:
ACK ce893c0497
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
Code introduced in #15649 added usage of `timingsafe_bcmp()`, if
available, otherwise falling back to our own implementation. However
the relevant build system check was never added, so currently, we'll
always just use our implementation, as HAVE_TIMINGSAFE_BCMP will never
be defined.
Add the check for timingsafe_bcmp. Note that as far as I'm aware, it's
only available on OpenBSD.
Rename `GenerateSelectSet()` to `GenerateWaitSockets()` and adapt it to
generate a wait data suitable for `Sock::WaitMany()`. Then call it from
`CConnman::SocketHandler()` and feed the generated data to
`Sock::WaitMany()`.
This way `CConnman::SocketHandler()` can be unit tested because
`Sock::WaitMany()` is mockable, so the usage of real sockets can be
avoided.
Resolves https://github.com/bitcoin/bitcoin/issues/21744
d575413fb8 doc: add `desig` to ignore-words (brunoerg)
c06cc41ddb doc: fix typo in kernel/context.h (brunoerg)
Pull request description:
This PR fixes a typo in `kernel/context.h` (libary => library) and add `desig` to ignore-words since it's a valid word, see:
b9416c3847/src/net.cpp (L1105-L1117)
ACKs for top commit:
fanquake:
ACK d575413fb8
Tree-SHA512: 2d548c737b8184d0243445c7503f3f68256ecb0970bd834d52de099de3cd8c8b9c140e2b77d55e2542fbd45b1d21cbdee639f5b2ef8138c37b8b72e5211029c3
It allows waiting concurrently on more than one socket. Being a
`virtual` `Sock` method it can be overriden by tests.
Will be used to replace `CConnman::SocketEvents()`.
8888bd43c1 Remove redundant nLastTry check (MarcoFalke)
00001e57fe Remove redundant nTime checks (MarcoFalke)
Pull request description:
Split out from https://github.com/bitcoin/bitcoin/pull/24697 because it makes sense on its own.
ACKs for top commit:
dergoegge:
re-ACK 8888bd43c1
naumenkogs:
utACK 8888bd43c1
Tree-SHA512: 32c6cde1c71e943c76b7991c2c24caf29ae467ab4ea2d758483a0cee64625190d1a833b468e8eab1f834beeb2c365af96552c14b05270f08cf63790e0707581d
We are skipping the non-spendable coins that appear in vCoins ('AvailableCoins' result) later, in several parts of the CreateTransaction and GetBalance flows:
GetAvailableBalance (1) gets all the available coins calling AvailableCoins and, right away, walk through the entire vector, skipping the non-spendable coins, to calculate the total balance.
Inside CreateTransactionInternal —> SelectCoins(vCoins,...), we have several calls to AttemptSelection which, on each of them internally, we call twice to GroupOutputs which internally has two for-loops over the entire vCoins vector that skip the non-spendable coins.
So, Purpose is not add the non-spendable coins into the AvailableCoins result (vCoins) in the first place for the processes that aren’t using them at all, so we don’t waste resources skipping them later so many times.
Note: this speedup is for all the processes that call to CreateTransaction and GetBalance* internally.
This will be used in a follow-up commit to prevent extra 'GetWalletTx' lookups if the function caller already have the wtx and can just provide the scriptPubKey directly.
Instead of accepting a `vCoins` reference that is cleared at the beginning of the method.
Note:
This new struct, down the commits line, will contain other `AvailableCoins` useful results.
Here we update only the log messages that manually print a category.
In upcoming commits, LogPrintCategory will likely be used in many
other cases, such as to replace `LogPrintf` where it makes sense.
292828cd77 [test] Test addr cache for multiple onion binds (dergoegge)
3382905bef [net] Seed addr cache randomizer with port from binding address (dergoegge)
f10e80b6e4 [net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed addr cache randomizer (dergoegge)
Pull request description:
The addr cache id randomizer is currently supposed to be seeded with the network of the inbound connection and the local socket (only the address is used not the port): a8098f2cef/src/net.cpp (L2800-L2804)
For inbound onion connections `CNode::addr.GetNetwork()` returns `NET_UNROUTABLE` and `CNode::addrBind` is set to `127.0.0.1:<onion bind port>`. This results in the same addr cache for all inbound connections on 127.0.0.1 binds.
To avoid the same addr cache across all onion and other 127.0.0.1 binds, we should seed the addr cache randomizer with the correct network for inbound onion connections (using `CNode::ConnectedThroughNetwork()`) as well as the port of `CNode::addrBind`.
ACKs for top commit:
sipa:
utACK 292828cd77
mzumsande:
Code Review ACK 292828cd77
naumenkogs:
utACK 292828cd77
Tree-SHA512: d0be13bab6bc121c2926d4b168687f6c2ed4ce0c9dd19be71eb4886adeba8afc3daacdc4e232a0ba3b03a89d69b618abc5595b69abd1ad0c476d825bc6ea1f9f
57fb37c275 wallet: CommitTransaction, remove extra wtx lookup and add exception for a possible db write error. (furszy)
Pull request description:
Two points for `CWallet::CommitTransaction`:
1) The extra wtx lookup:
As we are calling to `AddToWallet` first, which returns the recently added/updated wtx pointer, there is no need to look up the wtx again few lines later. We can just use it.
2) The db write error:
`AddToWallet` can only return a nullptr if the db write fails, which inside `CommitTransaction` translates to an exception throw cause. We expect everywhere that `CommitTransaction` always succeed.
------------------------------------------------
Extra note:
This finding generated another working path for me :)
It starts with the following question: why are we returning a nullptr from `AddToWallet` if the db write failed without removing the recently added transaction from the wallet's map?..
Can led to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.
-- I'm writing it here to gather some feedback first and not forget it, will create a follow-up PR in the coming days 🚜 --
ACKs for top commit:
achow101:
ACK 57fb37c275
jonatack:
ACK 57fb37c
ryanofsky:
Code review ACK 57fb37c275. Seems like a clear improvement. Better to fail earlier with a better error message if the failure is going to happen anyway
Tree-SHA512: 80e59c01852cfbbc70a5de1a1c2c59b5e572f9eaa08c2175112cb515256e63fa04c7942f92a513b620d6b06e66392029ebe8902287c456efdbee58a7a5ae42da
d40550d725 scripted-diff: remove duplicate categories from LogPrint output (Jon Atack)
Pull request description:
This is the first commit from #25203.
- Scripted-diff: de-duplicate logging category output for the tor, i2p, net, zmq, and prune messages (e.g. where I found duplicates), as these category prefixes are now printed automatically since #24464
examples before
```
[tor] tor: Successfully connected!
[i2p] I2P: Creating SAM session with 127.0.0.1:7656
[zmq] zmq: Initialize notification interface
[net] net: enabling extra block-relay-only peers
```
after
```
[tor] Successfully connected!
[i2p] Creating SAM session with 127.0.0.1:7656
[zmq] Initialize notification interface
[net] enabling extra block-relay-only peers
```
ACKs for top commit:
klementtan:
crACK d40550d725
MarcoFalke:
cr ACK d40550d725
Tree-SHA512: 63b799f2f899f0597981dd1acb91ef4439cd00b257a9eb19d67c4ce2c4dc72a95ac5761cb78f2a19090a10be74f23ea1db6929ed942ba0d008b4be563f0d5e7e
fa4068b4e2 Move minRelayTxFee to policy/settings (MacroFake)
Pull request description:
Seems a bit confusing to put policy stuff into validation, so fix that.
Also fix includes via `iwyu`.
ACKs for top commit:
ariard:
ACK fa4068b, the includes move compiles well locally.
ryanofsky:
Code review ACK fa4068b4e2. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.
Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
e593ae07c4 Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block (Luke Dashjr)
Pull request description:
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last block pruned was returned, subject to a bug if there were blocks left unpruned due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it makes more sense to fix the behaviour to match the documentation.
~~(Additionally, the description of "pruneheight" in getblockchaininfo is fixed to be technically correct)~~
ACKs for top commit:
fjahr:
utACK e593ae07c4
ryanofsky:
Code review ACK e593ae07c4. Just rebased since last review. Maybe some of the original reviewers of #15991 will want to take a look at this to correct the mistake that was introduced there!
Tree-SHA512: c2d511df80682d57260aae8af1665f9d7eaed16448f185f4c9f23c78fa9b8289a02053da7a0b83643fef57610d601ea63b59ff39661a51f4827f1eb27cc30594
...also adjust callers
Changes:
- In BlockAssembler::CreateNewBlock, we now only lock m_mempool->cs and
call addPackageTxs if m_mempool is not nullptr
- BlockAssembler::addPackageTxs now takes in a mempool reference, and is
annotated to require that mempool's lock.
- In TestChain100Setup::CreateBlock and generateblock, don't construct
an empty mempool, just pass in a nullptr for mempool
3a9b9bb38e test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)
Pull request description:
Fixes#25127
If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different.
However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning:
192d639a6b/src/rpc/output_script.cpp (L166-L169)
So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type.
ACKs for top commit:
jonatack:
ACK 3a9b9bb38e
laanwj:
Code review ACK 3a9b9bb38e
Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
d1b7bcbca2 qt: Drop no longer supported Android architecture (Hennadii Stepanov)
Pull request description:
The `i686-linux-android` arch support has been dropped since bitcoin/bitcoin#23744.
ACKs for top commit:
katesalazar:
ACK d1b7bcbca2
icota:
utACK d1b7bcbca2
prusnak:
Approach ACK d1b7bcbca2
Tree-SHA512: 13689ec8c63c92b9a52a3c25edc35536b8e51ff583f57c45b168515f928d020d6bb85d03db9efd8d5efd57b944dfd313a89f5ff8a52f99982ccc8d9671f6e7a9
f3a50c9dfe miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot)
c5fe5163dc miniscript: nit: don't return after assert(false) (Antoine Poinsot)
7bbaca9d8d miniscript: explicit the threshold size computation in multi() (Antoine Poinsot)
8323e4249d miniscript: add an OpCode typedef for readability (Antoine Poinsot)
7a549c6c59 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot)
8c0f8bf7bc fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot)
be34d5077b fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot)
7eb70f0ac0 miniscript: tiny doc fixups (Antoine Poinsot)
5cea85f12c miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot)
a0f064dc14 miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot)
ed45ee3882 miniscript: use optional instead of bool/outarg (Antoine Poinsot)
1ab8d89fd1 miniscript: make equality operator non-recursive (Antoine Poinsot)
5922c662c0 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot)
c5f65db0f0 miniscript: remove a workaround for a GCC 4.8 bug (Antoine Poinsot)
Pull request description:
The Miniscript repository and the Miniscript integration PR here have been a moving target for the past months, and some final cleanups were done there that were not included here. I initially intended to add some small followup commits to #24148 but i think there are enough of them to be worth a followup PR on its own.
Some parts of the code did not change since it was initially written in 2019, and the code could use some modernization. (Use std::optional instead of out args, remove old compiler workarounds).
We refactored the helpers to be more meaningful, and also did some renaming. A new fuzz target was also added and both were merged in a single file. 2 more will be added in #24149 that will be contained in this file too.
The only behaviour change in this PR is to rule out Miniscript with duplicate keys from sane Miniscripts. In a P2WSH context, signatures can be rebounded (Miniscript does not use CODESEPARATOR) and it's reasonable to assume that reusing keys across the Script drops the malleability guarantees.
It was previously assumed such Miniscript would never exist in the first place since a compiler should never create them. We finally agreed that if one were to exist (say, written by hand or from a buggy compiler) it would be very confusing if an imported Miniscript descriptor (after #24148) with duplicate keys was deemed sane (ie, "safe to use") by Bitcoin Core. We now check for duplicate keys in the constructor.
This is (still) joint work with Pieter Wuille. (Actually he entirely authored the cleanups and code modernization.)
ACKs for top commit:
sipa:
utACK f3a50c9dfe (with the caveat that a lot of it is my own code)
sanket1729:
code review ACK f3a50c9dfe. Did not review the fuzz tests.
Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb
d87784ac87 kernel: SanityChecks: Return an error struct (Carl Dong)
265d6393bf Move init::SanityCheck to kernel::SanityCheck (Carl Dong)
fed085a1a4 init: Initialize globals with kernel::Context's life (Carl Dong)
7d03feef81 kernel: Introduce empty and unused kernel::Context (Carl Dong)
eeb4fc20c5 test: Use Set/UnsetGlobals in BasicTestingSetup (Carl Dong)
Pull request description:
The full `init/common.cpp` is dependent on things like ArgsManager (which we wish to remove from libbitcoinkernel in the future) and sanity checks. These aren't necessary for libbitcoinkernel so we only extract the portion that is necessary (namely `init::{Set,Unset}Globals()`.
ACKs for top commit:
theuni:
ACK d87784ac87
vasild:
ACK d87784ac87
Tree-SHA512: cd6b4923ea1865001b5f0caed9a4ff99c198d22bf74154d935dc09a47fda22ebe585ec912398cea69f722454ed1dbb4898faab5a2d02fb4c5e719c5c8d71a3f9
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last
block pruned was returned, subject to a bug if there were blocks left unpruned
due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new
bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it
makes more sense to fix the behaviour to match the documentation.
3a171f742c logging: fix logging empty threadname (klementtan)
Pull request description:
Currently, `leveldb` background thread does not have a thread name and as a result, an empty thread name is logged.
This PR fixes this by logging thread name as `"unknown"` if the thread name is empty
On master:
```txt
2022-06-02T14:30:38Z [] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes
```
On this PR:
```txt
2022-06-02T14:30:38Z [unknown] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes
```
ACKs for top commit:
laanwj:
Code review ACK 3a171f742c
hebasto:
ACK 3a171f742c
Tree-SHA512: 0af0fa5c4ddd3640c6dab9595fe9d97f74d0e0f4b41287a6630cf8ac5a21240250e0659ec4ac5a561e888d522f5304bf627104de2aba0fd0a86c1222de0897c2
48262a00f5 Add functional test for block sync from inbound peers (Suhas Daftuar)
0569b5c4bb Sync chain more readily from inbound peers during IBD (Suhas Daftuar)
Pull request description:
When in IBD, if the honest chain is only known by inbound peers, then we must
eventually sync from them in order to learn it. This change allows us to
perform initial headers sync and fetch blocks from inbound peers, if we have no
blocks in flight.
The restriction on having no blocks in flight means that we will naturally
throttle our block downloads to any such inbound peers that we may be
downloading from, until we leave IBD. This is a tradeoff between preferring
outbound peers for most of our block download, versus making sure we always
eventually will get blocks we need that are only known by inbound peers even
during IBD, as otherwise we may be stuck in IBD indefinitely (which could have
cascading failure on the network, if a large fraction of the network managed to
get stuck in IBD).
Note that the test in the second commit fails on master, without the first commit.
ACKs for top commit:
ajtowns:
ACK 48262a00f5
sipa:
ACK 48262a00f5
Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d
d2f8f1b307 use testing setup mempool in ComplexMemPool bench (glozow)
aecc332a71 create and use mempool transactions using real coins in MempoolCheck (glozow)
2118750631 [test util] to populate mempool with random transactions/packages (glozow)
5374dfc4e3 [test util] use -checkmempool for TestingSetup mempool check ratio (glozow)
d7d9c7b266 [test util] add chain name to TestChain100Setup ctor (glozow)
Pull request description:
Fixes#24634 by using the `testing_setup`'s actual mempool rather than a locally-declared mempool for running `check()`.
Also creates a test utility for populating the mempool with a bunch of random transactions. I imagine this could be useful in other places as well; it was necessary here because we needed the mempool to contain transactions *spending coins available in the current chainstate*. The existing `CreateOrderedCoins()` is insufficient because it creates coins out of thin air.
Also implements the separate suggestion to use the `TestingSetup` mempool in `ComplexMemPool` bench.
ACKs for top commit:
laanwj:
Code review ACK d2f8f1b307
Tree-SHA512: 44ab5a9e55b126b5a5bc33f05fbad1380b9c43c84736c7cf487be025e0e3f5d75216ccf5a3088b0935da817e3dacfba99d2885f75bcb6e7eaa24cd20a82c24c8
a4741bd8d4 kernel: pass params to BlockManager rather than using a global (Cory Fields)
Pull request description:
In a discussion today, dongcarl and I realized that is the only usage of the global `Params()` left in the kernel code.
We can use the readily available reference in `ChainstateManager` instead.
Note: There are still some uses of `BaseParams` in the kernel, so it doesn't make sense to rearrange the definitions quite yet. Once those are gone we can split the globals into new files.
ACKs for top commit:
MarcoFalke:
cr ACK a4741bd8d4
laanwj:
Code review ACK a4741bd8d4
Tree-SHA512: bfcc0c35e6c23689e968ccc96ceda39dd5a47fe94fbe617902110fe5865c30a40ea614bcfd4b4a2c846d2e84340aa8973e70b0938786af0fecfa3e6016d7fcad
...instead of explicitly calling init::{Set,Unset}Globals.
Cool thing about this is that in both the testing and bitcoin-chainstate
codepaths, we no longer need to explicitly unset globals. The
kernel::Context goes out of scope and the globals are unset
"automatically".
Also construct kernel::Context outside of AppInitSanityChecks()
fa72e0ba15 Use designated initializers (MarcoFalke)
Pull request description:
Designated initializers are supported since gcc 4.7 (Our minimum required is 8) and clang 3 (Our minimum required is 7). They work out of the box with C++17, and only msvc requires the C++20 flag to be set. I don't expect any of our msvc users will run into issues due to this. See also https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2022/bitcoin-core-dev.2022-03-10-19.00.log.html#l-114
ACKs for top commit:
kristapsk:
ACK fa72e0ba15
hebasto:
ACK fa72e0ba15
Tree-SHA512: a198e9addd9af69262a7e79ae4377b55697c8dfe768b8b3d444526544b1d1f85df46f0ae81b7541bf2f73e5868fb944b159e5bf234303c7b8b9d778afb0b2840
025c6ca509 Squashed 'src/univalue/' changes from 6c19d050a9..de4f73ddca (MacroFake)
9b50a309ff refactor: Replace get_int by getInt<int> alias (MacroFake)
e4e8186ab4 refactor: Explicitly convert atomic<int> to int (João Barbosa)
Pull request description:
This bumps the univalue subtree and changes two lines of our code. Apart from the get_int -> getInt change, this is mostly a rebase of https://github.com/bitcoin/bitcoin/pull/15975, which was closed back then.
However, given the numerous UniValue copy bugs and performance regressions in the past years, I think it makes sense to finally go through with the changes and disable potentially expensive implicit UniValue copies, which may cause OOM.
The changes here are not strictly required for that, but make future changes less verbose and easier to review.
ACKs for top commit:
laanwj:
Code review ACK fa0cc61b7f
fanquake:
ACK fa0cc61b7f
Tree-SHA512: 9ab9e371e6a745a80c441e99fb9cd407602a8066df883135e0ea7eced7b0c6ef0e9bc88f1d99a2b4804128d636727229f44d72b5615dbf2d70da4af63fa6adec
46a890960e build: pass bdb cppflags only where needed (fanquake)
Pull request description:
Move bdb cppflags out of the catch-all `BITCOIN_INCLUDES`, and pass them
only where they are needed, which is in libbitcoin_node/wallet and the tests.
ACKs for top commit:
hebasto:
ACK 46a890960e
Tree-SHA512: ac639bf88be1c21c5205bb14c983330c72c37f17ec4c15a746a86c54dd83853e3d75802eff48c4647484b07c1bae943ca2c3d61d2a8b0bed07d9c5b4a71dcec4
...instead of calling initialization functions directly and having to
keep around a ECCVerifyHandle member variable.
This makes the initialization codepath of our tests more closely
resemble those of AppInitMain and potentially eases the review of
subsequent commit removing init::{Set,Unset}Globals.
[META] In a future commit, we will introduce a kernel::Context which
calls init::{Set,Unset}Globals in its ctor and dtor. It will be
owned by node::NodeContext, so in the end, this patchset won't
have made the previously local ECCVerifyHandle global.
f565b2836d Fixup option name in bench message (Ben Woosley)
bf209ac7a7 doc: Fix spelling errors identified by codespell in coments (Ben Woosley)
Pull request description:
From the output [here](https://cirrus-ci.com/task/5275612980969472?logs=lint#L849):
```
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
~~I left the 'nd' in miniscript_tests as-is, as it's valid miniscript,
and I'm wary of whitelisting it.~~
ACKs for top commit:
dunxen:
ACK f565b28
Tree-SHA512: 501a426c5f6f9761e2c8f980d5d955611428a827321888f53e0ae9526b0fecd43f9d1fa845fc70ae2489d77be6dc0b5b371dff55c5146f4b39ed874f4a1ea917
a35f963edf Add test for getheaders behavior (Suhas Daftuar)
ef6dbe6863 Respond to getheaders if we have sufficient chainwork (Suhas Daftuar)
Pull request description:
Previously, we would check to see if we were in IBD and ignore getheaders requests accordingly. However, the IBD criteria -- an optimization mostly targeted at behavior when we have peers serving us many blocks we need to download -- is difficult to reason about in edge-case scenarios, such as if the network were to go a long time without any blocks found and nodes are getting restarted during that time.
To make things simpler to reason about, just use `nMinimumChainWork` as our anti-DoS threshold for responding to a getheaders request; as long as our chain has that much work, it should be fine to respond to a peer asking for our headers (and this should allow such a peer to request blocks from us if needed).
ACKs for top commit:
klementtan:
crACK a35f963edf
naumenkogs:
ACK a35f963edf
MarcoFalke:
review ACK a35f963edf 🗯
Tree-SHA512: 131e3872e7fe80382ea9c1ec202d6c2dc59c006355c69000aa3f4ce6bccd02a6c689c8cb8f3542b5d9bc48bfa61edcbd1a78535c0b79018971d02bed2655d284
151009cf76 qt, wallet, refactor: Drop unused `WalletModel::PaymentRequestExpired` (Hennadii Stepanov)
Pull request description:
The `PaymentRequestExpired` value in the `WalletModel::StatusCode` enumeration has been unused since bitcoin/bitcoin#17165.
ACKs for top commit:
furszy:
ACK 151009cf, no usage for it.
kristapsk:
cr ACK 151009cf76, checked that `PaymentRequestExpired` is not referenced anywhere else.
Tree-SHA512: c2ea3443af5d369ca294d79559869f688aaa806b91ffe0090f3b34638a8377ec2f11d6f5c09cc2d11ab55035850237e60e992acba671097a6642c6bb9e709273
As stated on the website, duplicate keys make it hard to reason about
malleability as a single signature may unlock multiple paths.
We use a custom KeyCompare function instead of operator< to be explicit
about the requirement.
Assigning TIME_INIT to nTime was needed to fully re-initialize a dirty
object where the deserialization might skip nTime.
See https://github.com/bitcoin/bitcoin/pull/19020/files#r427620111
Now that the without-nTime logic is removed in commit
dbcb5742c4 and commit
e08770bed1, the logic here can be removed
as well.
Also, remove confusing and redundant preprocessor code.
Also, remove no longer needed version.h include, which was needed for
INIT_PROTO_VERSION.
cc61bc2e19 compat: remove glibcxx sanity checks (fanquake)
Pull request description:
These checks were added in #4339, (see also #4081), to test
our back-compat stubs, however, those stubs no-longer exist (#22930),
meaning that these checks are now just testing some specific standard
library behaviour, without a particular rationale, or reason, compared
to any other standard library functionlity we use.
There has also been some discussion about our sanity checks in the
context of the libbitcoinkernel refactoring, see https://github.com/bitcoin/bitcoin/pull/25065#discussion_r880668218.
Removing the checks removes the need to worry about atleast the
glibcxx checks.
Also remove the list of checks from the doc in `init.h`, because it is
incomplete, and anyone who wants to know what checks are included can
look at the function.
Guix Build (arm64):
```bash
e18a81e25b4707cbe113fb4d3ba2459013c1178e7cecfe446e4f14ee5ecd2ce8 guix-build-cc61bc2e19b1/output/arm-linux-gnueabihf/SHA256SUMS.part
9928cc38b79f827018cba0bdde98666b31806afcc79dd95a00acb8e153c36eec guix-build-cc61bc2e19b1/output/arm-linux-gnueabihf/bitcoin-cc61bc2e19b1-arm-linux-gnueabihf-debug.tar.gz
ebf4635ba4688899ae62e4bb17ebb2afb25c538c4a8068ef515920fd4e43754e guix-build-cc61bc2e19b1/output/arm-linux-gnueabihf/bitcoin-cc61bc2e19b1-arm-linux-gnueabihf.tar.gz
74c7e35b47c6d101fda7205f144d37150329b4c360db09d37b8c1437f3390898 guix-build-cc61bc2e19b1/output/arm64-apple-darwin/SHA256SUMS.part
6e12643b17be9326f1d873dfe51a52c082671540792877af624b42ca9f6e1791 guix-build-cc61bc2e19b1/output/arm64-apple-darwin/bitcoin-cc61bc2e19b1-arm64-apple-darwin-unsigned.dmg
1d86d0416c7a50afd7bd8d850f416b7c7277464ccc95e4dae53b5b59415fc83d guix-build-cc61bc2e19b1/output/arm64-apple-darwin/bitcoin-cc61bc2e19b1-arm64-apple-darwin-unsigned.tar.gz
84070843f23839e7191ad3a667eb63c45f668eb95afbfb3fcdfb8363320f67d4 guix-build-cc61bc2e19b1/output/arm64-apple-darwin/bitcoin-cc61bc2e19b1-arm64-apple-darwin.tar.gz
bf6ccd7b8c40476b1dc52b491757313ea3e96c43a01c8aabaf39f94dc1837329 guix-build-cc61bc2e19b1/output/dist-archive/bitcoin-cc61bc2e19b1.tar.gz
25e7e1ff7d8de38632abf9926343c8ba556209f3d03109c92864ffe72813a05f guix-build-cc61bc2e19b1/output/powerpc64-linux-gnu/SHA256SUMS.part
d0398de83841607b1bf921d4553b30ad5e2d70d0570e96a2eaaf2762e1103c79 guix-build-cc61bc2e19b1/output/powerpc64-linux-gnu/bitcoin-cc61bc2e19b1-powerpc64-linux-gnu-debug.tar.gz
f09cdc2ac2a2bb644f4749f3d74b5210ddb531594c33d127a907f0223e7793e5 guix-build-cc61bc2e19b1/output/powerpc64-linux-gnu/bitcoin-cc61bc2e19b1-powerpc64-linux-gnu.tar.gz
ef36a68ef4e5ee9b311df40062cd2296f897e7b1550e39e0643601cd7d469010 guix-build-cc61bc2e19b1/output/powerpc64le-linux-gnu/SHA256SUMS.part
937b600a2b86304ccc5b6c71a7eaf8aa5e2020592724cef6a933a1955995480b guix-build-cc61bc2e19b1/output/powerpc64le-linux-gnu/bitcoin-cc61bc2e19b1-powerpc64le-linux-gnu-debug.tar.gz
eca4eec41e71fdf7a7b0fa4065afa49c47d3b9541ed2cb4d083ad4a0de102e37 guix-build-cc61bc2e19b1/output/powerpc64le-linux-gnu/bitcoin-cc61bc2e19b1-powerpc64le-linux-gnu.tar.gz
981c0968c19905925a599cff357ec259c1e806bdb7691c7b52039be450bdad7c guix-build-cc61bc2e19b1/output/riscv64-linux-gnu/SHA256SUMS.part
89c709967f9a157256281fbf682aad246f2eaad9c2f1797c2787253cbabe12f8 guix-build-cc61bc2e19b1/output/riscv64-linux-gnu/bitcoin-cc61bc2e19b1-riscv64-linux-gnu-debug.tar.gz
454cd830dd382e176f5a23041fc33f93937668245481b0dd29fc04882d9528eb guix-build-cc61bc2e19b1/output/riscv64-linux-gnu/bitcoin-cc61bc2e19b1-riscv64-linux-gnu.tar.gz
e0812c2dc492e5c5f06e3685d19da8fb29ed38d3b32821d293ef01cb4fefbd79 guix-build-cc61bc2e19b1/output/x86_64-apple-darwin/SHA256SUMS.part
0e7d4241d8ac882a8091fa00a7813db87a3e5afec59627e45b6c910cfdd4a7b0 guix-build-cc61bc2e19b1/output/x86_64-apple-darwin/bitcoin-cc61bc2e19b1-x86_64-apple-darwin-unsigned.dmg
3faaca046cbb2642445a7dd1f389ed7bf94a65de8372441c36d5cb79c030ce31 guix-build-cc61bc2e19b1/output/x86_64-apple-darwin/bitcoin-cc61bc2e19b1-x86_64-apple-darwin-unsigned.tar.gz
73080f032a42db679baf0d09619671ac5b9d85be84a68bdd6b6709eb0e6465bd guix-build-cc61bc2e19b1/output/x86_64-apple-darwin/bitcoin-cc61bc2e19b1-x86_64-apple-darwin.tar.gz
07b6e1b6291404bca1044df4a45b6958b882ffb88c143ba98f1959960a394897 guix-build-cc61bc2e19b1/output/x86_64-linux-gnu/SHA256SUMS.part
16b455f62398f4aa0d3821abb1cceb8151e31c2664e3f974a764a5b8702b50f3 guix-build-cc61bc2e19b1/output/x86_64-linux-gnu/bitcoin-cc61bc2e19b1-x86_64-linux-gnu-debug.tar.gz
3c1a3a6a343f17b83f3b3d47e9426eccd2d0bcc7f824cd958fcf2cf06cdc3276 guix-build-cc61bc2e19b1/output/x86_64-linux-gnu/bitcoin-cc61bc2e19b1-x86_64-linux-gnu.tar.gz
f05afa688ea7211b0049555385fb2acc26986e24d8d00893389160e07037e693 guix-build-cc61bc2e19b1/output/x86_64-w64-mingw32/SHA256SUMS.part
8bcbae67dd0746c42e1e7c7db67725a69289b08e9aa97b873d443d0aa355615d guix-build-cc61bc2e19b1/output/x86_64-w64-mingw32/bitcoin-cc61bc2e19b1-win64-debug.zip
efa45e3b76e5ae08a8392d58e741325df572d92c7dd69b65d876cdcda541d2fc guix-build-cc61bc2e19b1/output/x86_64-w64-mingw32/bitcoin-cc61bc2e19b1-win64-setup-unsigned.exe
3a8c2461ca826138c3017d06279a79b4c6bee2a507ad362aa6e424f76678596c guix-build-cc61bc2e19b1/output/x86_64-w64-mingw32/bitcoin-cc61bc2e19b1-win64-unsigned.tar.gz
e56ae4f609d4e6a3ca5917a4bb763c91012ece2d236d6b62a666358791e43525 guix-build-cc61bc2e19b1/output/x86_64-w64-mingw32/bitcoin-cc61bc2e19b1-win64.zip
```
ACKs for top commit:
MarcoFalke:
lgtm ACK cc61bc2e19
laanwj:
Concept and code review ACK cc61bc2e19
Tree-SHA512: 3da6aba44eef3f864fcbe897db1faa964923756e68c6a713e444b5d01c6d3542c3d7ca26678760e81a7a9e3cd40bd90622d0a7b697c27166817ba4f1023661ef
20ff4991e5 rpc: Capture potentially large UniValue by ref for rpcdoccheck (Martin Zumsande)
Pull request description:
Capturing it by reference instead of value should save us from making a copy of a potentially large object. Saw this while having a look at #25229 although I couldn't reproduce an actual leak, so this is not a fix for that issue.
ACKs for top commit:
MarcoFalke:
review ACK 20ff4991e5
theStack:
Code-review ACK 20ff4991e5
furszy:
Code ACK 20ff4991
Tree-SHA512: faf7bb14e37f8324b93a39095b07693626329da47c4a1ac8929bf99385e2e0567008e959e7e8540bc7d454d08fa41cccd39f55253c9a839fa88362922058a93b
885694d794 doc: add release note about removal of `deprecatedrpc=fees` flag (Sebastian Falbesoner)
387ae8bc09 rpc: remove deprecated fee fields from mempool entries (Sebastian Falbesoner)
Pull request description:
Deprecating the top-level fee fields (`fee`, `modifiedfee`, `ancestorfees` and `descendantfees`) from the mempool entries and introducing `-deprecatedrpc=fees` was done in PR #22689 (released in v23.0). For the next release v24.0, this configuration option can be removed.
ACKs for top commit:
fanquake:
ACK 885694d794
Tree-SHA512: fec6b5be5c3f0cd55738a888b390ef9271e70b2dba913a14ce82427dac002e999f93df298bb3b494f3d1b850a23d2b5b3e010e901543b0d18db9be133579e1ec
1) `Wallet::AddToWallet` is already returning the pointer to the inserted `CWalletTx`, so there is no need to look it up in the map again.
2) `Wallet::AddToWallet` can only return a nullptr if the db `writeTx` call failed. Which should be treated as an error.
These checks were added in #4339, (see also #4081), to test
our back-compat stubs, however, those stubs no-longer exist (#22930),
meaning that these checks are now just testing some specific standard
library behaviour, without a particular rationale, or reason, compared
to any other standard library functions we use.
There has also been some discussion about the sanity checks in the
context of the libbitcoinkernel refactoring, see
https://github.com/bitcoin/bitcoin/pull/25065#discussion_r880668218.
Removing the checks removes the need to worry about atleast the glibcxx
checks.
Also remove the list of check from the doc in init.h, because it is
incomplete, and anyone who wants to know what checks are included can
look at the function.
fa27ee88ed Get time less often in AddrManImpl::ResolveCollisions_() (MarcoFalke)
Pull request description:
First commit split out from https://github.com/bitcoin/bitcoin/pull/24697
ACKs for top commit:
sipa:
utACK fa27ee88ed
fanquake:
ACK fa27ee88ed
Tree-SHA512: 40c8594d2a5ce02a392ac5f9f120c24c6bcd495b0bcc901fd6064dde9f6123cd109504cee7b612a9555b70cfd7759cbd6cd496d007bb374c27610d01b464191c
Since UpdatePackagesForAdded is a helper function that's only used in
addPackageTxs we can make it static and avoid the unnecessary interface
and in-header lock annotation.
SkipMapTxEntry is a short helper function that's only used in
addPackageTxs, we can just inline it, keep the comments, and avoid the
unnecessary interface and lock annotations.
`Misbehaving` has several coding related issues (ignoring the conceptual
issues here for now):
* It is public, but it is not supposed to be called from outside of
net_processing. Fix that by making it private and creating a public
`UnitTestMisbehaving` method for unit testing only.
* It doesn't do anything if a `nullptr` is passed. It would be less
confusing to just skip the call instead. Fix that by passing `Peer&`
to `Misbehaving()`.
* It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be
propagated. This is harmless, but verbose. Fix it by removing the no
longer needed call to `GetPeerRef` and the no longer needed lock
annotations.
44904aa632 multiprocess build cleanup: comment on manual dependencies (Ryan Ofsky)
6e1c16c144 multiprocess build fix: ipc/capnp/init.capnp.h: No such file or directory (Ryan Ofsky)
Pull request description:
Error was reported by SatoriHoshiAiko in https://github.com/bitcoin/bitcoin/issues/25207 and happens unpredictably because make doesn't always build dependencies in the same order.
The source file `src/ipc/capnp/protocol.cpp` includes some generated headers so needs to have an explicit dependency specified in the makefile so the headers will be generated before the file is compiled. #19160 added the explicit dependency, but it was incorrect because it referred to an old file path from before the source file was renamed (`ipc.cpp` -> `protocol.cpp`)
ACKs for top commit:
hebasto:
re-ACK 44904aa632
Tree-SHA512: 578ef4ef35a97e9d8d4e6d4edf39e57a32674234bf145e8159268324cb6ba15b420517afc07f6f42bb11a562954ea74af686c3fb92ce178c1fc378421b352531
4185570340 Add RPC to get mempool txs spending outputs (t-bast)
Pull request description:
We add an RPC to fetch mempool transactions spending any of the given outpoints.
Without this RPC, application developers need to first call `getrawmempool` which returns a long list of `txid`, then fetch each of these transactions individually (`getrawtransaction`) to check whether they spend the given outpoints, which wastes a lot of bandwidth (in the worst case we need to transfer the whole mempool).
For example in lightning, when we discover that one of our channel funding transactions has been spent, we need to find the spending transaction to claim our outputs from it. We are currently forced to fetch the whole mempool to do the analysis ourselves, which is quite costly.
I believe that this RPC is also generally useful when doing some introspection on your mempool after one of your transactions failed to broadcast, for example when you implement RBF at the application level. Fetching and analyzing the conflicting transaction gives you more information to successfully replace it.
ACKs for top commit:
darosior:
re-utACK 4185570340
vincenzopalazzo:
re-ACK 4185570340
danielabrozzoni:
re-tACK 4185570340
w0xlt:
reACK 4185570340
Tree-SHA512: 206687efb720308b7e0b6cf16dd0a994006c0b5a290c8eb386917a80130973a6356d0d5cae1c63a01bb29e066dd721594969db106cba7249214fcac90d2c3dbc
7036cf52aa Delete UpdatePackagesForAdded at beginning of addPackageTxs. (KevinMusgrave)
Pull request description:
In `CreateNewBlock` (in miner.cpp), `inBlock` is cleared before `addPackageTxs`, so `inBlock` will be empty in the first call to `UpdatePackagesForAdded`. I saw this brought up in these [PR review club logs](https://bitcoincore.reviews/24538) and there didn't seem to be a definitive answer for why the call is necessary. There's also an [old PR](https://github.com/bitcoin/bitcoin/pull/10200) where this change was going to be applied, but it got closed.
If `addPackageTxs` can be called when `inBlock` is not empty, then maybe a test should be added for that case. All the tests seem to pass with this deletion.
ACKs for top commit:
glozow:
utACK 7036cf52aa
Tree-SHA512: 9e757b71b9035f68a0c6fef229b8cd83f1bdbe23f05bb02cc1bab8c3c177805b388bceb2bb1f0bce354791ccb29f351a6c51979b96ffe4d9fc6c978f83e36afc
c4c5b9ca6e consensus/params: set default values for BIP9Deployment (Anthony Towns)
Pull request description:
Adds default values for `vDeployments` in `consensus/params.h` so that undefined behaviour is avoided if a deployment is not initialized. Also adds a check in the unit tests to alert if this is happening, since even if it doesn't invoke undefined behaviour it's probably a mistake.
ACKs for top commit:
laanwj:
Code review ACK c4c5b9ca6e
Tree-SHA512: 22d7ff86a817d9e9e67c47301fc3b7e9d5821c13565d7706711f113dea220eea29b413a7c8d029691009159cebc85a108d77cb52418734091c196bafb2b12423
6b636730f4 tracing: fix `coin_selection:aps_create_tx_internal` calling logic (Sebastian Falbesoner)
Pull request description:
According to the documentation, the tracepoint `coin_selection:aps_create_tx_internal` "Is called when the second `CreateTransactionInternal` with Avoid Partial Spends enabled completes."
Currently it is only called if the second call to `CreateTransactionInternal` succeeds, i.e. the third parameter is always `true` and we don't get notified in the case that it fails. This PR fixes this by moving the tracepoint call and the `use_aps` boolean variable outside the if body.
ACKs for top commit:
achow101:
ACK 6b636730f4
furszy:
re-ACK 6b636730
Tree-SHA512: 453825123aa10748642c7dd94324ced2d07df0f4fac478b0947a34820b515ae300f75721679a90a164f3127029739df55c4de035c4567e663893c3c6dbdef216
f9fdcec7e9 settings: Add resetSettings() method (Ryan Ofsky)
77fabffef4 init: Remove Shutdown() node.args reset (Ryan Ofsky)
0e55bc6e7f settings: Add update/getPersistent/isIgnored methods (Ryan Ofsky)
Pull request description:
Add `interfaces::Node` `updateSetting`, `forceSetting`, `resetSettings`, `isSettingIgnored`, and `getPersistentSetting` methods so GUI is able to manipulate `settings.json` file and use and modify node settings.
(Originally this PR also contained GUI changes to unify bitcoin-qt and bitcoind persistent settings and call these methods, but the GUI commits have been dropped from this PR and moved to bitcoin-core/gui/pull/602)
ACKs for top commit:
vasild:
ACK f9fdcec7e9
hebasto:
re-ACK f9fdcec7e9, only a function renamed since my recent [review](https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-979324357).
Tree-SHA512: 4cac853ee29be96d2ff38404165b9dfb7c622b2a9c99a15979596f3484ffde0da3d9c9c372677dff5119ca7cffa6383d81037fd9889a29cc9285882a8dc0c268
This also effectively reverts 58e8364dcd from
#18077, applying upnp and natpmp settings from the optionsmodel class instead
of the optionsdialog class. This makes sense because model code, not view code
is responsible for applying all other settings, and because leaving these
settings half-applied in optionsmodel seems error prone and could lead to bugs.
(These things were discussed a little in
https://github.com/bitcoin/bitcoin/pull/18077#discussion_r560381734)
This is just the first of multiple settings that will be stored in the bitcoin
persistent setting file and shared with bitcoind, instead of being stored as Qt
settings backed by the windows registry or platform specific config files which
are ignored by bitcoind.
Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>
be6d4315c1 doc: remove misleading AreInputsStandard() comment (James O'Beirne)
Pull request description:
This check isn't any longer just about bad pay-to-script-hash inputs; it
also excludes any kind of nonstandard input, unknown witness versions,
coinbases, etc.
ACKs for top commit:
laanwj:
ACK be6d4315c1
dunxen:
ACK be6d431
jonatack:
ACK be6d4315c1
Tree-SHA512: 1c4befadff6a7b5789901ca2a2cc39adc35c688f7e3c093ab5292123f9193ce078731016b773b3d05f7004ff01ee62f23f8362ae8d05134d41dc097ba094a42b
c4e7717727 refactor: Change LogPrintLevel order to category, severity (laanwj)
ce920713bf leveldb: Log messages from leveldb with category and debug level (laanwj)
18ec120bb9 http: Use severity-based logging for messages from libevent (laanwj)
bd971bffb0 logging: Unconditionally log levels >= WARN (laanwj)
Pull request description:
Log messages from leveldb and libevent libraries in the severity+level based log format introduced in bitcoin/bitcoin#24464.
Example of messages before:
```
2022-05-24T18:11:57Z [libevent] libevent: event_add: event: 0x55da963fcc10 (fd 10), EV_READ call 0x7f1c7a254620
2022-05-24T18:11:57Z [libevent] libevent: Epoll ADD(1) on fd 10 okay. Old events were 0; read change was 1 (add); write change was 0 (none); close change was 0 (none)
2022-05-24T18:12:08Z leveldb: Generated table #609127@1: 6445 keys, 312916 bytes
2022-05-24T18:12:08Z leveldb: Generated table #609128@1: 5607 keys, 268548 bytes
2022-05-24T18:12:08Z leveldb: Generated table #609129@1: 189 keys, 9384 bytes
2022-05-24T18:12:08Z leveldb: Generated table #609130@1: 293 keys, 13818 bytes
```
Example of messages after:
```
2022-05-24T17:59:52Z [libevent:debug] event_add: event: 0x5652f44dac10 (fd 10), EV_READ call 0x7f210f2e6620
2022-05-24T17:59:52Z [libevent:debug] Epoll ADD(1) on fd 10 okay. Old events were 0; read change was 1 (add); write change was 0 (none); close change was 0 (none)
2022-05-24T17:59:53Z [leveldb:debug] Recovering log #1072
2022-05-24T17:59:53Z [leveldb:debug] Level-0 table #1082: started
2022-05-24T17:59:53Z [leveldb:debug] Level-0 table #1082: 193 bytes OK
2022-05-24T17:59:53Z [leveldb:debug] Delete type=3 #1070
2022-05-24T17:59:53Z [leveldb:debug] Delete type=0 #1072
```
The first commit changes it so that messages with level Warning and Error are always logged independent of the `-debug` setting. I think this is useful to make sure warnings and errors, which tend to be important, are not lost. In the future this should be made more configurable.
Last commit changes LogPrintLevel argument order to category, severity: This is more consistent with the other functions, as well as with the logging output itself. If we want to make this change, we should do it before it's all over the place.
ACKs for top commit:
jonatack:
Tested ACK c4e7717727
Tree-SHA512: ea48a1517f8c1b23760e59933bbd64ebf09c32eb947e31b8c2696b4630ac1f4303b126720183e2de052bcede3a17e475bbf3fbb6378a12b0fa8f3582d610930d
bd7c5e2f0a Add BIP-341 specified constraints to `ComputeTaprootMerkleRoot` (David Bakin)
Pull request description:
[**N.B.:** This PR **_does not change the consensus_**. It only adds `assert` statements according to the current consensus in consensus-sensitive code (`interpreter.cpp`). So that's why the bot added the "consensus" tag and I prefixed the PR title with "consensus".]
BIP 341 specifies [constraints on the size of the control block _c_ used to compute the taproot merkle root](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#script-validation-rules).
> The last stack element is called the control block _c_, and must have length _33 + 32m_, for a value of _m_ that is an integer between 0 and 128, inclusive. Fail if it does not have such a length.
The actual merkle root is computed in `ComputeTaprootMerkleRoot` ([interpreter.cpp@1833](https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1833)) - this code does _not_ check these constraints.
All the callers do check the constraints before calling `ComputeTaprootMerkleRoot`. But in the future there may be more callers, and these checks may be inadvertently omitted at those future calls. Also, code at/near the current call sites may also change and skip these checks. Therefore _this PR adds those checks as `asserts` directly in `ComputeTaprootMerkleRoot`_ to help prevent that error.
No unit tests provided: they'd have to be death tests as these are `assert` statements which raise `SIGABRT` and kill the program. Boost Test has a way to implement death tests (see the in-progress draft PR #25097 at [this code (you may have to click to expand the diff)](https://github.com/bitcoin/bitcoin/pull/25097/files#diff-21483d0e032747850208f21325b29cde89e9c1f55f83a7a166a388cc5c27115aR1089) and could be added here if desired by reviewers.
Current callers of `ComputeTaprootMerkleRoot`:
- `InferTaprootTree` ([standard.cpp@1552](https://github.com/bitcoin/bitcoin/blob/master/src/script/standard.cpp#L546))
- `VerifyTaprootCommittment` ([interpreter.cpp@1859](https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1859)) does a partial check, but it is called from `VerifyWitnessProgram` ([interpreter.cpp@1922](https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1918)) where a full check is done
ACKs for top commit:
sipa:
ACK bd7c5e2f0a
theStack:
ACK bd7c5e2f0a
Tree-SHA512: cec5aebfdf9fc9d13011abdf8427c934edf1df1ffc32b3e7cc5580f12809f24cf83e64ab0c843fabfce3591873bfcfa248277b30820fd786684319a196e4e550
BIP 341 specifies constraints on the size of the control block _c_ used
to compute the taproot merkle root.
> The last stack element is called the control block _c_, and must have
> length _33 + 32m_, for a value of m that is an integer between 0 and
> 128, inclusive. Fail if it does not have such a length.
(See BIP-341 "Script Validation Rules" here: https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#script-validation-rules)
Error was reported by SatoriHoshiAiko in
https://github.com/bitcoin/bitcoin/issues/25207 and happens unpredictably
because make doesn't always build dependencies in the same order.
The source file src/ipc/capnp/protocol.cpp includes some generated headers so
needs to have an explicit dependency specified in the makefile so the headers
will be generated before the file is compiled. #19160 added the explicit
dependency, but it was incorrect because it referred to an old file path from
before the source file was renamed (ipc.cpp -> protocol.cpp)
This check isn't any longer just about bad pay-to-script-hash inputs; it
also excludes any kind of nonstandard input, unknown witness versions,
coinbases, etc.
This is more consistent with the other functions, as well as with the
logging output itself. If we want to make this change, we should do it
before it's all over the place.
Messages with level `WARN` or higher should be logged even when
the category is not provided with `-debug=`, to make sure important
warnings are not lost.
This makes the code less verbose. Also, future changes that change how
to get the time are less verbose.
Moreover, GetAdjustedTime() might arbitrarily change the value during
the execution of this function. For example, the system time advances
over a second boundary, or the network adjusts the time arbitrarily.
Most of the time however the value will not change, so it seems better
to always lock the value in this scope for clarity.
From the output here:
src/qt/test/addressbooktests.cpp:185: wilcard ==> wildcard
src/qt/test/addressbooktests.cpp:191: wilcard ==> wildcard
src/test/miniscript_tests.cpp:227: nd ==> and, 2nd
src/test/versionbits_tests.cpp:260: everytime ==> every time
src/util/time.h:89: precicion ==> precision
src/util/time.h:90: precicion ==> precision
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
https://cirrus-ci.com/task/5275612980969472?logs=lint#L849
I added 'nd' to the spelling.ignored-words.txt, as it's valid miniscript.
e11cdc9303 logging: Add log severity level to net.cpp (klementtan)
a8290649a6 logging: Add severity level to logs. (klementtan)
Pull request description:
**Overview**: This PR introduces a new macro, `LogPrintLevel`, that allows developers to add logs with the severity level. Additionally, it will also print the log category if it is specified.
Sample log:
```
2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XX.XX.XXX.XXX:YYYYY lastseen=2.7hrs
```
**Motivation**: This feature was suggested in #20576 and I believe that it will bring the following benefits:
* Allow for easier filtering of logs in `debug.log`
* Can be extended to allow users to select the minimum level of logs they would like to view (not in the scope of this PR)
**Details**:
* New log format. `... [category:level]...`. ie:
* Do not print category if `category == NONE`
* Do not print level if `level == NONE`
* If `category == NONE` and `level == NONE`, do not print any fields (current behaviour)
* Previous logging functions:
* `LogPrintf`: no changes in log as it calls `LogPrintf_` with `category = NONE` and `level = NONE`
* `LogPrint`: prints additional `[category]` field as it calls `LogPrintf_` with `category = category` and `level = NONE`
* `net.cpp`: As a proof of concept, updated logs with obvious severity (ie prefixed with `Warning/Error:..`) to use the new logging with severity.
**Testing**:
* Compiling and running `bitcoind` with this PR should instantly display logs with the category name (ie `net/tor/...`)
* Grepping for `net:debug` in `debug.log` should display the updated logs with severity level:
<details>
<summary>Code</summary>
```
$ grep "net:debug" debug.log
2022-03-04T16:41:15Z [opencon] [net:debug] trying connection XXX:YYY lastseen=2.7hrs
2022-03-04T16:41:16Z [opencon] [net:debug] trying connection XXX:YYY lastseen=16.9hrs
2022-03-04T16:41:17Z [opencon] [net:debug] trying connection XXX:YYY lastseen=93.2hrs
2022-03-04T16:41:18Z [opencon] [net:debug] trying connection XXX:YYY lastseen=2.7hrs
```
</details>
ACKs for top commit:
laanwj:
Code review and lightly tested ACK e11cdc9303
Tree-SHA512: 89a8c86667ccc0688e5acfdbd399aac1f5bec9f978a160e40b0210b0d9b8fdc338479583fc5bd2e2bc785821363f174f578d52136d228e8f638a20abbf0a568f
664a14ba7c coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f100687566 kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa52387e8 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a9298c scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456f04 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
80970985c9 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce4b2 coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe02b Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e410a5 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463daf6 coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc56a coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f2b8 coinstats: Extract hash_type in-member to in-param (Carl Dong)
102294898d includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9c35 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939993 kernel: Remove unnecessary blockfilter{index,}.cpp (Carl Dong)
Pull request description:
Part of: #24303
Depends on: #24322
The `GetUTXOStats` function has 2 codepaths:
- One which queries the `CoinStatsIndex` for the UTXO hash
- One which actually performs the hashing
For `libbitcoinkernel`, the only place where we call `GetUTXOStats` is in `PopulateAndValidateSnapshots`, which uses the `SHA256D` hash, and is therefore unable to use the `CoinStatsIndex` since that only provides `MuHash` hashes. Not that I think indices necessarily belong in `libbitcoinkernel` anyway.
This PR separates these 2 aforementioned codepaths of `GetUTXOStats`, uses the hashing codepath in `PopulateAndValidateSnapshots`, and removes the need to link in `index/coinstatsindex.cpp` and `node/coinstats.cpp`.
-----
Logistically, this PR:
- Extracts out the `index_requested` and `hash_type` members of `CoinStats`, which served as "in-params" to `GetUTXOStats` embedded within the `CoinStats` struct. This allows `CoinStats` to only consist of "out-param" members, and be returned by `GetUTXOStats` without needing to be an "in-out" param
- Introduce the purely virtual `UTXOHashers` class, with 3 implementations: `SHA256DHasher`, `MuHashHasher`, and `NullHasher`. These replace the existing template-based polymorphism.
- Split `GetUTXOStats` into:
- `CalculateUTXOStatsWithHasher(UTXOHasher&, ...)`, and
- `LookupUTXOStatsWithIndex(CoinStatsIndex&, ...)`
- Use `CalculateUTXOStatsWithHasher` directly where appropriate (`src/validation.cpp` and `src/fuzz`)
- Move `GetUTXOStats` to `rpc/blockchain`, which is the only place that depends on `GetUTXOStats`'s weird fallback behaviour
- Move `LookupUTXOStatsWithIndex` to `index/coinstatsindex`
Code organization:
- `src/`
- `kernel/` → only contains the hashing codepath
- `coinstats.cpp` → hashing codepath implementations
- `coinstats.h` → header for `kernel/coinstats.cpp`
- `index/` → only contains the index codepath
- `coinstatsindex.cpp` → index codepath implementations
- `coinstatsindex.h`
- `validation.cpp` → only uses the hashing codepath
- `rpc/blockchain.cpp` → uses both the hashing and index codepath, old `GetUTXOStats` fallback logic moved here as static
- `test/fuzz/coins_view.cpp` → only uses the hashing codepath
TODOs:
- [x] Commit messages could be fleshed out more
Would love any feedback!
ACKs for top commit:
laanwj:
Code review ACK 664a14ba7c
Tree-SHA512: 18722c7bd279174d2d1881fec33ea04a9b261aae1c12e998cf434ef297d8ded47de69c526c8033a2ba7abc93ba3d2ff5faf4ce05e8888c725c31cf885ce3ef73
e280087946 qt: Use `QRegularExpression` in `AddressBookSortFilterProxyModel` class (Hennadii Stepanov)
5c5d8f2465 qt, test: Add tests for searching in `AddressBookPage` dialog (Hennadii Stepanov)
Pull request description:
This is a step in [migration](https://github.com/bitcoin/bitcoin/pull/24798) to Qt 6.
Related:
- bitcoin-core/gui#578
- bitcoin-core/gui#585
No behavior change. To ensure this, tests have been added.
ACKs for top commit:
hebasto:
> tACK [e280087](e280087946) on Ubuntu 21.10 Qt 5.15.2
promag:
Tested ACK e280087946 with Qt6 on macOS 12 M1.
w0xlt:
tACK e280087946 on Ubuntu 21.10 Qt 5.15.2
jarolrod:
Tested ACK e280087946 on M1 mac, x86 mac, x86 Linux with Qt5 and separately with Qt6
Tree-SHA512: 664baacc1504deb2f7fa651ea4a44f3942f5c9058befe4d2ce292beed032d4b1697710cfd10c0909602d8a4a6eeb680414e4a1f56d2038478c1ae2f34965d74f
31122aa979 refactor: Pass interfaces::Node references to OptionsModel constructor (Ryan Ofsky)
Pull request description:
Giving OptionsModel access to the node interface is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings.
It has been split off from #602 to simplify that PR. Previously these commits were part of bitcoin/bitcoin#15936 and also had some review discussion there.
ACKs for top commit:
promag:
Code review ACK 31122aa979.
furszy:
Code ACK 31122aa9
jarolrod:
ACK 31122aa979
Tree-SHA512: b8529322fd7ba97e19864129e6cf5f9acc58c124f2e5a7c50aca15772c8549de3c24e8b0c27e8cf2c06fd26529e9cdb898b0788a1de3cbfdfbdd3f85c9f0fe69
rpc/blockchain.cpp is now the only user of the vestigial
GetUTXOStats(...). And since GetUTXOStats(...)'s special fallback logic
was only really relevant/meant for rpc/blockchain.cpp, we can just move
it there.
This is the "fruit of our labor" for this patchset.
ChainstateManager::PopulateAndValidateSnapshot can now directly call
ComputeUTXOStats(...).
Our consensus engine is now fully decoupled from all indices.
See the src/Makefile.am for some satisfying removals.
Most of this commit is pure-move.
After this change:
- kernel/coinstats.h
-> Contains declarations for:
- enum class CoinStatsHashType
- struct CCoinsStats
- GetBogoSize(...)
- TxOutSer(...)
- ComputeUTXOStats(...)
- node/coinstats.h
-> Just GetUTXOStats, which will be removed as we change callers to
directly use the hashing/indexing codepaths in future commits.
As mentioned in a previous commit, the hashing codepath can now be moved
to a separate file. This decouples callers that only rely on the hashing
codepath from the indexing one.
This is key for libbitcoinkernel, which needs to have the CoinsStats
hashing codepath for AssumeUTXO, but does not wish to be coupled with
indexes.
Note that only the .cpp file is split in this commit, the header files
will be split in a subsequent commit and the #includes to
node/coinstats.h will be adjusted to only #include the necessary
headers.
The indexing codepath logic in node/coinstats.cpp is simple enough to be
moved into CoinStatsIndex::LookUpStats, avoiding an additional layer of
function calls. Callers are modified accordingly.
Also, add 2 missed BOOST_CHECKs to the coinstatsindex_initial_sync unit
test.
Split out ComputeUTXOStats and LookupUTXOStatsWithIndex from
GetUTXOStats, since the hashing and indexing codepaths are quite
disparate in practice.
Also allow add a constructor to CCoinsStats for it to be constructed
from a a block height and hash. This is used in both codepaths.
Also add a note in GetUTXOStats documenting a behaviour quirk that
predates this patchset.
[META] This allows the hashing codepath to be moved to a separate file
in a future commit, decoupling callers that only rely on the
hashing codepath from the indexing one. This is key for
libbitcoinkernel, which needs to have the hashing codepath for
AssumeUTXO, but does not wish to be coupled with indexes.
In previous commits in this patchset, we removed all in-param members of
CCoinsStats. Now that that's done, we can modify GetUTXOStats to return
an optional CCoinsStats instead of a status bool. Callers are modified
accordingly.
In rpc/blockchain.cpp, we discover that GetUTXOStats' status bool when
getting UTXO stats for pprev was not checked for error. We fix this as
well.
a17c5e96b6 Rename NetinfoRequestHandler::is_block_relay data member to is_tx_relay (Jon Atack)
f0bb7db34c Fix frequent -netinfo JSON errors from null getpeerinfo#relaytxes (Jon Atack)
Pull request description:
CLI -netinfo frequently returns "error: JSON value is not a boolean as expected" since the merge of #21160, which moved fRelayTxes (renamed to m_relay_txs in that pull) from CNodeStats to CNodeStateStats.
This change made getpeerinfo "relaytxes" an optional field that can return UniValue IsNull(). It is the only optional field consumed by -netinfo where the latter didn't already handle that case. See also https://github.com/bitcoin/bitcoin/pull/24691.
Also rename the NetinfoRequestHandler::is_block_relay data member to is_tx_relay and inverse its boolean logic. The naming is out of date and incorrect, as lack of request of tx relay does not imply block relay, and a preference for tx relay doesn't imply that block relay isn't happening. Thanks to Marco Falke and Martin Zumsande for their feedback on this.
(I may look at reducing the number of optional node stats fields via refactoring at the net processing level, but ongoing refactoring there may make that slow or complicated and this is a one-line fix that works now.)
ACKs for top commit:
mzumsande:
Code review ACK a17c5e96b6
Tree-SHA512: dc54ce80b78122874a6794555f99e5b328a1574b52bb3e7f974c699c2b759a60ea0807a6483c5bc0414a950d853c0eeeb13dcc1b790d3917c6ee4c9c99fe159f
6fbb0edac2 Set effective_value when initializing a COutput (ishaanam)
Pull request description:
Previously in COutput, effective_value was initialized as the absolute value of the txout and the fee as 0. effective_value along with the fee was calculated outside of the COutput constructor and set after the object had been initialized.
These changes will allow either the fee or the feerate to be passed in a COutput constructor and the fee and effective_value are calculated and set in the constructor. As a result, AvailableCoins also needs to be passed the feerate when utxos are being spent. When balance is calculated or the coins are being listed and feerate is neither available nor required, AvailableCoinsListUnspent is used instead, which runs AvailableCoins while providing the default value for `feerate`. Unit tests for the calculation of effective value have also been added.
ACKs for top commit:
achow101:
re-ACK 6fbb0edac2
Xekyo:
re-ACK 6fbb0edac2
w0xlt:
Code Review ACK 6fbb0edac2
furszy:
Looks good, have been touching this area lately, code review ACK 6fbb0eda.
Tree-SHA512: 5943ee4f4b0c1dcfe146f2fc22853e607259d6d53156b80a8a8f4baa70760a8b25ab822777b7f5d21ecb02dac08bdee704a9a918d5660276d6994b19a284b256
baa3ddc49c doc: add release notes about `getreceivedbylabel` returning an error if the label is not in the address book. (furszy)
8897a21658 rpc: getreceivedbylabel, don't loop over the entire wallet txs map if no destinations were found for the input label. (furszy)
Pull request description:
Built on top of #23662, coming from comment https://github.com/bitcoin/bitcoin/pull/23662#pullrequestreview-971407999.
If `wallet.GetLabelAddresses()` returns an empty vector (the wallet does not have stored destinations with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
Otherwise, we are walking through all the wallet txs + outputs for no reason (`output_scripts` is empty).
ACKs for top commit:
achow101:
ACK baa3ddc49c
theStack:
re-ACK baa3ddc49c
w0xlt:
ACK baa3ddc49c
Tree-SHA512: 00e10365b179bf008da2f3ef8fbb3ee04a330426374020e3f2d0151b16991baba4ef2b944e4659452f3e4d6cb20f128d0918ddf0453933a25a4d9fd8414a1911
71a8dbe5da refactor: Remove defunct attributes.h includes (Ben Woosley)
Pull request description:
Since the removal of NODISCARD in 81d5af42f4,
the only attributes.h def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
See also #20499.
Top commit has no ACKs.
Tree-SHA512: f3e10a5cda5ab78371b77b702f4a241ff69d490a16cc6059f1a4202b97c584accdbc951cc7b6120eae94bee3b9249e9117b45cf6ed1a5228ca23b5638fcf7b7b
252f363f2f qt: Replace `QCoreApplication::quit()` with `QCoreApplication::exit(0)` (Hennadii Stepanov)
Pull request description:
### Qt 5:
- no behavior change.
See https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/kernel/qcoreapplication.cpp?h=5.15#n2012:
```cpp
void QCoreApplication::quit()
{
exit(0);
}
```
### Qt 6:
- this change avoids sending a duplicated `QEvent::Quit`
We use `QEvent::Quit` to [handle](https://github.com/bitcoin-core/gui/pull/547) macOS dock menu events. Qt 6 uses `QEvent::Quit` more [widely](89f7a2759c). We do not want a duplicated `QEvent::Quit` which fires `Assert(node.args);` in the [`Shutdown()`](d1b3dfb275/src/init.cpp (L200)) function.
ACKs for top commit:
promag:
Code review ACK 252f363f2f.
Tree-SHA512: 6a04cbcf523c0375158a59b29afadf18da99738c8db8b8728f99319a8cdc10806d2f06dc5a7d3b8b0e1a5f1711be778a75d4ecdefef7cf66e26ae2848f7f57db
a63b60f02b refactor: Add OptionsModel getOption/setOption methods (Ryan Ofsky)
Pull request description:
This is a trivial change which is needed as part of #602 to get bitcoind and bitcoin-qt to use the same settings instead of different settings. It is split off from #602 because it causes a lot of rebase conflicts (any time there is a GUI options change).
This PR is very small and easy to review ignoring whitespace: https://github.com/bitcoin-core/gui/pull/600/files?w=1
ACKs for top commit:
vasild:
ACK a63b60f02b
furszy:
Code review ACK a63b60f0
promag:
Code review ACK a63b60f02b.
Tree-SHA512: 1d99a1ce435b4055c1a38d2490702cf5b89bacc7d168c9968a60550bfd9f6dace649d5e98699de68d6305f02d5d1e3eb7e177ab578b98b996dd873b1df0ed236
Element count used in the GCSFilter benchmarks are increased to 100,000
from 10,000. Testing the benchmarks with different element counts showed
that a filter with 100,000 elements resulted in the same ns/op. This
this a desirable thing to have as it allows us to reason about how long
a single filter element takes to process, letting us easily calculate
how long a filter with N elements (where N > 100,000) would take to
process.
GCSFilterConstruct benchmark is now called without batch. This makes
intra-bench results more intuitive as all benchmarks are in ns/op
instead of a custom unit. There are no downsides to this change as
testing showed that there is no observable difference in error rates
in the benchmarks when calling without batch.
This benchmark allows us to compare the differences between doing the
sanity check for corruption via GolombRiceDecode() vs checking the hash
of the encoded block filter.
All of the benchmarks are standardized on the BASIC filter parameters
so we can compare between all the benchmarks. All the GCS
benchmarks are renamed to have "GCS" as the prefix.
Since the removal of NODISCARD in 81d5af42f4,
the only attributes def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
Previously in COutput, effective_value was initialized as the absolute
value of the txout, and fee as 0. effective_value along with fee were
calculated outside of the COutput constructor and set after the
object had been initialized. These changes will allow either the fee
or the feerate to be passed in a COutput constructor. If either are
provided, fee and effective_value are calculated and set in the
constructor. As a result, AvailableCoins also needs to be passed the
feerate when utxos are being spent. When balance is calculated or the
coins are being listed and feerate is neither available nor required,
AvailableCoinsListUnspent is used instead, which runs AvailableCoins
while providing the default value for feerate. Unit tests for the
calculation of effective value have also been added.
Currently, CCoinsStats is a struct with both in-params and out-params
where the hash_type and index_requested members are the only in-params.
This change removes CCoinsStats' hash_type in-param member and adds it
to the relevant functions instead.
[META] In subsequent commits, all of CCoinsStats' members which serve as
in-params will be moved out so as to make CCoinsStats a pure
out-param struct.
In the GetUTXOStats fuzz case, GetUTXOStats is always called with a
CCoinsViewCache. Which is guaranteed to throw a std::logic_error when
its ::Cursor() method is called on the first line of GetUTXOStats.
In the fuzz case, we basically catch this logic error and declare
victory if we caught it.
There is no point to fuzzing this deterministic logic.
Confirmed with IWYU that the node/coinstats.h #include is no longer
necessary.
It is no longer necessary to link in blockfilter.cpp and
index/blockfilterindex.cpp after merge of PR#21726 since validation has
been decouple from the blockfilterindex.
If wallet.GetLabelAddresses() returns an empty vector (the wallet does not have addresses with that label in the addressbook) or if none of the returned destinations are from the wallet, we can return the function right away.
53494bc739 validation: Have ChainstateManager own m_chainparams (Carl Dong)
04c31c1295 Add ChainstateManager::m_adjusted_time_callback (Carl Dong)
dbe45c34f8 Add ChainstateManagerOpts, using as ::Options (Carl Dong)
Pull request description:
```
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
```
Top commit has no ACKs.
Tree-SHA512: a093ec6ecacdc659d656574f05bd31ade6a6cdb64d5a97684f94ae7e55c0e360b78177553d4d1ef40280192674464d029a0d68e96caf8711d9274011172f1330
3f8def51d5 add 3 new test cases for SelectCoins() (akankshakashyap)
Pull request description:
Three new tests have been added.
1. More coins should be selected when effective fee < long term fee.
2. Less coin should be selected when effective fee > long term fee.
3. If a coin is preselected, it should be selected even if disadvantageous.
ACKs for top commit:
achow101:
ACK 3f8def51d5
brunoerg:
ACK 3f8def51d5
Tree-SHA512: 8db6dd942b02a38c99953b801605f98c4c17729768fdfcf7605c5bbdb17509500a39d0a78a4b19aab37812d2994ec7630d2b4e78d1d348f1c27b67588d74e155
We want m_chainparams to be alive for the duration of
ChainstateManager's lifetime since ChainstateManager's behaviour depends
on m_chainparams.
We could allow for a std::shared_ptr to be passed in as m_chainparams,
but that complicates things further. Given that CChainParams is not an
entity class or struct, we can just copy it and have ChainstateManager
own it.
This decouples validation.cpp from netaddress.cpp (transitively,
timedata.cpp, and asmap.cpp).
This is important for libbitcoinkernel as:
- There is no reason for the consensus engine to be coupled with
netaddress, timedata, and asmap
- Users of libbitcoinkernel can now easily supply their own
std::function that provides the adjusted time.
See the src/Makefile.am changes for some satisfying removals.
[META] Although it seems like we don't need it for just one option,
we're going to introduce another member to this struct *in the
next commit*. In future patchsets for libbitcoinkernel decoupling
it from ArgsManager, even more members will be added here.
and inverse its logic.
The naming is out of date and incorrect, as lack of request of tx relay does not
imply block relay, and a preference for tx relay doesn't imply that block relay
isn't happening.
fa1b76aeb0 Do not call global Params() when chainman is in scope (MacroFake)
fa30234be8 Do not pass CChainParams& to PeerManager::make (MacroFake)
fafe5c0ca2 Do not pass CChainParams& to BlockAssembler constructor (MacroFake)
faf012b438 Do not pass Consensus::Params& to Chainstate helpers (MacroFake)
fa4ee53dca Do not pass time getter to Chainstate helpers (MacroFake)
Pull request description:
It seems confusing to pass chain params, consensus params, or a time function around when it is not needed.
Fix this by:
* Inlining the passed time getter function. I don't see a use case why this should be mockable.
* Using `chainman.GetConsensus()` or `chainman.GetParams()`, where possible.
ACKs for top commit:
promag:
Code review ACK fa1b76aeb0.
vincenzopalazzo:
ACK fa1b76aeb0
Tree-SHA512: 1abff5cba4b4871d97f17dbcdf67bc9255ff21fa4150a79a74e39b28f0610eab3e7dee24d56872dd6e111f003b55e288958cdd467e6218368d896f191e4ec9cd
"error: JSON value is not a boolean as expected"
due to fRelayTxes/m_relay_txs being moved in PR 21160 from CNodeStats to
CNodeStateStats, which made getpeerinfo#relaytxes an optional field that
can return UniValue IsNull().
Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_tx_relay_mutex
Could be verified with
$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
$ make clean
$ make 2>&1 | grep m_most_recent_block_mutex
bcbf982553 qt, doc: Remove unneeded comments (Hennadii Stepanov)
9bd1565f65 qt: Revamp ClientModel code to handle {Block|Header}Tip core signals (Hennadii Stepanov)
48f6d39659 qt: Revamp ClientModel code to handle BannedListChanged core signal (Hennadii Stepanov)
36b12af7ee qt: Revamp ClientModel code to handle AlertChanged core signal (Hennadii Stepanov)
bfe5140c50 qt: Revamp ClientModel code to handle NetworkActiveChanged core signal (Hennadii Stepanov)
639563d7fe qt: Revamp ClientModel code to handle NumConnectionsChanged core signal (Hennadii Stepanov)
508e2dca5e qt: Revamp ClientModel code to handle ShowProgress core signal (Hennadii Stepanov)
Pull request description:
This PR:
- is a pure refactoring with no behavior change
- gets rid of `QMetaObject::invokeMethod()` "dynamic" calls, i.e., without compile-time checks of a called function name and its parameters
- replaces `std::bind`s with lambdas, making parameter permutation (including parameter omitting) explicit
- makes code simpler, more concise, and easier to reason about
Additionally, debug messages have been unified.
ACKs for top commit:
promag:
Code review ACK bcbf982553
w0xlt:
tACK bcbf982553 on Ubuntu 21.10, Qt 5.15.2.
Tree-SHA512: 35f62b84f916b3ad7442f0fea945d344b3c448878b33506ac7b81fdf5e49bd2a82e12a6927dc91f62c335487bf2305cc45e2f08985303eef31c3ed2dd39e1037
e3daecae03 scripted-diff: replace deprecated Q_OS_MAC with Q_OS_MACOS (João Barbosa)
Pull request description:
`Q_OS_MAC` is deprecated but it is also defined when Qt is configured with `-xplatform macx-ios-clang`, and currently it guards some features not available on iOS, like `QProcess`.
ACKs for top commit:
jarolrod:
tACK e3daecae03
hebasto:
ACK e3daecae03.
Tree-SHA512: 17b4b891c70f027f6a420be830e61bd87fde5297a4473a5b122e4e34bdf83141635bd5cf5143efe95a0dd6f8cf50bc67a2de6cbfed7956952369587c74ece225
facd1fb911 refactor: Use Span of std::byte in CExtKey::SetSeed (MarcoFalke)
fae1006019 util: Add ParseHex<std::byte>() helper (MarcoFalke)
fabdf81983 test: Add test for embedded null in hex string (MarcoFalke)
Pull request description:
This adds the hex->`std::byte` helper after the `std::byte`->hex helper was added in commit 9394964f6b
ACKs for top commit:
pk-b2:
ACK facd1fb911
laanwj:
Code review ACK facd1fb911
Tree-SHA512: e2329fbdea2e580bd1618caab31f5d0e59c245a028e1236662858e621929818870b76ab6834f7ac6a46d7874dfec63f498380ad99da6efe4218f720a60e859be
a4703ce9d7 doc: add release notes about removal of the `deprecatedrpc=exclude_coinbase` (Sebastian Falbesoner)
ef0aa74836 rpc: wallet: remove `-deprecatedrpc=exclude_coinbase` logic (Sebastian Falbesoner)
Pull request description:
Including coinbase transactions in `receivedby` RPCs and adding the `-deprecatedrpc=exclude_coinbase` was done in PR #14707 (released in v23.0). For the next release v24.0, this configuration option can be removed.
ACKs for top commit:
fanquake:
ACK a4703ce9d7
Tree-SHA512: 97cd4e78501e64f678c78d2ebb5be5376688c023e34fced71dd24e432d27aa31a74b5483545f49ba0bdf48656d8b8b7bee74e3db26cf6daf112613f1caa4dfa4
fafae678f6 build: Enable RPC_DOC_CHECK on --enable-debug (MacroFake)
Pull request description:
This probably makes no large difference, as the setting is already enabled by default in the functional tests. However, I think it is nice to also enable it in debug builds by default to catch issues while manually testing without the runtime flags specified.
See also https://github.com/bitcoin/bitcoin/issues/24709
ACKs for top commit:
vincenzopalazzo:
utACK fafae678f6
Tree-SHA512: cea3276fc9b5a3bc0f6d9819be9a50b19ecf762729d3e3975abdf00da06beaa3f664b18a826fbab1fedd9143bc0624a95a490bfe40c4b5b0a0f94dbc565ce738
1d4122dfef init: Allow -proxy="" setting values (Ryan Ofsky)
Pull request description:
This drops the `No proxy server specified. Use -proxy=<ip> or -proxy=<ip:port>` error when a empty `-proxy=` command line argument, `bitcoin.conf` value, or `settings.json` value is specified, and just makes bitcoin connect and listen normally in these cases.
The error was originally added in https://github.com/bitcoin/bitcoin/pull/20003 to prevent a bare `-proxy` command line argument with no assignment from clearing proxy settings. But it was implemented in an overbroad way breaking empty `-proxy=` assignments as well.
The motivation for this change is to prevent a GUI bug that happens with https://github.com/bitcoin/bitcoin/pull/15936, reported in https://github.com/bitcoin/bitcoin/pull/15936#pullrequestreview-937685759 by vasild, that happens after a proxy setting is enabled and disabled in the GUI. But this change also makes sense on its own to remove a potentially confusing error message.
ACKs for top commit:
hebasto:
re-ACK 1d4122dfef, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/24830#pullrequestreview-941255672).
Tree-SHA512: 753adfce199ed078a6cd9e0ea78e76c0b14070f8fcfe2a4632cd0c6dfe6b4e135ddffbe11a97e5e30520ea9e5bda00bad1493cbaef74cf425aa8613249167f53
fa305fd92c Add mockable clock type and TicksSinceEpoch helper (MarcoFalke)
Pull request description:
This will be used primarily by the addr time refactor (https://github.com/bitcoin/bitcoin/pull/24697) to make addr relay time type safe. However, it can also be used in other places, and can be reviewed independently, so I split it up.
ACKs for top commit:
jonatack:
ACK fa305fd92c per `git range-diff 7b3343f fa20781 fa305fd`
ajtowns:
ACK fa305fd92c
Tree-SHA512: da00200126833c1f55b1b1e68f596eab5c9254e82b188ad17779c08ffd685e198a7c5270791b4b69a858dc6ba4e051fe0c8b445d203d356d0c884f6365ee1cfe
Allows the GUI to clear settings.json file and save settings.json.bak file when
GUI "Reset Options" button is pressed or -resetguisettings command line option
is used. (GUI code already backs up and resets the "guisettings.ini" file this
way, so this just makes the same behavior possible for "settings.json")
Will allow OptionsModel to read/write settings to the node settings.json
file and share settings with the node, instead of storing them
externally in QSettings.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
fa9af21878 scripted-diff: Use getInt<T> over get_int/get_int64 (MacroFake)
Pull request description:
Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).
ACKs for top commit:
fanquake:
ACK fa9af21878
Tree-SHA512: 284aa2527d0f663ca01550115025c9c64c787531d595f866c718f6ad09b9b0cac1e683a7d77f8009b75de990fd37166b44063ffa83fba8a04e9a31600b4c2725
This commit removes the `node.args = nullptr` assignment in the Shutdown()
function.
Clearing node.args there never made sense because it made the
Shutdown() function not idempotent, making it fragile and causing issues like
https://github.com/bitcoin/bitcoin/issues/23186.
The assignment also causes segfaults in GUI unit tests when a new
node().initParameterInteraction() call is added in OptionsModel to apply to Qt
settings (happens because AppTests calls Shutdown() which sets node.args to
null, and OptionTests runs after AppTests and then needs node.args not to be
null.)
Add interfaces::Node methods to give GUI finer grained control over
settings.json file. Update method is used to write settings to the file,
getPersistent and isIgnored methods are used to find out about settings
file and command line option interactions.
7171ebc7cb index: Don't commit a best block before indexing it during sync (Martin Zumsande)
Pull request description:
This changes the periodic commit of the best block during the index sync phase to use the already indexed predecessor of the current block index, instead of committing the current one that will only be indexed (by calling `WriteBlock()`) after committing the best block.
The previous code would leave the index database in an inconsistent state until the block is actually indexed - if an unclean shutdown happened at just this point in time, the index could get corrupted because at next startup, we'd assume that we have already indexed this block.
ACKs for top commit:
ryanofsky:
Code review ACK 7171ebc7cb. Looks great! Just commit message changes since last review
Tree-SHA512: a008de511dd6a1731b7fdf6a90add48d1e53f7f7d6402672adb83e362677fc5b9f5cd021d3111728cb41d73f1b9c2140db79d7e183df0ab359cda8c01b0ef928
Committing a block prior to indexing would leave the index database
in an inconsistent state until it is indexed, which could corrupt the
index in case of a unclean shutdown. Thus commit its predecessor.
Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
bf6526f4a0 [test] Remove segwit argument from build_block_on_tip() (John Newbery)
c65bf50b44 Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor (John Newbery)
Pull request description:
This implements two of the suggestions from code reviews of PR 20799:
- Remove fUseWTXID parameter from CBlockHeaderAndShortTxIDs constructor
- Remove segwit argument from build_block_on_tip()
ACKs for top commit:
dergoegge:
Code review ACK bf6526f4a0
naumenkogs:
ACK bf6526f4a0
Tree-SHA512: d553791d1364b9e655183755e829b195c9b47f59c62371dbae49d9c0f8d84fec58cf18f4dde89591672ef5658e18c9cf0206c2efd70606980f87e506bc3bd4e5
9db82f1bca [net processing] Don't initialize TxRelay for non-tx-relay peers. (John Newbery)
b0a4ac9c26 [net processing] Add m_tx_relay_mutex to protect m_tx_relay ptr (John Newbery)
290a8dab02 [net processing] Comment all TxRelay members (John Newbery)
42e3250497 [net processing] [refactor] Move m_next_send_feefilter and m_fee_filter_sent (John Newbery)
Pull request description:
block-relay-only connections are additional outbound connections that bitcoind makes since v0.19. They participate in block relay, but do not propagate transactions or addresses. They were introduced in #15759.
When creating an outbound block-relay-only connection, since we know that we're never going to announce transactions over that connection, we can save on memory usage by not a `TxRelay` data structure for that connection. When receiving an inbound connection, we don't know whether the connection was opened by the peer as block-relay-only or not, and therefore we always construct a `TxRelay` data structure for inbound connections.
However, it is possible to tell whether an inbound connection will ever request that we start announcing transactions to it. The `fRelay` field in the `version` message may be set to `0` to indicate that the peer does not wish to receive transaction announcements. The peer may later request that we start announcing transactions to it by sending a `filterload` or `filterclear` message, **but only if we have offered `NODE_BLOOM` services to that peer**. `NODE_BLOOM` services are disabled by default, and it has been recommended for some time that users not enable `NODE_BLOOM` services on public connections, for privacy and anti-DoS reasons.
Therefore, if we have not offered `NODE_BLOOM` to the peer _and_ it has set `fRelay` to `0`, then we know that it will never request transaction announcements, and that we can save resources by not initializing the `TxRelay` data structure.
ACKs for top commit:
MarcoFalke:
review ACK 9db82f1bca🖖
dergoegge:
Code review ACK 9db82f1bca
naumenkogs:
ACK 9db82f1bca
Tree-SHA512: 83a449a56cd6bf6ad05369f5ab91516e51b8c471c07ae38c886d51461e942d492ca34ae63d329c46e56d96d0baf59a3e34233e4289868f911db3b567072bdc41
ac6fbf2c83 tidy: use modernize-use-default-member-init (fanquake)
7aa40f5563 refactor: use C++11 default initializers (fanquake)
Pull request description:
Refactor and then enable [`modernize-use-default-member-init`](https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-default-member-init.html) in our `clang-tidy` job.
Top commit has no ACKs.
Tree-SHA512: 536b406f20639f8c588fe9e96175ec60c7bb825506b2670b562370b2f572801c24203c483443be3c199e1b958c0765d4532e57c57a4e78689162a1dd422d844f
Delay initializing the TxRelay data structure for a peer until we receive
a version message from that peer. At that point we'll know whether it
will ever relay transactions. We only initialize the m_tx_relay
data structure if:
- this isn't an outbound block-relay-only connection; AND
- fRelay=true OR we're offering NODE_BLOOM to this peer
(NODE_BLOOM means that the peer may turn on tx relay later)
This fully comments all the TxRelay members. The only significant change
is to the comment for m_relay_txs. Previously the comment stated that
one of the purposes of the field was that "We don't relay tx invs before
receiving the peer's version message". However, even without the
m_relay_txs flag, we would not send transactions to the peer before
receiving the `version` message, since SendMessages() returns
immediately if fSuccessfullyConnected is not set to true, which only
happens once a `version` and `verack` message have been received.
Move m_next_send_feefilter and m_fee_filter_sent out of the `TxRelay`
data structure. All of the other members of `TxRelay` are related to
sending transactions _to_ the peer, whereas m_fee_filter_sent and
m_next_send_feefilter are both related to receiving transactions _from_
the peer. A node's tx relay behaviour is not always symmetrical (eg a
blocksonly node will ignore incoming transactions, but may still send
out its own transactions), so it doesn't make sense to group the
feefilter sending data with the TxRelay data in a single structure.
This does not change behaviour, since IsBlockOnlyConn() is always equal
to !peer.m_tx_relay. We still don't send feefilter messages to outbound
block-relay-only peers (tested in p2p_feefilter.py).
3a998d2e37 Use steady_clock in ConnectAndCallRPC and inline time call in loop conditional (Jon Atack)
3799d2dcdd Fix -rpcwait with -netinfo printing negative time durations (Jon Atack)
Pull request description:
- Fix `bitcoin-cli -rpcwait -netinfo 1` returning negative time durations on its first invocation after node startup in the "send", "recv", and "age" columns (potentially the "txn" and "blk" columns also). To reproduce, start bitcoind on mainnet (for a longer startup time) and run `bitcoin-cli -rpcwait -netinfo <n>` where n is 1 or larger. The negative time durations are larger with a slower CPU speed or e.g. higher `checkblocks`/`checklevel` config option settings.
Examples:
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual onion -126 -126 -2 0
ms ms sec sec min min min
```
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual cjdns -64 -64 -1 0
ms ms sec sec min min min
```
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual ipv4 -89 -89 * . -1 0
ms ms sec sec min min min
```
```
<-> type net mping ping send recv txn blk hb addrp addrl age id
out manual ipv6 -133 * . -2 0
ms ms sec sec min min min
```
- Use `steady_clock` in ConnectAndCallRPC and inline the time call in the loop conditional to avoid unnecessary invocations and an unneeded local variable allocation.
ACKs for top commit:
MarcoFalke:
cr ACK 3a998d2e37
Tree-SHA512: 141430d47189ad9f646ce8e51cb31c21b395f6294bb27ba9f7ae4c1e1505a63209a4a19662a0b462806437a9cfd07f1ea114e775adc2872d87397fe823f8b8dc
a55db4ea1c Add more proper thread safety annotations (Hennadii Stepanov)
8cfe93e3fc Add proper thread safety annotation to `CWallet::GetTxConflicts()` (Hennadii Stepanov)
ca446f2c59 Add proper thread safety annotation to `CachedTxGetAvailableCredit()` (Hennadii Stepanov)
Pull request description:
In non-test/benchmarking code, there are three cases of the `NO_THREAD_SAFETY_ANALYSIS` annotation which are accompanied with `TODO` comments.
This PR adds proper thread safety annotations instead of `NO_THREAD_SAFETY_ANALYSIS`.
ACKs for top commit:
laanwj:
Code review ACK a55db4ea1c
Tree-SHA512: 806d72eebc1edf088bfa435c8cd11465be0de6789798dd92abd008425516768acb864a73d834a49d412bb10f7fccfb47473f998cb72739dab6caeef6bcfaf191
ada8358ef5 Sanitize port in `addpeeraddress()` (amadeuszpawlik)
Pull request description:
In connection to #22087, it has been [pointed out](https://github.com/bitcoin/bitcoin/pull/22087#pullrequestreview-674786285) that `addpeeraddress` needs to get its port-value sanitized.
ACKs for top commit:
fanquake:
ACK ada8358ef5
Tree-SHA512: 48771cd4f6940aa7840fa23488565c09dea86bd5ec5a5a1fc0374afb4857aebcd2a1f51e2d4cb7348460e0ad9793dc5d2962df457084ed2b8d8142cae650003f
4f31c21b7f bench: Make all arguments -kebab-case (laanwj)
652b54e532 bench: Add `--sanity-check` flag, use it in `make check` (laanwj)
Pull request description:
The benchmarks are run as part of `make check` for a crash-sanity check. The actual results are being ignored. So only run them for one iteration.
This makes the `bench_bitcoin` part take 2m00 instead of 5m20 here. Which is still too long (imo), but this needs to be solved in the `WalletLoading*` benchmarks which take that long per iteration.
Also change all `bench_bitcoin` arguments to kebab-case to be consistent with the other tools (in a separate commit).
ACKs for top commit:
jonatack:
ACK 4f31c21b7f on the sanity-check version per `git diff c52a71e 4f31c28` (modulo s/--sanity check/--sanity-check/ in src/bench/bench.cpp::L61)
hebasto:
ACK 4f31c21b7f, tested on Ubuntu 22.04.
Tree-SHA512: 2661d130fd82e57c9041755190997a4af588fadddcdd05e04fd024f75da1202480e9feab5764566e8dfe7930e8ae0ec71e93f40ac373274953d274072723980d
