`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.
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
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
d41ed3215355582879c8eb6c99c2da33852f6cb1 p2p: Avoid InitError when downgrading peers.dat (junderw)
Pull request description:
fixes#24188 (also see https://github.com/bitcoin/bitcoin/pull/22762#issuecomment-951063826)
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded Bitcoin Core version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one.
ACKs for top commit:
prayank23:
reACK d41ed32153
kallewoof:
reACK d41ed3215355582879c8eb6c99c2da33852f6cb1
Tree-SHA512: c8e625fe36ce0b1aab6c8ef7241c8954038bb856f2de27bdc4814dc9a60e51be28815c7d77d0f96eace49687a0cea02deb713978bbd3a5add742f50a675f2a40
fixes#24188
When downgrading, a peers.dat with a future version that has a minimum
required version larger than the downgraded version would cause an InitError.
This commit changes this behavior to overwrite the existing peers.dat with
a new empty one, while creating a backup in peers.dat.bak.
fad7ddf9e3710405d727f61d8200d5efed1e705b test: Run symlink regression tests on Windows (MarcoFalke)
Pull request description:
Seems odd to add tests, but not run them on the platform that needs them most.
ACKs for top commit:
laanwj:
Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b
ryanofsky:
Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b, just removing new test. Would be nice if the test could be added later, of course.
Tree-SHA512: 64b235967a38c2eb90657e8d7a0447bcc8ce81d1b75a275b6c48bd42efd9ea7e7939257e484f297ee84598def3738eaeb289561aeba1dd6a99b258d389995139
Given the low possibility of a reorg reverting the segwit soft fork,
there is no need to check whether segwit is active here. Also,
TestBlockValidity is run on the block template after it has been
created.
5a89bed410d724360b8f90bd9d7d28d6e62331c0 contrib: address gen-manpages feedback from #24263 (fanquake)
2618fb8d15d01dca967856c92ebf3e4cc09699a2 Output license info when binaries are passed -version (fanquake)
4c3e3c57463b029d335e685d3dcdaf26456666cf refactor: shift CopyrightHolders() and LicenseInfo() to clientversion.cpp (fanquake)
Pull request description:
Addresses a review comment from #24263, and addresses the [comment](https://github.com/bitcoin/bitcoin/pull/24263#issuecomment-1030582925) where it was pointed out that we are inconsistent with emitting our copyright. After this change, the copyright is always emitted with `-version`, rather than `-help`, i.e:
```bash
bitcoind -version
Bitcoin Core version v22.99.0-fc1f355913f6-dirty
Copyright (C) 2009-2022 The Bitcoin Core developers
Please contribute if you find Bitcoin Core useful. Visit
<https://bitcoincore.org/> for further information about the software.
The source code is available from <https://github.com/bitcoin/bitcoin>.
This is experimental software.
Distributed under the MIT software license, see the accompanying file COPYING
or <https://opensource.org/licenses/MIT>
```
The info is also added to binaries other than `bitcoind`/`bitcoin-qt`. This change also prevents duplicate copyright info appearing in the `bitcoind` man page.
ACKs for top commit:
laanwj:
Tested ACK 5a89bed410d724360b8f90bd9d7d28d6e62331c0
Tree-SHA512: 0ac2a1adf9e9de0c3206f35837008e3f93eaf15b193736203d71609273f0887cca20b8a90972cb9f941ebd62b330d61a0cbb5fb1b7a7f2dbc715ed8a0c1569d9
48742693acc9de837735674057c9aae2fe90bd1d Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd43441ecb6e5978d65348501c57d856030 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes#24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693acc9de837735674057c9aae2fe90bd1d
hebasto:
re-ACK 48742693acc9de837735674057c9aae2fe90bd1d, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
For testing the USDT tracepoint API in the functional tests we
require:
- that we are on a Linux system*
- that Bitcoin Core is compiled with tracepoints
- that bcc and the the Python bcc module [0] is installed
- that we run the tests with the required permissions**
otherwise we skip the tests.
*: We currently only support tracepoints on Linux. Tracepoints are
not compiled on other platforms.
**: Currently, we check for root permissions via getuid == 0. It's
unclear if it's even possible to run the tests a non-root user
with e.g. CAP_BPF, CAP_PERFMON, and access to /sys/kernel/debug/
tracing/. Anyone running these tests as root should carefully
review them first and then run them in a disposable VM.
[0]: https://github.com/iovisor/bcc/blob/master/INSTALL.md
bfcd60f5d505334230013de4115483b22a7898ee test: activate all index types in feature_init.py (Martin Zumsande)
0243907faee0aa6af09974131d9a46a7f9c3ef38 index: Don't commit without valid m_best_block_index (Martin Zumsande)
Pull request description:
When an index thread receives an interrupt during init before it got to index anything (so `m_best_block_index == nullptr` still), it will still try to commit previous "work" before stopping the thread. That means that `BaseIndex::CommitInternal()` calls `GetLocator(nullptr)`, which returns an locator to the tip ([code](06b6369766/src/chain.cpp (L31-L32))), and saves it to the index DB.
On the next startup, this locator will be read and it will be assumed that we have successfully synced the index to the tip, when in reality we have indexed nothing.
In the case of coinstatsindex, this would lead to a shutdown of bitcoind without any indication what went wrong. For the other indexes, there would be no immediate shutdown, but the index would be corrupt.
This PR fixes this by not committing when `m_best_block_index==nullptr`, and it also adds an error log message to the silent coinstatsindex shutdown path.
This is another small bug found by `feature_init.py` - the second commit enables blockfilterindex and coinstatsindex for this test, enabling coinstatsindex without the first commit would have led to frequent failures.
ACKs for top commit:
fjahr:
reACK bfcd60f5d505334230013de4115483b22a7898ee
shaavan:
reACK bfcd60f5d505334230013de4115483b22a7898ee
Tree-SHA512: 8e2bac0fc40cde209518a9e59b597ae0a5a875a2a90898673987c91733718d40e528dada942bf552b58bc021bf46e59da2d0cc5a61045f48f9bae2b1baf6033b
f11dad22a506e10fbbfbcb6ccf32754bf8e72b72 test: refactor: remove unneeded bytes<->hex conversions in `byte_to_base58` (Sebastian Falbesoner)
Pull request description:
It seems like the only reason for using hex strings in this method was to have a convenient way to convert to an integer from the input data interpreted as big-endian. In Python3 we have `int.from_bytes(..., 'big')` for that purpose, hence there is no need for that anymore and we can simply operate on bytes only.
ACKs for top commit:
laanwj:
Code review ACK f11dad22a506e10fbbfbcb6ccf32754bf8e72b72
Tree-SHA512: 9b1563010066ca74d85139c3b9259e9a5bb49e1f141c30b6506a0445afddb2bde7fd421fdd917dc516956e66f93610e2c21d720817640daee8f57f803be76ee4
It seems like the only reason for using hex strings in this
method was to have a convenient way to convert to an integer
from the input data interpreted as big-endian.
In Python3 we have `int.from_bytes(..., 'big')` for that
purpose, hence there is no need for that anymore and we can
simply operate on bytes only.
5d399f9f3df513a0400049238f5ef0ef2352d57e build: remove native B2 package (fanquake)
2037a3b6c1222d2802ff7c8463f2bb79ba8b57d8 build: header-only Boost (fanquake)
39e66e938fb688f5400ad94a1b317fcc2a87bc31 build: use header-only Boost unit test (fanquake)
Pull request description:
This PR converts our Boost usage to header only. We switch from using our last remaining Boost lib (unit test), to using it's header-only implementation (see https://www.boost.org/doc/libs/1_78_0/libs/test/doc/html/boost_test/adv_scenarios/single_header_customizations/multiple_translation_units.html).
Also related to #24291.
Guix build:
```bash
```
ACKs for top commit:
hebasto:
re-ACK 5d399f9f3df513a0400049238f5ef0ef2352d57e
MarcoFalke:
approach ACK 5d399f9f3df513a0400049238f5ef0ef2352d57e 📞
Tree-SHA512: e60835ee9c11aa941a64679616da2002d6cd86e464895372fafdd42ad6499d7eb1dde6f0013c60adaeb97bd191198430cb158a7a7417b38080dd7106b28e3ba5
fa6065661a86656a29e89ed1a3529cb7103f5394 refactor: Avoid unsigned integer overflow in core_write (MarcoFalke)
Pull request description:
Also, I find the new code a bit easier to understand.
ACKs for top commit:
shaavan:
Code Review ACK fa6065661a86656a29e89ed1a3529cb7103f5394
Tree-SHA512: cd751e3b4dc97ef525eb8be8d0a49e9629389cb114df18d59a06e05388822af2939078e937f01494e6b317d601743b1a433ba47aa40c4dc602372d1f0fd0dc11
b75f4c89ec4d33a3014ccd5151964881b5e0aa1c RPC: Return external_signer in getwalletinfo (Kristaps Kaupe)
Pull request description:
Add `external_signer` to the result object of `getwalletinfo` RPC which indicates whether `WALLET_FLAG_EXTERNAL_SIGNER` flag is set for the wallet.
ACKs for top commit:
S3RK:
utACK b75f4c89ec4d33a3014ccd5151964881b5e0aa1c
achow101:
ACK b75f4c89ec4d33a3014ccd5151964881b5e0aa1c
prayank23:
utACK b75f4c89ec
brunoerg:
utACK b75f4c89ec4d33a3014ccd5151964881b5e0aa1c
Tree-SHA512: 066ccb97541fd4dc3d9728834645db714a3c8c93ccf29142811af4d79cfb9440a97bbb6c845434a909bc6e1775ef3737fcbb368c1f0582bc63973f6deb17a45f
fac62056b56e0a28baf0b6f285752d83fbf96074 Fix integer sanitizer suppressions in validation.cpp (MarcoFalke)
Pull request description:
It doesn't seem ideal to have an integer sanitizer enabled, but then disable it for the whole validation.cpp file.
Fix it with a refactor and remove the suppression.
ACKs for top commit:
hebasto:
ACK fac62056b56e0a28baf0b6f285752d83fbf96074, I have reviewed the code and it looks OK, I agree it can be merged.
prayank23:
Code Review ACK fac62056b5
Tree-SHA512: efc5b9887cb2e207033b264ebf425bae5ff013e909701c049aea5d79a21f10495826e962d171b3d412717cbf0a4723e5124133b5401b35a73915212e85e91020
d1fab9d5d27a2db2546db0f610e0f6929ec4864e test: Call ceildiv helper with integer (Martin Zumsande)
Pull request description:
On master,
`assert_fee_amount(Decimal("0.00000993"), 217, Decimal("0.00004531"))` passes
`assert_fee_amount(Decimal("0.00000993"), Decimal("217"), Decimal("0.00004531"))` fails.
the reason is that the // operator in `ceildiv(a,b) = -(-a//b)` has a different behavior for Decimals, see [doc](https://docs.python.org/3/library/decimal.html#decimal-objects).
`wallet_send.py` calls this function with Decimals, and I think this is the reason for the failure reported in the OP of #24151 (`wallet_send.py --legacy-wallet` line 332, the numbers used in the example above are from there). However, the other failures reported there cannot be explained by this, so this is just a partial fix.
ACKs for top commit:
ryanofsky:
Code review ACK d1fab9d5d27a2db2546db0f610e0f6929ec4864e. Tracking down this problem was a good find, and code seems safer and easier to understand now
Tree-SHA512: 5bf0568cd1a0824f6b1a15a03580b6e9391b4f51112a97c1d00469d255bf6dda45c49a36fa567a5ba9b9973efe1d9cdd480db91965c9f4c2aa963629a8a32cba
a036358994546e2041d0bf0cc911bab4e4baba3c test: Repair failfast option for test runner (Martin Zumsande)
Pull request description:
Fixes#23990
After #23799, the `--failfast` option in the test runner for the functional tests stopped working, because a second outer loop was introduced, which would have needed a `break` too for the test runner to fail immediately. This also led to the errors reported in #23990.
This provides a straightforward fix for that.
There is also #23995 which is a larger refactor, but that hasn't been updated in a while to fix the failfast issue.
ACKs for top commit:
pg156:
Tested ACK a036358994546e2041d0bf0cc911bab4e4baba3c. I agree adding the `all_passed` flag to break out of the outer loop when needed makes sense. The "failfast" option works after this change.
Tree-SHA512: 3e2f775e36c13d180d32a05cd1cfe0883274e8615cdbbd4e069a9899e9b9ea1091066cf085e93f1c5326bd8ecc6ff524e0dad7c638f60dfdb169fefcdb26ee52
