9b85a5e2f7 tests: Test for dumpwallet lock order issue (Andrew Chow)
25d99e6511 Reorder dumpwallet so that cs_main functions go first (Andrew Chow)
Pull request description:
When a wallet is loaded which has an unconfirmed transaction in the mempool, it will end up establishing the lock order of cs_wallet -> cs_main -> cs_KeyStore. If `dumpwallet` is used on this wallet, then a lock order of cs_wallet -> cs_KeyStore -> cs_main will be used, which causes a lock order assertion. This PR fixes this by reordering `dumpwallet` and `GetKeyBirthTimes` (only used by `dumpwallet`). Specifically, in both functions, the function calls which lock cs_main are done prior to locking cs_KeyStore. This avoids the lock order issue.
Additionally, I have added a test case to `wallet_dump.py`. Of course testing this requires `--enable-debug`.
Fixes#22489
ACKs for top commit:
MarcoFalke:
review ACK 9b85a5e2f7🎰
ryanofsky:
Code review ACK 9b85a5e2f7. Nice to reduce lock scope, and good test!
prayank23:
tACK 9b85a5e2f7
lsilva01:
Tested ACK 9b85a5e2f7 under the same conditions reported in issue #22489 and the `dumpwallet` command completed successfully.
Tree-SHA512: d370a8f415ad64ee6a538ff419155837bcdbb167e3831b06572562289239028c6b46d80b23d227286afe875d9351f3377574ed831549ea426fb926af0e19c755
5a77abd4e6 [style] Clean up BroadcastTransaction() (John Newbery)
7282d4c036 [test] Allow rebroadcast for same-txid-different-wtxid transactions (glozow)
cd48372b67 [mempool] Allow rebroadcast for same-txid-different-wtxid transactions (John Newbery)
847b6ed48d [test] Test transactions are not re-added to unbroadcast set (Duncan Dean)
2837a9f1ea [mempool] Only add a transaction to the unbroadcast set when it's added to the mempool (John Newbery)
Pull request description:
1. Only add a transaction to the unbroadcast set when it's added to the mempool
Currently, if BroadcastTransaction() is called to rebroadcast a
transaction (e.g. by ResendWalletTransactions()), then we add the
transaction to the unbroadcast set. That transaction has already been
broadcast in the past, so peers are unlikely to request it again,
meaning RemoveUnbroadcastTx() won't be called and it won't be removed
from m_unbroadcast_txids.
Net processing will therefore continue to attempt rebroadcast for the
transaction every 10-15 minutes. This will most likely continue until
the node connects to a new peer which hasn't yet seen the transaction
(or perhaps indefinitely).
Fix by only adding the transaction to the broadcast set when it's added to the mempool.
2. Allow rebroadcast for same-txid-different-wtxid transactions
There is some slightly unexpected behaviour when:
- there is already transaction in the mempool (the "mempool tx")
- BroadcastTransaction() is called for a transaction with the same txid
as the mempool transaction but a different witness (the "new tx")
Prior to this commit, if BroadcastTransaction() is called with
relay=true, then it'll call RelayTransaction() using the txid/wtxid of
the new tx, not the txid/wtxid of the mempool tx. For wtxid relay peers,
in SendMessages(), the wtxid of the new tx will be taken from
setInventoryTxToSend, but will then be filtered out from the vector of
wtxids to announce, since m_mempool.info() won't find the transaction
(the mempool contains the mempool tx, which has a different wtxid from
the new tx).
Fix this by calling RelayTransaction() with the wtxid of the mempool
transaction in this case.
The third commit is a comment/whitespace only change to tidy up the BroadcastTransaction() function.
ACKs for top commit:
duncandean:
reACK 5a77abd
naumenkogs:
ACK 5a77abd4e6
theStack:
re-ACK 5a77abd4e6
lsilva01:
re-ACK 5a77abd4e6
Tree-SHA512: d1a46d32a9f975220e5b432ff6633fac9be01ea41925b4958395b8d641680500dc44476b12d18852e5b674d2d87e4d0160b4483e45d3d149176bdff9f4dc8516
5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 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 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
Now that m_recent_confirmed_transactions is owned by PeerManagerImpl,
and PeerManagerImpl's lifetime is managed by the node context, we can
just default initialize m_recent_confirmed_transactions during object
initialization. We can also remove the unique_ptr indirection.
Now that recentRejects is owned by PeerManagerImpl, and
PeerManagerImpl's lifetime is managed by the node context, we can just
default initialize recentRejects during object initialization. We can
also remove the unique_ptr indirection.
Instead of deserializing addresses, placing them in the buckets, and
then removing them if they're invalid, check first and don't place in
the buckets if they're invalid.
54c7754f31 build: use aarch64 Clang if cross-compiling for darwin on aarch64 (fanquake)
Pull request description:
If we're cross-compiling for darwin on aarch64 hardware, we need to
use a Clang that will run on that hardware.
Only tested in a Linux Docker container (aarch64-unknown-linux-gnu),
running on an Apple M1 mac-mini (aarch64-apple-darwin20.5.0).
ACKs for top commit:
hebasto:
ACK 54c7754f31, I agree it can be merged (fix in #22448 is orthogonal to this one).
Tree-SHA512: 66c530097a5dc072a0a00dc22eb3d4a7d923dfa8ab8160f7c3e395cbe58da324f367548d673c0510606f5225d5d37bb5607a76b1703b8b03ac7d2cceeccbd542
e6a94d4446 guix: Bump to version-1.3.0 from upstream (Carl Dong)
90fd13b954 guix: Pin kernel header version (Carl Dong)
Pull request description:
```
- Use 4.19 for riscv64 (earliest LTS release w/ riscv64 support)
- Use 4.9 for all others (second-oldest LTS release, released in
combination with glibc glibc 2.24 in Debian stretch)
```
```
The chosen commit is the HEAD of Guix's version-1.3.0 branch as of July
15th, 2021.
Also fix visual indenting.
```
-----
This + the documentation PR should make our Guix system ready for release!
ACKs for top commit:
MarcoFalke:
review ACK e6a94d4446 to change to vanilla guix. Did not review the kernel change.
laanwj:
ACK e6a94d4446
fanquake:
ACK e6a94d4446
Tree-SHA512: a175e4ddb3ee786a39f5e800ce336932ad2f6797a3a28400a6f723875d0f19833fd36cedc41b3580e4604110517211bd9f557be36adf7265fd8e591c434ae032
0a5723beea macdeploy: cleanup .temp.dmg if present (fanquake)
ecffe8689d macdeploy: remove qt4 related code (fanquake)
639f064253 macdeploy: select the plugins we need, rather than excluding those we don't (fanquake)
3d26b6b9e9 macdeploy: fix framework printing when passing -verbose (fanquake)
dca6c90329 macdeploy: remove unused plistlib import (fanquake)
Pull request description:
This includes [one followup](https://github.com/bitcoin/bitcoin/pull/20422#discussion_r534207899) and [one bug fix](3d26b6b9e9) from #20422, as well as some simplifications to the `macdeployqtplus` code.
ACKs for top commit:
hebasto:
ACK 0a5723beea, tested on macOS Big Sur 11.4 (20F71, x86_64) + Homebrew's Qt 5.15.2.
Tree-SHA512: cfad9505eacd32fe3a9d06eb13b2de0b6d2cad7b17778e90b503501cbf922e53d4e7f7f74952d1aed58410bdae9b0bb3248098583ef5b85689cb27d4dc06c029
fac4814106 doc/release-process: Add torrent creation details (Carl Dong)
5d24cc3d82 guix/INSTALL: Guix installs init scripts in libdir (Carl Dong)
5da2ee49d5 guix/INSTALL: Add coreutils/inotify-dir-recreate troubleshooting (Carl Dong)
318c60700b guix: Adapt release-process.md to new Guix process (Carl Dong)
fcab35b229 guix-attest: Produce and sign normalized documents (Carl Dong)
c2541fd0ca guix: Overhaul README (Carl Dong)
46ce6ce378 tree-wide: Rename gitian-keys to builder-keys (Carl Dong)
fc4f8449f3 guix: Update various check_tools lists (Carl Dong)
263220a85c guix: Check for a sane services database (Carl Dong)
Pull request description:
Based on: #21462
Keeping the README in one file so that it's easy to search through. Will add more jumping links later so navigation is easier.
Current TODOs:
- [x] Shell installer option: prompt user to re-login for `/etc/profile.d` entry to be picked up
- [x] Binary tarball option: prompt user to create `/etc/profile.d` entry and re-login
- [x] Fanquake docker option: complete section
- [x] Arch Linux AUR option: prompt to start `guix-daemon-latest` unit after finishing "optional setup" section
- [x] Building from source option: Insert dependency tree diagram that I made
- [x] Building from source option: redo sectioning, kind of a mess right now
- [x] Optional setup: make clear which parts are only needed if building from source
- [x] Workaround 1 for GnuTLS: perhaps mention how to remove Guix build farm's key
- [x] Overall (after everything): Make the links work.
Note to self: wherever possible, tell user how to check that something is true rather than branching by installation option.
ACKs for top commit:
fanquake:
ACK fac4814106 - going to go ahead and merge this now. It's a lot of documentation, and could probably be nit-picked / improved further, however, that can continue over the next few weeks. I'm sure more (backportable) improvements / clarifications will be made while we progress through RCs towards a new release.
Tree-SHA512: dc46c0ecdfc67c7c7743ca26e4a603eb3f54adbf81be2f4c1f4c20577ebb84b5250b9c9ec89c0e9860337ab1c7cff94d7963c603287267deecfe1cd987fa070a
facd56750c scripted-diff: Revert "fuzz: Add Temporary debug assert for oss-fuzz issue" (MarcoFalke)
Pull request description:
No longer needed, as it wouldn't help to debug this issue. See https://github.com/bitcoin/bitcoin/pull/22472#issuecomment-882692900
ACKs for top commit:
fanquake:
ACK facd56750c
Tree-SHA512: 13352b3529c43d6e65ab127134b32158d3072dc2fbbb326fea9adfeada5a8610d0477ea75748b8b68e7abb3b9869a989df3a3169e92bdd458053d64bae6ed379
That way we can easily combine the document and detached signature to
produce cleartext signature files for upload during the release process.
See subsequent commits which modify doc/release-process.md for more
details.
Adds a test for the condition which can trigger a lock order assertion.
Specifically, there must be an unconfirmed transaction in the mempool
which belongs to the wallet being loaded. This will establish the order
of cs_wallet -> cs_main -> cs_KeyStore. Then dumpwallet is called on
that wallet. Previously, this would have used a lock order of cs_wallet
-> cs_KeyStore -> cs_main, but this should be fixed now. The test
ensures that.
DEBUG_LOCKORDER expects cs_wallet, cs_main, and cs_KeyStore to be
acquired in that order. However dumpwallet would take these in the order
cs_wallet, cs_KeyStore, cs_main. So when configured with
`--enable-debug`, it is possible to hit the lock order assertion when
using dumpwallet.
To fix this, cs_wallet and cs_KeyStore are no longer locked at the same
time. Instead cs_wallet will be locked first. Then the functions which
lock cs_main will be run. Lastly cs_KeyStore will be locked afterwards.
This avoids the lock order issue.
Furthermore, since GetKeyBirthTimes (only used by dumpwallet) also uses
a function that locks cs_main, and itself also locks cs_KeyStore, the
same reordering is done here.
816f29eab2 addrman: detect on-disk corrupted nNew and nTried during unserialization (Vasil Dimov)
Pull request description:
Negative `nNew` or `nTried` are not possible during normal operation.
So, if we read such values during unserialize, report addrman
corruption.
Fixes https://github.com/bitcoin/bitcoin/issues/22450
ACKs for top commit:
MarcoFalke:
cr ACK 816f29eab2
jonatack:
ACK 816f29eab2
lsilva01:
Code Review ACK 816f29eab2. This change provides a more accurate description of the error.
Tree-SHA512: 01bdd72d2d86a0ef770a319fee995fd1e147b24a8db84ddb8cd121688e7f94fed73fddc0084758e7183c4f8d08e971f0b1b224f5adb10928a5aa4dbbc8709d74
a4bcd687c9 Improve tests using statistics (John Newbery)
f424d601e1 Add logging and addr rate limiting statistics (Pieter Wuille)
b4ece8a1cd Functional tests for addr rate limiting (Pieter Wuille)
5648138f59 Randomize the order of addr processing (Pieter Wuille)
0d64b8f709 Rate limit the processing of incoming addr messages (Pieter Wuille)
Pull request description:
The rate at which IP addresses are rumoured (through ADDR and ADDRV2 messages) on the network seems to vary from 0 for some non-participating nodes, to 0.005-0.025 addr/s for recent Bitcoin Core nodes. However, the current codebase will happily accept and process an effectively unbounded rate from attackers. There are measures to limit the influence attackers can have on the addrman database (bucket restrictions based on source IPs), but still - there is no need to permit them to feed us addresses at a rate that's orders of magnitude larger than what is common on the network today, especially as it will cause us to spam our peers too.
This PR implements a [token bucket](https://en.wikipedia.org/wiki/Token_bucket) based rate limiter, allowing an average of 0.1 addr/s per connection, with bursts up to 1000 addresses at once. Whitelisted peers as well as responses to GETADDR requests are exempt from the limit. New connections start with 1 token, so as to not interfere with the common practice of peers' self-announcement.
ACKs for top commit:
laanwj:
ACK a4bcd687c9
vasild:
ACK a4bcd687c9
jnewbery:
ACK a4bcd687c9
jonatack:
ACK a4bcd687c9
Tree-SHA512: b757de76ad78a53035b622944c4213b29b3b55d3d98bf23585afa84bfba10808299d858649f92269a16abfa75eb4366ea047eae3216f7e2f6d3c455782a16bea
8465978f23 Make IsSegWitOutput return true for taproot outputs (Pieter Wuille)
Pull request description:
This fixes a bug: currently `utxoupdatepsbt` will not fill in UTXO data for PSBTs spending taproot outputs.
ACKs for top commit:
achow101:
Code Review ACK 8465978f23
jonatack:
ACK 8465978f23
meshcollider:
utACK 8465978f23
Tree-SHA512: 2f8f873450bef4b5a4ce5962a231297b386c6b1445e69ce5f36ab28eca7343be3a11bc09c38534b0f75e6f99ba15d78d3ba5d484f6c63e5a9775e1f3f55a74e0
a2aca207b1 Move implementations of non-template fuzz helpers (Sriram)
Pull request description:
There are 78 cpp files that include `util.h` (`grep -iIr "#include <test/fuzz/util.h>" src/test/fuzz | wc -l`). Modifying the implementation of a fuzz helper in `src/test/fuzz/util.h` will cause all fuzz tests to be recompiled. Keeping the declarations of these non-template fuzz helpers in `util.h` and moving their implementations to `util.cpp` will skip the redundant recompilation of all the fuzz tests, and builds these helpers only once in `util.cpp`.
Functions moved from `util.h` to `util.cpp`:
- `ConsumeTxMemPoolEntry`
- `ContainsSpentInput`
- `ConsumeNetAddr`
- Methods of `FuzzedFileProvider::(open, read, write, seek, close)`
ACKs for top commit:
MarcoFalke:
review ACK a2aca207b1🍂
Tree-SHA512: e7037ebb86d0fc56048e4f3d8733eefc21da11683b09d2b22926bda410719628d89c52ddd9b4c18aa243607a66fdb4d13a63e62ca010e66b3ec9174fd18107f0
5012a7912e Test that descriptor wallet upgrade does nothing (Andrew Chow)
48bd7d3b77 Change ScriptPubKeyMan::Upgrade to default to return true (Andrew Chow)
Pull request description:
When adding a new ScriptPubKeyMan, it's likely that there will be nothing for `Upgrade` to do. If it is called (via `upgradewallet`), then it should do nothing, successfully. This PR changes the default `ScriptPubKeyMan::Upgrade` function so that it returns a success instead of failure when doing nothing.
Fixes#22460
ACKs for top commit:
jonatack:
ACK 5012a7912e
meshcollider:
utACK 5012a7912e
Tree-SHA512: 578c6521e997f7bb5cc44be2cfe9e0a760b6bd4aa301026a6b8b3282e8757473e4cb9f68b2e79dacdc2b42dddae718450072e0a38817df205dfea177a74d7f3d
1edddf5de4 Avoid GCC 7.1 ABI change warning in guix build (Pieter Wuille)
Pull request description:
The arm-linux-gnueabihf guix build output is littered with warnings like:
```
/gnu/store/7a96hdqdb2qi8a39f09n84xjy2hr23rs-gcc-cross-arm-linux-gnueabihf-8.4.0/include/c++/bits/stl_vector.h:1085:4: note:
parameter passing for argument of type '__gnu_cxx::__normal_iterator<CRecipient*, std::vector<CRecipient> >' changed in GCC 7.1
```
These are irrelevant for us. Disable them using `-Wno-psabi`.
ACKs for top commit:
laanwj:
ACK 1edddf5de4
hebasto:
ACK 1edddf5de4, after thorough reading related materials, I agree this change can be merged. As I mentioned above, I have been compiling my arm-32bit binaries with `-Wno-psabi` flag for two years, and no related flaws were observed.
Tree-SHA512: 485c7500547ac5da567ad23847341c18ff832607f5a1002676404cc647e437cf3445b6894ecff5b52929ca52bea946c06bd90eace1997c895e56204e787065e4
1155978d8f build, qt: Do not install *.prl files (Hennadii Stepanov)
763793b60e build, qt: Fix wrong cross-compiling detection on macOS (Hennadii Stepanov)
30982721ab build, qt: Force bootstrap while building linguist tools (Hennadii Stepanov)
689320e307 build, qt: Drop translations.pro hack (Hennadii Stepanov)
6a1f98f253 build, qt: Drop lrelease dependency patch (Hennadii Stepanov)
39e561e087 build, qt: Add linguist_tools list (Hennadii Stepanov)
27d3def1c6 build: Use Qt top-level build facilities (Hennadii Stepanov)
Pull request description:
This PR:
- uses Qt top-level build facilities without the need to download all-in-one archive
- is based on **BlockMechanic**'s [idea](https://github.com/bitcoin/bitcoin/pull/20600), and is an alternative to #20600
- makes it easy to integrate [new modules](https://github.com/bitcoin/bitcoin/pull/16883) into static builds
- has the minimal diff
- makes the qt package build process streamlined by dropping some patches and hacks (an alternative to #21420 and #20642)
Fixes#18536 (a non-intrusive alternative to #21589 and #19785).
Fixes#14648.
Fixes#21588 (a non-intrusive alternative to #21591).
Required for adding [Wayland support](https://github.com/bitcoin/bitcoin/issues/19950) on Linux.
---
**Note for reviewers**: With 9046de8a4cbc3899fed9eae084115f423e7ac5bd from #21995 it is easy to verify that there are no changes in the resulted `qt` package archive on the per commit basis. For example, for `HOST=i686-pc-linux-gnu` no commit in this PR introduces any changes.
ACKs for top commit:
fanquake:
ACK 1155978d8f
Tree-SHA512: 667b06b72cb7ff26d68b9b88e8dddb51084783ca9e3d80b3392710794c1dc7fd77bbcc3ccf4ccece9919d33b9bf8fce13c5059502bd228021dc7c93fdb87ca7a
fb7be92b09 Mark print-% target as phony. (Dmitry Goncharov)
Pull request description:
.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on force and mark force as phony.
This change ensures print-% rule works even when there is a file that matches the target.
```
$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch force
$ make print-host
host=x86_64-pc-linux-gnu
```
ACKs for top commit:
hebasto:
ACK fb7be92b09, tested on Linux Mint 20.2 (x86_64).
Tree-SHA512: b89ae66aa8c7aa6a7ab5f0956f9eb3b3ef9d56994b60dc2a97d498d4c1bba537845c190723e8a10310280b1b35df2cd935cc30aeb76735cac2dc621ad7823772
3c4c8e79ba build: Add -Werror=implicit-fallthrough compile flag (Hennadii Stepanov)
014110c47d Use C++17 [[fallthrough]] attribute, and drop -Wno-implicit-fallthrough (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
fanquake:
ACK 3c4c8e79ba - looks ok to me now. Checked that warnings occur in our code & leveldb by removing a `[[fallthrough]]` or `FALLTHROUGH_INTENDED`.
jarolrod:
ACK 3c4c8e79ba
theStack:
ACK 3c4c8e79ba
Tree-SHA512: 4dce91f0f26b8a3de09bd92bb3d7e1995e078e3a8b3ff861c4fbf6c0b32b2327d063633b07b89c4aa94a1141d7f78d46d9d43ab8df865273e342693ad30645b6
- Use 4.19 for riscv64 (earliest LTS release w/ riscv64 support)
- Use 4.9 for all others (second-oldest LTS release, released in
combination with glibc glibc 2.24 in Debian stretch)
-> remove unneeded get-out-of IBD generate()
(The test framework already sets up the nodes to be out of IBD
in setup_nodes(), if setup_clean_chain is not set to True)
-> remove duplicate code line assigning an utxo
MarcoFalke