Since accounts were removed in commit c9c32e6b84,
this function is only called at one place and thus can be as well inlined. Also,
avoid a duplicate lookup by using the find() method and dereference, instead of
calling count() and operator[].
fa55c3dc1b Raise InitError when peers.dat is invalid or corrupted (MarcoFalke)
fa4e2ccfd8 Inline ReadPeerAddresses (MarcoFalke)
fa5aeec80c Move LoadAddrman from init to addrdb (MarcoFalke)
Pull request description:
peers.dat is silently erased when it can not be parsed or when it appears corrupted. Fix that by notifying the user. This might help in the following examples:
* The user provided the database, but picked the wrong one.
* A future version of Bitcoin Core wrote the file and it can't be read.
* The file was corrupted by a logic bug in Bitcoin Core.
* The file was corrupted by a disk failure.
ACKs for top commit:
jonatack:
Code review re-ACK fa55c3dc1b per `git range-diff eb1f570 fa59c6d fa55c3` and verified the new tests fail on master, except "Check mocked addrman is valid", as expected
prayank23:
tACK fa55c3dc1b
vasild:
ACK fa55c3dc1b
Tree-SHA512: 78264a78ee570a3c3262cf9c8542b5ffaffa5f52da1eef66c8c381f346989272967cfe1769c573502d9d7d3f7ad68c3ac3b2ec734185d2e4e7595b7122b14196
32748da0f4 whitespace fixups after move and scripted-diff (glozow)
fa47622e8d scripted-diff: rename variables in policy/rbf (glozow)
ac761f0a23 MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf (glozow)
9c2f9f8984 MOVEONLY: check that fees > direct conflicts to policy/rbf (glozow)
3f033f01a6 MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf (glozow)
7b60c02b7d MOVEONLY: BIP125 Rule 2 to policy/rbf (glozow)
f8ad2a57c6 Make GetEntriesForConflicts return std::optional (glozow)
Pull request description:
This PR does not change behavior. It extracts the BIP125 logic into helper functions (and puts them in the policy/rbf* files). This enables three things - I think each one individually is pretty good:
- Implementation of package RBF (see #22290). I want it to be as close to BIP125 as possible so that it doesn't become a distinct fee-bumping mechanism. Doing these move-only commits first means the diff is mostly mechanical to review, and I just need to create a function that mirrors the single transaction validation.
- We will be able to isolate and test our RBF logic alone. Recently, there have been some discussions on discrepancies between our code and BIP125, as well as proposals for improving it. Generally, I think making this code more modular and de-bloating validation.cpp is probably a good idea.
- Witness Replacement (replacing same-txid-different-wtxid when the witness is significantly smaller and therefore higher feerate) in a BIP125-similar way. Hopefully it can just be implemented with calls to the rbf functions (i.e. `PaysForRBF`) and an edit to the relevant mempool entries.
ACKs for top commit:
mjdietzx:
ACK 32748da0f4
theStack:
Code-review ACK 32748da0f4📐
MarcoFalke:
review ACK 32748da0f4🦇
Tree-SHA512: d89985c8b4b42b54861018deb89468e04968c85a3fb1113bbcb2eb2609577bc4fd9bf254593b5bd0e7ab059a0fa8192d1a903b00f77e6f120c7a80488ffcbfc0
853c4edb70 [net] Remove asmap argument from CNode::CopyStats() (John Newbery)
9fd5618610 [asmap] Make DecodeAsmap() a utility function (John Newbery)
bfdf4ef334 [asmap] Remove SanityCheckASMap() from netaddress (John Newbery)
07a9eccb60 [net] Remove CConnman::Options.m_asmap (John Newbery)
Pull request description:
These small cleanups to the asmap code are the first 4 commits from #22910. They're minor improvements that are independently useful whether or not 22910 is merged.
ACKs for top commit:
naumenkogs:
ACK 853c4edb70
theStack:
Concept and code-review ACK 853c4edb70🗺️
fanquake:
ACK 853c4edb70
Tree-SHA512: 64783743182592ac165df6ff8d18870b63861e9204ed722c207fca6938687aac43232a5ac4d8228cf8b92130ab0349de1b410a2467bb5a9d60dd9a7221b3b85b
At this point, or minimum required glibc is implicitly 2.18, due to
thread_local support being enabled by default. However, users can
disable thread_local support to maintain 2.17 ccompat for now, which is
currently done in the Guix build.
f530202353 Make unexpected time type in BCLog::LogMsg() a compile-time error (Martin Ankerl)
bddae7e7ff Add util/types.h with ALWAYS_FALSE template (MarcoFalke)
498b323425 log, timer: improve BCLog::LogMsg() (Jon Atack)
8d2f847ed9 sync: inline lock contention logging macro to fix time duration (Jon Atack)
Pull request description:
Follow-up to #22736.
The first commit addresses the issue identified and reported by Martin Ankerl in https://github.com/bitcoin/bitcoin/pull/22736#discussion_r703019629 to fix the lock contention duration reporting.
The next three commits make improvements to the timer code in `BCLog::LogMsg()` and add `util/types.h` with an `ALWAYS_FALSE` template, that springboard from https://github.com/bitcoin/bitcoin/pull/22736#discussion_r702747920 by Marco Falke.
ACKs for top commit:
martinus:
re-ACK f530202353. I ran a fully synced node for about a day. My node was mostly idle though so not much was going on. I [wrote a little script](https://github.com/martinus/bitcoin-stuff/blob/main/scripts/parse-debuglog-contention-single.rb) to parse the `debug.log` and summarize the output to see if anything interesting was going on, here is the result:
theStack:
ACK f530202353
Tree-SHA512: 37d093eac5590e1b5846ab5994d0950d71e131177d1afe4a5f7fcd614270f977e0ea117e7af788e9a74ddcccab35b42ec8fa4db3a3378940d4988df7d21cdaaa
fa309ee61c bench: Fix 32-bit compilation failure in addrman bench (MarcoFalke)
fae0295a79 ci: Switch multiprocess to i686 build (MarcoFalke)
Pull request description:
Building for i686 with clang helps to catch bugs early for:
* The OSS-Fuzz i686 clang libFuzzer build
* The arm 32-bit native clang build
Fixes #22889
ACKs for top commit:
hebasto:
ACK fa309ee61c
Tree-SHA512: 581820d319aae2fcd4dd44979ee3d4164a575f0438476890aa2a7447f1392a5da26766cd6ab954530499b54f66eec2417bdeefdd7efb19bc27dd679cd2b9d0ce
e6998838e5 doc: Add IPv6 address to zmq example (nthumann)
8abe5703a9 test: Add IPv6 test to zmq (nthumann)
ded449b726 zmq: Enable IPv6 on listening socket (nthumann)
Pull request description:
This PR adds support for listening on IPv6 addresses with bitcoinds ZMQ interface, just like the RPC server.
Currently, it is not possible to specify an IPv6 address, as the `ZMQ_IPV6` [socket option](http://api.zeromq.org/master:zmq-setsockopt#toc27) is not set and therefore the ZMQ initialization fails, if one does so. The absence of this option has also been noted [here](https://github.com/bitcoin/bitcoin/issues/15198#issuecomment-617378512).
With this PR one can e.g. set `-zmqpubhashblock=tcp://[::1]:28333` to listen on the IPv6 loopback address.
ACKs for top commit:
laanwj:
Code review ACK e6998838e5
theStack:
Tested ACK e6998838e5🌱
Tree-SHA512: 43c3043d8d5c79794d475926259c1be975b694db4fcc1f7750a9a28e242f0fa1b531735a63ea5777498003aa5834f6243f39742d0f3941f2f37593d0c7890700
Init should only concern itself with the initialization order, not the
detailed initialization logic of every module.
Also, inlining logic into a method that is ~800 lines of code, makes it
impossible to unit test on its own.
fade9a1a4d Remove confusing CAddrDB (MarcoFalke)
fa7f77b7d1 Fix addrdb includes (MarcoFalke)
fa3f5d0dae Move addrman includes from .h to .cpp (MarcoFalke)
Pull request description:
Split out from #22762 to avoid having to carry it around in (an)other rebase(s)
ACKs for top commit:
ryanofsky:
Code review ACK fade9a1a4d
lsilva01:
Code Review ACK fade9a1a4d
Tree-SHA512: 7615fb0b6235d0c1e6f8cd6263dd18c4d95890567c2b797fe6fce6cb12cc85ce6eacbe07dbb6d81b05d179ef03b42edfd61c940e35a1044ce6d363b54c2dae5c
fdd71448e7 system: skip trying to set the locale on NetBSD (fanquake)
Pull request description:
Just treat it the same as the other BSDs.
Fixes #17379.
ACKs for top commit:
laanwj:
Code review ACK fdd71448e7
practicalswift:
cr ACK fdd71448e7
Tree-SHA512: 5fe0a66f014279ad2683b548692a36af493377fb92d1f28b15dc4feef871190fe08ef40dcc4f5ba21a525fe365c42fb429fe4be0673a1e96db163af587c23204
fa57fa1a2e Enable clang-tidy bugprone-argument-comment and fix violations (MarcoFalke)
Pull request description:
Named arguments can be dangerous when they are wrong, because they are not enforced by the compiler. Currently there are only minor typos, no actual bugs.
Fix the typos and add the `.clang-tidy` file to make it easier to find them in the future.
ACKs for top commit:
practicalswift:
cr ACK fa57fa1a2e
fanquake:
ACK fa57fa1a2e
Tree-SHA512: b66f01e0a1e77e56ed8454002176df660cc2cc0947a90785aa33cc5b8003a1f99fd8b2f8f89f2a0bf180ff2c42c031d69e669d127bb557b879c17975275a220b
fab0b55cf0 addrman: Fix format string in deserialize error (MarcoFalke)
facce4ca44 test: Remove useless overwrite (MarcoFalke)
Pull request description:
The format string is evaluated differently on modern compilers (clang 10 and later, as well as gcc 10 and later).
Work around the behaviour change in compilers by pinning the underlying type of the format arguments.
Can be tested by observing a failing test when running against master compiled with clang 10 or gcc 10 (or later).
ACKs for top commit:
jonatack:
ACK fab0b55cf0 verified the test fails on master as expected only at line 61 (assertion fixed by the code change); the last two test additions pass as expected
mzumsande:
ACK fab0b55cf0
Tree-SHA512: 07462901435107f3bc79098fd7d06446bfe8fe065fffdd35adfcba8f1dd3c499575006557afe7bc74b79d690c5ef7b58e3e031e908161be5529cf237e3b30609
DecopeAsmap is a pure utility function and doesn't have any
dependencies on addrman, so move it to util/asmap.
Reviewer hint: use:
`git diff --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
The class only stores the file path, reading it from a global. Globals
are confusing and make testing harder.
The method reading from a stream does not even use any class members, so
putting it in a class is also confusing.
5fabde6fad wallet: AddWalletDescriptor requires cs_wallet lock (João Barbosa)
32d036e8da wallet: GetLabelAddresses requires cs_wallet lock (João Barbosa)
Pull request description:
This is another small change towards non recursive wallet lock.
ACKs for top commit:
hebasto:
ACK 5fabde6fad, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 00506f0159c56854a171e58a451db8dd9b9f735039697b1cf2ca7f54de61fb51cc1e5eff42265233e041b4b1bfd29c2247496dc4456578e1a23c323bdec2901b
d319c4dae9 qt, refactor: Replace WalletFrame::addWallet with WalletFrame::addView (Hennadii Stepanov)
92ddc02a16 qt, refactor: Declare getWalletModel with const and noexcept qualifiers (Hennadii Stepanov)
ca0e680bdc qt, refactor: Drop redundant checks of walletModel (Hennadii Stepanov)
404373bc6a qt, refactor: Pass WalletModel object to WalletView constructor (Hennadii Stepanov)
Pull request description:
An instance of the `WalletView` class without the `walletModel` data member being set is invalid. So, it is better to set it in the constructor.
Establishing one more `WalletView` class's invariant in constructor:
- allows to drop all of checks of the`walletModel` in member functions
- makes reasoning about the code that uses instances of the `WalletView` class easier
Possible follow ups could extend this approach to other classes, e.g., `OverviewPage`, `TransactionView`, `ReceiveCoinsDialog`, `SendCoinsDialog`, `AddressBookPage`.
ACKs for top commit:
ShaMan239:
Code review ACK d319c4dae9
promag:
Code review ACK d319c4dae9.
jarolrod:
ACK d319c4dae9
Tree-SHA512: b0c61f82811bb5aba2738067b53dc9ea4439230d547ce5c8fd85c480d8d70ea15f9942dbf13842383acbce467fba1ab4e132e37c56b654b46ba897301a41066e
