The weight unit is merely a consensus rule detail and is largely
irrelevant for fee-rate calculations and mempool policy rules (e.g. for
package relay and TRUC limits), so there doesn't seem to be any value of
using a granularity that we can't even guarantee to reach exactly
anyway.
Switch to the more natural unit of vsize instead, which simplifies both
the padding implementation and the current tests that take use of this
padding. The rather annoying multiplications by `WITNESS_SCALE_FACTOR`
can then be removed and weird-looking magic numbers like `4004` can be
replaced by numbers that are more connected to actual policy limit
constants from the codebase, e.g. `1001` for exceeding
`TRUC_CHILD_MAX_VSIZE` by one.
f5a2000579 test: re-bucket long-running tests (willcl-ark)
Pull request description:
Re-bucket:
- `p2p_node_network_limited -v*transport`
- `feature_assume_utxo`
On CI runners these tests are taking longer than their current bucket suggests, often being among the last to finish.
Re-bucket them to improve CI efficiency.
ACKs for top commit:
maflcko:
review ACK f5a2000579
Tree-SHA512: 3da5c888db64a311276338270ba1dcad3eb2a24e205f6bb86fc92f767ecfa63682f13fafffff569fa0cfaea607ccb538f31e3934a086d482c3fe1be5d39f8791
faf801515f test: Add missing sync_mempools() to fill_mempool() (MarcoFalke)
fa48be6f02 test: Refactor fill_mempool to extract send_batch helper (MarcoFalke)
Pull request description:
Not doing the sync will lead to (intermittent) issues, as explained in https://github.com/bitcoin/bitcoin/issues/30922#issuecomment-2364529013.
Fix all issues by doing the sync by default and disable it in places that do not need the sync.
Fixes#30922
ACKs for top commit:
mzumsande:
Tested ACK faf801515f
ismaelsadeeq:
Tested ACK faf801515f
marcofleon:
Tested ACK faf801515f
Tree-SHA512: 2de62d168cbb6857a9fb8bc12c42a9093fedf5e9beb6f83a32b3fa72a5ba3cf03631055fd25ef553399a27a6fe0d71c44cfe60660a4d31986debd13b8ab00228
f20fe33e94 test: Add basic balance coverage to wallet_assumeutxo.py (Fabian Jahr)
037b101e80 test: Add coverage for best block locator write in wallet_backup (Fabian Jahr)
31c0df0389 wallet: migration, write best locator before unloading wallet (furszy)
7e3dbe4180 wallet: Write best block to disk before backup (Fabian Jahr)
Pull request description:
I discovered that we don't write the best block to disk when trying to explain the behavior described here: https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882
In the context of that test, the behavior is confusing and I think it also shows that one of the already existing tests in `wallet_assumeutxo.py` doesn't actually test what it says. It only fails because the best block isn't written and actually, the height of the backup that is loaded is at the snapshot height during backup. So it really shouldn't fail since it's past the background sync blocks already.
I'm not sure if this is super relevant in practice though so I am first looking for concept ACKs on the `BackupWallet` code change. Either way, I think this behavior should be documented better if it is left as is and the test should be changed.
ACKs for top commit:
achow101:
ACK f20fe33e94
furszy:
ACK f20fe33
Tree-SHA512: bb384a940df5c942fffe2eb06314ade4fc5d9b924012bfef3b1c456c4182a30825d1e137d8ae561d93d3a8a2f4d1c1ffe568132d20fa7d04844f1e289ab4a28b
7942951e3f Remove unused g_best_block (Ryan Ofsky)
e3a560ca68 rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a09c Remove unused CRPCSignals (Sjors Provoost)
dca923150e Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1121 rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855b3a rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072925 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9fb51 rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27cf05 Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf160 node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215f23 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74bbb refactor: rename BlockKey to BlockRef (Sjors Provoost)
Pull request description:
This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.
`waitTipChanged()` uses a new kernel notification `notifications().m_tip_block_mutex`, which this PR also introduces (a previous version used `g_best_block`).
In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.
There's a commit to add (direct) tests for the methods that are about to be refactored:
- `waitfornewblock` was already implicitly tested by `feature_shutdown.py`.
- `waitforblockheight` by `feature_coinstatsindex.py` and `example_test.py`
This PR renames `getTipHash()` to `getTip()` and returns a `BlockRef` (renamed from `BlockKey`) so that callers can use either the height or hash.
The later commits make trivial improvements to the `waitfor*` RPC calls (not needed for this PR).
The `waitTipChanged()` method could probably also be used for the longpoll functionality in `getblocktemplate`, but I'm a bit reluctant to touch that.
`RPCServer::OnStarted` no longer does anything and `RPCServer::OnStopped` merely prints a log statement. They were added in #5711 as a refactor. This PR drops them entirely.
Finally `g_best_block` is also dropped.
ACKs for top commit:
achow101:
ACK 7942951e3f
ryanofsky:
Code review ACK 7942951e3f. Just rebased since last review
TheCharlatan:
Re-ACK 7942951e3f
Tree-SHA512: a5559446b4000c95e07aad33284b7ee2e57aafd87e1ae778b3825d59689566d047a8047e47a10f76e6e341e7dc72fd265a65afbc0a9c011d17c4cafd55031837
- p2p_node_network_limited -v*transport
- feature_assume_utxo
On CI runners these tests are taking longer than their current bucket
suggests, often being among the last to finish.
Re-bucket them to improve CI efficiency.
e6994efe08 fix: increase rpcbind check robustness (tdb3)
d38e3aed89 fix: handle invalid rpcbind port earlier (tdb3)
83b67f2e6d refactor: move host/port checking (tdb3)
73c243965a test: add tests for invalid rpcbind ports (tdb3)
Pull request description:
Previously, when an invalid port was specified in `-rpcbind`, the `SplitHostPort()` return value in `HTTPBindAddresses()` was ignored and attempt would be made to bind to the default rpcbind port (with the host/port treated as a host).
This rearranges port checking code in `AppInitMain()` to handle the invalid port before reaching `HTTPBindAddresses()`. Also adds a check in `HTTPBindAddresses()` as a defensive measure for future changes.
Adds then updates associated functional tests as well.
ACKs for top commit:
achow101:
ACK e6994efe08
ryanofsky:
Code review ACK e6994efe08
zaidmstrr:
Code review ACK [e6994ef](e6994efe08)
Tree-SHA512: bcc3e5ceef21963821cd16ce6ecb83d5c5657755882c05872a7cfe661a1492b1d631f54de22f41fdd173512d62dd15dc37e394fe1a7abe4de484b82cd2438b92
54227e681a rpc, cli: improve error message on multiwallet mode (pablomartin4btc)
Pull request description:
Running a CLI command when multiple wallets are loaded and `-rpcwallet` is not specified, should return a clearer error.
Currently in `master`:
```
$ bitcoin-cli -regtest -generate 1
error code: -19
error message:
Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).
Try adding "-rpcwallet=<filename>" option to bitcoin-cli command line.
```
With this change:
```
$ bitcoin-cli -regtest -generate 1
error code: -19
error message:
Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<walletname> URI path. Or for the CLI, specify the "-rpcwallet=<walletname>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).
```
ACKs for top commit:
maflcko:
review ACK 54227e681a
achow101:
ACK 54227e681a
furszy:
utACK 54227e681a
mzumsande:
Code Review ACK 54227e681a
jonatack:
ACK 54227e681a
Tree-SHA512: 51ff24f64858aa6be6adf6f20105c9f076ebea743780bf2a4399f7fe8b5239cbb1ea06d32b2ef5e850da2369abb0ef7a52c50c2b8f31f4ca90d3a486abc9b77e
facbcd4cef log: Use ConstevalFormatString (MarcoFalke)
fae9b60c4f test: Use LogPrintStr to test m_log_sourcelocations (MarcoFalke)
fa39b1ca63 doc: move-only logging warning (MarcoFalke)
Pull request description:
This changes all logging (including the wallet logging) to produce a
`ConstevalFormatString` at compile time, so that the format string can be
validated at compile-time.
I tested with `clang` and found that the compiler will use less than 1% more of time and memory.
When an error is found, the compile-time error depends on the compiler, but it may look similar to:
```
src/util/string.h: In function ‘int main(int, char**)’:
src/bitcoind.cpp:265:5: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>(((const char*)"Hi %s %s"))’
src/util/string.h:38:98: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(std::basic_string_view<char>(((const char*)((util::ConstevalFormatString<1>*)this)->util::ConstevalFormatString<1>::fmt)))’
src/util/string.h:78:34: error: expression ‘<throw-expression>’ is not a constant expression
78 | if (num_params != count) throw "Format specifier count must match the argument count!";
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
This refactor does not change behavior of the compiled executables.
ACKs for top commit:
hodlinator:
re-ACK facbcd4cef
l0rinc:
ACK facbcd4cef
ryanofsky:
Code review ACK facbcd4cef
pablomartin4btc:
re-ACK facbcd4cef
stickies-v:
Approach ACK and code LGTM facbcd4cef modulo a `tinyformat::format_error` concern.
Tree-SHA512: 852f74d360897020f0d0f6e5064edc5e7f7dacc2bec1d5feff22c634a2fcd2eb535aa75be0b7191d9053728be6108484c737154b02d68ad3186a2e5544ba0db8
a9964c0444 doc: Updating docs from autotools to cmake (kevkevinpal)
Pull request description:
A bit of a followup from https://github.com/bitcoin/bitcoin/pull/30840
- In this change the documentation where we refer to the `./configure` script which is now gone and have converted the configure params to use the `cmake` equivalent.
ACKs for top commit:
maflcko:
ACK a9964c0444
jonatack:
utACK a9964c0444
jarolrod:
ACK a9964c0444
tdb3:
ACK a9964c0444
pablomartin4btc:
re-ACK a9964c0444
Tree-SHA512: f7ed20b8ad61f028c0d242b9cc70650d8da63057d4a8f7da88f0117a8d3241c5fe8fcf19d56ec82088160b9fee9b175fe9f64e5a845260d3696dc7e94bfdd0bd
replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes
replaced --enable-multiprocess with -DWITH_MULTIPROCESS=ON
replaced --disable-zmq with -DWITH_ZMQ=OFF
Previously, when an invalid port was specified
in `-rpcbind`, the `SplitHostPort()` return value
in `HTTPBindAddresses()` was ignored and attempt
would be made to bind to the default rpcbind port
(with the host/port treated as a host).
This rearranges port checking code in
`AppInitMain()` to handle the invalid
port before reaching `HTTPBindAddresses()`.
Also adjusts associated functional tests.
The primary objective is to provide users with clearer
and more informative error messages when encountering
the RPC_WALLET_NOT_SPECIFIED error, which occurs when
multiple wallets are loadad.
This commit also rectifies the error message consistency
by bringing the error message in line with the definition
established in protocol.h ("error when there are multiple
wallets loaded").
This changes all logging (including the wallet logging) to produce a
ConstevalFormatString at compile time, so that the format string can be
validated at compile-time.
Also, while touching the wallet logging, avoid a copy of the template
Params by using const Params&.
6a1aa510e3 rpc: check block index before reading block / undo data (Martin Zumsande)
6cbf2e5f81 rpc: Improve gettxoutproof error when only header is available. (Martin Zumsande)
69fc867ea1 test: add coverage to getblock and getblockstats (Martin Zumsande)
5290cbd585 rpc: Improve getblock / getblockstats error when only header is available. (Martin Zumsande)
e5b537bbdf rest: improve error when only header of a block is available. (Martin Zumsande)
Pull request description:
Fixes#20978
If a block was pruned, `getblock` already returns a specific error: "Block not available (pruned data)".
But if we haven't received the full block yet (e.g. in a race with block downloading after a new block was received headers-first, or during IBD) we just return an unspecific "Block not found on disk" error and log
`ERROR: ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0) `
which suggest something went wrong even though this is a completely normal and expected situation.
This PR improves the error message and stops calling `ReadRawBlockFromDisk()`, when we already know from the header that the block is not available on disk.
Similarly, it prevents all other rpcs from calling blockstorage read functions unless we expect the data to be there, so that `LogError()` will only be thrown when there is an actual file system problem.
I'm not completely sure if the cause is important enough to change the wording of the rpc error, that some scripts may rely on.
If reviewers prefer it, an alternative solution would be to keep returning the current "Block not found on disk" error, but return it immediately instead of calling `ReadRawBlockFromDisk`, which would at least prevent the log error and also be an improvement in my opinion.
ACKs for top commit:
fjahr:
re-ACK 6a1aa510e3
achow101:
ACK 6a1aa510e3
andrewtoth:
re-ACK 6a1aa510e3
Tree-SHA512: 491aef880e8298a05841c4bf8eb913ef84820d1ad5415fd17d9b441bff181959ebfdd432b5eb8347dc9c568433f9a2384ca9d84cd72c79d8a58323ca117538fe
72c9a1fe94 test: Check that network stays suspended after dumptxoutset if it was off before (Fabian Jahr)
Pull request description:
Follow-up to #30817 which covered the robustness of `dumptxoutset`: network is deactivated during the run but re-activated even when an issue was encountered. But it did not cover the case if the user had deactivated the network themselves before. In that case the user may want the network to stay off so the network is not reactivated after `dumptxoutset` finishes. A test for this behavior is added here.
ACKs for top commit:
achow101:
ACK 72c9a1fe94
pablomartin4btc:
ACK 72c9a1fe94
theStack:
utACK 72c9a1fe94
tdb3:
tested ACK 72c9a1fe94
Tree-SHA512: 18a57c5782e99a018414db0597e88debeb32666712c2a75dddbb55135d8f1ddd1eeacba8bbbd35fc03b6c4ab0522fe074ec08edea729560b018f51efabf00e89
This avoids low-level log errors that are supposed to only occur when
there is an actual problem with the block on disk missing unexpectedly,
but not in the case where the block and/or undo data are expected not to be there.
It changes behavior such that in the first case (block index indicates
data is available but retrieving it fails) an error is thrown.
It also adjusts a functional tests that tried to simulate not
having undo data (but having block data) by deleting the undo file.
This situation should occur reality because block and undo data are pruned together.
Instead, test this situation with a block that hasn't been connected.
This improves the error message of the getblock and getblockstats rpc and prevents calls to
ReadRawBlockFromDisk(), which are unnecessary if we know
from the header nStatus field that the block is not available.
19f4a7c95a test: Wait for local services to update in feature_assumeutxo (Fabian Jahr)
Pull request description:
Closes#30878
It seems like there is a race where the block is stored locally and `getblock` does not error anymore, but `ActivateBestChain` has not finished yet, so the local services are not updated yet either. Fix this by waiting for the local services to update.
Can be reproduced locally by adding the sleep here:
```cpp
──────────────────────────────────────────────────────────────────────────────────────────────────────────┐
src/validation.cpp:3567: bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< │
──────────────────────────────────────────────────────────────────────────────────────────────────────────┘
}
if (WITH_LOCK(::cs_main, return m_disabled)) {
std::this_thread::sleep_for(std::chrono::seconds(10));
// Background chainstate has reached the snapshot base block, so exit.
// Restart indexes to resume indexing for all blocks unique to the snapshot
```
ACKs for top commit:
maflcko:
review-only ACK 19f4a7c95a
achow101:
ACK 19f4a7c95a
pablomartin4btc:
tACK 19f4a7c95a
furszy:
Code review ACK [19f4a7c](19f4a7c95a).
Tree-SHA512: 70dad3795988956c5e20f2d2d895fb56c5e3ce257c7547d3fd729c88949f0e24cb34594da1537bce8794ad02b2db44e7e46e995aa32539cd4dd84c4f1d4bb1c4
fa5bc450d5 util: Use compile-time check for LogConnectFailure (MarcoFalke)
fa7087b896 util: Use compile-time check for FatalErrorf (MarcoFalke)
faa62c0112 util: Add ConstevalFormatString (MarcoFalke)
fae7b83eb5 lint: Remove forbidden functions from lint-format-strings.py (MarcoFalke)
Pull request description:
The `test/lint/lint-format-strings.py` was designed to count the number of format specifiers and assert that they are equal to the number of parameters passed to the format function. The goal seems reasonable, but the implementation has many problems:
* It is written in Python, meaning that C++ code can not be parsed correctly. Currently it relies on brittle regex and string parsing.
* Apart from the parsing errors, there are also many logic errors. For example, `count_format_specifiers` allows a mix of positional specifiers and non-positional specifiers, which can lead to runtime format bugs. Also, `count_format_specifiers` silently skipped over "special" format specifiers, which are valid in tinyformat, which again can lead to runtime format bugs being undetected.
* The brittle logic has a history of breaking in pull requests that are otherwise fine. This causes the CI to fail and the pull request being blocked from progress until the bug in the linter is fixed, or the code is rewritten to work around the bug.
* It is only run in the CI, or when the developer invokes the script. It would be better if the developer got the error message at compile-time, directly when writing the code.
Fix all issues by using a `consteval` checked format string in `FatalErrorf` and `LogConnectFailure`.
This is the first step toward https://github.com/bitcoin/bitcoin/issues/30530 and a follow-up will apply the approach to the other places.
ACKs for top commit:
stickies-v:
re-ACK fa5bc450d5
l0rinc:
ACK fa5bc450d5
hodlinator:
ACK fa5bc450d5
ryanofsky:
Code review ACK fa5bc450d5
Tree-SHA512: d6189096b16083143687ed1b1559cf4f92f97dd87bc5d00673e44f4fb9fce7bb7b215cfdfc39b6e6a24f0b75a79a03ededce966639e554f7172e1fc22cf015ae
72b46f28bf test: fix exclude parsing for functional runner (Max Edwards)
Pull request description:
This restores previous behaviour of being able to exclude a test by name without having to specify .py extension.
It was noticed in https://github.com/bitcoin/bitcoin/issues/30851 that tests were no longer being excluded.
PR https://github.com/bitcoin/bitcoin/pull/30244 introduced being able to exclude a specific tests based on args (such as `--exclude "rpc_bind.py --ipv6`) but it made the wrong assumption that test names intended to be excluded would include the .py extension.
The following https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687 shows that this is not how the `--exclude` flag was used in CI.
https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687 gave three examples of `--exclude` being used in CI so I compared the number of tests that the runner would run for these three examples in three situations, before #30244 was introduced, in master today and with this PR applied.
Example:
`--previous-releases --coverage --extended --exclude feature_dbcrash`
Test count:
Before #30244 introduced: 314
Master: 315
With this PR: 314
Example:
`--exclude feature_init,rpc_bind,feature_bind_extra`
Test count:
Before #30244 introduced: 306
Master 311
With this PR: 306
Example:
`--exclude rpc_bind,feature_bind_extra`
Before #30244 introduced: 307
Master 311
With this PR: 307
I've also tested that the functionality introduced with #30244 remains and we can still exclude specific tests by argument.
ACKs for top commit:
maflcko:
review ACK 72b46f28bf
willcl-ark:
ACK 72b46f28bf
Tree-SHA512: 37c0e3115f4e3efdf9705f4ff8cd86a5cc906aacc1ab26b0f767f5fb6a953034332b29b0667073f8382a48a2fe9d649b7e60493daf04061260adaa421419d8c8
2ad560139b Remove unused src_dir param from run_tests (Lőrinc)
Pull request description:
The `src_dir` usage was removed in a8a2e364ac (diff-437d7f6e9f2229879b60aae574a8217f14c643bbf3cfa9225d8011d6d52df00cL598), making the parameter unused.
Top commit has no ACKs.
Tree-SHA512: 1fd8b93811b4ab467ba5a160a4fe204e9606e1bf237c7595ed6f8b7821cf59d2a776c0e1e154852a45b2a35e5bdbd8996314e4f63a9c750f21b9a17875cb636a
992f83bb6f test: add coverage for assumeUTXO honest peers disconnection (furszy)
6d5812e5c8 assumeUTXO: fix peers disconnection during sync (furszy)
Pull request description:
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (`NODE_NETWORK`), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness from the AssumeUTXO
node.
This lack of response occurs because nodes ignore `getdata` requests when they do
not have the block data available (further discussion can be found in #30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal `NODE_NETWORK_LIMITED` support. Then, full-node (`NODE_NETWORK`)
support will be re-enabled once the background chain sync is completed.
Thanks mzumsande for a post-#30385 convo too.
Testing notes:
Just cherry-pick the second commit (bb08c22) on master.
It will fail there, due to the IBD node requesting historical blocks to the snapshot
node - which is bad because the snapshot node will ignore the requests and
stall + disconnect after some time.
ACKs for top commit:
achow101:
ACK 992f83bb6f
naumenkogs:
ACK 992f83bb6f
mzumsande:
ACK 992f83bb6f
Tree-SHA512: fef525d1cf3200c2dd89a346be9c82d77f2e28ddaaea1f490a435e180d1a47a371cadea508349777d740ab56e94be536ad8f7d61cc81f6550c58b609b3779ed3
082779d606 test: Add explicit onion bind to p2p_permissions (Ava Chow)
Pull request description:
When the bind option is replaced in the bitcoin.conf, bitcoind will attempd to bind to the default tor listening port. If another bitcoind is running that is already bound to that port, the bind will fail which, since #22729, causes the test to fail.
This failure can be avoided by explicitly binding the tor port when the bind is removed.
ACKs for top commit:
tdb3:
ACK 082779d606
theStack:
re-ACK 082779d606
glozow:
ACK 082779d606
Tree-SHA512: 4acb69ea2e00aeacf9e7c9ab9595ceaf0e0d2adbd795602034b2184197d9bad54c7bc9f3da43ef9c52a71869fe96ba8c87fc5b7c37880f258f5a2aaab2b4046c
Exercising and verifying the following points:
1. An IBD node can sync headers from an AssumeUTXO node at
any time.
2. IBD nodes do not request historical blocks from AssumeUTXO
nodes while they are syncing the background-chain.
3. The assumeUTXO node dynamically adjusts the network services
it offers according to its state.
4. IBD nodes can fully sync from AssumeUTXO nodes after they
finish the background-chain sync.
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (NODE_NETWORK), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness (lack of response)
from the AssumeUTXO node.
This lack of response occurs because nodes ignore getdata requests when they do
not have the block data available (further discussion can be found in PR 30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK')
support will be re-enabled once the background chain sync is completed.
When the bind option is replaced in the bitcoin.conf, bitcoind will
attempd to bind to the default tor listening port. If another bitcoind
is running that is already bound to that port, the bind will fail which,
since #22729, causes the test to fail.
This failure can be avoided by explicitly binding the tor port when the
bind is removed.
c2b779da4e refactor: Manage dumptxoutset RAII classes with std::optional (Fabian Jahr)
4b5bf335ad test: Add coverage for failing dumptxoutset behavior (Fabian Jahr)
Pull request description:
This adds a test that checks that network activity is not suspended if dumptxoutset fails in the middle of its process which is implemented with the `NetworkDisable` RAII class. I would have liked to add coverage for the `TemporaryRollback` RAII class but that seems a lot more tricky since the failure needs to happen at some point after the rollback and on the scale of our test chain here I couldn't find a way to do it yet. This was requested by pablomartin4btc here: https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2280450117. To test the test you can comment out the content of the destructor of `NetworkDisable`.
It also addresses the feedback by ryanofsky to use `std::optional` instead of `std::unique_ptr` for the management of the RAII object: https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228
ACKs for top commit:
achow101:
ACK c2b779da4e
pablomartin4btc:
cr & tACK c2b779da4e
tdb3:
ACK c2b779da4e
BrandonOdiwuor:
Code Review ACK c2b779da4e
theStack:
ACK c2b779da4e
Tree-SHA512: 9556e75014a2599bb870b70faf887608b332f2312626333f771d4ec11c04f863a2cf17e223ec473d4e8b0c9e8008394a4e0c321561f7ef3a2eec713dcfaea58a
ee47ca29d6 init: fix fatal error on '-wallet' negated option value (furszy)
Pull request description:
Currently, if users provide a double negated value such as '-nowallet=0' or a non-boolean
convertible value to a negated option such as '-nowallet=not_a_boolean', the initialization
process results in a fatal error, causing an unclean shutdown and displaying a poorly
descriptive error message:
"JSON value of type bool is not of expected type string." (On bitcoind. The GUI
does not display any error msg - upcoming PR -).
This PR fixes the issue by ensuring that only string values are returned in the
the "wallet" settings list, failing otherwise. It also improves the clarity of the
returned error message.
Note:
This bug was introduced in https://github.com/bitcoin/bitcoin/pull/22217. Where the `GetArgs("-wallet")` call was
replaced by `GetSettingsList("-wallet")`.
ACKs for top commit:
achow101:
ACK ee47ca29d6
ryanofsky:
Code review ACK ee47ca29d6, just adding the suggested test since last review
TheCharlatan:
ACK ee47ca29d6
ismaelsadeeq:
Tested ACK ee47ca29d6
Tree-SHA512: 5f01076f74a048019bb70791160f0accc2db7a457d969cb23687bed81ccbbdec1dda68311e7c6e2dd56250e23e8d926d4066e5014b2a99a2fc202e24ed264fbd
27c976d11a fix: increase consistency of rpcauth parsing (tdb3)
2ad3689512 test: add norpcauth test (tdb3)
67df0dec1a test: blank rpcauth CLI interaction (tdb3)
ecc98ccff2 test: add cases for blank rpcauth (tdb3)
Pull request description:
The current `rpcauth` parsing behavior is inconsistent and unintuitive (see https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-1972085251 and additional details below).
The current behavior inconsistently treats empty `rpcauth` as an error (or not) depending on the location within CLI/bitcoin.conf and the location of adjacent valid `rpcauth` params.
Empty `rpcauth` is now consistently treated as an error and prevents bitcoind from starting.
Continuation of the upforgrabs PR #29141.
### Additional details:
Current `rpcauth` behavior is nonsensical:
- If an empty `rpcauth` argument was specified as the last command line argument, it would cause all other `rpcauth` arguments to be ignored.
- If an empty `rpcauth` argument was specified on the command line followed by any nonempty `rpcauth` argument, it would cause an error.
- If an empty `rpcauth=` line was specified after non-empty rpcauth line in the config file it would cause an error.
- If an empty `rpcauth=` line in a config file was first it would cause other rpcauth entries in the config file to be ignored, unless there were `-rpcauth` command line arguments and the last one was nonempty, in which case it would cause an error.
New behavior is simple:
- If an empty rpcauth config line or command line argument is used it will cause an error
ACKs for top commit:
naiyoma:
Tested ACK [27c976d11a)
achow101:
ACK 27c976d11a
ryanofsky:
Code review ACK 27c976d11a. Since last review commit message was just tweaked to clarify previous behavior.
Tree-SHA512: af2e9dd60d1ad030409ae2c3805ab139c7435327823d9f8bbeede815f376cb696a5929b08a6e8c8b5f7278ed49cfb231789f9041bd57f1f03ec96501b669da5b
fadbcd51fc bench: Remove redundant logging benchmarks (MarcoFalke)
fa8dd952e2 bench: Use LogInfo instead of the deprecated alias LogPrintf (MarcoFalke)
Pull request description:
`LogPrint*ThreadNames` is redundant with `LogWith(out)ThreadNames`,
because they all measure toggling the thread names (and check that it
has no effect on performance).
Fix it by removing the redundant ones. This also allows to drop a deprecated logging alias.
ACKs for top commit:
stickies-v:
ACK fadbcd51fc
Tree-SHA512: 4fe137f374aa4ee1aa0e1da4a1f9839c0e52c23dbb93198ecafee98de39d311cc47304bba4191f3807aa00c51b1eae543e3f270f03d341c84910e5e341a1d475
In case of a failure to create the dump, the node should not be left in an inconsistent state like deactivated network activity or an invalidated blockchain.
LogPrint*ThreadNames is redundant with LogWith(out)ThreadNames, because
they all measure toggling the thread names (and check that it has no
effect on performance).
This also allows to remove unused and deprecated macros.
8131bf7483 ci: parse TEST_RUNNER_EXTRA into an array (Max Edwards)
c4762b0aa0 test: allow excluding func test by name and arg (Max Edwards)
Pull request description:
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
Update:
While testing this it was noticed that `test_runner.py` when given `--exclude "rpc_bind.py --ipv6"` will exclude all `rpc_bind.py` tests so this PR has been updated to include a change to the test runner to only exclude the specific test if you pass an arg or exclude all tests of that name if you do not pass an arg. `--exclude rpc_bind.py` will exclude all three variants and `--exclude rpc_bind --ipv6` will only exclude the IPV6 variant.
ACKs for top commit:
maflcko:
ACK 8131bf7483
achow101:
ACK 8131bf7483
hebasto:
ACK 8131bf7483, tested on Ubuntu 23.10 and Windows 11.
Tree-SHA512: 82b73f12d627f533d8e5be4a518d455ef4427a755bbe03ccd11d0bb70c7ff3cee76220b0264fcfb236661c4cf5deba034cbfc2372b96d5861f3436c21eae8264
fafdb7df34 lint: Speed up flake8 checks (MarcoFalke)
faf17df7fb lint: Document missing py_lint dependency (MarcoFalke)
faebeb828f lint: Remove python whitespace and shadowing lint rules (MarcoFalke)
7777047835 lint: Remove python lint rules that are SyntaxError (MarcoFalke)
faaf3e53f0 test: [refactor] Fix F841 flake8 (MarcoFalke)
444421db69 test: [refactor] Fix E714 pycodestyle (MarcoFalke)
Pull request description:
The checks have many issues:
* Some checks that could in theory hide bugs are not applied -> Fix them and apply them going forward
* Some checks are redundant Python 2 checks, or of low value -> Remove them
* The checks are slow -> Speed them up from ~10 seconds to about ~20 milliseconds
ACKs for top commit:
davidgumberg:
review and tested reACK fafdb7df34
kevkevinpal:
ACK [fafdb7d](fafdb7df34)
achow101:
ACK fafdb7df34
Tree-SHA512: a0488b722cfaf7071bd6848cd3be002e0b6c38af80d8b5cbb08613c0b174ef63277289f960db8ac31adb09fe563a4973203b8fb10b83cbcfdc6f0ef39bd04410
c8e6771af0 test: restrict multiple CLI arguments (naiyoma)
8838c4f171 common/args.h: automate check for multiple cli commands (naiyoma)
Pull request description:
This PR is a continuation of the validation suggested [here](https://github.com/bitcoin/bitcoin/pull/27815) to ensure that only one Request Handler can be specified at a time.
ACKs for top commit:
stratospher:
reACK c8e6771.
achow101:
ACK c8e6771af0
tdb3:
cr re ACK c8e6771af0
Tree-SHA512: f4fe036fee342a54f1a7ac702ac35c40bf3d420fde6ab16313a75327292d5ee5c8ece1be9f852a13fcf73da1148b143b37b4894e294052abdde6eefb2e8c6f3f
ec317bc44b test: update satoshi_round function (naiyoma)
Pull request description:
This PR refactors `satoshi_round` to accept different rounding modes and make rounding a required argument.
Continuation of https://github.com/bitcoin/bitcoin/pull/23225
ACKs for top commit:
maflcko:
review ACK ec317bc44b
achow101:
ACK ec317bc44b
AngusP:
ACK ec317bc44b
Tree-SHA512: 070f0aa6f66e58bff7412cae6b71f5f4ab8c718c7b5eeba4bb604fe22c6416a1ced0474294f504e1c28943ddc073104466b5944b43bae27e42bee5ca85afa468
fb6d51eb25 signet/miner: Use argparse exclusive groups (Anthony Towns)
338a266a9a signet/miner: add support for a poolnum/poolid tag in mined blocks (Anthony Towns)
409ab7d35b signet/miner: add Generate.mine function (Anthony Towns)
7b31332370 signet/miner: add Generate.gbt function (Anthony Towns)
85c5c0bea9 signet/miner: add Generate.next_block_time function (Anthony Towns)
5540e6ca49 signet/miner: move next_block_* functions into new Generator class (Anthony Towns)
35f4631196 signet/miner: rename do_decode_psbt to decode_psbt (Anthony Towns)
aac040b439 signet/miner: drop create_coinbase function (Anthony Towns)
16951f549e signet/miner: drop do_createpsbt function (Anthony Towns)
3aed0a4284 signet/miner: drop get_reward_address function (Anthony Towns)
Pull request description:
Refactors the code a bunch, and adds `--poolnum` / `--poolid` options so that signers can tag their coinbases in a way that explorers can recognise (see also https://github.com/bitcoin-data/mining-pools/pull/82 and https://github.com/mempool/mempool/issues/2903).
The refactoring in particular helps enable the "try using inquisition's getblocktemplate, and if that doesn't work fall back to core's getblocktemplate" logic, as described/implemented in https://github.com/bitcoin-inquisition/bitcoin/pull/7
ACKs for top commit:
achow101:
ACK fb6d51eb25
danielabrozzoni:
Code review ACK fb6d51eb25
Tree-SHA512: d84095c4045ab196685b847e04ce2cdaedf387bc2527430ede918318dc5b70bf3d87b754264016f895f506fac70d4fdea5ef3cd8c3c375fd586afeae01e045e5
6eeb188d40 test: adds seednode functional tests (Sergi Delgado Segura)
3270f0adad net: Favor peers from addrman over fetching seednodes (Sergi Delgado Segura)
Pull request description:
This is a follow-up of #28016 motivated by https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-1913140932 and https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1984448937.
The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues:
- First, if permanently set (e.g. running with seednode in a config file) we'd be signaling such seed every time we restart our node
- Second, we will be giving the seed node way too much influence over our addrman, populating the latter with data from the former even when unnecessary
This changes the behavior to only add seednodes to `m_addr_fetch` if our addrman is empty, or little by little after we've spent some time trying addresses from our addrman. Also, seednodes are added to `m_addr_fetch` in random order, to avoid signaling the same node in case more than one seed is added and we happen to try them over multiple restarts
ACKs for top commit:
achow101:
ACK 6eeb188d40
cbergqvist:
ACK 6eeb188d40
itornaza:
Tested ACK 6eeb188d40
tdb3:
ACK 6eeb188d40
Tree-SHA512: b04445412f22018852d6bef4d3f1e88425ee6ddb434f61dcffa9e0c41b8e31f8c56f83858d5c7686289c86dc4c9476c437df15ea61a47082e2bb2e073cc62f15
94b0adcc37 rpc, refactor: Prevent potential race conditions in dumptxoutset (Fabian Jahr)
e868a6e070 doc: Improve assumeutxo guide and add more docs/comments (Fabian Jahr)
b29c21fc92 assumeutxo: Remove devtools/utxo_snapshot.sh (Fabian Jahr)
20a1c77aa7 contrib: Remove test_utxo_snapshots.sh (Fabian Jahr)
8426850352 test: Test for dumptxoutset at specific height (Fabian Jahr)
993cafe7e4 RPC: Add type parameter to dumptxoutset (Fabian Jahr)
fccf4f91d2 RPC: Extract ReconsiderBlock helper (Fabian Jahr)
446ce51c21 RPC: Extract InvalidateBlock helper (Fabian Jahr)
Pull request description:
This adds a height parameter to the `dumptxoutset` RPC. This internalizes the workflow that was previously done by scripts: roll back the chain to the height we actually want the snapshot from, create the snapshot, roll forward to the real tip again.
The nice thing about internalizing this functionality is that we can write tests for the code and it gives us more options to make the functionality robust. The shell scripts we have so far will be more cumbersome to maintain in the long run, especially since we will only notice later when we have broken them. I think it's safe to remove these `test_utxo_snapshots.sh` as well when we have this option in `dumptxoutset` because we have also added some good additional functional test coverage for this functionality.
ACKs for top commit:
Sjors:
re-utACK 94b0adcc37
achow101:
ACK 94b0adcc37
mzumsande:
ACK 94b0adcc37
pablomartin4btc:
re-ACK 94b0adcc37
Tree-SHA512: a4c9af5f687d1ca7bfb579a36f363882823386b5fa80c05de531b05a2782b5da6ff5baf3ada4bca8f32f63975d86f1948175abed9affe51fc958472b5f838dab
When the tracepoint was introduced in 8f37f5c2a5,
the connect_block duration was passed in microseconds `µs`.
By starting to use steady clock in fabf1cdb20
this changed to nanoseconds `ns`. As the test only checked if the
duration value is `> 0` as a plausibility check, this went unnoticed.
I detected this when setting up monitoring for block validation time
as part of the Great Consensus Cleanup Revival discussion.
This change casts the duration explicitly to nanoseconds (as it has been
nanoseconds for the last three releases; switching back now would 'break'
the broken API again; there don't seem to be many users affected), updates
the documentation and adds a check for an upper bound to the tracepoint
interface tests. The upper bound is quite lax as mining the block takes
much longer than connecting the empty test block. It's however able to
detect incorrect duration units passed.
This is used in both cases - TCP server (accept) and TCP client (connect).
The message "Connected & Listening address:port" is confusing.
Print both ends of the TCP connection.
fa247e6e8c test: Avoid intermittent timeout in p2p_headers_sync_with_minchainwork.py (MarcoFalke)
Pull request description:
Similar to https://github.com/bitcoin/bitcoin/pull/30705:
The goal of this test case is to check that the sync works at all, not to check any timeout.
On extremely slow hardware (for example qemu virtual hardware), downloading the 4110 BLOCKS_TO_MINE may take longer than the block download timeout.
Fix it by pinning the time using mocktime temporarily, and advance it immediately after the sync.
ACKs for top commit:
stratospher:
ACK fa247e6. Checked the timeout downloading block logs before/after using `setmocktime`.
tdb3:
ACK fa247e6e8c
Tree-SHA512: f61632a8d9e484f1b888aafbf87f7adf71b8692387bd77f603cdbc0de49f30d42e654741d46ae1ff8b9706a5559ee0faabdb192ed0db7449010b68bfcdbaa42d
fa09cb41f5 refactor: Remove unused LogPrint (MarcoFalke)
3333415890 scripted-diff: LogPrint -> LogDebug (MarcoFalke)
Pull request description:
`LogPrint` has many issues:
* It seems to indicate that something is being "printed", however config options such as `-printtoconsole` actually control what and where something is logged.
* It does not mention the log severity (debug).
* It is a deprecated alias for `LogDebug`, according to the dev notes.
* It wastes review cycles, because reviewers sometimes point out that it is deprecated.
* It makes the code inconsistent, when both are used, possibly even in lines right next to each other (like in `InitHTTPServer`)
Fix all issues by removing the deprecated alias.
I checked all conflicting pull requests and at the time of writing there are no conflicts, except in pull requests that are marked as draft, are yet unreviewed, or are blocked on feedback for other reasons. So I think it is fine to do now.
ACKs for top commit:
stickies-v:
ACK fa09cb41f5
danielabrozzoni:
utACK fa09cb41f5
TheCharlatan:
ACK fa09cb41f5
Tree-SHA512: 14270f4cfa3906025a0b994cbb5b2e3c8c2427c0beb19c717a505a2ccbfb1fd1ecf2fd03f6c52d22cde69a8d057e50d2207119fab2c2bc8228db3f10d4288d0f
faa382ae76 ci, doc: Drop reference to `src/.bear-tidy-config` (Hennadii Stepanov)
d71ac76842 build: Remove Autotools-based build system (Hennadii Stepanov)
e268b48419 doc: Adjust `doc/design/libraries.md` (Hennadii Stepanov)
d209e4f156 doc: Drop mentions of `share/genbuild.sh` (Hennadii Stepanov)
Pull request description:
This PR deletes the Autotools-based build system.
The MSVC build system is deleted in https://github.com/bitcoin/bitcoin/pull/30731.
ACKs for top commit:
maflcko:
re-ACK faa382ae76🍦
TheCharlatan:
ACK faa382ae76
fanquake:
ACK faa382ae76
Tree-SHA512: 53df977b5b199a1c38f7f61a042a62b24831c559ba65a461b4ac1c96a1a56e2dfd676df79f1358fd1cc1749ff27e7b548086157f337d4f596c1054cb3d2d5739
bd7ce05f9d test: fix `TestShell` initialization (late follow-up for #30463) (Sebastian Falbesoner)
Pull request description:
Creating a `TestShell` instance as stated in the [docs](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test-shell.md) currently fails on master:
```
$ python3
Python 3.10.13 (main, Mar 15 2024, 07:36:23) [Clang 16.0.6 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/home/thestack/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2, setup_clean_chain=True)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/thestack/bitcoin/test/functional/test_framework/test_shell.py", line 70, in __new__
TestShell.instance = TestShell.__TestShell()
TypeError: BitcoinTestFramework.__init__() missing 1 required positional argument: 'test_file'
```
Since #30463, BitcoinTestFramework instances expect the path of the calling test at construction, in order to find shared data like the configuration (config.ini) and the cache. Note that in contrast to actual functional tests, we can't simply pass `__file__` here, as the test shell module sits within the `test_framework` subfolder, so we have to navigate up to the parent directory and append some dummy test file name.
On the long-term we should probably add some TestShell instantation smoke-test to detect issues like this early. As I'm not too familiar with the CI I'm not sure what is a good way to achieve this (a functional test obviously can't be used, as that's already a BitcoinTestFramework test in itself), but happy to take suggestions.
ACKs for top commit:
ismaelsadeeq:
Tested ACK bd7ce05f9d
danielabrozzoni:
tACK bd7ce05f9d
brunoerg:
ACK bd7ce05f9d
Tree-SHA512: c3a2365e2cda48a233ee724673c490787981354914f33e10eadbbad9c68e8403d84c5551229a611401e743886539de380ba4bfcb77032b6c85731e3bbe962dc1
a563f41232 Remove second node since only 1 is needed for the test (Martin Saposnic)
1f4cdb3d69 Replace custom funding tx creation with MiniWallet. (Martin Saposnic)
Pull request description:
In response to issue https://github.com/bitcoin/bitcoin/issues/30600, optimizations have been implemented to enhance test efficiency and readability:
This PR refactors the `rpc_signrawtransactionwithkey.py` functional test to use MiniWallet for creating funding transactions. This simplifies the test code and improves performance by eliminating the need to mine new blocks for each funding transaction.
Key changes:
- Replaced custom `send_to_address` method with MiniWallet's `send_to` method
- Removed unnecessary setup of a clean chain and second node
- Simplified transaction creation and signing process
ACKs for top commit:
glozow:
ACK a563f41232
ismaelsadeeq:
code review ACK a563f41232
theStack:
ACK a563f41232
Tree-SHA512: 318959f89702b169453d537dafb822f5ef1921db1088941d8bbdb3171dd7a6ecad590e57a3802bc37bcf8992267ed6ffa7f156b229d9817ebf812bd35df509b5
Previously they may have taken more than 10 seconds. Now they should
finish in less than one second.
This also allows to drop one dependency to be installed.
The rules have many issues:
* Most are redundant, because Python already has a built-in
IndentationError, a subclass of SyntaxError, to enforce whitespace.
* They are not enforced consistently anyway, see for examples [1][2]
below.
* They are stylistic rules where the author intentionally formatted the
code to be easier to read. Starting to enforce them now would make the
code harder to read and create frustration in the future.
Fix all issues by removing them.
[1]:
test/functional/feature_cltv.py:63:35: E272 [*] Multiple spaces before keyword
|
61 | # | Script to prepend to scriptSig | nSequence | nLockTime |
62 | # +-------------------------------------------------+------------+--------------+
63 | [[OP_CHECKLOCKTIMEVERIFY], None, None],
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E272
[2]:
contrib/asmap/asmap.py:395:13: E306 [*] Expected 1 blank line before a nested definition, found 0
|
393 | prefix.pop()
394 | hole = not fill and (lhole or rhole)
395 | def candidate(ctx: Optional[int], res0: Optional[list[ASNEntry]],
| ^^^ E306
Any kind of syntax error is already reported, so there is no need to
enumerate all possible types of syntax errors of ancient versions of
Python 2 or 3.
a0abcbd382 doc: Mention multipath specifier (Ava Chow)
0019f61fc5 tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4b wallet: Move internal to be per key when importing (Ava Chow)
1692245525 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9 rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd22 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2da descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02 descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b1034 descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae15 descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f72 descriptors: Add PubkeyProvider::Clone (Ava Chow)
Pull request description:
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.
This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.
The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes#17190
ACKs for top commit:
darosior:
re-ACK a0abcbd382
mjdietzx:
reACK a0abcbd382
pythcoiner:
reACK a0abcbd
furszy:
Code review ACK a0abcbd
glozow:
light code review ACK a0abcbd382
Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
41051290ab cmake: Ignore build subdirectories within source directory (Hennadii Stepanov)
6ce50fd9d0 doc: Update for CMake-based build system (Hennadii Stepanov)
9730288a0c ci: Migrate CI scripts to CMake (Hennadii Stepanov)
c360837ca5 cmake, lint: Adjust `lint_includes_build_config` (Hennadii Stepanov)
3885441ee0 cmake: Add presets for native Windows builds (Hennadii Stepanov)
7681746b20 cmake: Add vcpkg manifest file (Hennadii Stepanov)
8b6f1c4353 cmake: Add `Coverage` and `CoverageFuzz` scripts (Hennadii Stepanov)
65bdbc1ff2 cmake: Add `docs` build target (Hennadii Stepanov)
fb75ebbc33 cmake: Add compiler diagnostic flags (Hennadii Stepanov)
e821f0a37a cmake: Migrate Guix build scripts to CMake (Hennadii Stepanov)
747adb6ffe cmake: Add `Maintenance` module (Hennadii Stepanov)
1f60b30df0 cmake: Add `APPEND_{CPP,C,CXX,LD}FLAGS` cache variables (Hennadii Stepanov)
2b43c45b13 cmake: Add `AddWindowsResources` module (Hennadii Stepanov)
973a3b0c5d cmake: Implement `install` build target (Hennadii Stepanov)
84ac35cfd4 cmake: Add cross-compiling support (Hennadii Stepanov)
0d01c228a7 build: Generate `toolchain.cmake` in depends (Hennadii Stepanov)
91a799247d depends: Add host-specific `cmake_system_version` variables (Hennadii Stepanov)
9b31209b4c depends: Rename `cmake_system` -> `cmake_system_name` (Hennadii Stepanov)
4a5208a81d Revert "build, qt: Do not install *.prl files" (Hennadii Stepanov)
6522af62af depends: Amend handling flags environment variables (Hennadii Stepanov)
90cec4d251 cmake: Add `MULTIPROCESS` option (Hennadii Stepanov)
bb1a450dcb cmake: Build `bitcoin-chainstate` executable (Hennadii Stepanov)
aed38ea58c cmake: Build `bitcoinkernel` library (Hennadii Stepanov)
975d67369b cmake: Build `test_bitcoin-qt` executable (Hennadii Stepanov)
10fcc668a3 cmake: Add `WITH_DBUS` option (Hennadii Stepanov)
5bb5a4bc75 cmake: Add `libqrencode` optional package support (Hennadii Stepanov)
57a6e2ef4a cmake: Build `bitcoin-qt` executable (Hennadii Stepanov)
30f642952c cmake: Add `WERROR` option (Hennadii Stepanov)
c98d4a4c34 cmake: Add `REDUCE_EXPORTS` option (Hennadii Stepanov)
a01cb6e63f cmake: Add `HARDENING` option (Hennadii Stepanov)
a8a2e364ac cmake: Add Python-based tests (Hennadii Stepanov)
3d85379570 cmake: Add fuzzing options (Hennadii Stepanov)
908530e312 cmake: Add `SANITIZERS` option (Hennadii Stepanov)
8bb0e85631 cmake: Build `bench_bitcoin` executable (Hennadii Stepanov)
801735163a cmake: Add external signer support (Hennadii Stepanov)
353e0c9e96 cmake: Add `systemtap-sdt` optional package support (Hennadii Stepanov)
d2fda82b49 cmake: Add `libzmq` optional package support (Hennadii Stepanov)
ae7b39a0e1 cmake: Add `libminiupnpc` optional package support (Hennadii Stepanov)
6480e1dcdb cmake: Add `libnatpmp` optional package support (Hennadii Stepanov)
e73e9304a1 cmake: Build `bitcoin-util` executable (Hennadii Stepanov)
027c6d7caa cmake: Build `bitcoin-tx` executable (Hennadii Stepanov)
d10c5c34c3 cmake: Add wallet functionality (Hennadii Stepanov)
ab2e99b0d9 cmake: Create test suite for `ctest` (Hennadii Stepanov)
959370bd76 cmake: Build `test_bitcoin` executable (Hennadii Stepanov)
b27bf9700d cmake: Build `bitcoin-cli` executable (Hennadii Stepanov)
a9813df826 cmake: Build `bitcoind` executable (Hennadii Stepanov)
97829ce2d5 cmake: Add `FindLibevent` module (Hennadii Stepanov)
3118e40c61 cmake: Build `bitcoin_consensus` library (Hennadii Stepanov)
809a2f1929 cmake: Build `bitcoin_util` static library (Hennadii Stepanov)
0a9a521a70 cmake: Build `bitcoin_crypto` library (Hennadii Stepanov)
958971f476 cmake: Build `univalue` static library (Hennadii Stepanov)
752747fda8 cmake: Generate `obj/build.h` header (Hennadii Stepanov)
1f0a78edf3 cmake: Build `minisketch` static library (Hennadii Stepanov)
12bfbc8154 cmake: Build `leveldb` static library (Hennadii Stepanov)
51985c5304 cmake: Build `crc32c` static library (Hennadii Stepanov)
db7a198f29 cmake: Build `secp256k1` subtree (Hennadii Stepanov)
dbb7ed14e8 cmake: Add `ccache` support (Hennadii Stepanov)
cedfdf6c72 cmake: Redefine/adjust per-configuration flags (Hennadii Stepanov)
b6b5e732c8 cmake: Add global compiler and linker flags (Hennadii Stepanov)
f98327931b cmake: Add `TryAppendLinkerFlag` module (Hennadii Stepanov)
4a0af29697 cmake: Add `TryAppendCXXFlags` module (Hennadii Stepanov)
35cffc497d cmake: Add POSIX threads support (Hennadii Stepanov)
fd72d00ffe cmake: Add position independent code support (Hennadii Stepanov)
07069e2bb0 cmake: Add introspection module (Hennadii Stepanov)
27d687fc1f cmake: Add `config/bitcoin-config.h` support (Hennadii Stepanov)
fe5cdace5f cmake: Print compiler and linker flags in summary (Hennadii Stepanov)
70683884c5 cmake: Introduce interface libraries to encapsulate common flags (Hennadii Stepanov)
a2317e27b7 cmake: Add root `CMakeLists.txt` file (Hennadii Stepanov)
Pull request description:
This PR introduces a new CMake-based build system, which is a drop-in replacement for the current Autotools-based build system.
ML announcement: https://groups.google.com/g/bitcoindev/c/hgKkfQWzrTo
As discussed during the recent CoreDev meetup in April, the switch from Autotools to CMake is intended to happen as soon as possible after branching 28.x off, which means that 29.0 will be built using CMake.
This PR branch is essentially the [staging branch](https://github.com/hebasto/bitcoin/tree/cmake-staging), with every change reviewed and tested by a group of contributors, including (in alphabetical order):
- [**achow101**](https://github.com/achow101)
- [**fanquake**](https://github.com/fanquake)
- [**maflcko**](https://github.com/maflcko)
- [**m3dwards**](https://github.com/m3dwards)
- [**pablomartin4btc**](https://github.com/pablomartin4btc)
- [**real-or-random**](https://github.com/real-or-random)
- [**ryanofsky**](https://github.com/ryanofsky)
- [**sipsorcery**](https://github.com/sipsorcery)
- [**TheCharlatan**](https://github.com/TheCharlatan)
- [**theStack**](https://github.com/theStack)
- [**theuni**](https://github.com/theuni)
- [**vasild**](https://github.com/vasild)
Reviewing in a separate staging repo was suggested in https://github.com/bitcoin/bitcoin/pull/27060#issuecomment-1431798320.
The accompanying changes to the OSS-Fuzz project are available in https://github.com/hebasto/oss-fuzz/pull/8.
Please refer to the [build options parity table](https://gist.github.com/hebasto/2ef97d3a726bfce08ded9df07f7dab5e). The "auto" value is no longer available; non-default values must be specified explicitly. Additionally, the new default values have been chosen to suit the everyday build experience for the majority of developers.
System requirements for using the CMake-based build system:
- CMake >= 3.22 (if not available in your system's repository, it can be downloaded from https://cmake.org/download/)
- a build tool of your choice:
- any Make (GNU Make is no longer a requirement); GNU Make is still required to build depends
- Ninja (https://ninja-build.org/)
- MSBuild
- Xcode
A note for Windows users: The default installation of the latest version of MSVC 17.10.4 includes both CMake 3.28.3 and the vcpkg package manager).
---
We, the build system developers, kindly ask reviewers to refrain from making suggestions that are not directly related to the migration process or can be implemented separately. Bugs in the scripts and errors in the updated documentation should be the focus of this PR. Please be advised that comments not aligned with this PR's goal may be ignored.
Thank you all for your understanding.
ACKs for top commit:
maflcko:
review ACK 41051290ab🐥
sipsorcery:
ACK 41051290ab.
vasild:
ACK 41051290ab
TheCharlatan:
ACK 41051290ab
pablomartin4btc:
tACK 41051290ab
i-am-yuvi:
tACK [`4105129`](41051290ab)
theuni:
ACK 41051290ab.
fanquake:
ACK 41051290ab
Tree-SHA512: 6c1445054436c6c00ad63bfa0f19d64091a2b25c9bd694f85bf2218ac358ffb774d6c000685b3ca1e9b50401babed989fa2a0694b774c211d226bfd1944c9b39
setup_clean_chain=True is deleted so it uses the default.
Also, vout is now returned from send_to_address,
so now there is no need to fetch it manually
Also remove not-needed code that was used with the old
transaction handling.
18d65d2772 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a329 node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb544 util: remove unused IsHexNumber (stickies-v)
8a44d7d3c1 node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87737 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbfcbe test: unittest chainstatemanager_args (stickies-v)
Pull request description:
Since fad2991ba0, `uint256S` has been [deprecated](fad2991ba0 (diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138)) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. _(see also #30532)_
This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change.
The main behaviour change introduced in this PR is:
- `-minimumchainwork` will raise an error when input is longer than 64 hex digits
- `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits
- test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits
After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.
ACKs for top commit:
hodlinator:
re-ACK 18d65d2772
l0rinc:
ACK 18d65d2772
achow101:
ACK 18d65d2772
ryanofsky:
Code review ACK 18d65d2772. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.
Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86
fa5b58ea01 test: Avoid intermittent block download timeout in p2p_ibd_stalling (MarcoFalke)
Pull request description:
Fixes#30704
The goal of the test is to check the stalling timeout, not the block download timeout.
On extremely slow hardware (for example qemu virtual hardware), downloading the 1023 blocks may take longer than the block download timeout.
Fix it by pinning the time using mocktime, and only advance it when testing the stalling timeout.
ACKs for top commit:
tdb3:
CR ACK fa5b58ea01
brunoerg:
utACK fa5b58ea01
Tree-SHA512: 9a9221f264bea52be5e9fe81fd319f5a6970cd315cc5e9f5e2e049c5d84619b19b9f6f075cda8d34565c2d6c17a88fb57e195c66c271e40f73119a77caecb6d7
Because we don't have type checking for command-line/settings/config
args, strings are interpreted as 'false' for non-boolean args.
By convention, this "forces" us to interpret negated strings as 'true',
which conflicts with the negated option definition in all the settings
classes (they expect negated options to always be false and ignore any
other value preceding them). Consequently, when retrieving all "wallet"
values from the command-line/settings/config, we also fetch the negated
string boolean value, which is not of the expected 'string' type.
This mismatch leads to an internal fatal error, resulting in an unclean
shutdown during initialization. Furthermore, this error displays a poorly
descriptive error message:
"JSON value of type bool is not of expected type string"
This commit fixes the fatal error by ensuring that only string values are
returned in the "wallet" settings list, failing otherwise. It also improves
the clarity of the returned error message.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
e1d5dd732d test: check xor.dat recreated when missing (tdb3)
d1610962bf test: add null block xor key (tdb3)
1ad999b9da refactor: lift NUM_XOR_BYTES (tdb3)
d8399584dd refactor: move read_xor_key() to TestNode (tdb3)
d43948c3ef refactor: use unlink rather than os.remove (tdb3)
c8176f758b test: add blocks_key_path (tdb3)
Pull request description:
Builds on PR #30657.
Refactors `read_xor_key()` from `util.py` to `test_node.py` (comment https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1723358327)
Adds a check that `xor.dat` is created when missing (comment https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717724161)
Help states:
```
-blocksxor
Whether an XOR-key applies to blocksdir *.dat files. The created XOR-key
will be zeros for an existing blocksdir or when `-blocksxor=0` is
set, and random for a freshly initialized blocksdir. (default: 1)
```
ACKs for top commit:
maflcko:
ACK e1d5dd732d
achow101:
ACK e1d5dd732d
theStack:
re-ACK e1d5dd732d
brunoerg:
reACK e1d5dd732d
Tree-SHA512: 325912ef646ec88e0a58e1ece263a2b04cbc06497e8fe5fcd603e509e80c6bcf84b09dd52dfac60e23013f07fc2b2f6db851ed0598649c3593f452c4a1424bd9
