fe3d17df04 net: ignore block-relay-only peers when skipping DNS seed (Anthony Towns)
Pull request description:
Since #17428 bitcoind will attempt to reconnect to two block-relay-only anchors before doing any other outbound connections. When determining whether to use DNS seeds, it will currently see these two peers and decide "we're connected to the p2p network, so no need to lookup DNS" -- but block-relay-only peers don't do address relay, so if your address book is full of invalid addresses (apart from your anchors) this behaviour will prevent you from recovering from that situation.
This patch changes it so that it only skips use of DNS seeds when there are two full-outbound peers, not just block-relay-only peers.
ACKs for top commit:
Sjors:
utACK fe3d17d
amitiuttarwar:
ACK fe3d17df04, this impacts the very common case where we stop/start a node, persisting anchors & have a non-empty addrman (although, to be clear, wouldn't be particularly problematic in the common cases where the addrman has valid addresses)
mzumsande:
ACK fe3d17df04
jonatack:
ACK fe3d17df04
prayank23:
tACK fe3d17df04
Tree-SHA512: 9814b0d84321d7f45b5013eb40c420a0dd93bf9430f5ef12dce50d1912a18d5de2070d890a8c6fe737a3329b31059b823bc660b432d5ba21f02881dc1d951e94
0829516d1f [refactor] Remove unused ForEachNodeThen() template (John Newbery)
09cc66c00e scripted-diff: rename address relay fields (John Newbery)
76568a3351 [net processing] Move addr relay data and logic into net processing (John Newbery)
caba7ae8a5 [net processing] Make RelayAddress() a member function of PeerManagerImpl (John Newbery)
86acc96469 [net processing] Take NodeId instead of CNode* as originator for RelayAddress() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all addr data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
laanwj:
Code review ACK 0829516d1f
mzumsande:
ACK 0829516d1f, reviewed the code and ran tests.
sipa:
utACK 0829516d1f
hebasto:
re-ACK 0829516d1f
Tree-SHA512: efe0410fac288637f203eb37d1999910791e345872d37e1bd5cde50e25bb3cb1c369ab86b3a166ffd5e06ee72e4508aa2c46d658be6a54e20b4f220d2f57d0a6
fafd121026 refactor: Make CFeeRate constructor architecture-independent (MarcoFalke)
Pull request description:
Currently the constructor is architecture dependent. This is confusing for several reasons:
* It is impossible to create a transaction larger than the max value of `uint32_t`, so a 64-bit `size_t` is not needed
* Policy (and consensus) code should be arch-independent
* The current code will print spurious compile errors when compiled on 32-bit systems:
```
policy/feerate.cpp:23:22: warning: result of comparison of constant 9223372036854775807 with expression of type 'size_t' (aka 'unsigned int') is always true [-Wtautological-constant-out-of-range-compare]
assert(nBytes_ <= uint64_t(std::numeric_limits<int64_t>::max()));
```
Fix all issues by making it arch-independent. Also, fix `{}` style according to dev notes.
ACKs for top commit:
theStack:
re-ACK fafd121026
promag:
Code review ACK fafd121026.
Tree-SHA512: e16f75bad9ee8088b87e873906d9b5633449417a6996a226a2f37d33a2b7d4f2fd91df68998a77e52163de20b40c57fadabe7fe3502e599cbb98494178591833
aca0e5dcdb Remove `GetDataDir(bool fNetSpecific = true)` function (Kiminuo)
b3e67f20a0 scripted-diff: Replace `GetDataDir(true)` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
4c3a5dcbfc scripted-diff: Replace `GetDataDir()` calls with `gArgs.GetDataDirNet()` calls (Kiminuo)
13bd8bb053 Make `ArgsManager.GetDataDirPath` private and drop needless suffix (Kiminuo)
4d8189f620 scripted-diff: Change `ArgsManager.GetDataDirPath()` to `ArgsManager.GetDataDirBase()` in tests (Kiminuo)
0f53df47d5 Add `ArgsManager.GetDataDirBase()` and `ArgsManager.GetDataDirNet()` as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)` (Kiminuo)
716de29dd8 Make `m_cached_blocks_path` mutable. Make `ArgsManager::GetBlocksDirPath()` const. (Kiminuo)
Pull request description:
This PR is a follow up PR to #21244. The PR attempts to move us an inch towards the [goal](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615307465) by removing `GetDataDir(net_specific)` and replacing it by `gArgs.GetDataDir(net_specific)` calls.
The approach of this PR attempts to be similar to the one chosen in "De-globalize ChainstateManager" (#20158). The goal is to pass `ArgsManager` to functions (or ideally to have `ArgsManager` as a member of a class where needed; inspiration from here: #21789) instead of having it as a global variable (i.e. `gArgs`).
**Notes:**
* First commit makes `m_cached_blocks_path` `mutable` as was suggested [here](https://github.com/bitcoin/bitcoin/pull/21244#discussion_r615274095) but not fully applied in #21244. (`m_cached_datadir_path` and `m_cached_network_datadir_path` were marked as `mutable` in #21244) This commit can be in a separate PR too.
* Other commits deal with removing of `GetDataDir(net_specific)` function.
* This was originally part of #21244 but it was [left]((https://github.com/bitcoin/bitcoin/pull/21244#pullrequestreview-633779754)) for a follow up PR.
* I think that the proposed changes show nicely where there is reliance on `gArgs` which is IMO a good thing.
If you know about a better approach how to do this, please share it here.
ACKs for top commit:
hebasto:
ACK aca0e5dcdb
MarcoFalke:
re-ACK aca0e5dcdb👃
Tree-SHA512: deec4d88edb32d7f4c818c3a74ffbb64709685819b88242dcf5dbaa1fb611f3ce2b29d2576ddb9e0dc5e75288e43538968224008c0a80e7149fc81c309f7c9da
4bea301692 test: use P2PK-MiniWallet for feature_csv_activation.py (Sebastian Falbesoner)
dc7eb64e83 test: MiniWallet: add P2PK support (Sebastian Falbesoner)
Pull request description:
This PR adds support for creating and spending transactions with raw pubkey (P2PK) outputs to MiniWallet, [as suggested by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/21900#discussion_r629524841). Using that mode in the test `feature_csv_activation.py`, all txs submitted to the mempool follow the standard policy, i.e. `-acceptnonstdtxn=1` can be removed.
Possible follow-ups:
* Improve MiniWallet constructor Interface; an enum-like parameter instead of two booleans would probably be better
* Look at other tests that could benefit from P2PK (e.g. feature_cltv.py?)
* Check vsize also for P2PK txs (vsize varies due to signature, i.e. a range has to be asserted)
ACKs for top commit:
laanwj:
Code review ACK 4bea301692
Tree-SHA512: 9b428e6b7cfde59a8c7955d5096cea88af1384a5f49723f00052e9884d819d952d20a5ab39bb02f9d8b6073769c44462aa265d84a33e33da33c2d21670c488a6
When building with Clang, if `-fstack-clash-protection` is used with an
unsupported target, it may result in hundreds of
`-Wunused-command-line-argument` warnings at compile time. This is
currently the case when building for at least Darwin using Apple or LLVM
Clang.
Unsupported targets may also include *BSD, however that is changing; see
further discussion in https://reviews.llvm.org/D92245 and
https://reviews.freebsd.org/D27366.
Note that this option is already skipped for Windows.
fad0867d6a Cleanup -includeconf error message (MarcoFalke)
fa9f711c37 Fix crash when parsing command line with -noincludeconf=0 (MarcoFalke)
Pull request description:
The error message has several issues:
* It may crash instead of cleanly shutting down, when `-noincludeconf=0` is passed
* It doesn't quote the value
* It includes an erroneous trailing `\n`
* It is redundantly mentioning `"-includeconf cannot be used from commandline;"` several times, when once should be more than sufficient
Fix all issues by:
* Replacing `get_str()` with `write()` to fix the crash and quoting issue
* Remove the `\n` and only print the first value to fix the other issues
Before:
```
$ ./src/bitcoind -noincludeconf=0
terminate called after throwing an instance of 'std::runtime_error'
what(): JSON value is not a string as expected
Aborted (core dumped)
$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=a b
-includeconf cannot be used from commandline; -includeconf=c
```
After:
```
$ ./src/bitcoind -noincludeconf=0
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=true
$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf="a b"
```
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34493
Testcase: https://github.com/bitcoin/bitcoin/files/6515429/clusterfuzz-testcase-minimized-system-6328535926046720.log
```
FUZZ=system ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-system-6328535926046720.log
```
See https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md
ACKs for top commit:
sipa:
utACK fad0867d6a
Tree-SHA512: b44af93be6bf71b43669058c1449c4c6999f03b5b01b429851b149b12d77733408cb207e9a3edc6f0bffd6030c4c52165e8e23a1c2718ff5082a6ba254cc94a4
QProgressDialog estimates the time the operation will take (based on
time for steps), and only shows itself if that estimate is beyond
minimumDuration. The default minimumDuration value is 4 seconds, and it
could make users think that the GUI is frozen.
QProgressDialog shows itself if the estimated time an operation will
take is beyond the minimumDuration value.
Direct call show() breaks that behavior on macos.
- "psbt" field is only returned in psbtbumpfee and not bumpfee
- bumpfee raises if privkeys are disabled, so drop "Only returned when wallet private keys are enabled."
- add missing space in RPC example
bbbb51877a fuzz: Speed up transaction fuzz target (MarcoFalke)
Pull request description:
`hashBlock` and `include_addresses` are orthogonal, so no need to do an exhaustive "search".
Might fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34491
ACKs for top commit:
practicalswift:
cr ACK bbbb51877a: patch looks correct, and `TxToUniv` surprisingly wide in the `transaction_fuzz_target` flame graph! Putting it on a diet makes sense.
Tree-SHA512: 1e7c30c7fecf96364a9a1597c0a22139389fdeb67db59f3c2c6fc088196e3332877b2865991a957980d542f99a2f48cc066dd7cc16c695a5113190fe06205089
393992b049 fuzz: Terminate immediately if a fuzzing harness ever tries to create a TCP socket (belt and suspenders) (practicalswift)
Pull request description:
Terminate immediately if a fuzzing harness ever to create a TCP socket (belt and suspenders).
Obviously this _should_ never happen, but if it _does_ happen we want immediate termination instead of a TCP socket :)
ACKs for top commit:
MarcoFalke:
ACK 393992b049
Tree-SHA512: 5bbff1f7e9a58b3eae24f742b7daf3fc870424c985f29bed5931e47a708d9c0984bfd8762f43658cffa9c69d32f86d56deb48bc7e43821e3398052174b6a160e
7eea659fc9 qt, test: use qsignalspy instead of qeventloop (Jarol Rodriguez)
Pull request description:
This PR refactors our GUI `apptests` to use [QSignalSpy](https://doc.qt.io/qt-5/qsignalspy.html) instead of [QEventLoop](https://doc.qt.io/qt-5/qeventloop.html).
`QSignalSpy` is more appropriate for our GUI test's as it is purpose-built for testing emission of signals and sets up its own `QEventLoop` when the `wait` function is called.
ACKs for top commit:
hebasto:
ACK 7eea659fc9, tested on Linux Mint 20.1 (Qt 5.12.8).
promag:
Code review ACK 7eea659fc9.
Tree-SHA512: 3adddbcc5efd726302b606980c9923025c44bb8ee16cb8a183e633e423179c0822db66de9ccba20dc5124fff34af4151a379c9cd18130625c60789ce809ee6fd
Define the Package type as an alias for a vector of transactions for now.
Add PackageValidationResult, similar to TxValidationResult and
BlockValidationResult for package-wide errors that cannot be reported
within a single transaction result, such as having too many
transactions in the package. We can update the concept of
what a package is and have different logic for packages vs lists of
transactions in the future, e.g. for package relay.
Allow CheckSequenceLocks to use heights and coins from any CoinsView and
CBlockIndex provided. This means that CheckSequenceLocks() doesn't need
to hold the mempool lock or cs_main. The caller is responsible for
ensuring the CoinsView and CBlockIndex are consistent before passing
them in. The typical usage is still to create a CCoinsViewMemPool from
the mempool and grab the CBlockIndex from the chainstate tip.
2a45134b56 qt: Add shortcuts for console font resize buttons (Hennadii Stepanov)
a2e122f0fe qt: Add GUIUtil::AddButtonShortcut (Hennadii Stepanov)
4ee9ee7236 qt: Use native presentation of shortcut (Hennadii Stepanov)
Pull request description:
On `master` the only way to resize the console font is to manually move your mouse and click the resize buttons. This PR introduces convenient keyboard shortcuts to resize the console font.
The common resize shortcuts for applications are `Ctrl+=`/`Ctrl++` and `Ctrl+-`/`Ctrl+_`. This means that the resize QPushButtons need two shortcuts each, but you cannot assign multiple shortcuts to a QPushButton. See: https://doc.qt.io/qt-5/qabstractbutton.html#shortcut-prop
To get around this, we introduce a new function in `guiutil`, which connects a supplied `QKeySequence` shortcut to a `QAbstractButton`. This function can be reused in other situations where more than one shortcut is needed for a button.
| PR on macOS | PR on Linux |
| ---------------- | ------------ |
|  |  |
ACKs for top commit:
hebasto:
re-ACK 2a45134b56
Talkless:
tACK 2a45134b56, tested on Debian Sid with Qt 5.15.2, shortcuts still work.
Tree-SHA512: e894ccb7e5c695ba83998c21a474d6c587c9c849f12ced665c5e0034feb6b143e41b32ba135cab6cfab22cbf153d5a52b1083b2a278e6dfca3f5ad14c0f6c573
ce6bca88e8 doc: release note for getnodeaddresses by network (Jon Atack)
3f89c0e990 test: improve getnodeaddresses coverage, test by network (Jon Atack)
6c98c09991 rpc: enable filtering getnodeaddresses by network (Jon Atack)
80ba294854 p2p: allow CConnman::GetAddresses() by network, add doxygen (Jon Atack)
a49f3ddbba p2p: allow CAddrMan::GetAddr() by network, add doxygen (Jon Atack)
c38981e748 p2p: pull time call out of loop in CAddrMan::GetAddr_() (João Barbosa)
d35ddca91e p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Jon Atack)
Pull request description:
This patch allows passing a network argument to CAddrMan::GetAddr(), CConnman::GetAddresses(), and rpc getnodeaddresses to return only addresses of that network.
It also contains a performance optimisation by promag.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK ce6bca88e8
vasild:
ACK ce6bca88e8
Tree-SHA512: 40e700d97091248429c73cbc0639a1f03ab7288e636a7b9026ad253e9708253c6b2ec98e7d9fb2d56136c0f762313dd648915ac98d723ee330d713813a43f99d
6e2eb0d63b rpc/wallet: use OMITTED_NAMED_ARG instead of Default(VNULL) (Karl-Johan Alm)
4983f4cba4 rpc/createwallet: omitted named arguments (Karl-Johan Alm)
dc4db23b30 rpc: address:amount dictionaries are OBJ_USER_KEYS (Karl-Johan Alm)
c8cf0a3d51 rpc/getpeerinfo: bytesrecv_per_msg is a dynamic dictionary (Karl-Johan Alm)
eb4fb7e507 rpc/gettxoutsetinfo: hash_or_height is a named argument (Karl-Johan Alm)
Pull request description:
This is a follow-up to #21897, and I believe covers the remaining cases, at least that I could find.
Edited to remove unrelated information about a side project.
ACKs for top commit:
laanwj:
Documentation diff ACK 6e2eb0d63b
promag:
Code review ACK 6e2eb0d63b.
Tree-SHA512: d26f6e074e13d64bbca2a114a0adc7f905d47d238c4e9bc49f70ca0b775afbebf9879fc3794ab29dc316a6dbd00ba8cbeb01197e236ee4ab2e9854db25f23f04
Instead of hijacking the effective_feerate to use the correct value
during coin selection, have OutputGroup be aware of whether we are
subtracting the fee from the outputs and provide the correct value to
use for selection.
To do this, OutputGroup now takes CoinSelectionParams and has a new
function GetSelectionAmount().
