71d14528ca refactor: enumerate each CNode argument on a separate line (Kittywhiskers Van Gogh)
f8d1e5b3ec merge bitcoin#25174: Add thread safety related annotations for CNode and Peer (Kittywhiskers Van Gogh)
4847f6e96f refactor: move `m_initial_sync_finished` out of header (Kittywhiskers Van Gogh)
dba4cf056b merge bitcoin#25591: Version handshake to libtest_util (Kittywhiskers Van Gogh)
b9b13bd8ec merge bitcoin#24141: Rename message_command variables in src/net* and src/rpc/net.cpp (Kittywhiskers Van Gogh)
a6aa3735be merge bitcoin#20196: fix GetListenPort() to derive the proper port (Kittywhiskers Van Gogh)
c443cf4825 merge bitcoin#24078: Rename CNetMessage::m_command with CNetMessage::m_type (Kittywhiskers Van Gogh)
182e31d04c merge bitcoin#23801: Change time variable type from int64_t to std::chrono::seconds in net_processing.cpp (Kittywhiskers Van Gogh)
6e6c9442fa merge bitcoin#23758: Use type-safe mockable time for peer connection time (Kittywhiskers Van Gogh)
7beeae77b9 merge bitcoin#23614: add unit test for block-relay-only eviction (Kittywhiskers Van Gogh)
cf8f17e423 merge bitcoin#22735: Don't return an optional from TransportDeserializer::GetMessage() (Kittywhiskers Van Gogh)
224fb687c8 merge bitcoin#20541: Move special CAddress-without-nTime logic to net_processing (Kittywhiskers Van Gogh)
30ac41e068 merge bitcoin#22284: performance improvements to ProtectEvictionCandidatesByRatio() (Kittywhiskers Van Gogh)
ad4369fd83 merge bitcoin#21571: make sure non-IP peers get discouraged and disconnected (Kittywhiskers Van Gogh)
Pull request description:
## Breaking Changes
None expected.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 71d14528ca
UdjinM6:
utACK 71d14528ca
Tree-SHA512: b214d50fd87a046f22a9b3baf676483b4eee45ad267a4e501b221bbc77ef1e4532f0ad1fc8931be66761a9c50f2033ceeabbaae2bdb42f7c9edf7708bac8a9eb
64f34770c5 refactor: remove unused header spork.h from rpc/masternode (Konstantin Akimov)
43e9ab4966 refactor: use new type of composite commands for masternode NNN (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
See #6051
## What was done?
Commands starting from 'masternode ...' uses new a new way to make composite commands.
## How Has This Been Tested?
Run unit/functional tests.
Please notice, we support both styles `masternodelist` and `masternode list` which are still both supported.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 64f34770c5
Tree-SHA512: 5a658588dff7f29d3025d2a720c8efe8096f79e8d632800911573f2f9a42450c2f27da070a94c5739e2027153413cdcacde82a7b4614467e036c008719692ed9
The PeerManager implementation was moved into the source file in
bitcoin#20811 (dash#5352) but the PR that introduced `m_initial_sync_finished`
bitcoin#19858 (dash#5869) was merged later and didn't account for the
out-of-order backport. We need to correct for that manually.
9cbfe40d8af8567682284890c080b0c3cf434490 net: remove useless call to IsReachable() from CConnman::Bind() (Vasil Dimov)
Pull request description:
`CConnman::Bind()` is called without `BF_EXPLICIT` only when passed
either `0.0.0.0` or `::`. For those addresses `IsReachable()` is always
true (regardless of the `-onlynet=` setting!), meaning that the `if`
condition never evaluates to true.
`IsReachable()` is always true for the "any" IPv4 and IPv6 addresses
because `CNetAddr::GetNetwork()` returns `NET_UNROUTABLE` instead of
`NET_IPV4` or `NET_IPV6` and the network `NET_UNROUTABLE` is always
considered reachable.
It follows that `BF_EXPLICIT` is unnecessary, remove it too.
ACKs for top commit:
naumenkogs:
ACK 9cbfe40d8af8567682284890c080b0c3cf434490
aureleoules:
ACK 9cbfe40d8af8567682284890c080b0c3cf434490
mzumsande:
ACK 9cbfe40d8af8567682284890c080b0c3cf434490
Tree-SHA512: 4e53ee8a73ddd133fd4ff25635135b65e5c19d1fc56fe5c30337406560664616c0adff414dca47602948919f34c81073aae6bfc2871509f3912663d86750928e
1cbf3b9a53 merge bitcoin-core/gui#206: Display fRelayTxes and bip152_highbandwidth_{to, from} in peer details (Kittywhiskers Van Gogh)
239062192e merge bitcoin#20764: cli -netinfo peer connections dashboard updates (Kittywhiskers Van Gogh)
06a6f8444c merge bitcoin#25147: follow ups to #20799 (removing support for v1 compact blocks) (Kittywhiskers Van Gogh)
6274a571b7 merge bitcoin#20799: Only support version 2 compact blocks (Kittywhiskers Van Gogh)
f4ce573538 merge bitcoin#22340: Use legacy relaying to download blocks in blocks-only mode (Kittywhiskers Van Gogh)
73b8f84fdb merge bitcoin#22147: p2p: Protect last outbound HB compact block peer (Kittywhiskers Van Gogh)
2ce481849a merge bitcoin#20599: Tolerate sendheaders and sendcmpct messages before verack (Kittywhiskers Van Gogh)
799214b2c8 merge bitcoin#19776: expose high bandwidth mode state via getpeerinfo (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Version 2 of BIP152 high-bandwidth mode/compact blocks implements SegWit support.
As Dash does not implement SegWit, there has never been a need to implement v2 (and therefore, have all the code necessary to support both v1 and v2, that gets removed as part of making support v2 only).
* Despite that, the changes surrounding removing support for both versions (that in our case, do not apply as we never have supported v2) refactor the code in other ways and influence their behaviour. In the interest of upstream alignment, those changes have been backported.
* [bitcoin#19776](https://github.com/bitcoin/bitcoin/pull/19776) doesn't seem to work on its own without successive backports, specifically [bitcoin#20799](https://github.com/bitcoin/bitcoin/pull/20799), despite the latter being a later backport.
<details>
<summary>19776-only p2p_compactblocks.py run (9f2c868947cc254d021e1a9bd00eb7bc80061e81)</summary>
```
dash@825a14c32b73:/src/dash$ ./test/functional/p2p_compactblocks.py
2024-06-09T12:29:09.777000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_kb2nr5oe
2024-06-09T12:29:16.341000Z TestFramework (INFO): Testing SENDCMPCT p2p message...
2024-06-09T12:29:31.432000Z TestFramework (INFO): Testing compactblock construction...
2024-06-09T12:29:40.068000Z TestFramework (INFO): Testing compactblock requests...
2024-06-09T12:29:44.597000Z TestFramework (INFO): Testing getblocktxn handler...
2024-06-09T12:29:59.808000Z TestFramework (INFO): Testing compactblock requests/announcements not at chain tip...
2024-06-09T12:30:03.855000Z TestFramework (INFO): Testing handling of incorrect blocktxn responses...
2024-06-09T12:30:05.868000Z TestFramework (INFO): Testing reconstructing compact blocks from all peers...
2024-06-09T12:30:09.389000Z TestFramework (INFO): Testing end-to-end block relay...
2024-06-09T12:30:10.404000Z TestFramework (INFO): Testing handling of invalid compact blocks...
2024-06-09T12:30:12.418000Z TestFramework (INFO): Testing invalid index in cmpctblock message...
2024-06-09T12:30:14.384000Z TestFramework (INFO): Testing high-bandwidth mode states via getpeerinfo...
2024-06-09T12:30:16.893000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/src/dash/test/functional/test_framework/test_framework.py", line 158, in main
self.run_test()
File "./test/functional/p2p_compactblocks.py", line 849, in run_test
self.test_highbandwidth_mode_states_via_getpeerinfo()
File "./test/functional/p2p_compactblocks.py", line 791, in test_highbandwidth_mode_states_via_getpeerinfo
hb_test_node.send_and_ping(msg_block(block))
File "/src/dash/test/functional/test_framework/p2p.py", line 579, in send_and_ping
self.sync_with_ping(timeout=timeout)
File "/src/dash/test/functional/test_framework/p2p.py", line 596, in sync_with_ping
self.wait_until(test_function, timeout=timeout)
File "/src/dash/test/functional/test_framework/p2p.py", line 487, in wait_until
wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
File "/src/dash/test/functional/test_framework/util.py", line 249, in wait_until_helper
if predicate():
File "/src/dash/test/functional/test_framework/p2p.py", line 484, in test_function
assert self.is_connected
AssertionError
2024-06-09T12:30:17.396000Z TestFramework (INFO): Stopping nodes
2024-06-09T12:30:18.400000Z TestFramework (WARNING): Not cleaning up dir /tmp/dash_func_test_kb2nr5oe
2024-06-09T12:30:18.401000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/dash_func_test_kb2nr5oe/test_framework.log
2024-06-09T12:30:18.401000Z TestFramework (ERROR):
2024-06-09T12:30:18.401000Z TestFramework (ERROR): Hint: Call /src/dash/test/functional/combine_logs.py '/tmp/dash_func_test_kb2nr5oe' to consolidate all logs
2024-06-09T12:30:18.401000Z TestFramework (ERROR):
2024-06-09T12:30:18.401000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-06-09T12:30:18.402000Z TestFramework (ERROR): https://github.com/dashpay/dash/issues
2024-06-09T12:30:18.402000Z TestFramework (ERROR):
```
</details>
<details>
<summary>20799-incl p2p_compactblocks.py run (aa116c4f0b4753b615f9483aa03adec5ee4fd655)</summary>
```
dash@825a14c32b73:/src/dash$ ./test/functional/p2p_compactblocks.py
2024-06-09T12:34:27.169000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_7d65lmhz
2024-06-09T12:34:32.695000Z TestFramework (INFO): Testing SENDCMPCT p2p message...
2024-06-09T12:34:51.288000Z TestFramework (INFO): Testing compactblock construction...
2024-06-09T12:34:55.325000Z TestFramework (INFO): Testing compactblock requests...
2024-06-09T12:34:59.861000Z TestFramework (INFO): Testing getblocktxn handler...
2024-06-09T12:35:07.460000Z TestFramework (INFO): Testing compactblock requests/announcements not at chain tip...
2024-06-09T12:35:09.503000Z TestFramework (INFO): Testing handling of incorrect blocktxn responses...
2024-06-09T12:35:11.519000Z TestFramework (INFO): Testing reconstructing compact blocks from all peers...
2024-06-09T12:35:15.039000Z TestFramework (INFO): Testing end-to-end block relay...
2024-06-09T12:35:16.055000Z TestFramework (INFO): Testing handling of invalid compact blocks...
2024-06-09T12:35:17.062000Z TestFramework (INFO): Testing invalid index in cmpctblock message...
2024-06-09T12:35:19.139000Z TestFramework (INFO): Testing high-bandwidth mode states via getpeerinfo...
2024-06-09T12:35:22.159000Z TestFramework (INFO): Stopping nodes
2024-06-09T12:35:23.163000Z TestFramework (INFO): Cleaning up /tmp/dash_func_test_7d65lmhz on exit
2024-06-09T12:35:23.163000Z TestFramework (INFO): Tests successful
```
</details>
* The backport of [bitcoin-core/gui#206](https://github.com/bitcoin-core/gui/pull/206) is a continuation of 3e8ba24c87 from [dash#5964](https://github.com/dashpay/dash/pull/5964)
* The backport of [bitcoin#20764](https://github.com/bitcoin/bitcoin/pull/20764) is a continuation of bd934c71eb from [dash#6034](https://github.com/dashpay/dash/pull/6034)
## Breaking changes
* The `getpeerinfo` RPC returns two new boolean fields, `bip152_hb_to` and `bip152_hb_from`, that respectively indicate whether we selected a peer to be in compact blocks high-bandwidth mode or whether a peer selected us as a compact blocks high-bandwidth peer.
High-bandwidth peers send new block announcements via a `cmpctblock` message rather than the usual inv/headers announcements. See BIP 152 for more details.
* Blocks-only mode will use legacy relaying instead of BIP152 high-bandwidth mode
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 1cbf3b9a53
PastaPastaPasta:
utACK 1cbf3b9a53
knst:
utACK 1cbf3b9a53
Tree-SHA512: 5947b622d8d57a1dc9445cd6e07d4ad690379416d0fcf04ed574269975d1beb704691a79ff081341f3c800cf11869d401f1ed90baa5449f371f9ce658f2d2e95
d54ec27bac388d7b84cf7b6cb4506bb0c25f2f88 qt: Add helper to load font (João Barbosa)
Pull request description:
Originally submitted as https://github.com/bitcoin-core/gui-qml/pull/49.
ACKs for top commit:
hebasto:
re-ACK d54ec27bac388d7b84cf7b6cb4506bb0c25f2f88
stratospher:
Tested ACK d54ec27. Refactoring the code and defining `loadFont()` in `src/qt/guiutil.cpp` reduces redundant imports of the `QFontDatabase` and is a better design.
shaavan:
ACK d54ec27bac388d7b84cf7b6cb4506bb0c25f2f88
Tree-SHA512: b156bb6ffb08dd57476f383a29bbb0a1108b62794d430debb77252f7d09df1409a7532b09d17d8836d1c2ab7c126a6618231164b9d0def1b8f361a81ef22d107
4832737c7dcc87afea5e1e88945ec311417aa876 qt: connection type translator comments (Jarol Rodriguez)
Pull request description:
This PR introduces Qt translator comments for `Connection Type` strings in `guiutil.cpp` as well as `rpcconsole.cpp`.
This is an alternate implementation of the idea presented in the last three commits of #289. It is especially inspired by commit 842f4e834dfe5fd2786a5092f78ea28da1b36e4f.
Per [Qt Dev Notes](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Developer-Notes-for-Qt-Code), it is better to not break up strings when not necessary. This way we preserve the full context for translators.
ACKs for top commit:
jonatack:
Code review re-ACK 4832737c7dcc87afea5e1e88945ec311417aa876 per `git diff 371e2b9 4832737`, changes are translator comment edits since my review yesterday (thank you for updating)
hebasto:
ACK 4832737c7dcc87afea5e1e88945ec311417aa876
Tree-SHA512: 67e1741e10a2e30cde6d50d3293eec89f0b7641b34463865dc6909d2926cdcf33a7d8c1dc8055d2f85906ad2002cdaa594d37b184d16e2f06614b6c5ad00c982
a93fec6f2d merge bitcoin#23380: Fix AddrMan::Add() return semantics and logging (Kittywhiskers Van Gogh)
d1a4b14b48 merge bitcoin#23354: Introduce new V4 format addrman (Kittywhiskers Van Gogh)
7a97aabfe0 test: restore pre-bitcoin#23306 tests to validate port distinguishment (Kittywhiskers Van Gogh)
1a050d6cb4 merge bitcoin#23306: Make AddrMan support multiple ports per IP (Kittywhiskers Van Gogh)
d56702aa0c merge bitcoin#23140: Make CAddrman::Select_ select buckets, not positions, first (Kittywhiskers Van Gogh)
19b0145379 merge bitcoin#22839: improve addrman logging (Kittywhiskers Van Gogh)
3910c68028 merge bitcoin#23053: Use public methods in addrman fuzz tests (Kittywhiskers Van Gogh)
b6ec8ab6df merge bitcoin#22950: Pimpl AddrMan to abstract implementation details (Kittywhiskers Van Gogh)
236cf36d88 merge bitcoin#22734: Avoid crash on corrupt data, Force Check after deserialize (Kittywhiskers Van Gogh)
2420ac9e53 merge bitcoin#23041: Add addrman deserialization error tests (Kittywhiskers Van Gogh)
8aefa9b93a merge bitcoin#22762: Raise InitError when peers.dat is invalid or corrupted (Kittywhiskers Van Gogh)
c9a645f814 merge bitcoin#22879: Fix format string in deserialize error (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependent on https://github.com/dashpay/dash/pull/6040.
* Dash already introduced support for storage of address-port pairs (referred to as "port discrimination") and allowed the usage of non-default ports in P2P with [dash#2168](https://github.com/dashpay/dash/pull/2168).
* Albeit this was only permitted for networks with `fAllowMultiplePorts` enabled (which at the time was `devnet` and as it stands currently, on every network except `mainnet`).
* Keeping in line with the above policy (discussion on lifting such restrictions on `mainnet` is outside the scope of this PR), when backporting [bitcoin#23306](https://github.com/bitcoin/bitcoin/pull/23306), changes have been made to retain the effects of `discriminate_ports`.
* This involves appending placing a `!m_discriminate_ports` condition to behaviour that otherwise would be _removed_ entirely.
* Additionally, in line with upstream backports, changes have been made that render port distinguishment _enabled_ as the new default in `addrman_tests` (the old default was to keep it _disabled_, to mirror `mainnet` and pre-change upstream behaviour).
* To ensure distinguishment _disabled_ works as expected, affected pre-backport tests were reintroduced with the `_nondiscriminate` suffix.
---
I would propose at some point to rename the flag to `ignore_port`/`suppress_port` (if not remove it altogether should the `mainnet` restriction be lifted) as discriminate (or distinguish) isn't immediately clear with if address entries will be discriminated/distinguished _using_ ports (i.e. considered) or ports will be discriminated _against_ (i.e. ignored).
## Breaking Changes
It's unclear if these backports result in serialization issues for older versions, as Dash Core technically supported address-port pairs since 0.12 and suppressed it on `mainnet` by setting zero-ing out the port ([source](19512988c6/src/addrman.cpp (L135-L138))), meaning even with port discrimination _disabled_, the serialization format should remain the same.
Regardless, following upstream backports, a new version has been introduced (v4) that marks the AddrMan format incompatible with older versions of Dash Core.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK a93fec6f2d
PastaPastaPasta:
utACK a93fec6f2d
Tree-SHA512: 49b35af3e4eb660249ce9a65d5a539205d852e9c728da22dc88779f6b3b15c13cf91522896a313bfe2a91889fedf3b6b2cebdea12cc2bbe865ec3b85b6a5dfa8
6777ab73a2 Merge bitcoin-core/gui#682: Don't directly delete abandoned txes from GUI (Hennadii Stepanov)
6e1a8c1fdc Merge bitcoin/bitcoin#26628: RPC: Reject RPC requests with same named parameter specified multiple times (MarcoFalke)
7c28b01c78 Merge bitcoin/bitcoin#26666: refactor: Deleted unreachable code in httpserver.cpp (MarcoFalke)
478fe51ead Merge bitcoin/bitcoin#26100: doc: clarify that NetPermissionFlags::Implicit is only about whitelists (MarcoFalke)
69b19cbfc0 Merge bitcoin/bitcoin#26546: test: remove unused class `NodePongAdd1` (fanquake)
245df942c4 Merge bitcoin/bitcoin#26380: Revert "test: check importing wallets when blocks are pruned throw an error" (MacroFake)
3db3bd0d31 Merge bitcoin/bitcoin#24269: test: add functional test for `-discover` (Andrew Chow)
c72ef299da Merge bitcoin/bitcoin#25896: wallet: Log when Wallet::SetMinVersion sets a different minversion (Andrew Chow)
12b438cb46 Merge bitcoin/bitcoin#25925: doc: add `{import,list}descriptors` to list of descriptor RPCs (Andrew Chow)
73d64c48a9 Merge bitcoin/bitcoin#25738: depends: use a patch instead of sed in libxcb (fanquake)
1fae0c2bbb Merge bitcoin/bitcoin#25333: test: Fix out-of-range port collisions (MacroFake)
b75089667c Merge bitcoin/bitcoin#25231: ci: Install documented packages for "Win64" CI task (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Batch of trivial backports
## What was done?
Trivial backports
## How Has This Been Tested?
Built; ran tests locally
## Breaking Changes
None
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
UdjinM6:
utACK 6777ab73a2
Tree-SHA512: 60d68c8d0fb9875d0b2421cad97b46a9d4deb79e03ca011d66cc0595ed68bb7ad207f2fd95973a3b7a0b9bca2c2f0deaf0e1116f0312a4d9d315f009228fe485
Dash has supported the storage of address-port pairs and multi-port
support before upstream for ease of development (on select networks,
most notably, not mainnet). Bitcoin has introduced support for the above
mentioned features and has modified tests to validate new functionality.
As Dash already has this functionality and *selectively* allows it, we
need to test both cases, the new upstream case with distinguishment
enabled and Dash's case of distinguishment disabled (the former upstream
case).
9413ecdc66 fix: disable linter 'check-rpc-mapping.py' for composite commands (Konstantin Akimov)
f4bc19fb99 refactor: proper support for composite commands such as 'bls generate' (Konstantin Akimov)
Pull request description:
## Issue being fixed or feature implemented
We have composite commands such as 'bls generate' that do not exist in bitcoin's implementation of rpc.
It doesn't let to backport yet bitcoin#18531 which enforced extra checks for arguments name (name of rpc and list arguments in rpc help and actual implementation must match).
## What was done?
This PR improves support of composite commands in Dash Core. New style of composite commands are applied for `bls` composite commands: `bls generate` and `bls fromsecret` as proof of concept.
Once this PR is merged, I will provide similar fixes for other "compose" rpc commands: `protx`, `masternode` (and everything else if any).
Beside better validation of arguments and command names, it improves suggest menu in Qt app (see a screenshot) for composite commands.
## How Has This Been Tested?
Run unit and functional tests. Also extra tests to conduct in qt app:
- check suggest for 'bls ....' and 'help bls ...'
- check output of next commands:
```
help bls
help bls generate
help bls from secret
bls
bls generate
bls generate 1
```
- also let's see that old-fashion composite commands are not broken:
```
help protx
help protx diff
protx diff 00000021c7604b9992254f9f1ed91de5d65eaade33c773abea63a7b0e93293ee 000000e44f9894838ebf768b464177cfce8859dcf92b0509f5c2fba774315996
```
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 9413ecdc66
Tree-SHA512: c8accd2f1ea1d2168d7a2bf25311c92b7839782fc300c587c7537880df5d3fbbf7e79cedb2c6fa88ab067b696994beaf7d92185ae3e64fe1a5f70ad9e8620bec
e75d2276324d54a01971afdf531df91748275bd5 Minor fix: Don't directly delete abandoned txes (John Moffett)
Pull request description:
This fully closesbitcoin/bitcoin#12179. Currently, when a user abandons a transaction by clicking "Abandon Transaction" in the context menu, a call is made to remove it from the GUI view:
`model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);`
(The `false` parameter is for `bool showTransaction`)
This behavior is probably unwanted, as the transaction is not actually removed from the wallet and would show up again if the node is restarted.
However, the previous line, `model->wallet().abandonTransaction(hash);`, changes the underlying model and calls `NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);`, which queues a signal that eventually calls back to `updateTransaction`, this time with `showTransaction` set to `true`. This runs on a separate thread, so it gets called *after* the 'subsequent' `updateTransaction`. The transaction gets removed from the GUI and immediately added back.
In a nutshell, `updateTransaction` gets called twice. The first (direct) call deletes the transaction from the GUI. The second (sent via a queued signal) brings it back to the GUI. The first direct call is redundant and unwanted. Worse, if the `abandonTransaction` call fails for any reason, the transaction still gets removed from the GUI. (This is what caused bitcoin#12179. It can still be triggered if, eg., a user clicks "Abandon Transaction" the moment after a new block is found.)
There are no conditions (to my knowledge) where an abandoned transaction should be directly removed from the GUI. If the underlying model changes, the deletion should be reflected anyway by the queued signal to `updateTransaction`.
The behavior is borne out by the QT logs. To reproduce, send a transaction with RBF enabled, then bump the fee, then 'abandon transaction' on the first transaction. The logs will show something like this:
```
2022-11-28T14:48:00Z [qt] GUI: "NotifyTransactionChanged: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 status= 1"
2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
2022-11-28T14:48:00Z [qt] GUI: " inModel=1 Index=381-382 showTransaction=0 derivedStatus=2"
2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
2022-11-28T14:48:00Z [qt] GUI: " inModel=0 Index=381-381 showTransaction=1 derivedStatus=0"
```
Notice the duplicate `updateWallet` calls with different `showTransaction` values.
ACKs for top commit:
hebasto:
ACK e75d2276324d54a01971afdf531df91748275bd5
jarolrod:
tACK e75d2276324d54a01971afdf531df91748275bd5
Tree-SHA512: 00f150f747c2ee1605af861a21d5c3b9773a4a9985e8dab62e48bd32885b1bfa4e8cbf805ad61af77aec9d3ccefaed3f4311a29086aa8c22d55d5326ba68ece6
8c3ff7d52ae3314959e1e66da8718a3f0d30abaa test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca56382512df3084fce7353bf1e8b66cae61bc bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20b8cf27aa72ec2907342787e6fc9f94c50 rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18e671e347e422d696d1cbdd9f82b2ce468 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)
Pull request description:
Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.
Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.
After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.
ACKs for top commit:
MarcoFalke:
review ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa 🗂
kristapsk:
ACK 8c3ff7d52ae3314959e1e66da8718a3f0d30abaa
stickies-v:
ACK 8c3ff7d52
Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
ddca614017 merge bitcoin-core/gui#325: Align numbers in the "Peer Id" column to the right (Kittywhiskers Van Gogh)
09a8fc1df9 merge bitcoin-core/gui#280: Remove user input from URI error message (Kittywhiskers Van Gogh)
a3ac6941a7 merge bitcoin-core/gui#194: Save/restore RPCConsole geometry only for window (Kittywhiskers Van Gogh)
504f8344ba merge bitcoin-core/gui#296: Do not use QObject::tr plural syntax for numbers with a unit symbol (Kittywhiskers Van Gogh)
c33afea7fd merge bitcoin-core/gui#166: Use enum type as switch argument in *TableModel (Kittywhiskers Van Gogh)
5a67b52725 merge bitcoin-core/gui#214: Disable requests context menu actions when appropriate (Kittywhiskers Van Gogh)
dc9b25e04c merge bitcoin-core/gui#219: Prevent the main window popup menu (Kittywhiskers Van Gogh)
7774bdb423 partial bitcoin-core/gui#79: Embed monospaced font (Kittywhiskers Van Gogh)
722bd8d7f3 qt: introduce GUIUtil::fixedPitchFont (Kittywhiskers Van Gogh)
afd0648959 merge bitcoin-core/gui#179: Add Type column to peers window, update peer details name/tooltip (Kittywhiskers Van Gogh)
57597df2f2 merge bitcoin-core/gui#203: Display plain "Inbound" in peer details (Kittywhiskers Van Gogh)
582c6ec870 merge bitcoin-core/gui#180: connection type follow-ups (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
| `develop` (d7413ffb) | This PR |
| ------------------------------------------------------------ | ------------------------------------------------------------ |
|  |  |
## Breaking Changes
None expected, changes are limited to refactoring, cleanup and refinement.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK ddca614017
Tree-SHA512: 8b07c6261c5e111f6db06be528aab7d3862bb31cf23e7df84a1e211ae1e1b5d52a51a520b24ed1b058ac19f845204fe458f16ba5920db9c1b1d1939a1fff6711
32f8fda7d6 merge bitcoin#24991: allow startup with -onlynet=onion -listenonion=1 (Kittywhiskers Van Gogh)
e67ed92d3d merge bitcoin#25173: add coverage for unknown network in -onlynet (Kittywhiskers Van Gogh)
77efd36112 merge bitcoin#24687: Check an invalid -i2psam will raise an init error (Kittywhiskers Van Gogh)
fb1416f7cb merge bitcoin#24205: improve network reachability test coverage and safety (Kittywhiskers Van Gogh)
7cb7479829 merge bitcoin#24663: add links to doc/cjdns.md (Kittywhiskers Van Gogh)
c736ebf566 merge bitcoin#24555: create initial doc/cjdns.md for CJDNS how-to documentation (Kittywhiskers Van Gogh)
554bd24186 partial bitcoin#24468: improve -onlynet help and related tor/i2p documentation (Kittywhiskers Van Gogh)
5436b6a82d merge bitcoin#24165: extend inbound eviction protection by network to CJDNS peers (Kittywhiskers Van Gogh)
d52724d039 merge bitcoin#22834: respect -onlynet= when making outbound connections (Kittywhiskers Van Gogh)
f9d1a9a00d merge bitcoin#23077: Full CJDNS support (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Depends on https://github.com/dashpay/dash/pull/6034
* Depends on https://github.com/dashpay/dash/pull/6035
* If `-proxy=` is given together with `-noonion` then the provided proxy will not be set as a proxy for reaching the Tor network. So it will not be possible to open manual connections to the Tor network for example with the `addnode` RPC. To mimic the old behavior use `-proxy=` together with `-onlynet=` listing all relevant networks except `onion`.
* [bitcoin#24165](https://github.com/bitcoin/bitcoin/pull/24165) has been backported _before_ [bitcoin#23758](https://github.com/bitcoin/bitcoin/pull/23758) and to account for this, minor changes were made in `src/test/net_peer_eviction_tests.cpp` (using `nTimeConnected` instead of `m_connected`). When backporting [bitcoin#23758](https://github.com/bitcoin/bitcoin/pull/23758), these changes will have to be reversed as they won't be covered by the cherry-pick diff.
* CJDNS support has been labelled as being introduced in Dash Core 21.0, in line with the milestone designation of the PR. Should `develop` be used for a new minor/patch release, `doc/cjdns.md` will have to be modified to reflect the correct version number.
## Breaking changes
No expected protocol or consensus changes.
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK 32f8fda7d6
Tree-SHA512: e23b22ca5edbe4c4abeab0bc07780303e68e7c4cc46b7697300b0837c5acd3a98649b6b03bd07a23c827bd85f64210173027b0b0eea31872c031fa4ed04eeb0c
8f5c560e11a9f3c7c5877f444caa605bdc296e81 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos)
7fd3b9491b0e553aa7d60a68bda82968a79da8f5 refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos)
Pull request description:
Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes.
Continuation of [#26570 ](https://github.com/bitcoin/bitcoin/pull/26570)
ACKs for top commit:
stickies-v:
re-ACK [8f5c560](8f5c560e11)
Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
f362920c2ca85853b24c1cbc1d596dd1ff2fd556 doc: clarify that NetPermissionFlags::Implicit is only about whitelists (Vasil Dimov)
Pull request description:
`NetPermissionFlags::Implicit` applies just to connections from `-whitebind` or `-whitelist`, clarify that in its comment.
ACKs for top commit:
Zero-1729:
crACK f362920c2ca85853b24c1cbc1d596dd1ff2fd556
aureleoules:
ACK f362920c2ca85853b24c1cbc1d596dd1ff2fd556
hernanmarino:
re ACK f362920c2ca85853b24c1cbc1d596dd1ff2fd556
Tree-SHA512: 03f6f8be221c6819bdd0b5b56b69b4e3a6dd25e5ca5a247eeb1261113144b9b74cf064a0b7815317782a0a18365dd3dab97963bd238e9b231dbe7e1cf0395683
835bd27e9a0dd627f266e3dc0a7422d8d0612eff Wallet::SetMinVersion - Log the new minversion (Ali Sherief)
Pull request description:
This change prints a single additional line in the debug.log when bitcoin-cli loads a wallet using `loadwallet` (*not* `createwallet`).
When Bitcoin Core creates a wallet, it's `minversion` is set to `FEATURE_BASE`, which is 10500. However, once the wallet is unloaded using `unloadwallet` or through program termination, and subsequently loaded again, `loadwallet` updates the `minversion` in the wallet.dat file to `FEATURE_LATEST`, currently 169900.
The current logging format prints the very old wallet version during `createwallet`, and then the actual version in calls to `loadwallet`. This has confused at least one person ([reference](https://bitcointalk.org/index.php?topic=5410650.0) - I was the one who asked there if there were plans to change that behavior, and was subsequently redirected here by achow), so it will be very helpful to users to explicitly specify in the logs what the walletdb is doing.
ACKs for top commit:
achow101:
ACK 835bd27e9a0dd627f266e3dc0a7422d8d0612eff
Tree-SHA512: 967c8c617e06a84915ddb147378ec3c8b0343e45f43145ec78df9cbc0201867f49c8e11cd068c403eb5ec06e07d38c3c0d3864dad8edc5efbb134a3fb30be41f
76279c1a37 Merge bitcoin/bitcoin#25149: refactor: Add thread safety annotation to `BanMan::SweepBanned()` (MacroFake)
6269c6f1db Merge bitcoin/bitcoin#25053: Guard `#include <config/bitcoin-config.h>` (fanquake)
d50f0b016f Merge bitcoin/bitcoin#24977: rpc: Explain active and internal in listdescriptors (fanquake)
3c44399d55 Merge bitcoin/bitcoin#24856: lint: Converting lint-assertions.sh to lint-assertions.py (laanwj)
e9f5b4b735 Merge bitcoin/bitcoin#24213: refactor: use Span in random.* (laanwj)
7e0474ac1c Merge bitcoin/bitcoin#24632: add `(none)` in -getinfo `Warnings:` if no warning returned (laanwj)
57e9b56bad Merge bitcoin/bitcoin#24145: mempool: Clear vTxHashes when mapTx is cleared (laanwj)
fe56d9b994 Merge bitcoin/bitcoin#24698: test: -peerblockfilters without -blockfilterindex raises an error (MarcoFalke)
3cabce645e Merge bitcoin/bitcoin#24472: fuzz: execute each file in dir without fuzz engine (MarcoFalke)
f5116a7d31 Merge bitcoin-core/gui#549: refactor: use std::chrono for formatDurationStr() helper (Hennadii Stepanov)
3fa8158510 Merge bitcoin/bitcoin#22317: doc: Highlight DNS requests part in tor.md (Andrew Chow)
72b62edd5a Merge bitcoin/bitcoin#23834: wallettool: Check that the dumpfile checksum is the correct size (laanwj)
ee9b3cdb0e Merge bitcoin/bitcoin#23979: test: wait for rather than assert presence of file in startupnotify test (MarcoFalke)
2ec5940399 Merge bitcoin/bitcoin#23532: test: add functional test for -startupnotify (MarcoFalke)
5a31be9608 Merge bitcoin/bitcoin#23812: test: fix intermittent failures in p2p_timeouts.py (MarcoFalke)
10828f5b3d Merge bitcoin/bitcoin#23733: fuzz: Move ISO8601 to one place (MarcoFalke)
7f39b5af41 Merge bitcoin/bitcoin#23635: test: Bump shellcheck version to 0.8.0 (fanquake)
Pull request description:
## Issue being fixed or feature implemented
Trivial batch of backports
## What was done?
trivial backports
## How Has This Been Tested?
Unit tests ran; waiting on CI
## Breaking Changes
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
Top commit has no ACKs.
Tree-SHA512: a3f97003e6441468951b827b2c3ea607740e5b9d36b96c2f93e45e7fb4088ecf4d0a2b7038de050ca0e7d61379c364969f4a8caff98ec1cc69016f4114e64c6a
f619f8ff80 merge bitcoin#20234: don't bind on 0.0.0.0 if binds are restricted to Tor (Kittywhiskers Van Gogh)
1698336fc8 merge bitcoin#22974: Improve performance of Good (Kittywhiskers Van Gogh)
29f4482e01 merge bitcoin#22911: Minor cleanups to asmap (Kittywhiskers Van Gogh)
5706edad5f merge bitcoin#22915: Remove confusing CAddrDB (Kittywhiskers Van Gogh)
3f69606827 merge bitcoin#22848: Expose BanMapToJson / BanMapFromJson (Kittywhiskers Van Gogh)
9065eed969 merge bitcoin#22791: Fix asmap/addrman initialization order bug (Kittywhiskers Van Gogh)
99b7812271 merge bitcoin#22849: Remove unused SERIALIZE_METHODS for CBanEntry (Kittywhiskers Van Gogh)
a30379c557 merge bitcoin#22740: Move serialization code to cpp (Kittywhiskers Van Gogh)
d4e79aa377 merge bitcoin#22697: Remove CAddrMan::Clear() function (Kittywhiskers Van Gogh)
77d8f6c918 merge bitcoin#22725: Move addrman ser/deser tests to addrman_tests.cpp (Kittywhiskers Van Gogh)
4ba3f49afc merge bitcoin#22627: De-duplicate Add() function (Kittywhiskers Van Gogh)
49af8185e0 merge bitcoin#20233: Make consistency checks a runtime option (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/6043
* [bitcoin#22915](https://github.com/bitcoin/bitcoin/pull/22915) is backported before [bitcoin#21850](https://github.com/bitcoin/bitcoin/pull/21850), which is the reason {`Dump`, `Read`}`PeerAddresses` takes in an `const ArgsManager&` but doesn't do anything with it.
* This will need to be accounted for when backporting [bitcoin#21850](https://github.com/bitcoin/bitcoin/pull/21850)
## Breaking Changes
None expected. No changes to serialization format.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that apply._
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
PastaPastaPasta:
utACK f619f8ff80
Tree-SHA512: 19599f0322f049b123790a45530ecb2ec74b1463b47e0102921b489bad1d5cb671a94900b3b35dbd7c1d80bc0680f1ed01e6fac675ac0be81621878388be0bcd
88044a14d9b2c6c70a3330ee1545a9eb39d14d89 Guard `#include <config/bitcoin-config.h>` (Hennadii Stepanov)
Pull request description:
A fix for builds when the `HAVE_CONFIG_H` macro is not defined.
ACKs for top commit:
Empact:
Code Review ACK 88044a14d9
Tree-SHA512: f2bf1693c7671d7113dccaf66ae34a84719d86cb3271fa18b36611deab93a48d787b3ccfbd735d3b763017d709971cb1151d8d7f30390720009e6e2a6275b5b0
4637bbe448ae7370528f40092ce230c32602a6d6 rpc: Explain active and internal in listdescriptors (Andrew Chow)
Pull request description:
The current help text for active and internal in listdescriptors is not particularly helpful. They require the reader to already know what those terms mean. This help text is updated to actually explain the definitions of those words in context of a descriptor wallet.
ACKs for top commit:
S3RK:
ACK 4637bbe448ae7370528f40092ce230c32602a6d6
jarolrod:
ACK 4637bbe448ae7370528f40092ce230c32602a6d6
w0xlt:
ACK 4637bbe448
Tree-SHA512: 0af2c04f3b9920799cf616ad618bde9248eb9f74cc28f443b5b0f6646deba76e9b1415aca0865ad3bcc24aa6af0e9d07ad7b7cd80f0fe80838cf847f1b944426
3ae7791bcaa88f5c68592673b8926ee807242ce7 refactor: use Span in random.* (pasta)
Pull request description:
~This PR does two things~
1. use a Span<unsigned char> for GetRandBytes and GetStrongRandBytes
~2. make GetRand a template for which any integral type can be used, where the default behavior is to return a random integral up to the max of the integral unless a max is provided.
This simplifies a lot of code from `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()`~
MarcoFalke this was inspired by your comment here: https://github.com/bitcoin/bitcoin/pull/24185#issuecomment-1025514263 about using Span, so hopefully I'll be able to get this PR done and merged 😂
~Also, if requested I could revert the `GetRand(std::numeric_limits<uint64_t>::max()` -> `GetRand<uint64_t>()` related changes if it ends up causing too many conflicts~
ACKs for top commit:
laanwj:
Thank you! Code review re-ACK 3ae7791bcaa88f5c68592673b8926ee807242ce7
Tree-SHA512: 12375a83b68b288916ba0de81cfcab4aac14389a66a36811ae850427435eb67dd55e47df9ac3ec47db4e214f4330139e548bec815fff8a3f571484ea558dca79
0cea7b10f1180e9993c14473e1a3b6525ef6ba01 print `(none)` if no warnings in -getinfo (/dev/fd0)
Pull request description:
Adds `(none)` in warnings when no warnings returned by -getinfo
Reviewers can test this by making the following change in `/src/warnings.cpp`:
```diff
bilingual_str GetWarnings(bool verbose)
{
bilingual_str warnings_concise;
std::vector<bilingual_str> warnings_verbose;
LOCK(g_warnings_mutex);
// Pre-release build warning
if (!CLIENT_VERSION_IS_RELEASE) {
- warnings_concise = _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications");;
+ warnings_concise = _("");;
```
Before this pull request:
```
$ bitcoin-cli -getinfo
Chain: regtest
Blocks: 0
Headers: 0
Verification progress: 100.0000%
Difficulty: 4.656542373906925e-10
Network: in 0, out 0, total 0
Version: 239900
Time offset (s): 0
Proxies: n/a
Min tx relay fee rate (BTC/kvB): 0.00001000
Warnings:
```
After this pull request:
```diff
$ bitcoin-cli -getinfo
Chain: regtest
Blocks: 0
Headers: 0
Verification progress: 100.0000%
Difficulty: 4.656542373906925e-10
Network: in 0, out 0, total 0
Version: 239900
Time offset (s): 0
Proxies: n/a
Min tx relay fee rate (BTC/kvB): 0.00001000
Warnings: (none)
```
ACKs for top commit:
jonatack:
ACK 0cea7b10f1180e9993c14473e1a3b6525ef6ba01
laanwj:
Tested ACK 0cea7b10f1180e9993c14473e1a3b6525ef6ba01
Tree-SHA512: a12499d11ff84bc954db354f968eb1f5ee4999d8b80581fe0bdf604732b2e2f608cb5c35c4ca8cb5a430f3991954a6207f0758302618662e6b9505044cf2dc95
9d65ad365c539557e969a35d22723d92e0b422fe Clear vTxHashes when mapTx is cleared (Peter Bushnell)
Pull request description:
vTxHashes is a vector of all entries in mapTx, if you clear one you should clear the other, lest someone try to use the txiter in vTxHashes which would result in a segfault.
ACKs for top commit:
laanwj:
Code review ACK 9d65ad365c539557e969a35d22723d92e0b422fe
Tree-SHA512: 6832755e43ab7f528b46817aeadcb6ffdc965b97f59ab96bb053dedbb7e68155ba3db52286355dca33b509237f80eda249760b26db493762bc50d8e2cef16d8f
f59bee3fb242c9e02781a35272cf9644f37e7fc1 fuzz: execute each file in dir without fuzz engine (Anthony Towns)
Pull request description:
Phony fuzzing (phuzzing)! Run the fuzz testing code against known inputs to detect errors. Advantage is you can easily test using the existing qa-assets datasets without having to compile with fuzzing enabled; disadvantage is that it doesn't do any actual fuzzing.
Example usage:
```
$ for a in ${QA_ASSETS}/fuzz_seed_corpus/*; do echo ${a##*/}; done | xargs -P8 -I {} /bin/sh -c "FUZZ={} test/fuzz/fuzz ${QA_ASSETS}/fuzz_seed_corpus/{}"
No fuzzer for address_deserialize.
No fuzzer for addrdb.
No fuzzer for banentry_deserialize.
addition_overflow: succeeded against 848 files in 0s.
asmap: succeeded against 981 files in 0s.
checkqueue: succeeded against 211 files in 0s.
...
```
(`-P8` says run 8 of the tasks in parallel)
If there are failures, the first one will be reported and the program will abort with output like:
```
fuzz: test/fuzz/versionbits.cpp:336: void (anonymous namespace)::versionbits_fuzz_target(FuzzBufferType): Assertion `exp_state != ThresholdState::FAILED' failed.
Error processing seed "corpus/versionbits/35345ae8e722234095810b1117a29b63af7621af"
```
Rebase of #22763, which was a rebase of #21496, but also reports the name of the fuzzer and the time taken.
Fixes#21461
Top commit has no ACKs.
Tree-SHA512: d8d046d4a309652eb13de42116276bf992480bc887ad3535a8ff18b354cb24826bc562b06af63802ec945c637f046563b6a5601d6321b46a5543127daafea09b
6f2593dc23565abaa3d176595cba6e07883f512e gui, refactor: use std::chrono for formatDurationStr() helper (Jon Atack)
Pull request description:
Updates `formatDurationStr()` to use the `chrono` standard lib. No change in behavior.
ACKs for top commit:
RandyMcMillan:
tACK 6f2593dc23565abaa3d176595cba6e07883f512e
shaavan:
ACK 6f2593dc23565abaa3d176595cba6e07883f512e
w0xlt:
tACK 6f2593d on Ubuntu 21.10 Qt 5.15.2
promag:
Code review ACK 6f2593dc23565abaa3d176595cba6e07883f512e.
Tree-SHA512: 61e9afdb1db779150df338e6af08727c34f69639add465c2f7003ff775d97dce3e78e78d325bc6dea5bc13f0fce9ef1c3506d13f1661a5e083e52bba8a32ba44
ac617cc141fe05bea0dc5e8f9df3da43c0945842 wallettool: Check that the dumpfile checksum is the correct size (Andrew Chow)
Pull request description:
After parsing the checksum, make sure that it is the size that we expect it to be.
This issue was reported by Pedro Baptista.
ACKs for top commit:
laanwj:
Code review ACK ac617cc141fe05bea0dc5e8f9df3da43c0945842
Tree-SHA512: 8135b3fb1f4f6b6c91cfbac7d1d3421f1f6c664a742c92940f68eae857f92ce49d042cc3aa5c2df6ef182825271483d65efc7543ec7a8ff047fd7c08666c8899
fa72dd314fe857d827d9b311bdf0453e9463746f fuzz: Move ISO8601 to one place (MarcoFalke)
Pull request description:
Seems confusing to split this to two places.
Also fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=42178
ACKs for top commit:
fanquake:
ACK fa72dd314fe857d827d9b311bdf0453e9463746f
Tree-SHA512: 637b0671078848ea417fdf66b92715602040fad34d4ca5f7b843a519a1cfeebe5d992a79a399deba39926905125681d66ab0dc05f66f79a26f3bf555e12fb0ba
f530202353a4f8bb444966559aa15681ab3cebc6 Make unexpected time type in BCLog::LogMsg() a compile-time error (Martin Ankerl)
bddae7e7ff7bb5931ed807acaef7336f2ee98476 Add util/types.h with ALWAYS_FALSE template (MarcoFalke)
498b323425d960274c40472a6a847afc1982201d log, timer: improve BCLog::LogMsg() (Jon Atack)
8d2f847ed913f15677ae978a412015ac844ffceb 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 f530202353a4f8bb444966559aa15681ab3cebc6. 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 f530202353a4f8bb444966559aa15681ab3cebc6
Tree-SHA512: 37d093eac5590e1b5846ab5994d0950d71e131177d1afe4a5f7fcd614270f977e0ea117e7af788e9a74ddcccab35b42ec8fa4db3a3378940d4988df7d21cdaaa
def1e64bb4fc902bfc19d6487b33cd940f0e0f34 scripted-diff: Drop redundant QString calls (Hennadii Stepanov)
Pull request description:
The return type of `QObject::tr` function _is_ `QString` 🐅
ACKs for top commit:
jarolrod:
ACK def1e64bb4fc902bfc19d6487b33cd940f0e0f34, tested on macOS 10.14.6 Qt 5.15.2
Tree-SHA512: ef405c87a30d6965f6887511d8666b6da57d258ca07833a3fa2dc9fd147d0539d33c57f7551ee13c1dd8024d6057139595c6ce5d088dd6efd7aa13db2a3eebdb
fa55159b9ede4a915f8ef9e5b90e3e99eadedf91 net: Log to net debug in MaybeDiscourageAndDisconnect except for noban and manual peers (MarcoFalke)
Pull request description:
The goal is to avoid local peers (e.g. untrusted peers on the local network or inbound onion peers via a local onion proxy) filling the debug log (and thus the disk).
ACKs for top commit:
practicalswift:
ACK fa55159b9ede4a915f8ef9e5b90e3e99eadedf91
vasild:
ACK fa55159b9ede4a915f8ef9e5b90e3e99eadedf91
Tree-SHA512: de233bf57334580f9b91f369fafd131d71c5ae25db25b09cc8fa8cbf34c0648f083c52260a6a912238751467e3c3c5f5d2309c145710753058d44a0003f88f4f
8353e8cecc3e34a8699b53849df0adab622c1c14 peers-tab: bug fix right panel toggle (randymcmillan)
Pull request description:
Initial Presentation:
When node row selected - panel is presented:
When network disabled - right panel is hidden:
ACKs for top commit:
jarolrod:
ACK 8353e8cecc3e34a8699b53849df0adab622c1c14
jonatack:
ACK 8353e8cecc3e34a8699b53849df0adab622c1c14 tested rebased on current master. Behavior is initially a bit surprising but this would allow more columns to be added to the peers tab window. Verified that selecting more than one peer, clicking on a column header, or running `disconnectnode "" <currently-selected-peer-id>` in the console (or on the CLI with the `-server` startup option) returns the window to its full size. If this is merged, it might be nice to have an obvious way to close the details area like a clickable "close this" icon in the upper left corner of the area.
Talkless:
tACK 8353e8cecc3e34a8699b53849df0adab622c1c14, tested on Debian Sid. Made `bitcoind` connect to `bitcoin-qt` with the PR changes, and after I quit the `bitcoind` instance, right panel do disappear, compared to the previous commit where it didn't.
Tree-SHA512: 8fc156f40bdd61e3ba8db333c729a2a07fd5f0fd1eed56f2fd2aa5ae5864756f8ab6fad74ae2fb0552ee7518b6d489f5800709e6c80c6f31f61fd8ce21cece5f
b3e9bcaac85a64a1b41d534b28c8cfb1f08e14e5 qt, refactor: Drop no longer used PeerTableModel::getNodeStats function (Hennadii Stepanov)
49c604077c572fcdea8739eb3383467dbbbc5f52 qt: Use PeerTableModel::StatsRole (Hennadii Stepanov)
35007edf9c0f592303f0cbda3ade776c87fd80b1 qt: Add PeerTableModel::StatsRole (Hennadii Stepanov)
Pull request description:
This PR allows to access to the `CNodeCombinedStats` instance directly from any view object.
The `PeerTableModel::getNodeStats` member function removed as a kind of layer violation.
No behavior changes.
Also other pulls (bugfixes) are based on this one: #18 and #164.
ACKs for top commit:
jonatack:
Tested re-ACK b3e9bcaac85a64a1b41d534b28c8cfb1f08e14e5 per `git range-diff ae8f797 4c05fe0 b3e9bca`
promag:
Code review ACK b3e9bcaac85a64a1b41d534b28c8cfb1f08e14e5.
jonasschnelli:
utACK b3e9bcaac85a64a1b41d534b28c8cfb1f08e14e5
Tree-SHA512: 6ba50d5dd2c0373655d491ce8b130c47d598da2db5ff4b00633f447404c7e70f8562ead53ddf166e851384d9632ff9146a770c99845c2cdd3ff7250677e4c130
d7c35d0538 refactor: remove `llmq::CQuorumManager` global, move to `LLMQContext` (Kittywhiskers Van Gogh)
5b86df6ca3 refactor: reduce `llmq::CQuorumManager` globals use, use args (Kittywhiskers Van Gogh)
1efd219173 refactor: remove `llmq::CQuorumBlockProcessor` global, move to `LLMQContext` (Kittywhiskers Van Gogh)
1d3afe742b evo: use `gsl::not_null` in `CTxMemPool::ConnectManagers` (Kittywhiskers Van Gogh)
805537e695 evo: add `CMNHFManager::`(`Dis`)`connectManagers` to remove global use (Kittywhiskers Van Gogh)
cbb6828ec7 refactor: access `llmq::CQuorumManager` through arg in `MNHFTx`, functions (Kittywhiskers Van Gogh)
Pull request description:
## Breaking Changes
None expected.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_
ACKs for top commit:
knst:
utACK d7c35d0538
knst:
re-utACK d7c35d0538
PastaPastaPasta:
utACK d7c35d0538
Tree-SHA512: 07e7494bc061f85259a3a6cfb13d052403419cdc94fc51904ac03468bbdbd1ed74429546309d2bfc6a31ba184b93dc32842e49d2960819d19313bcb7e403008e
6c280adcd865ae3da4df53d630c9bf737283a56f net: Return IPv6 scope id in `CNetAddr::ToStringIP()` (W. J. van der Laan)
Pull request description:
If a scope id is provided, return it back in the string representation. Also bring back the test (now in platform independent fashion). Closes#21982. Includes #21961 (apart from the MacOS remark).
ACKs for top commit:
practicalswift:
cr ACK 6c280adcd865ae3da4df53d630c9bf737283a56f
Tree-SHA512: 77792c35679b6c3545fd3a8d3d74c4f515ac2ee9f02d983251aeaaac715d55c122bbb0141abbeac272011f15520b439bd2db4ec8541a58df9b366921d212ca5f
65332b1178c75e1f83415bad24918996a1524866 [addrman] Remove RemoveInvalid() (John Newbery)
Pull request description:
PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs:
- #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed)
- #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets)
Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants.
Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`.
ACKs for top commit:
sipa:
utACK 65332b1178c75e1f83415bad24918996a1524866. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any.
fanquake:
ACK 65332b1178c75e1f83415bad24918996a1524866 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code.
Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
34c80473a8 Merge #19877: [test] clarify rpc_net & p2p_disconnect_ban functional tests (Wladimir J. van der Laan)
e42412924f Merge #19770: RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (MarcoFalke)
f96966b7ea Merge #20043: doc: Add 19501 release notes (fanquake)
6a164eaea9 Merge #19501: send* RPCs in the wallet returns the "fee reason" (MarcoFalke)
b6c8d852e3 Merge #19725: [RPC] Add connection type to getpeerinfo, improve logs (MarcoFalke)
f86263b180 Merge #18202: refactor: consolidate sendmany and sendtoaddress code (Samuel Dobson)
fab41fd3c5 partial Merge #18878: test: Add test for conflicted wallet tx notifications (Wladimir J. van der Laan)
db5bd34ee8 Merge #19202: log: remove deprecated `db` log category (MarcoFalke)
Pull request description:
## Issue being fixed or feature implemented
Regular backports from bitcoin v21
## What was done?
- bitcoin/bitcoin#19202
- partial bitcoin/bitcoin#18878
- bitcoin/bitcoin#18202
- bitcoin/bitcoin#19725
- bitcoin/bitcoin#19501
- bitcoin/bitcoin#20043
- bitcoin/bitcoin#19770
- bitcoin/bitcoin#19877
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
- (RPC) The `getpeerinfo` RPC no longer returns the `addnode` field by default. This
field will be fully removed in the next major release. It can be accessed
with the configuration option `-deprecatedrpc=getpeerinfo_addnode`. However,
it is recommended to instead use the `connection_type` field (it will return
`manual` when addnode is true)
- (Settings) The `sendtoaddress` and `sendmany` RPCs accept an optional `verbose=True`
argument to also return the fee reason about the sent tx.
- (Settings) The `-debug=db` logging category, which was deprecated in v0.18 and replaced by
`-debug=walletdb` to distinguish it from `coindb`, has been removed.
- (RPC) To make RPC `sendtoaddress` more consistent with `sendmany` the following error
`sendtoaddress` codes were changed from `-4` to `-6`:
- Insufficient funds
- Fee estimation failed
- Transaction has too long of a mempool chain
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK 34c80473a8
Tree-SHA512: 725a103e04c9c7d44a79da6f3f54e7745c7fb98ec906e7228ae16f7662d568e48c015c855902ff8485f2908f0f71815e769ca394cf6c3ca2e5fd920dd39cca74
We used to store an alias to the `CQuorumBlockProcessor` instance present
in the global state in `LLMQContext` but handled construction and
destruction of the instance in `LLMQContext`. As direct global usage no
longer exists, we can fully subsume it into `LLMQContext`.
5b57dc5458800e56b4dddfeb32a1813804a62b0f RPC: getpeerinfo: Wrap long help line for bytesrecv_per_msg (Luke Dashjr)
d681a28219d3876a2b6e3cd2fb0d92963674903e RPC: getpeerinfo: Deprecate "whitelisted" field (replaced by "permissions") (Luke Dashjr)
Pull request description:
If we were going to continue support for "whitelisted", we should have probably made it true if any permission flag was set, rather than only if "default permissions" were used.
This corrects the description, and deprecates it.
ACKs for top commit:
laanwj:
ACK 5b57dc5458800e56b4dddfeb32a1813804a62b0f
Tree-SHA512: a2e2137f8be8110357c1b2fef2c923fa8c7c4a49b0b2b3a2d78aedf12f8ed5cc7e140018a21b37e6ec7770ed4007542aeef7ad4558973901b107e8e0f81d6003
fa710a6d67b2de64bde90def77c70d0a052f9030 doc: Add 19501 release notes (MarcoFalke)
faf60dee34ae3dbe8e103a2c1b0679f13df6a921 doc: Remove double-whitespace from help string, other whitespace fixups (MarcoFalke)
Pull request description:
Adds release notes and fixes up some whitespace nits for the touched RPCs
ACKs for top commit:
fanquake:
ACK fa710a6d67b2de64bde90def77c70d0a052f9030
laanwj:
Code review ACK fa710a6d67b2de64bde90def77c70d0a052f9030
Tree-SHA512: b84a96386a9a8ed69f464c7dffdd600cf9a8b33a06120798b141b300991baed369ab91ae48df6446e89e1d62534ccd8ae721454e7a19b48900b317e9192afc47
69cf5d4eeb73f7d685e915fc17af64634d88a4a2 [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e20d56acf7246008b7832efde68ab21 [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb73f7d685e915fc17af64634d88a4a2
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
a512925e19a70d7f6b80ac530a169f45ffaafa1c [doc] Release notes (Amiti Uttarwar)
50f94b34a33c954f6e207f509c93d33267a5c3e2 [rpc] Deprecate getpeerinfo addnode field (Amiti Uttarwar)
df091b9b509f0b10e4315c0bfa2da0cc0c31c22f [refactor] Rename test file to allow any getpeerinfo deprecations. (Amiti Uttarwar)
395acfa83a5436790c1a722a5609ac9d48df235f [rpc] Add connection type to getpeerinfo RPC, update tests (Amiti Uttarwar)
49c10a9ca40967d28ae16dfea9cccc6f3a6624a1 [log] Add connection type to log statement (Amiti Uttarwar)
Pull request description:
After #19316, we can more directly expose information about the connection type on the `getpeerinfo` RPC. Doing so also makes the existing addnode field redundant, so this PR begins the process of deprecating this field.
This PR also includes one commit that improves a log message, as both use a shared function to return the connection type as a string.
Suggested by sdaftuar- https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468001604 & https://github.com/bitcoin/bitcoin/pull/19316#discussion_r468018093
ACKs for top commit:
jnewbery:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
sipa:
utACK a512925e19a70d7f6b80ac530a169f45ffaafa1c
guggero:
Tested and code review ACK a512925e.
MarcoFalke:
cr ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c 🌇
promag:
Code review ACK a512925e19a70d7f6b80ac530a169f45ffaafa1c.
Tree-SHA512: 601a7a38aee235ee59aca690784f886dc2ae4e418b2e6422c4b58cd597376c00f74910f66920b08a08a0bec28bf8022e71a1435785ff6ba8a188954261aba78e
08fc6f6cfc3b06fd170452a766696d7b833113fa [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost)
Pull request description:
I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination.
Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this.
Salvaged from #18201.
ACKs for top commit:
fjahr:
Code review ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
jonatack:
ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
meshcollider:
Code review & functional test run ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa
Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909
bde72a41fe merge bitcoin#23324: print peer counts for all reachable networks in -netinfo (Kittywhiskers Van Gogh)
4b245441a0 merge bitcoin#22959: Display all proxies in -getinfo (Kittywhiskers Van Gogh)
30b0fcf4a6 merge bitcoin#22544: drop torv2; torv3 becomes onion per GetNetworkName() (Kittywhiskers Van Gogh)
b6ca36edda merge bitcoin#22547: Add progress bar for -getinfo (Kittywhiskers Van Gogh)
1f89bfd176 merge bitcoin#21832: Implement human readable -getinfo (Kittywhiskers Van Gogh)
2200b78a15 merge bitcoin#20877: user help and argument parsing improvements (Kittywhiskers Van Gogh)
bd934c71eb partial bitcoin#20764: cli -netinfo peer connections dashboard updates (Kittywhiskers Van Gogh)
b2d865633f merge bitcoin#21261: update inbound eviction protection for multiple networks, add I2P peers (Kittywhiskers Van Gogh)
0b16b50fcb cli: fix loop counter comparison in `ProcessReply` (Kittywhiskers Van Gogh)
Pull request description:
## Additional Information
* Dependency for https://github.com/dashpay/dash/pull/6035
* Dependency for https://github.com/dashpay/dash/pull/6031
* In [dash#5904](https://github.com/dashpay/dash/pull/5904) ([bitcoin#21595](https://github.com/bitcoin/bitcoin/pull/21595)), one of the loops in `ProcessReply` is supposed to iterate `rows.size()` times (which at the time was hardcoded to `3`), the backport erroneously set the value to `m_networks.size()` (which also evaluated to `3`) as part of increasing `m_networks.size()` usage.
As this pull request includes [bitcoin#23324](https://github.com/bitcoin/bitcoin/pull/23324), which changes it over to `rows.size()`, the above has been corrected in a separate commit for documentation purposes.
* `-addrinfo` output
* `-getinfo` output (diamonds are due to rendering limitations of my terminal and are not indicative of the symbols used)
* `-netinfo` output
## Breaking Changes
* CLI `-addrinfo` now returns a single field for the number of `onion` addresses known to the node instead of separate `torv2` and `torv3` fields, as support for TorV2 addresses was removed from Dash Core in 18.0.
* `-getinfo` has been updated to return data in a user-friendly format that also reduces vertical space.
## Checklist
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
- [x] I have added or updated relevant unit/integration/functional/e2e tests
- [x] I have made corresponding changes to the documentation **(note: N/A)**
- [x] I have assigned this pull request to a milestone
ACKs for top commit:
PastaPastaPasta:
utACK bde72a41fe
Tree-SHA512: 921cb45b7e243a321a32c835eb23d5ba8df610ff234a548a9051436a2c21845ce70097fb9a9bb812b77b04373f9f0a9f90264168d97b08da1890be06bfd9f99c
5730a43703f7e5a5ca26245ba3b55fbdd027d0b6 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f93979b274a37de013502b05d25fad8 net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d9072b7d5a36a6491784bdeb9247e91fb0 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450dde6a54bd785504c923adfb45c7060 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
naumenkogs:
ACK 5730a43703f7e5a5ca26245ba3b55fbdd027d0b6
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
c514a4f59a7430f05dbe20465ddf4ca323329f1e doc: release note for `db` log category removal (Jon Atack)
4c0c89307dabbf51a32551471c54966ddf7c5bc3 log: remove deprecated `db` log category (Jon Atack)
Pull request description:
The `db` log category was renamed to `walletdb` (like `coindb`) in #17410 and its upcoming removal announced in the 0.20 release notes.
```
- The `-debug=db` logging category has been renamed to
`-debug=walletdb` to distinguish it from `coindb`. The `-debug=db`
option has been deprecated and will be removed in the next major
release. (#17410)
```
This PR removes the warning and reverts to the usual behavior for an unrecognised log category.
```
$ bitcoin-cli logging '["db"]'
error code: -8
error message:
unknown logging category db
```
```
$ ./src/bitcoind -debug=db
Warning: Unsupported logging category -debug=db.
2020-06-07T15:30:45Z Bitcoin Core version v0.20.99.0-4c0c89307d (debug build)
2020-06-07T15:30:45Z Warning: Unsupported logging category -debug=db.
2020-06-07T15:30:45Z Assuming ancestors of block 0000000000000000000f2adce67e49b0b6bdeb9de8b7c3d7e93b21e7fc1e819d have valid signatures.
2020-06-07T15:30:45Z Setting nMinimumChainWork=00000000000000000000000000000000000000000e1ab5ec9348e9f4b8eb8154
2020-06-07T15:30:45Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2020-06-07T15:30:45Z Using RdSeed as additional entropy source
```
ACKs for top commit:
MarcoFalke:
ACK c514a4f59a7430f05dbe20465ddf4ca323329f1e 🔄
Tree-SHA512: fd62fd7ae0dc65446ba4401d75b4047e055396a33f7f1b176e79a7753250aec2a474ae604163d3f7e68710443c0ed2f45e44435d15f35612d794807e2142d5a3
