2147483647 is the maximum positive value of a signed int32, and - currently -
the maximum value that the deriveaddresses bitcoin RPC call accepts as
derivation index due to its input validation routines.
Before this change, when the derivation index (and thus range_end) reached
std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which
is declared as int, and as such 32 bits in size on most platforms) would be
incremented at the end of the first iteration and then warp back to
-2147483648. This caused SIGABRT in bitcoind and a core dump.
This change assigns "i" an explicit size of 64 bits on every platform,
sidestepping the problem.
Fixes#26274.
Github-Pull: #26275
Rebased-From: addf9d6502db12cebcc5976df3111cac1a369b82
38d4601fe826069ee43b341e9d467beb7d14b5f0 Prevent data race for `pathHandlers` (Hennadii Stepanov)
Pull request description:
Backport of https://github.com/bitcoin/bitcoin/pull/25983 to the 23.x branch.
ACKs for top commit:
dergoegge:
ACK 38d4601fe826069ee43b341e9d467beb7d14b5f0
Tree-SHA512: b235d6d2cb374baf1b54c09f4cd2feca7b6c1588d081532e987fd5def8ed0dee4b8255112b130a77aca633ec6a63cfd81f215b2e7a403c213eb6048a54774d26
Watchonly wallets do not have any private keys to encrypt. It does not
make sense to encrypt such wallets, so disable the option to encrypt
them.
This avoids an assertion that can be hit when encrypting watchonly descriptor
wallets.
Github-Pull: bitcoin-core/gui#631
Rebased-From: 4c495413e138ec1dd6874e41b44e689f0c15e0e3
If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.
Github-Pull: #25314
Rebased-From: 99b9e5f3a9fa29bbc1e45fc958470fbcc207ef23
7fcac96597218691724002c2b8ecb3dbcf60bf06 rpc: Capture potentially large UniValue by ref for rpcdoccheck (Martin Zumsande)
Pull request description:
ACKs for top commit:
fanquake:
ACK 7fcac96597218691724002c2b8ecb3dbcf60bf06
Tree-SHA512: 7b60d35c84a47ff2b16aed06b4ea82ff3f60cc24409e2927b9ed618f8f939252903417ef93b711171e22b15daf5d857964687fde25ec554153337c996e5e27fa
Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Github-Pull: bitcoin/bitcoin#24711
Rebased-From: 0c12f0116ca802f55f5ab43e6c4842ac403b9889
If the user has unchecked "Allow incoming connections" in
`Settings->Options...->Network` then `fListen=false` is saved in
`~/.config/Bitcoin/Bitcoin-Qt.conf`. This flips `-listen` to `false`
during startup, but leaves `-listenonion` to `true`.
This flipping of `-listen` is done in `OptionsModel::Init()` after
`InitParameterInteraction()` has been executed which would have flipped
`-listenonion`, should it have seen `-listen` being `false`
(this is a difference between `bitcoind` and `bitcoin-qt`).
Fixes: https://github.com/bitcoin-core/gui/issues/567
Github-Pull: bitcoin-core/gui#568
Rebased-From: 7f90dc26c8938f348938929b6d8bf1ea6f149209
Before this change the send confirmation dialog would keep the Send option disabled. The Create Unsigned choice would actually send. This is potentially confusing.
With this change the Create Unsigned button will not attempt to sign and always produce a PSBT. The Send button will attempt to sign, and only return a PSBT if more signatures are needed.
When using an external signer, the Create Unsigned option only appears when PSBT controls are enabled in the wallet settings.
This commit maintains the pre-existing behavior of filling the PSBT (without signing) even when not using an external signer.
Closes#551
Co-authored-by: Jon Atack <jon@atack.com>
Github-Pull: bitcoin-core/gui#555
Rebased-From: 2efdfb88aab6496dcf2b98e0de30635bc6bade85
Does not change behavior.
Review hint:
git show --color-moved --color-moved-ws=allow-indentation-change
Github-Pull: bitcoin-core/gui#555
Rebased-From: 4b5a6cd14967b8ec3cb525e4cb18628de6c15091
This commit does not change behavior.
Review hint:
git show --color-moved --color-moved-ws=allow-indentation-change
Github-Pull: bitcoin-core/gui#555
Rebased-From: 026b5b4523317fdefc69cf5cec55f76f18ad0c0a
This was changed in #22818 from 0 to 1. Since it changes
BLOCK_OPT_WIT of the genesis block, older versions of bitcoin
core would not read regtest directories created with newer versions
without a reindex.
Github-Pull: bitcoin#24527
Rebased-From: 5ce3057
and harmonize them as follows
- s/outgoing/automatic outbound/
- s/Incoming/Inbound and manual/ (are not affected by this option.)
- s/only through network/only to network/
- s/this option. This option/this option. It/
- s/network types/networks/
and also pick up a few nits in doc/p2p-bad-ports.md
Github-Pull: bitcoin#24468
Rebased-From: a1db99a
Fix GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 that happens if
settings.json contains an integer value for any of the configuration
options which GUI settings can currently clash with (-dbcache, -par,
-spendzeroconfchange, -signer, -upnp, -natpmp, -listen, -server, -proxy,
-proxy, -onion, -onion, -lang, and -prune).
Fix is a one-line change in ArgsManager::GetArg.
Github-Pull: bitcoin/bitcoin#24498
Rebased-From: 5b1aae12ca4a99c6b09349981a4902717a6a5d3e
Just add tests. No changes to application behavior. Tests will be
updated in the next commit changing & improving current behavior.
Include a Qt test for GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg
behavior that happens if settings.json contains an integer value for any
of the configuration options which GUI settings can currently clash with
(-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen,
-server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
Github-Pull: bitcoin/bitcoin#24498
Rebased-From: 84b0973e35dae63cd1b60199b481e24d54e58c97
If `-bind=` is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
`GetListenPort()` uses a simple logic: "if `-port=P` is given, then we
must be listening on `P`, otherwise we must be listening on `8333`".
This is however not true if `-bind=` has been provided with `:port` part
or if `-whitebind=` has been provided. Thus, extend `GetListenPort()` to
return the port from `-bind=` or `-whitebind=`, if any.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (cases 1. 2. 3. 5.)
Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
`std::function` variable called `CaptureMessage` whose value can be
changed by unit tests, should they need to inspect message contents.
Add a new function `TestOnlyResetTimeData()` which would reset the
internal state used by `GetTimeOffset()`, `GetAdjustedTime()` and
`AddTimeData()`.
This is needed so that unit tests that call `AddTimeData()` can restore
the state in order not to confuse other tests that rely on it.
Currently `timedata_tests/addtimedata` is the only test that modifies
the state (via `AddTimeData()`) and also the only test that relies on
that state.
5d7f22595ff2de9b9883e468e3ce7182fc3f183b Do not use `LocalTestingSetup` in getarg_tests test file. (Kiminuo)
Pull request description:
Avoid using a test fixture in getarg_tests for better readability. Change was implemented by _kiminuo_ and posted https://github.com/bitcoin/bitcoin/pull/24306#issuecomment-1036643216
ACKs for top commit:
kiminuo:
ACK 5d7f22595ff2de9b9883e468e3ce7182fc3f183b
Tree-SHA512: 0fd98622010e6923e91c66447a1d0861bf344a65d86a313dff7d428c089b1740a25f699327f6ed4c163255f270bcbd4f7be962bb551862214f9b9e395d40df04
b7be28cac50046b9f2ddfe63ecafccc80649a36c test: add combined CJDNS/I2P/localhost/onion eviction protection tests (Jon Atack)
0a1bb84770b403ab5cbd9d5474c76f91ce58e8f6 test: add tests for inbound eviction protection of CJDNS peers (Jon Atack)
0c00c0c981fc0b6cec101e68e8c1aeda1ccf33bb test: fix off-by-one logic in an eviction protection test (Jon Atack)
f7b8094d611531c6b41a94715dbc01f56257ccd2 p2p: extend inbound eviction protection by network to CJDNS peers (Jon Atack)
Pull request description:
Extend inbound eviction protection for peers connected over CJDNS, as is the case for peers connected via onion, localhost, and I2P since #21261 and #20197. CJDNS peers seem to have better min ping latency than onion and I2P peers but still higher than that of unencrypted IPv4/6 peers and can be disadvantaged under our eviction criteria. They are also very few in number, which is a further reason to protect them, as the goal of this logic is to favorise the diversity of our peer connections. CJDNS support was added in #23077 for the upcoming v23 release.
ACKs for top commit:
laanwj:
Concept and code review ACK b7be28cac50046b9f2ddfe63ecafccc80649a36c
w0xlt:
tACK b7be28c
Tree-SHA512: 89ebdd217602e16ae14b9bd0d5a25fc09f9b2384c951f820bc0f5a6d8452bbc9042065db817d5d5296c0ad22988491a83fc5b9a611e660c40ebd4f03448c4061
c4d76c6faa3adf06f192649e169ca860ce420d30 tests: Tests for inactive HD chains (Andrew Chow)
8077862c5e8a3ed501f0baabc33536eb16922ceb wallet: Refactor TopUp to be able to top up inactive chains too (Andrew Chow)
70134eb34f58f0c572e7c3775e292d408f03b5ab wallet: Properly set hd chain counters when loading (Andrew Chow)
961b9e4e40019a87eaa11c8a9c3305870f7a6d75 wallet: Parse hdKeypath if key_origin is not available (Andrew Chow)
0652ee73ec880a66ec88bde007ee03c0b9d1b074 Add size check on meta.key_origin.path (Rob Fielding)
Pull request description:
Currently inactive HD chains are only derived from at the time a key in that chain is found to have been used. However, at that time, the wallet may not be able to derive keys (e.g. it is locked). Currently we would just move on and not derive any new keys, however this could result in missing funds.
This PR resolves this problem by adding memory only variables to `CHDChain` which track the highest known index. `TopUp` is modified to always try to top up the inactive HD chains, and this process will use the new variables to determine how much to top up. In this way, after an encrypted wallet is unlocked, the inactive HD chains will be topped up and hopefully funds will not be missed.
Note that because these variables are not persisted to disk (because `CHDChain`s for inactive HD chains are not written to disk), if an encrypted wallet is not unlocked in the same session as a key from an inactive chain is found to be used, then it will not be topped up later unless more keys are found.
Additionally, wallets which do not have upgraded key metadata will not derive any keys from inactive HD chains. This is resolved by using the derivation path string in `CKeyMetadata.hdKeypath` to determine what indexes to derive.
ACKs for top commit:
laanwj:
Code review ACK c4d76c6faa3adf06f192649e169ca860ce420d30
Tree-SHA512: b2b572ad7f1b1b2847edece09f7583543d63997e18ae32764e5a27ad608dd64b9bdb2d84ea27137894e986a8e82f047a3dba9c8015b74f5f179961911f0c4095
36ee76d1afbb278500fc8aa01606ec933b52c17d net: remove unused CNetAddr::GetHash() (Vasil Dimov)
d0abce9a50dd4f507e3a30348eabffb7552471d5 net: include the port when deciding a relay destination (Vasil Dimov)
2e38a0e6865187d1f0d0f016d3df7cce414a7c4f net: add CServiceHash constructor so the caller can provide the salts (Vasil Dimov)
97208634b96f2d9a55f2ead7b0ef407da729d7bd net: open p2p connections to nodes that listen on non-default ports (Vasil Dimov)
Pull request description:
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
ACKs for top commit:
laanwj:
Concept and light code review ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
prayank23:
ACK 36ee76d1af
stickies-v:
tACK 36ee76d1a
jonatack:
ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
glozow:
utACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
Tree-SHA512: 7f45ab7567c51c19fc50fabbaf84f0cc8883a8eef84272b76435c014c31d89144271d70dd387212cc1114213165d76b4d20a5ddb8dbc958fe7e74e6ddbd56d11
0eea83a85ec6b215d44facc2b16ee1b035275a6b scripted-diff: rename `proxyType` to `Proxy` (Vasil Dimov)
e53a8505dbb6f9deaae8ac82793a4fb760a1e0a6 net: respect -onlynet= when making outbound connections (Vasil Dimov)
Pull request description:
Do not make outbound connections to hosts which belong to a network
which is restricted by `-onlynet`.
This applies to hosts that are automatically chosen to connect to and to
anchors.
This does not apply to hosts given to `-connect`, `-addnode`,
`addnode` RPC, dns seeds, `-seednode`.
Fixes https://github.com/bitcoin/bitcoin/issues/13378
Fixes https://github.com/bitcoin/bitcoin/issues/22647
Supersedes https://github.com/bitcoin/bitcoin/pull/22651
ACKs for top commit:
naumenkogs:
utACK 0eea83a85ec6b215d44facc2b16ee1b035275a6b
prayank23:
reACK 0eea83a85e
jonatack:
ACK 0eea83a85ec6b215d44facc2b16ee1b035275a6b code review, rebased to master, debug built, and did some manual testing with various config options on signet
Tree-SHA512: 37d68b449dd6d2715843fc84d85f48fa2508be40ea105a7f4a28443b318d0b6bd39e3b2ca2a6186f2913836adf08d91038a8b142928e1282130f39ac81aa741b
c7376cc8d728f3a7c40f79bd57e7cef685def723 tests: Test upgrading wallet with privkeys disabled (Andrew Chow)
3d985d4f43b5344f998bcf6db22d02782e647a2a wallet: Don't generate keys when privkeys disabled when upgrading (Andrew Chow)
Pull request description:
When we're upgrading a wallet, we shouldn't be trying to generate new keys for wallets where private keys are disabled.
Fixes#23610
ACKs for top commit:
laanwj:
Code review ACK c7376cc8d728f3a7c40f79bd57e7cef685def723
benthecarman:
tACK c7376cc8d728f3a7c40f79bd57e7cef685def723 this fixed the issue for me
Tree-SHA512: fa07cf37df9196ff98671bb1ce5c9aa0bab46495066b4dab796d7e8e5d5c7adb414ff56adae4fd3e15658a610995bd19a9e1edb00c46144b0df635c5b343f3a6
fa7991601c93761bc12ef33b672a927d48a95569 Fixup style of VerifyDB (MarcoFalke)
fa462ea787d124c56d6ba7ef79a9b5b23f0411c5 Avoid implicit-integer-sign-change in VerifyLoadedChainstate (MarcoFalke)
Pull request description:
This happens when checking all blocks (`-1`).
To test:
```
./configure CC=clang CXX=clang++ --with-sanitizers=undefined,integer
make
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./test/functional/rpc_blockchain.py
ACKs for top commit:
theStack:
Code-review ACK fa7991601c93761bc12ef33b672a927d48a95569
brunoerg:
crACK fa7991601c93761bc12ef33b672a927d48a95569
Tree-SHA512: bcbe6becf2fbedd21bbde83a544122e79465937346802039532143b2e4165784905a8852c0ccb088b964874df5e5550931fdde3629cbcee3ae237f2f63c43a8e
d80dc12097ee2f16e3edfcf39363cc4d240e5c57 net: Update hardcoded seeds for 23.x (laanwj)
9f27157894ee689736d0d0936d1af1620fd8f7d8 contrib: make-seeds updates for 23.x (laanwj)
Pull request description:
Update hardcoded P2P network seeds for 23.x, and update the generation script and documentation as necessary
Tool output:
```
IPv4 IPv6 Onion Pass
469910 72944 0 Initial
469910 72944 0 Skip entries with invalid address
469910 72944 0 After removing duplicates
469909 72944 0 Skip entries from suspicious hosts
165760 65113 0 Enforce minimal number of blocks
160668 63183 0 Require service bit 1
4951 1376 0 Require minimum uptime
4406 1051 0 Require a known and recent user agent
4307 1031 0 Filter out hosts with multiple bitcoin ports
ERR: Could not resolve ASN for "2001:678:7dc:8::2": The DNS query name does not exist: 8.0.0.0.c.d.7.0.8.7.6.0.1.0.0.2.origin6.asn.cymru.com.
512 134 0 Look up ASNs and limit results per ASN and per net
```.
ACKs for top commit:
achow101:
ACK d80dc12097ee2f16e3edfcf39363cc4d240e5c57
jonatack:
ACK d80dc12097ee2f16e3edfcf39363cc4d240e5c57 reviewed the changes and ran the README steps
Tree-SHA512: c651b0501cc28d397cc0778eff6aed4273669082d6ef207ce58ce198b443be66532bf1e8d618ccae3ba671ae4cccfd9b4dd2dfebacc97f3c3bd4e9fa58a3d7a3
