127b4608e9 test: Check if specified config file cannot be opened (nthumann)
6bb54708e6 util: Check if specified config file cannot be opened (nthumann)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22612.
When running e.g. `./src/bitcoind -datadir=/tmp/bitcoin -regtest -conf=/tmp/bitcoin/regtest/bitcoin.conf` and the specified config cannot be opened (doesn't exist, permission denied, ...), the initialization silently uses the default config.
As voidburn already noted:
> I can't think of a situation in which a config file is specified explicitly (in the startup options, as per service unit linked above), but inaccessible, where the fail condition should be to keep booting using defaults instead.
With this patch applied, the initialization will fail immediately, if the specified config file cannot be opened. If no config file is explicitly specified, the behavior is unchanged. This not only affects `bitcoind`, but also `bitcoin-cli` and `bitcoin-qt`.
In the example below the datadir is accessible, but the config file is not due to insufficient permissions:
```
$ ./src/bitcoind -datadir=/tmp/bitcoin -regtest --debug=1 -conf=/tmp/bitcoin/regtest/bitcoin.conf
Error: Error reading configuration file: specified config file "/tmp/bitcoin/regtest/bitcoin.conf" could not be opened.
```
ACKs for top commit:
0xB10C:
ACK 127b4608e9
Zero-1729:
tACK 127b4608e9
theStack:
Tested ACK 127b4608e9
Tree-SHA512: 4fe487921485426f1d1da8d256c388af517b984b639d776aec7b159b3e23b669824093d3bdd31139d9415ed5f5de405b3e6a51b110c8ab471f12b9c99ac67cc1
a3b559c970 test: added test for disabled wallet (Shubhankar Gambhir)
Pull request description:
This PR enables a part of the non-wallet functional test (rpc_signmessage.py) to be run even with the Bitcoin Core wallet disabled, it is inspired by #20078.
Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled that provides extra test which was not performed earlier.
* we need bitcoincore wallet to run rpc_signmessage.py, but it is olny required for signing messages with address and not for signing messages wih private key, so latter one can be in a seperate test which can run without wallet
* verifying message doesn't require wallet, so it can be used in both tests without any problem
* 2 tests are named as wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py to provide clarity of what they are testing.
ACKs for top commit:
vasild:
ACK a3b559c970
theStack:
Code-review ACK a3b559c970
Tree-SHA512: 1bfca3baf3123a02f0a2389e55e141d64430c3bed40ff5a5fb97ef2c66e2853c46e4b2dff62b948eb94dc574cb89d061769330f0535e2d5d1be76b60101136ac
a38137479b net: do not advertise address where nobody is listening (Jadi)
Pull request description:
If the bitcoind starts when listen=0 but listenonion=1, the daemon will
advertise its onion address but nothing is listening for it.
This update will enforce listenonion=0 when the listen is 0.
ACKs for top commit:
vasild:
ACK a38137479b
jarolrod:
ACK a38137479b
amitiuttarwar:
ACK a38137479b
Tree-SHA512: e84a0a9a51f2217edf35d06c6cd9085d1e664452655ba92027195a1e88ba081d157310c84e9709a99ce5d46c94f231477ca2d36f010648b0c8b4f2a737d54e5d
08f57a0057 Assert that IsComplete() in GetSpendData() (Pieter Wuille)
d8f4b976d5 Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator (Pieter Wuille)
c7048aae95 Simplify SignTransaction precomputation loop (Pieter Wuille)
addb9b5a71 Improve comments in taproot signing logic (Pieter Wuille)
Pull request description:
This addresses a few review comments from #21365 that were left at the time of merge (as well as some from #22166 applying to the commit it shared with #21365).
I do not think any are blockers for a 22.0 release.
ACKs for top commit:
theStack:
re-ACK 08f57a0057🌴
Zero-1729:
crACK 08f57a0
jonatack:
Code review ACK 08f57a0057 per `git range-diff e9d6eb1 9336670 08f57a0` followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check
Tree-SHA512: 968750109ba8d6faf3016035a38f81c6aefb724c632a3cab0bbf43cf31b9d187b6b0fddd8772768f57338df11eb07ab9c4c6dacdf3cf35b71f29699c67a301ea
2f0190320d Avoid temporary vectors/uint256s in VerifyTaprootCommitment (Pieter Wuille)
Pull request description:
As XOnlyPubKey has a Span-based constructor, that can be used directly without needing to first convert the byte sequence into a vector, only to convert that to a uint256, which only then can then be passed as a span to the constructor.
Reported by @ roconnor-blockstream.
ACKs for top commit:
Zero-1729:
crACK 2f0190320d
theStack:
re-ACK 2f0190320d
jonatack:
ACK 2f0190320d
Tree-SHA512: f5e809d693cf6f6e899278cd706548eb4341e73b3f7ca8926b5fb50afb2098077d691579aea84fd7db2a7edd76be8e400aa2ed886091ee3416651b8a36efba37
8733a8e84c the result of CWallet::IsHDEnabled() was initialized with true. (Saibato)
Pull request description:
the result of CWallet::IsHDEnabled() was initialized with true.
But in case of no keys or a blank hd wallet the iterator would be skipped
and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only
icon display in GUi when reloading a wallet after closing.
ACKs for top commit:
achow101:
ACK 8733a8e84c
hebasto:
ACK 8733a8e84c
theStack:
utACK 8733a8e84c
meshcollider:
utACK 8733a8e84c
Tree-SHA512: 79b976594f7174d05c29fe3819037ead59aaef27498d95415ceba74d633a8e035f6b03b521000ac3370684a8cb09319d8be1a443ce2d29b3ff4089e399f6b719
But in case of no keys or a blank hd wallet the iterator would be skipped
and not set to false but true, since the loop would be not entered.
That had resulted in a wrong return and subsequent false HD and watch-only
icon display in gui when reloading a wallet after closing.
Update src/wallet/wallet.cpp
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
The us=... message in the debug log when sending a version message is
always [::]:0, because we no longer send our own address there.
Therefore, this information is entirely redundant. Remove it.
Historically, the VERSION message contains an "addrMe" and an "addrYou". As
these are sent before version negotiation is complete, the protocol version is
INIT_PROTO_VERSION (209), and in that protocol, CAddress is serialized without
nTime.
This is in fact the only situation left where a CAddress is (de)serialized
without nTime. As it's such a simple structure (CService for ip/port + uint64_t
for nServices), just inline that structure in the few places where it occurs,
and remove the logic for dealing with missing nTime from CAddress.
021daedfa1 refactor: replace remaining binascii method calls (Zero-1729)
Pull request description:
This PR removes the remaining `binascii` method calls outside `test/functional` and `test_framework`, as pointed out here https://github.com/bitcoin/bitcoin/pull/22619#pullrequestreview-722153458.
Follow-up to #22593 and #22619Closes#22605
ACKs for top commit:
josibake:
re-ACK 021daedfa1
theStack:
re-ACK 021daedfa1
Tree-SHA512: 2ae9fee8917112c91a5406f219ca70f24cd8902b903db5a61fc2de85ad640d669a772f5c05970be0fcee6ef1cdd32fae2ca5d1ec6dc9798b43352c8160ddde6f
As XOnlyPubKey has a Span-based constructor, that can be used directly
without needing to first convert the byte sequence into a vector, only
to convert that to a uint256, which only then can then be passed as a
span to the constructor.
47c48b5f35 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz)
77349713b1 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz)
86dbd54ae8 test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz)
Pull request description:
I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds
I noticed this improvement should probably be made when testing performance/runtimes of https://github.com/bitcoin/bitcoin/pull/22698. But I wanted to separate this out from that PR so the affects of each is decoupled
Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here
ACKs for top commit:
theStack:
Tested ACK 47c48b5f35
Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
fa35efa84b ci: Run arm task on arm64 hardware (MarcoFalke)
Pull request description:
It will still run cross-compilation to armhf, but run the binaries on the hardware itself, not qemu.
There shouldn't be any significant difference, other than maybe a slight speedup.
ACKs for top commit:
hebasto:
re-ACK fa35efa84b
Tree-SHA512: d03bb2b55d0a6b5021243eedff6e143f4fad03a1a8913000c2c5c74665e4bb1c77cb30aa112dbddbc0bb7c645bec18f6b8b8ba91049587b464bbe5ec07dd46bd
5a9e255e5a ci: Run fuzzer task for the master branch only (Hennadii Stepanov)
Pull request description:
https://github.com/bitcoin/bitcoin/pull/22629#issuecomment-896454270:
> I think we need to decide whether running the fuzzer CI in any branch other than master is something we want to be doing / maintaining. This seems pretty unsustainable unless we at least make changes in regards to the fuzz inputs being used by the different branches. I'm pretty sure Marco has mentioned this before.
This PR makes CI ignore fuzz tests by forcing `RUN_FUZZ_TESTS=false` for all cases when it is not the master branch or a PR based on it.
See #22731 as a demo for the 22.x branch.
ACKs for top commit:
MarcoFalke:
cr ACK 5a9e255e5a no opinion on the concept, also didn't test
fanquake:
ACK 5a9e255e5a - didn't test other than to look at #22731.
Tree-SHA512: 48f8f02f1814d4f15293a8804b76d544a08784ea7acd930b5c6d4608564d30aa5a608b1a511386ffda6975feec700c1bbeb86a30a75a7e48a1c5b167a227dbdd
132cae44f2 doc: Mention the flat directory structure for uploads (Andrew Chow)
fb17c99e35 guix: Don't include directory name in SHA256SUMS (Andrew Chow)
Pull request description:
The SHA256SUMS file can be used in a sha256sum -c command to verify downloaded binaries. However users are likely to download just a single file and not place this file in the correct directory relative to the SHA256SUMS file for the simple verification command to work. By not including the directory name in the SHA256SUMS file, it will be easier for users to verify downloaded binaries.
ACKs for top commit:
Zero-1729:
re-ACK 132cae44f2
fanquake:
ACK 132cae44f2
Tree-SHA512: c9ff416b8dfb2f3ceaf4d63afb84aac9fcaefbbf9092f9e095061b472884ec92c7a809e6530c7132a82cfe3ab115a7328e47994a412072e1d4feb26fc502c8c5
4d2fa97031 [addrman] Clean up ctor (John Newbery)
7e6e65918f [addrman] inline Clear() into CAddrMan ctor (John Newbery)
406be5ff96 [addrman] Remove all public uses of CAddrMan.Clear() from the tests (John Newbery)
ed9ba8af08 [tests] Remove CAddrMan.Clear() call from CAddrDB::Read() (John Newbery)
e8e7392311 [addrman] Don't call Clear() if parsing peers.dat fails (John Newbery)
181a1207ba [addrman] Move peers.dat parsing to init.cpp (John Newbery)
Pull request description:
`CAddrMan::Clear()` exists to reset the internal state of `CAddrMan`. It's currently used in two places:
- on startup, if deserializing peers.dat fails, `Clear()` is called to reset to an empty addrman
- in tests, `Clear()` is called to reset the addrman for more tests
In both cases, we can simply destruct the `CAddrMan` and construct a new, empty addrman. That approach is safer - it's possible that `Clear()` could 'reset' the addrman to a state that's not equivalent to a freshly constructed addrman (one actual example of this is that `Clear()` does not clear the `m_tried_collisions` set). On the other hand, if we destruct and then construct a fresh addrman, we're guaranteed that the new object is empty.
This wasn't possible when addrman was initially implemented, since it was a global, and so it would only be destructed on shutdown. However, addrman is now owned by `node.context`, so we have control over its destruction/construction.
ACKs for top commit:
laanwj:
Code review ACK 4d2fa97031
vasild:
ACK 4d2fa97031
Zero-1729:
crACK 4d2fa97031
Tree-SHA512: f715bf2cbff4f8c3a9dbc613f8c7f11846b065d6807faf3c7d346a0b0b29cbe7ce1dc0509465c2c9b88a8ad55299c9182ea53f5f743e47502a69a0f375e09408
The uploaded binaries need to match the same flat directory structure of
the SHA256SUMS file in order for torrent downloaders to be able to
verify the download without moving files. Mention this in the release
process doc.
8dcbbbea64 test: fix bug in 22686 (S3RK)
Pull request description:
It seems there is a bug in the test in #22686, the code behaviour itself looks correct.
Instead of verifying the scenario from #22670 with both `upper_bound` and `lower_bound` for the transaction amount, the tests verified `lower_bound` two times. This fix is to properly use function parameter instead of a variable from the scope. The test still passes with both values, so no code changes are required.
ACKs for top commit:
achow101:
ACK 8dcbbbea64
Tree-SHA512: 82837b2e00bd075a7b6b7a31031f4d3ba1ab4bd5967e90488f527fcc618aeb3945d6ae3ed66b9eb11616dda3ef376c5001559d2a7a79a759fc6800358bf52537
f52a72af56 ci: Invalidate depends caches when sources have been changed (Hennadii Stepanov)
939640f87e ci: Reorder scripts to make git available before depends_sources_cache (Hennadii Stepanov)
Pull request description:
On master (502d22ceed) the Cirrus CI do _not_ invalidate depends caches when their sources has been changed, i.e. source version updates, a new dependency etc.
This behavior causes:
- breaking CI build for Android APK (see #22708)
- the cache sizes growing
It is assumed that caches could be invalidated via Cirrus CI API/GUI, but it is inconvenient and it does not work as expected in all cases.
The common part of `fingerprint_key`s for both `depends_sources_cache` and `depends_built_cache` is the recent commit that changes sources in the `src/depends` sub-directory.
For `depends_built_cache` additionally `CIRRUS_TASK_NAME` is used to avoid sharing of the same cache between tasks with different OSes and different `DEP_OPTS`.
---
The `depends_sources` cache:
- in master (502d22ceed)
- with this PR
ACKs for top commit:
MarcoFalke:
cr ACK f52a72af56
Zero-1729:
crACK f52a72af56
Tree-SHA512: 0d14814ff308317c7c18cc725cf69055139030c207200af003872068f5567bd4b9c1f6783845d11742e314ebcc1dc5cae16aa86475376b623d921af5fcda14dd
Clear() is now only called from the ctor, so just inline the code into
that function.
The LOCK(cs) can be removed, since there can be no data races in the ctor.
Also move the function definition out of the header and into the cpp file.
Just use unique_ptr<CAddrMan>s and reset the pointer if a frest addrman is required.
Also make CAddrMan::Clear() private to ensure that no call sites are missed.
1ea11e10ac doc: link to managing-wallets from doc readme (fanquake)
Pull request description:
This was forgotten in #22523.
ACKs for top commit:
achow101:
ACK 1ea11e10ac
jarolrod:
ACK 1ea11e10ac
Tree-SHA512: b82664b282cc0fe733b752c011621593df0f846d2188f12dbc5fedb7ffed2bd161293ce2a369ca973926030795b5f7acde7a1cbf5e337042a6f665906069c656
cd37356ff9 [crypto] Fix K1/K2 use in ChaCha20-Poly1305 AEAD (Dhruv Mehta)
Pull request description:
BIP324 mentions K1 is used for the associated data and K2 is used for the payload. The code does the opposite. This is not a security problem but will be a problem across implementations based on the HKDF key derivations.
BIP324 author Jonas Schnelli thinks a [code update will be better](https://github.com/bitcoin/bitcoin/pull/15649#discussion_r440780669) than a BIP update.
If this PR is merged:
- [ ] We need to update the test vector 3 in BIP324
ACKs for top commit:
jonasschnelli:
utACK cd37356ff9
Tree-SHA512: e2165117bfbf7a031060e7376912f9af1c1bfc57916383799a0fa2c040e2caaab0d6aafc3425c083a233b96c84fafec75c938e00ceb6bd7d52607d58607cb145
The SHA256SUMS file can be used in a sha256sum -c command to verify
downloaded binaries. However users are likely to download just a single
file and not place this file in the correct directory relative to the
SHA256SUMS file for the simple verification command to work. By not
including the directory name in the SHA256SUMS file, it will be easier
for users to verify downloaded binaries.
Co-authored-by: Carl Dong <contact@carldong.me>
f12fbad5a1 windres: use PACKAGE_VERSION rather than building more version numbers (fanquake)
Pull request description:
Rather than defining more strings, reuse PACKAGE_VERSION, which is already available.
We also already use PACKAGE_VERSION for `ProductVersion` and `FileVersion` in setup.nsi.
ACKs for top commit:
MarcoFalke:
cr ACK f12fbad5a1
laanwj:
Code review ACK f12fbad5a1
Tree-SHA512: b74a37cbba105d208d4da9264d295d7e052009fdd6b0ed54a0d9968bbe2deeba1766d6d310438b2939a81555faa0cbd67d5e53f0c8a2de669ce56353c1c67d22
