Commit graph

31302 commits

Author SHA1 Message Date
Kiminuo
2ec38bdebb Remove gArgs from wallet.h and wallet.cpp
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
2021-11-09 11:27:06 +01:00
MarcoFalke
94db963de5
Merge bitcoin/bitcoin#23300: test: Implicitly sync after generate*, unless opted out
facc352648 test: Implicitly sync after generate*, unless opted out (MarcoFalke)

Pull request description:

  The most frequent failure in functional tests are intermittent races. Fixing such bugs is cumbersome because it involves:
  * Noticing the failure
  * Fetching and reading the log to determine the test case that failed
  * Adding a `self.sync_all()` where it was forgotten
  * Spamming out a pr and waiting for review, which is already sparse

  Also, writing a linter to catch those is not possible, nor is review effective in finding these bugs prior to merge.

  Fix all future intermittent races caused by a missing sync_block call by calling `sync_all` implicitly after each `generate*`, unless opted out. This ensures that the code is race-free (with regards to blocks) when the tests pass once, instead of our current approach where the code can never be guaranteed to be race-free.

  There are some scripted-diff cleanups (see https://github.com/bitcoin/bitcoin/pull/22567), but they will be submitted in a follow-up to reduce the conflicts in this pull.

ACKs for top commit:
  lsilva01:
    tACK facc352 on Ubuntu 20.04
  brunoerg:
    tACK facc352648 on MacOS 11.6

Tree-SHA512: 046a40a066b4a3bd28a3077bd654fa8887442dd1f0ec6fd11671865809ef02376f126eb667a1320ebd67b6e372c78c00dbf8bd25d86ed86f1d9a25363103ed97
2021-11-09 09:58:51 +01:00
MarcoFalke
49143477e7
Merge bitcoin/bitcoin#23469: doc: Remove Boost build note from build-unix.md
ea360d69e5 doc: remove redundant optionals from build-unix.md (fanquake)
b97185812e doc: remove Boost build note from build-unix.md (fanquake)

Pull request description:

  We don't have build instructions for any other dependency, and in any case, this isn't something we should maintain in our docs. If someone wants to know how to build Boost, or any other dependency, they can look at the relevant website / external build documentation.

ACKs for top commit:
  jarolrod:
    ACK ea360d69e5

Tree-SHA512: c7389b5f051f79c728d8ea0725143affeb2c35b1e3c20d5cd441c6ac540d230698c47bf2c57feb12e7b6bffd2f337c1c83a9cd3a928ce5c479cdbbdf8f61fba4
2021-11-09 09:52:30 +01:00
fanquake
ea360d69e5
doc: remove redundant optionals from build-unix.md
Calling dependencies optional in a list of already optional dependencies
is redundant.
2021-11-09 07:50:43 +08:00
fanquake
b97185812e
doc: remove Boost build note from build-unix.md
We don't do this for any other dependency, and users are better looking
at the actual Boost site/docs. This isn't something we should need to
have in our build docs.
2021-11-09 07:50:38 +08:00
fanquake
c702d1fefd
Merge bitcoin/bitcoin#23466: doc: Suggest keys.openpgp.org as keyserver in SECURITY.md
90f1f849e9 doc: Suggest `keys.openpgp.org` as keyserver in SECURITY.md (Tim Ruffing)

Pull request description:

  `--recv-keys` without a `--keyserver` arg simply failed for me on a fresh Arch Linux installation, so I think it's a good idea to suggest a keyserver. OpenPGP ecosystem is broken in a number of ways, so the right way to approach this issue has some potential for bikeshedding. But the only thing that this PR does is to keep `SECURITY.md` in line with the instructions for builder keys, where there was agreement on switching to `keys.openpgp.org` (#22688).

ACKs for top commit:
  MarcoFalke:
    review ACK 90f1f849e9
  laanwj:
    Review ACK 90f1f849e9
  hebasto:
    ACK 90f1f849e9, agree with arguments above.
  Zero-1729:
    ACK 90f1f849e9

Tree-SHA512: 1ab20c837cd952aa32b57473772cbfd33411a08db6e88b951bce38f76a3c509c0e91d6944ec0ca5eac8d5eb4d98a5489276d55691328f2e2556b2640f8e7c108
2021-11-09 06:40:53 +08:00
W. J. van der Laan
8346004ac8
Merge bitcoin/bitcoin#23077: Full CJDNS support
420695c193 contrib: recognize CJDNS seeds as such (Vasil Dimov)
f9c28330a0 net: take the first 4 random bits from CJDNS addresses in GetGroup() (Vasil Dimov)
29ff79c0a2 net: relay CJDNS addresses even if we are not connected to CJDNS (Vasil Dimov)
d96f8d304c net: don't skip CJDNS from GetNetworkNames() (Vasil Dimov)
c2d751abba net: take CJDNS into account in CNetAddr::GetReachabilityFrom() (Vasil Dimov)
9b43b3b257 test: extend feature_proxy.py to test CJDNS (Vasil Dimov)
508eb258fd test: remove default argument of feature_proxy.py:node_test() (Vasil Dimov)
6387f397b3 net: recognize CJDNS addresses as such (Vasil Dimov)
e6890fcb44 net: don't skip CJDNS from GetNetworksInfo() (Vasil Dimov)
e9d90d3c11 net: introduce a new config option to enable CJDNS (Vasil Dimov)
78f456c576 net: recognize CJDNS from ParseNetwork() (Vasil Dimov)
de01e312b3 net: use -proxy for connecting to the CJDNS network (Vasil Dimov)
aedd02ef27 net: make it possible to connect to CJDNS addresses (Vasil Dimov)

Pull request description:

  CJDNS overview
  =====

  CJDNS is like a distributed, shared VPN with multiple entry points where every participant can reach any other participant. All participants use addresses from the `fc00::/8` network (reserved IPv6 range). Installation and configuration is done outside of applications, similarly to VPN (either in the host/OS or on the network router).

  Motivation
  =====

  Even without this PR it is possible to connect two Bitcoin Core nodes through CJDNS manually by using e.g. `-addnode` in environments where CJDNS is set up. However, this PR is necessary for address relay to work properly and automatic connections to be made to CJDNS peers. I.e. to make CJDNS a first class citizen network like IPv4, IPv6, Tor and I2P.

  Considerations
  =====

  An address from the `fc00::/8` network, could mean two things:
  1. Part of a local network, as defined in RFC 4193. Like `10.0.0.0/8`. Bitcoin Core could be running on a machine with such address and have peers with those (e.g. in a local network), but those addresses are not relayed to other peers because they are not globally routable on the internet.
  2. Part of the CJDNS network. This is like Tor or I2P - if we have connectivity to that network then we could reach such peers and we do relay them to other peers.

  So, Bitcoin Core needs to be able to tell which one is it when it encounters a bare `fc00::/8` address, e.g. from `-externalip=` or by looking up the machine's own addresses. Thus a new config option is introduced `-cjdnsreacable`:
  * `-cjdnsreacable=0`: it is assumed a `fc00::/8` address is a private IPv6 (1.)
  * `-cjdnsreacable=1`: it is assumed a `fc00::/8` address is a CJDNS one (2.)

  After setting up CJDNS outside of Bitcoin Core, a node operator only needs to enable this option.
  Addresses from P2P relay/gossip don't need that because they are properly tagged as IPv6 or as CJDNS.

  For testing
  =====
  ```
  [fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa]:8333
  [fc68:7026:cb27:b014:5910:e609:dcdb:22a2]:8333
  [fcb3:dc50:e1ae:7998:7dc0:7fa6:4582:8e46]:8333
  [fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]:8333
  [fcf2:d9e:3a25:4eef:8f84:251b:1b4d:c596]:8333
  ```

ACKs for top commit:
  dunxen:
    ACK 420695c
  jonatack:
    re-ACK 420695c193 per `git range-diff 23ae793 4fbff39 420695c`
  laanwj:
    Code review ACK 420695c193

Tree-SHA512: 21559886271aa84671d52b120fa3fa5a50fdcf0fcb26e5b32049c56fab0d606438d19dd366a9c8ce612d3894237ae6d552ead3338b326487e3534399b88a317a
2021-11-08 14:44:37 +01:00
W. J. van der Laan
aecc08f62e
Merge bitcoin/bitcoin#23409: refactor: Take Span in SetSeed
fa93ef5a8a refactor: Take Span in SetSeed (MarcoFalke)

Pull request description:

  This makes calling code less verbose and less fragile. Also, by adding
  the CKey::data() member function, it is now possible to call HexStr()
  with a CKey object.

ACKs for top commit:
  sipa:
    utACK fa93ef5a8a
  laanwj:
    Code review ACK fa93ef5a8a
  theStack:
    Code-review ACK fa93ef5a8a

Tree-SHA512: 73fb999320719ad4b9ab5544018a7a083d140545c2807ee3582ecf7f441040a30b5157e85790b6b840af82f002a7faf30bd8162ebba5caaf2067391c43dc7e25
2021-11-08 12:48:25 +01:00
Tim Ruffing
90f1f849e9 doc: Suggest keys.openpgp.org as keyserver in SECURITY.md
This is in line with 4c43b7d41d from
PR #22688.
2021-11-08 12:22:04 +01:00
MarcoFalke
c86f546f98
Merge bitcoin/bitcoin#23458: ci: Do not print git log for empty COMMIT_RANGE
095f07744c ci: Do not print `git log` for empty COMMIT_RANGE (Hennadii Stepanov)

Pull request description:

  On master (77a2f5d30c) a CI lint task [log](https://api.cirrus-ci.com/v1/task/4817858858319872/logs/lint.log) exceeds 20K lines.

  This PR fixes this issue.

ACKs for top commit:
  MarcoFalke:
    cr ACK 095f07744c

Tree-SHA512: 89180018aeccf1599cdf218924cbab12dcbae0f6674bb90e13b64e342cdd908a880b885039c23f0d1d03493e55a94fe04abf39481616ae6550c6a759f5ca9a35
2021-11-08 09:32:11 +01:00
MarcoFalke
f075e83b81
Merge bitcoin/bitcoin#23439: test: Open streams_test_tmp file in temporary folder
a04350b86c Open streams_test_tmp file in temporary folder (Martin Leitner-Ankerl)

Pull request description:

  The tests `streams_tests/streams_buffered_file` and `streams_tests/streams_buffered_file_rand`
  did not use a the temporary directory provided by  `BasicTestingSetup`, so it was not possible
  to execute multiple of them in parallel. This fixes that.

  To reproduce, run

  ```sh
  parallel --halt now,fail=1 './src/test/test_bitcoin --run_test=streams_tests/streams_buffered_file_rand' -- ::: {1..1000}
  ```

  This executes the test 1000 times, one job per CPU. It works on that commit but mergebase fails quickly.

ACKs for top commit:
  theStack:
    Tested ACK a04350b86c

Tree-SHA512: 705b127e99a0fdbf33362283cc94036ac3aeff364a5e75b0f0a2490114bbaa44cf310ac13d43c254a4b51b72d4e856e9ee584904ce56b2f496b92d995ac7dc24
2021-11-08 09:28:10 +01:00
fanquake
ed40f37e00
Merge bitcoin/bitcoin#22076: build: Fix make apk if ccache enabled, and enable it on CI
15fb57556e ci: Enable ccache for "ARM64 Android APK" job (Hennadii Stepanov)
7a777ec98c build: Fix `make apk` if ccache enabled (Hennadii Stepanov)

Pull request description:

  On master (456c8d6cd8) `make apk` is broken for Android targets if the build system was configured with `--enable-ccache` explicitly or by default.

  This PR fixes this bug, and enables `ccache` for "ARM64 Android APK" Cirrus CI job.

ACKs for top commit:
  fanquake:
    ACK 15fb57556e

Tree-SHA512: e3c94dccc9f07a712e0e01983462ee607122623b213173c74d17e91024243239de212a35e5db94036bfd0a347e727b133865c26c67d6bd279604416ea971ab94
2021-11-08 15:58:48 +08:00
fanquake
ddc90293c1
Merge bitcoin/bitcoin#23464: doc: remove mention of system univalue from build-unix.md
78e36700a0 doc: remove mention of system univalue (fanquake)

Pull request description:

  Should have been part of #22646.

ACKs for top commit:
  hebasto:
    ACK 78e36700a0

Tree-SHA512: a5d54d73526033825ce4467cc3c57c26064739eef546556975a4c6f1f5bea84004640acd426734f90f98bc7a76ec837d716aa31167f2bdce7ee3887ad92e3152
2021-11-08 15:43:47 +08:00
fanquake
170e2687ba
Merge bitcoin/bitcoin#23446: doc: Mention that BerkeleyDB is for legacy wallet in build-unix
7eb5b25e6e doc: Mention that BerkeleyDB is for legacy wallet in build-unix (W. J. van der Laan)

Pull request description:

  This updates build-unix for the descriptor wallet, and prepares for eventual legacy wallet deprecation.

  - Move 'descriptor wallet' dependencies above legacy wallet deps both for Debian and Fedora.
  - Explicitly mention 'legacy wallet' where referring to the BerkeleyDB wallet. Shorten BerkeleyDB instruction to a single paragraph.

ACKs for top commit:
  lsilva01:
    ACK 7eb5b25
  hebasto:
    ACK 7eb5b25e6e, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 8a0859bfaf170a1c5a8b3e8f873d6b788f3b1cebd8d006569348c5b415d5ebbdfd56a5a294c509114975addb599da8a50c4be618b0545ff402065e948f692ba1
2021-11-08 15:42:23 +08:00
Hennadii Stepanov
15fb57556e
ci: Enable ccache for "ARM64 Android APK" job 2021-11-08 07:19:21 +02:00
Hennadii Stepanov
7a777ec98c
build: Fix make apk if ccache enabled 2021-11-08 07:19:10 +02:00
fanquake
78e36700a0
doc: remove mention of system univalue
Should have been part of #22646.
2021-11-08 10:06:40 +08:00
fanquake
f21ce0bbbe
Merge bitcoin/bitcoin#23450: doc: update SECURITY.md inline with recent changes to bitcoincore.org
e7c0d506d5 doc: update SECURITY.md inline with recent changes to bitcoincore.org (fanquake)

Pull request description:

  See https://github.com/bitcoin-core/bitcoincore.org/pull/809 and https://github.com/bitcoin-core/bitcoincore.org/pull/819 and https://bitcoincore.org/en/contact/.

ACKs for top commit:
  laanwj:
    ACK e7c0d506d5, also checked that fingerprint matches
  kristapsk:
    ACK e7c0d506d5, checked that fingerprint matches.
  Zero-1729:
    ACK e7c0d506d5, fingerprint matches.

Tree-SHA512: 24aeb5c107db55a006b848bfd2c0f644219975acbd21f30749aff6a506832471d43de388100bbb195d48bca6f799e95e6fd2c54e06fc26c1a051bdcfb3db0982
2021-11-08 09:47:33 +08:00
fanquake
12f2b6ac01
Merge bitcoin/bitcoin#23453: Remove the build_msvc/testconsensus project
bb1c84082c Remove the build_msvc/testconsensus project (Aaron Clauson)

Pull request description:

  The testconsensus project is not integral to the Bitcoin Core code. It was originally added as a quick and dirty demo of how to do a consensus check with the msvc build. There are better examples.

  PR #23438 made a change that caused a compiler error with the buildmsvc/testconsensus code. Rather than leave it hanging around to incur potential bitrot, or further PR build failures, it should be removed.

ACKs for top commit:
  hebasto:
    ACK bb1c84082c, tested on Windows 10 Pro (20H2).

Tree-SHA512: d81b99eb09171b66c179961b15f0b2e2e97e5ee7f011f18667e890c90e3d169593ad9aedd05a8616e962212952048827b7159d3c2a2ecb7ac378136b80bf6b23
2021-11-08 09:43:48 +08:00
Hennadii Stepanov
095f07744c
ci: Do not print git log for empty COMMIT_RANGE 2021-11-07 17:04:06 +02:00
Aaron Clauson
bb1c84082c
Remove the build_msvc/testconsensus project
The testconsensus project is not integral to the Bitcoin Core code. It was originally added as a quick and dirty demo of how to do a consensus check with the msvc build. There are better examples.

PR #23438 made a change that caused a compiler error with the buildmsvc/testconsensus code. Rather than leave it hanging around to incur potential bitrot, or furhter PR build failures, it should be removed.
2021-11-06 21:32:34 +00:00
fanquake
e7c0d506d5
doc: update SECURITY.md inline with recent changes to bitcoincore.org
See https://github.com/bitcoin-core/bitcoincore.org/pull/809 and
https://github.com/bitcoin-core/bitcoincore.org/pull/819 and
https://bitcoincore.org/en/contact/.
2021-11-06 18:05:07 +08:00
W. J. van der Laan
7eb5b25e6e doc: Mention that BerkeleyDB is for legacy wallet in build-unix
This updates build-unix for the descriptor wallet, and prepares for
eventual legacy wallet deprecation.

- Move 'descriptor wallet' dependencies above legacy wallet deps both
for Debian and Fedora.
- Explicitly mention 'legacy wallet' where referring to the BerkeleyDB
wallet. Shorten BerkeleyDB instruction to a single paragraph.
2021-11-05 19:27:17 +01:00
MarcoFalke
77a2f5d30c
Merge bitcoin/bitcoin#23334: fuzz: Descriptor wallet
11115169a1 ci: Build fuzz with libsqlite3-dev (MarcoFalke)
fa7c6efca6 fuzz: Add wallet fuzz test (MarcoFalke)
fa59d2ce5b refactor: Use local args instead of global gArgs in CWallet::Create (MarcoFalke)
fadb44606f build: Inline FUZZ_SUITE_LDFLAGS_COMMON (MarcoFalke)

Pull request description:

  Initial sketch to fuzz descriptor wallets. Can be improved in the future.

ACKs for top commit:
  mjdietzx:
    Code review ACK 1111516

Tree-SHA512: b1d2f24504d1ed5f3c6a031210f04c27c13d4e15576c4acbf50ded37ac45f7b7a5c7553e91d81d4a06e9ea73b3d745a552218d3ef3b2932fa5325a8331b0d3fd
2021-11-05 10:29:21 +01:00
Martin Leitner-Ankerl
a04350b86c
Open streams_test_tmp file in temporary folder
The tests `streams_tests/streams_buffered_file` and `streams_tests/streams_buffered_file_rand`
did not use a the temporary directory provided by  `BasicTestingSetup`, so it was not possible
to execute multiple of them in parallel. This fixes that.

To reproduce, run

```sh
parallel --halt now,fail=1 './src/test/test_bitcoin --run_test=streams_tests/streams_buffered_file_rand' -- ::: {1..1000}
```

This executes the test 1000 times, one job per CPU. It works on that commit but mergebase fails quickly.
2021-11-04 18:47:14 +01:00
Samuel Dobson
24abd8312e
Merge bitcoin/bitcoin#22949: fee: Round up fee calculation to avoid a lower than expected feerate
80dc829be7 tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676 fees: Always round up fee calculated from a feerate (Andrew Chow)

Pull request description:

  When calculating the fee for a feerate, it is possible that the final calculation will have fractional satoshis. Currently those are ignored via truncation which results in the absolute fee being rounded down. Rounding down is problematic because it results in a feerate that is slightly lower than the feerate represented by the `CFeeRate` object. A slightly lower feerate particularly causes issues for coin selection as it can trigger an assertion error. To avoid potentially underpaying the feerate (and the assertion), always round up the calculated fee.

  A test is added for the assertion, along with a comment explaining what happens.

  It is unlikely that a user can trigger this as it requires a very specific set of rounding errors to occur as well as the transaction not needing any change and being right on the lower bound of the exact match window. However I was able to trigger the assertion while running coin selection simulations, albeit after thousands of transactions and with some weird feerates.

ACKs for top commit:
  ryanofsky:
    Code review ACK 80dc829be7
  promag:
    Tested ACK 80dc829be7.
  lsilva01:
    tACK 80dc829
  meshcollider:
    utACK 80dc829be7

Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642
2021-11-05 00:08:00 +13:00
Vasil Dimov
420695c193
contrib: recognize CJDNS seeds as such
An IPv6 address from fc00::/8 could be either from the CJDNS network or
from a private-unroutable-reserved segment of IPv6. A seed node with
such an address must be from the CJDNS network, otherwise other peers
will not be able to connect to it.
2021-11-03 14:58:55 +01:00
Vasil Dimov
f9c28330a0
net: take the first 4 random bits from CJDNS addresses in GetGroup()
CJDNS addresses start with constant 8 bits, so in order to account for
the first 4 random ones, we must take the first 12. Otherwise the entire
CJDNS network will belong to one group.
2021-11-03 14:58:54 +01:00
Vasil Dimov
29ff79c0a2
net: relay CJDNS addresses even if we are not connected to CJDNS
This will help with propagation, so that multi-homed nodes can learn
CJDNS addresses outside of the CJDNS network.
2021-11-03 14:58:53 +01:00
Vasil Dimov
d96f8d304c
net: don't skip CJDNS from GetNetworkNames() 2021-11-03 14:58:53 +01:00
Vasil Dimov
c2d751abba
net: take CJDNS into account in CNetAddr::GetReachabilityFrom()
This way `GetLocal()` will pick our CJDNS address for a CJDNS peer.
2021-11-03 14:58:52 +01:00
Vasil Dimov
9b43b3b257
test: extend feature_proxy.py to test CJDNS 2021-11-03 14:58:51 +01:00
Vasil Dimov
508eb258fd
test: remove default argument of feature_proxy.py:node_test()
The default bool argument makes it harder to read because the last but
one argument is also bool. Pass all of them as named arguments to
increase readability.

Another bool argument will be added to indicate whether to test CJDNS.

Co-authored-by: Jon Atack <jon@atack.com>
2021-11-03 14:58:50 +01:00
Vasil Dimov
6387f397b3
net: recognize CJDNS addresses as such
In some cases addresses come from an external source as a string or as a
`struct sockaddr_in6`, without a tag to tell whether it is a private
IPv6 or a CJDNS address. In those cases interpret the address as a CJDNS
address instead of an IPv6 address if `-cjdnsreachable` is set and the
seemingly-IPv6-address belongs to `fc00::/8`. Those external sources are:

* `-externalip=`
* `-bind=`
* UPnP
* `getifaddrs(3)` (called through `-discover`)
* `addnode`
* `connect`
* incoming connections (returned by `accept(2)`)
2021-11-03 14:58:50 +01:00
Vasil Dimov
e6890fcb44
net: don't skip CJDNS from GetNetworksInfo() 2021-11-03 14:58:49 +01:00
Vasil Dimov
e9d90d3c11
net: introduce a new config option to enable CJDNS
CJDNS is set up in the host OS, outside of the application. When the
routing is configured properly then connecting to fc00::/8 results in
connecting to the CJDNS network.

Introduce an option so that Bitcoin Core knows whether this is the case.
2021-11-03 14:58:48 +01:00
Vasil Dimov
78f456c576
net: recognize CJDNS from ParseNetwork()
This allows to use "cjdns" as an argument to the `getnodeaddresses` RPC
and to the `-onlynet=` parameter.
2021-11-03 14:41:14 +01:00
Vasil Dimov
de01e312b3
net: use -proxy for connecting to the CJDNS network
If `-proxy` is given, then also use it for connecting to the CJDNS
network.
2021-11-03 14:41:14 +01:00
Vasil Dimov
aedd02ef27
net: make it possible to connect to CJDNS addresses
Connecting to CJDNS addresses works without a proxy, just like
connecting to an IPv6 address. Thus adapt `CService::GetSockAddr()` to
retrieve the `struct sockaddr*` even for `CService::IsCJDNS()` objects.
2021-11-03 14:41:09 +01:00
MarcoFalke
23ae7931be
Merge bitcoin/bitcoin#23154: doc: add assumeutxo notes
9ab440199d doc: add assumeutxo notes (James O'Beirne)

Pull request description:

  This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)

  ---

  Adds some notes on assumeutxo design.

  Related: https://github.com/bitcoin/bitcoin/pull/21526#discussion_r715558994

ACKs for top commit:
  ariard:
    ACK 9ab4401
  naumenkogs:
    ACK 9ab4401
  michaelfolkson:
    ACK 9ab440199d
  fjahr:
    ACK 9ab440199d

Tree-SHA512: 2fca8373b78701754957d12bc43ce18aa6928507965448741cb4e8c56589ad61d261f8542e348094fc9631d46ee6a7afee75c965c0db993fc816758569137b74
2021-11-03 13:01:53 +01:00
MarcoFalke
e2b5192d1c
Merge bitcoin/bitcoin#23211: refactor: move update_* structs from txmempool.h to .cpp file
65aaf9495d refactor: move `update_*` structs from txmempool.h to .cpp file (Sebastian Falbesoner)
9947ce6262 refactor: use const reference for parents in `CTxMemPool::UpdateAncestorsOf` (Sebastian Falbesoner)

Pull request description:

  These helpers are exclusively used in txmempool.cpp, hence they should also be moved there. The PR also contains a commit which fixes const-correctness for parents in `CTxMemPool::UpdateAncestorsOf` and declares them as reference to avoid a copy.

ACKs for top commit:
  promag:
    Code review ACK 65aaf9495d. Verified move-only commit locally.

Tree-SHA512: 7ce29f3ba0e68b5355001f27725b00f6d54cc993015356eb40b61b8cdd17db49b980f4c3d798c8e0c940d245dc3a72c474bb9ff3c0ee971ead450786076812c2
2021-11-03 08:59:03 +01:00
MarcoFalke
3c4729a515
Merge bitcoin/bitcoin#23223: Disable lock contention logging in checkqueue_tests
6ae9f1cf96 Disable lock contention logging in checkqueue_tests (Jon Atack)

Pull request description:

  This patch disables lock contention logging in the checkqueue_tests as some of these tests are designed to be heavily contested to trigger race conditions or other issues. This created very large log files when run with DEBUG_LOCKCONTENTION defined (up to v22) or with lock logging enabled by default in current master.

  Examples running the following command:

  ```
  $ ./src/test/test_bitcoin -t checkqueue_tests/test_CheckQueue_Correct_Random -- DEBUG_LOG_OUT > testlog.txt

  -rw-r--r--   87042178 Oct  8 12:41 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run1.txt
  -rw-r--r--   73879896 Oct  8 12:42 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run2.txt
  -rw-r--r--   65150518 Oct  8 12:51 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run1.txt
  -rw-r--r--   65774554 Oct  8 12:52 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run2.txt
  -rw-r--r--   73493309 Oct  8 13:00 testlog-current-master-at-991753e-run1.txt
  -rw-r--r--   65616977 Oct  8 13:01 testlog-current-master-at-991753e-run2.txt
  -rw-r--r--       5093 Oct  8 13:04 testlog-with-this-commit-run1.txt
  -rw-r--r--       5093 Oct  8 13:05 testlog-with-this-commit-run2.txt
  ```

  Resolves #23167.

ACKs for top commit:
  vasild:
    ACK 6ae9f1cf96

Tree-SHA512: b16812ed60c58a1cf40c04ebeca9197ac076b2415f71673ac7bb5b7960a1ff80ba2c909345ad221c7689b0562d17f63a32a629f5d6dbcf0e57130bf5760388c1
2021-11-02 20:54:19 +01:00
MarcoFalke
9e3f7dcaa2
Merge bitcoin/bitcoin#22735: [net] Don't return an optional from TransportDeserializer::GetMessage()
f3e451bebf [net] Replace GetID() with id in TransportDeserializer constructor (Troy Giorshev)
8c96008ab1 [net] Don't return an optional from TransportDeserializer::GetMessage() (Troy Giorshev)

Pull request description:

  Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This
  throws an error if COMMAND_OTHER doesn't exist, which should never
  happen. `find()` instead just accessed the last element, which could make
  debugging more difficult.

  Resolves review comments from PR19107:

  - https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478718436
  - https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478714497

ACKs for top commit:
  theStack:
    Code-review ACK f3e451bebf
  ryanofsky:
    Code review ACK f3e451bebf. Changes since last review in https://github.com/bitcoin/bitcoin/pull/20364#pullrequestreview-534369904 were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit.

Tree-SHA512: 37de4b25646116e45eba50206e82ed215b0d9942d4847a172c104da4ed76ea4cee29a6fb119f3c34106a9b384263c576cb8671d452965a468f358d4a3fa3c003
2021-11-02 13:40:09 +01:00
MarcoFalke
76886ce11e
Merge bitcoin/bitcoin#23410: doc: Add output script descriptors BIPs 380..386
c02a674e97 doc: Add output script descriptors BIPs 380..386 (Hennadii Stepanov)

Pull request description:

  BIPs 380..385 are implemented as of v0.17.0.
  BIP 386 is implemented as of v22.0.

ACKs for top commit:
  sipa:
    ACK c02a674e97
  jarolrod:
    ACK c02a674e97
  shaavan:
    ACK c02a674e97

Tree-SHA512: 40f0252d3aad08c61a8e1476d26a590dbcf3f9d66c1f8315d15d13feb17288cc25b9c75df5b938f77695eafaba847dacc0020a880ba6034a511e7c9b7f40fd8f
2021-11-02 09:41:46 +01:00
Hennadii Stepanov
c02a674e97
doc: Add output script descriptors BIPs 380..386 2021-11-01 16:20:35 +02:00
MarcoFalke
5adc5c0280
Merge bitcoin/bitcoin#23403: test: Fix segfault in the psbt_wallet_tests/psbt_updater_test
68018e4c3e test: Avoid excessive locking of `cs_wallet` (Hennadii Stepanov)
7986faf2e0 test: Fix segfault in the psbt_wallet_tests/psbt_updater_test (Hennadii Stepanov)

Pull request description:

  The dcd6eeb64a commit (bitcoin/bitcoin#23288) introduced an intermittent failure in the `psbt_wallet_tests/psbt_updater_test` unit test. See bitcoin/bitcoin#23368.

  The test failure can be easily made reproducible with the following patch:
  ```diff
  --- a/src/scheduler.cpp
  +++ b/src/scheduler.cpp
  @@ -57,6 +57,8 @@ void CScheduler::serviceQueue()
               Function f = taskQueue.begin()->second;
               taskQueue.erase(taskQueue.begin());

  +            UninterruptibleSleep(100ms);
  +
               {
                   // Unlock before calling f, so it can reschedule itself or another task
                   // without deadlocking:
  ```

  This PR implements an idea which was mentioned in the [comment](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-953796339):
  > Yes, as I said before this looks like a race where the wallet is deleted before stopping the scheduler: [#23368 (comment)](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-952808824)
  >
  > IIRC, the order should be:
  >
  >    * stop scheduler
  >
  >    * delete wallet
  >
  >    * delete scheduler

  The second commit introduces a refactoring with no behavior change.

  Fixes bitcoin/bitcoin#23368.

ACKs for top commit:
  mjdietzx:
    Code review ACK 68018e4c3e

Tree-SHA512: d9103f6252aab807453628159dec243bc543a2595eecaa04ec761dca3c2370085592c55d6f50967d69a4ac6e8b5827eec30dd9b025132c99b0bb9aa5911ad915
2021-11-01 14:24:22 +01:00
MarcoFalke
fa93ef5a8a
refactor: Take Span in SetSeed
This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
2021-11-01 14:20:56 +01:00
fanquake
3fc3641043
Merge bitcoin/bitcoin#22766: refactor: Clarify and disable unused ArgsManager flags
c5d7e34bd9 scripted-diff: disable unimplemented ArgsManager BOOL/INT/STRING flags (Russell Yanofsky)
b8c069b7a9 refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage (Russell Yanofsky)
26a50ab322 refactor: Split InterpretOption into Interpret{Key,Value} functions (Russell Yanofsky)

Pull request description:

  This is preparation for #16545 or another PR implementing type validation for ArgsManager settings. It fixes misleading usages of existing flags, prevents flags from being similarly misused in the future, and allows validation logic to be added without breaking backwards compatibility.

  ---

  Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545).

  An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed.

  Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in three commits, with the first commit cleaning up some code, the second commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.

  None of the changes affect behavior in any way.

ACKs for top commit:
  ajtowns:
    utACK c5d7e34bd9
  promag:
    Code review ACK c5d7e34bd9, which as the new argument `-legacy`.

Tree-SHA512: cad0e06361e8cc584eb07b0a1f8b469e3beea18abb458c4e43d9d16e9f301b12ebf1d1d426a407fbd96f99724ad6c0eae5be05c713881da7c55e0e08044674eb
2021-11-01 11:25:42 +08:00
fanquake
994aaaa88d
Merge bitcoin/bitcoin#23380: addrman: Fix AddrMan::Add() return semantics and logging
61ec0539b2 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery)
2095df7b7b [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery)
2658eb6d68 [addrman] Rename Add_() to AddSingle() (John Newbery)
e58598e833 [addrman] Add doxygen comment to AddrMan::Add() (John Newbery)

Pull request description:

  Previously, Add() would return true if the function created a new
  AddressInfo object, even if that object could not be successfully
  entered into the new table and was deleted. That would happen if the new
  table position was already taken and the existing entry could not be
  removed.

  Instead, return true if the new AddressInfo object is successfully
  entered into the new table. This fixes a bug in the "Added %i addresses"
  log, which would not always accurately log how many addresses had been
  added.

ACKs for top commit:
  naumenkogs:
    ACK 61ec0539b2
  mzumsande:
    ACK 61ec0539b2
  shaavan:
    ACK 61ec0539b2

Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb
2021-11-01 10:58:27 +08:00
Hennadii Stepanov
68018e4c3e
test: Avoid excessive locking of cs_wallet 2021-10-31 13:48:00 +02:00