33381ea530 scripted-diff: Modernize nLocalServices to m_local_services (Fabian Jahr)
Pull request description:
The type of the `nLocalServices` variable was changed to `std::atomic<ServiceFlags>` in #30807 and I suggested the variable name to get updated with a scripted diff along with it. It wasn't included in the PR but I am still suggesting to do it as a follow-up since I had already prepared the commit.
ACKs for top commit:
sipa:
utACK 33381ea530
achow101:
ACK 33381ea530
furszy:
utACK 33381ea530
jonatack:
ACK 33381ea530
theStack:
ACK 33381ea530
Tree-SHA512: 407ea9eac694f079aa5b5c1611b5874d7a0897ba6bc3aa0570be94afe1bf3a826657b6890b6597c03c063e95b9dc868f0bdfbfc41e77ec7e06f5b045bf065c71
fa22e5c430 refactor: Remove dead code that assumed tip == nullptr (MarcoFalke)
fa2e443965 refactor: Replace g_genesis_wait_cv with m_tip_block_cv (MarcoFalke)
fa7f52af1a refactor: Use wait_for predicate to check for interrupt (MarcoFalke)
5ca28ef28b refactor: Split up NodeContext shutdown_signal and shutdown_request (Ryan Ofsky)
fad8e7fba7 bugfix: Mark m_tip_block_cv as guarded by m_tip_block_mutex (MarcoFalke)
fa18586c29 refactor: Add missing GUARDED_BY(m_tip_block_mutex) (MarcoFalke)
fa4c075033 doc: Clarify waitTipChanged docs (MarcoFalke)
Pull request description:
`g_genesis_wait_cv` is similar to `m_tip_block_cv` but shuffling everything through a redundant `boost::signals2`.
So remove it, along with some other dead code, as well as minor fixups.
ACKs for top commit:
ryanofsky:
Code review ACK fa22e5c430 (just rebased since last review)
Sjors:
ACK fa22e5c430
TheCharlatan:
ACK fa22e5c430
Tree-SHA512: a2cb59b651aaf85a3574723adfe403487566788ad945933b0458816ccc841fce08ca77b31afbd2d6adb5bf1deed7229c028bee74fb4bbaf6576e9edcfa0ad817
This combines the clusterlin_add_dependency and clusterlin_cluster_serialization
fuzz tests into a single clusterlin_depgraph_sim fuzz test. This tests starts
from an empty DepGraph and performs a arbitrary number of AddTransaction,
AddDependencies, and RemoveTransactions operations on it, and compares the
resulting state with a naive reimplementation.
This commits introduces support in DepGraph for the transaction positions to be
non-continuous. Specifically, it adds:
* DepGraph::RemoveTransactions which removes 0 or more positions from a DepGraph.
* DepGraph::Positions() to get a set of which positions are in use.
* DepGraph::PositionRange() to get the highest used position in a DepGraph + 1.
In addition, it extends the DepGraphFormatter format to support holes in a
compatible way (it serializes non-holey DepGraphs identically to the old code,
and deserializes them the same way)
This changes DepGraph::AddDependency into DepGraph::AddDependencies, which takes
in a single child, but a set of parent transactions, making them all dependencies
at once.
This is important for performance. N transactions can have O(N^2) parents combined,
so constructing a full DepGraph using just AddDependency (which is O(N) on its own)
could take O(N^3) time, while doing the same with AddDependencies (also O(N) on its
own) only takes O(N^2).
Notably, this matters for DepGraphFormatter::Unser, which goes from O(N^3) to O(N^2).
Co-Authored-By: Greg Sanders <gsanders87@gmail.com>
This does not change the serialization format.
It turns out that it is unnecessary to keep track of the order of transactions
in the so-far reconstructed DepGraph to decide how far from the end to insert
a new transaction.
This commit does not change the serialization format. Its purpose is making a
few changes already in order to reduce the diff size of the later commit that
introduces support for holes in DepGraph.
The previous approach was to immediately construct a transaction as soon as its
feerate was known in a preliminary position, and then undo that, and place it
in the correct position once the position information is known (such that a
deserialization error in between would not result in an inconsistent state).
The new approach is to delay the actual transaction creation until all its
information is known, avoiding the need to undo and redo. This requires a
different means of determining whether dependencies are redundant, but that has
the advantage that a later commit can apply all dependencies at once, reducing
the complexity of deserialization.
A fuzz test already relies on these operations, and a future commit will need
the same logic too. Therefore, abstract them out into proper member functions,
with proper testing.
This has been unused since #29648.
Noticed while running a newer version of clang-tidy (19.1.1):
```bash
[127/391][6.2s] /opt/homebrew/opt/llvm/bin/clang-tidy -p=build -quiet --config-file=/bitcoin/src/.clang-tidy /bitcoin/src/test/script_tests.cpp
bitcoin/src/test/script_tests.cpp:126:25: error: local copy 'tx2' of the variable 'tx' is never modified and never used; consider removing the statement [performance-unnecessary-copy-initialization,-warnings-as-errors]
126 | CMutableTransaction tx2 = tx;
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
127 | BOOST_CHECK_MESSAGE(VerifyScript(scriptSig, scriptPubKey, &scriptWitness, flags, MutableTransactionSignatureChecker(&tx, 0, txCredit.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &err) == expect, message);
512 warnings generated.
```
It is bad, because it is both printed for non-GUI users and does not
convey additional information.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
80761afced qt6: Handle different signatures of `QANEF::nativeEventFilter` (Hennadii Stepanov)
Pull request description:
Split from https://github.com/bitcoin/bitcoin/pull/30997.
This PR ensures compatibility across all supported Qt versions.
For more details, please refer to 3b38c73c7f.
No behaviour change.
ACKs for top commit:
maflcko:
lgtm ACK 80761afced
promag:
Code review ACK 80761afced.
Tree-SHA512: a265e1c33cc7da37003bb0e6fd40950acb5e948ca9ec63a59a79c5e2a1894334f48d5565539c91d4d777b48a589366958df1498eaa6935e3b7fb534493adb51a
fee4cba484 gui: Fix proxy details display in Options Dialog (pablomartin4btc)
Pull request description:
Currently, setting up a proxy (whether SOCKS5 or Tor) with an IPv6 address works correctly via the command line or configuration file in both `bitcoind` and `bitcoin-qt` (also from the UI the ipv6 address gets saved properly in `settings.json`). However, the UI does not reflect this properly, which can create confusion. Since some ISPs and VPNs still experience issues with IPv6, users may mistakenly think there is a problem with Bitcoin Core, when in fact the proxy setup is functioning as expected.
So this PR ensures that the proxy IP is displayed correctly in the UI when using an IPv6 address.
No functionality impact; changes only affect UI display.
<details>
<summary>Click her to see <b>before</b> and <b>after</b> screenshots.</summary>
- Before:
- After:
</details>
---
<details>
<summary>Test instructions</summary>
(Ubuntu 22.04)
1. Start ssh service on localhost.
`ssh -D [::1]:1080 -f -C -q -N localhost`
2. Check that the service is up and running.
```
ps aux | grep ssh
pepe 2860289 0.0 0.0 20456 5576 ? Ss 06:59 0:00 ssh -D [::1]:1080 -f -C -q -N localhost
```
3. Check with `bitcoind` if it works correctly.
`bitcoind -onlynet=ipv6 -proxy=[::1]:1080`
4. Check for established connections.
```
netstat -natl |grep 1080
tcp6 0 0 ::1:1080 :::* LISTEN
tcp6 0 0 ::1:47610 ::1:1080 ESTABLISHED
tcp6 0 0 ::1:1080 ::1:47610 ESTABLISHED
tcp6 0 0 ::1:1080 ::1:47606 TIME_WAIT
```
```./build/src/bitcoin-cli getpeerinfo
[
{
"id": 0,
"addr": "[2a01:4f9:4a:2a07::2]:8333",
"addrbind": "[::1]:47638",
"network": "ipv6",
...
```
5. Stop `bitcoind` and run `bitcoin-qt` adding the corresponding configuration in `settings.json`.
```
{
"onlynet": "ipv6",
"proxy": "[::1]:1080",
}
```
6. Open the Peers window to check available connections or run `getpeerinfo` on the rpc-console window.
7. Same can be done for Tor setting up `tor` service (I'll add instructions later) and configuring on its default port 9050 and forcing `"onlynet": "onion"` to verify easily the net traffic.
</details>
---
Thanks jarolrod and vasild for your help on validating ipv6 was not broken.
ACKs for top commit:
vasild:
ACK fee4cba484
promag:
Code review ACK fee4cba484.
hebasto:
ACK fee4cba484, I have reviewed the code and it looks OK.
Tree-SHA512: 4be9052569ccb1e17ce94fb15691debf0651fa172ed1a83d60696d10f20d469b19d70a979b65322951f5783cd7582d55b39b669edb588e20404d8d10e767c49a
98c1536852 test: add getorphantxs tests (tdb3)
93f48fceb7 test: add tx_in_orphanage() (tdb3)
34a9c10e8c rpc: add getorphantxs (tdb3)
f511ff3654 refactor: move verbosity parsing to rpc/util (tdb3)
532491faf1 net: add GetOrphanTransactions() to PeerManager (tdb3)
91b65adff2 refactor: add OrphanTxBase for external use (tdb3)
Pull request description:
This PR adds a new hidden rpc, `getorphantxs`, that provides the caller with a list of orphan transactions. This rpc may be helpful when checking orphan behavior/scenarios (e.g. in tests like `p2p_orphan_handling`) or providing additional data for statistics/visualization.
```
getorphantxs ( verbosity )
Shows transactions in the tx orphanage.
EXPERIMENTAL warning: this call may be changed in future releases.
Arguments:
1. verbosity (numeric, optional, default=0) 0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex
Result (for verbose = 0):
[ (json array)
"hex", (string) The transaction hash in hex
...
]
Result (for verbose = 1):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
]
},
...
]
Result (for verbose = 2):
[ (json array)
{ (json object)
"txid" : "hex", (string) The transaction hash in hex
"wtxid" : "hex", (string) The transaction witness hash in hex
"bytes" : n, (numeric) The serialized transaction size in bytes
"vsize" : n, (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
"weight" : n, (numeric) The transaction weight as defined in BIP 141.
"expiration" : xxx, (numeric) The orphan expiration time expressed in UNIX epoch time
"from" : [ (json array)
n, (numeric) Peer ID
...
],
"hex" : "hex" (string) The serialized, hex-encoded transaction data
},
...
]
Examples:
> bitcoin-cli getorphantxs 2
> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "getorphantxs", "params": [2]}' -H 'content-type: application/json' http://127.0.0.1:8332/
```
```
$ build/src/bitcoin-cli getorphantxs 2
[
{
"txid": "50128aac5deab548228d74d846675ad4def91cd92453d81a2daa778df12a63f2",
"wtxid": "bb61659336f59fcf23acb47c05dc4bbea63ab533a98c412f3a12cb813308d52c",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
1
],
"hex": "020000000001010b992959eaa2018bbf31a4a3f9aa30896a8144dbd5cfaf263bf07c0845a3a6620000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
},
{
"txid": "330bb7f701604a40ade20aa129e9a3eb8a7bf024e599084ca1026d3222b9f8a1",
"wtxid": "b7651f7d4c1a40c4d01f6a1e43a121967091fa0f56bb460146c1c5c068e824f6",
"bytes": 133,
"vsize": 104,
"weight": 415,
"expiration": 1725663854,
"from": [
2
],
"hex": "020000000001013600adfe41e0ebd2454838963d270916d2b47239c9eebb93a992b720d3589a080000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
}
]
```
ACKs for top commit:
glozow:
reACK 98c1536852
hodlinator:
re-ACK 98c1536852
danielabrozzoni:
ACK 98c1536852
pablomartin4btc:
tACK 98c1536852
itornaza:
reACK 98c1536852
Tree-SHA512: 66075f9faa83748350b87397302100d08af92cbef5fadb27f2b4903f028c08020bf34a23e17262b41abb3f379ca9f46cf6cd5459b8681f2b83bffbbaf3c03ff9
5be34bacf6 qt: Fix linking when configured with `-DENABLE_WALLET=OFF` (Hennadii Stepanov)
Pull request description:
Split from https://github.com/bitcoin/bitcoin/pull/30997.
When building with Qt 6 in my dev branch, I encountered a linker error when configured with `-DENABLE_WALLET=OFF`:
```
$ cmake -B build -DENABLE_WALLET=OFF -DBUILD_GUI=ON
$ cmake --build build -t bitcoin-qt
<snip>
[100%] Linking CXX executable bitcoin-qt
/usr/bin/ld: libbitcoinqt.a(rpcconsole.cpp.o): in function `QtPrivate::MetaObjectForType<WalletModel const*, void>::metaObjectFunction(QtPrivate::QMetaTypeInterface const*)':
/usr/include/x86_64-linux-gnu/qt6/QtCore/qmetatype.h:903:(.text._ZN9QtPrivate17MetaObjectForTypeIPK11WalletModelvE18metaObjectFunctionEPKNS_18QMetaTypeInterfaceE[_ZN9QtPrivate17MetaObjectForTypeIPK11WalletModelvE18metaObjectFunctionEPKNS_18QMetaTypeInterfaceE]+0x2b): undefined reference to `WalletModel::staticMetaObject'
/usr/bin/ld: libbitcoinqt.a(rpcconsole.cpp.o): in function `QMetaTypeIdQObject<WalletModel const*, 8>::qt_metatype_id()':
/usr/include/x86_64-linux-gnu/qt6/QtCore/qmetatype.h:1313:(.text._ZZN9QtPrivate16QMetaTypeForTypeIPK11WalletModelE17getLegacyRegisterEvENUlvE_4_FUNEv[_ZZN9QtPrivate16QMetaTypeForTypeIPK11WalletModelE17getLegacyRegisterEvENUlvE_4_FUNEv]+0x53): undefined reference to `WalletModel::staticMetaObject'
collect2: error: ld returned 1 exit status
gmake[3]: *** [src/qt/CMakeFiles/bitcoin-qt.dir/build.make:154: src/qt/bitcoin-qt] Error 1
gmake[2]: *** [CMakeFiles/Makefile2:2107: src/qt/CMakeFiles/bitcoin-qt.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:2114: src/qt/CMakeFiles/bitcoin-qt.dir/rule] Error 2
gmake: *** [Makefile:998: bitcoin-qt] Error 2
```
This PR resolves the issue.
ACKs for top commit:
promag:
ACK 5be34bacf6, all other changes in 33657e1c958146312e4c68765a92871920401396 are not required, makes the code harder to read.
pablomartin4btc:
tACK 5be34bacf6
Tree-SHA512: d10da665384e6539b8e9106dc905ba30e9e57cd4603fc7e5eb893fc899f55f26889c570687fa5daf55c6fc5bf4fcfcfcd9b70822cfe637f31f9151bb653b7941
5625840c11 qt6, test: Handle deprecated `QVERIFY_EXCEPTION_THROWN` (Hennadii Stepanov)
cb750b4b40 qt6, test: Use `qWarning()` instead of `QWARN()` macro (Hennadii Stepanov)
Pull request description:
Split from https://github.com/bitcoin/bitcoin/pull/30997.
This PR ensures compatibility across all supported Qt versions.
---
This PR can be tested on macOS using the first commit from https://github.com/bitcoin/bitcoin/pull/30997 and Homebrew's `qt` package.
ACKs for top commit:
promag:
Code review ACK 5625840c11.
Sjors:
tACK 5625840c11
Tree-SHA512: e7307eaf0027c6addc9481ba91ed31b81554ffb0d2ba77938e68915c9d490a7962e55a330f97ea31d49bbfb30f92773c3af3afc867a4215d00752405d7e3bb6d
a7498cc7e2 Fix bug in p2p_headers_presync harness (marcofleon)
Pull request description:
The calculation for the test chain's work (`total_work`) should be outside of the loop. Previously, `total_work` was being miscalculated due to multiple additions of work from the same headers. Now, each header's work is only counted once, providing an accurate total.
https://github.com/bitcoin/bitcoin/pull/30918 followup
ACKs for top commit:
dergoegge:
utACK a7498cc7e2
instagibbs:
ACK a7498cc7e2
glozow:
makes sense, utACK a7498cc7e2
mzumsande:
ACK a7498cc7e2
Tree-SHA512: b95f25dcf7ace220e30f1d72f50d85ee18777467927c0cc1ed8582b390cb7185ffc0e2f127309eb083044fb41f5a13fce5ebb15b7952718a899bafff26921be8
fa2b7d8d6b Remove redundant unterminated-logprintf tidy check (MarcoFalke)
bbbb2e43ee log: Enforce trailing newline, Remove redundant m_started_new_line (MarcoFalke)
Pull request description:
There are many problems around missing a trailing newline while logging:
* All log lines are currently terminated by a trailing newline. This means any runtime code trying to handle a "missing" newline is currently dead code.
* Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
* It requires extra code like `m_started_new_line` to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835.
* It requires a standalone `unterminated-logprintf` clang-tidy plugin, which is unmaintained (no one updated it for the new log function names), probably harder to maintain than normal C++ code (because it requires clang AST matcher knowledge), brittle (it can fail to detect issues at any time, if it goes out-of-sync, or be explicitly disabled via `NOLINT`), and annoying for devs (it is slow and intricate to run locally and thus only effectively run on CI or via the CI scripts).
Fix all issues by enforcing the trailing newline in logs directly in the code. Then remove all the other stuff.
This refactor does not change behavior.
ACKs for top commit:
stickies-v:
re-ACK fa2b7d8d6b
achow101:
ACK fa2b7d8d6b
ryanofsky:
Code review ACK fa2b7d8d6b. Just comment and test cleanup since last review
Tree-SHA512: 10ed420f6c2fdb0f491d6c880be8dd2e8beef628f510adebadf4c3849d9f5e28906519d5cbaeb295f4c7c1b07c4c88a9905b3cfe30fee3a2c91ac9fd24ae6755
Instead of having a single NodeContext::shutdown member that is used both to
request shutdowns and check if they have been requested, use separate members
for each. Benefits of this change:
1. Should make code a little clearer and easier to search because it is easier
to see which parts of code are triggering shutdowns and which parts are just
checking to see if they were triggered.
2. Makes it possible for init.cpp to specify additional code to run when a
shutdown is requested, like signalling the m_tip_block_cv condition variable.
Motivation for this change was to remove hacky NodeContext argument and
m_tip_block_cv access from the StopRPC function, so StopRPC can just be
concerned with RPC functionality, not other node functionality.
This is not strictly required, but all places using m_tip_block_cv
(except shutdown) already take the lock. The annotation makes it easier
to catch potential deadlocks before review.
Adding the missing lock to the shutdown sequence is a bugfix.
An alternative would be to take the lock and release it before
notifying, see
https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1778899716
It should be obvious that a wait is not needed if the tip does not
match.
Also, remove a comment that the blockTip notification was only meant for
the "UI". (It is used by other stuff for a long time)
e9d60af988 refactor: Replace init retry for loop with if statement (TheCharlatan)
c1d8870ea4 refactor: Move most of init retry for loop to a function (TheCharlatan)
781c01f580 init: Check mempool arguments in AppInitParameterInteractions (TheCharlatan)
Pull request description:
The for loop around the chain loading logic in `init.cpp` allows users of the GUI to retry once on failure with reindexing without having to manually set the reindex flag on startup. However this current mechanism has problems:
* It is badly documented and has led to confusion among developers and bugs making it into master. Examples:
* https://github.com/bitcoin/bitcoin/pull/28830/files#r1598392660
* https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2120741121
* It can only ever iterate once, making the choice of a for loop questionable.
* With its large scope it is easy for re-entry bugs to sneak in. Example:
* https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1601589963
Attempt to fix this by moving the bulk of the logic into a separate function and replacing the for loop with a simpler `if` statement.
The diff's in this pull request are best reviewed with `--color-moved-ws=ignore-all-space --color-moved=dimmed-zebra`. The error behaviour can be tested by either manually making `LoadChainstate` return a failure, or deleting some of the block index database files.
ACKs for top commit:
maflcko:
review ACK e9d60af988🚸
josibake:
crACK e9d60af988
achow101:
ACK e9d60af988
ryanofsky:
Code review ACK e9d60af988. Nice change to make AppInitMain shorter and more understandable.
Tree-SHA512: 5e5c0a5fd1b32225346450f8482f0ae8792e1557cdab1518112c1a3ec3a4400b64f5796692245cc5bf2f9010bb97b3a9558f07626a285ccd6ae525dd671ead13
5c7cacf649 ci: Remove natpmp build option and libnatpmp dependency (laanwj)
7e7ec984da doc: Remove mention of natpmp build options (laanwj)
061c3e32a2 depends: Drop natpmp and associated option from depends (laanwj)
20a18bf6aa build: Drop libnatpmp from build system (laanwj)
7b04709862 qt: Changes for built-in PCP+NAT-PMP (laanwj)
52f8ef66c6 net: Replace libnatpmp with built-in NATPMP+PCP implementation in mapport (laanwj)
97c97177cd net: Add PCP and NATPMP implementation (laanwj)
d72df63d16 net: Use GetLocalAddresses in Discover (laanwj)
e02030432b net: Add netif utility (laanwj)
754e425438 crypto: Add missing WriteBE16 function (laanwj)
Pull request description:
Continues #30005. Closes #17012..
This PR adds PCP (Port Control Protocol) from [RFC6887](https://datatracker.ietf.org/doc/html/rfc6887). This adds, in addition to the existing IPv4 port mapping (which now uses PCP, with fallback to NAT-PMP), support for IPv6 pinholing-that is, opening a port on the firewall to make it reachable.
PCP, like NAT-PMP is a simple UDP-based protocol, and the implementation is self-contained, so this gets rid of lthe libnatpnp dependency without adding a new one. It should otherwise be a drop-in replacement. NAT-PMP fallback is implemented so this will not make router support worse.
For now it is disabled by default, though in the future (not in this PR) we could consider enable it by default to increase the number of connectable nodes without adding significant attack surface.
To test:
```bash
bitcoind -regtest -natpmp=1 -debug=net
```
(most of the changes in this PR are, ironically, removing the libnatpmp dependency and associated build system and build docs)
## TODO
- [x] Default gateway discovery on Linux / FreeBSD
- [x] Default gateway discovery on Windows
- [x] Default gateway discovery on MacOS
- [x] Either solve FreeBSD compile issue (probably upstream issue) or remove FreeBSD support
## Things to consider for follow-up PRs
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658764974 avoid unreachable nets (not given to -onlynet=)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236 could announce an addr:port where we do not listen (no -bind)
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1684368824 could announce the wrong port because it uses GetListenPort()
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1679709347 if we requested one port but another was assigned, then which one to use in the renewal?
- https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772017020 Use `GetAdapterAddresses` to discover local addresses for Windows
ACKs for top commit:
Sjors:
ACK 5c7cacf649
achow101:
ACK 5c7cacf649
vasild:
ACK 5c7cacf649
Tree-SHA512: e35b69e56d5f5449a3d48a799f9b7b65107c65eeb3e245c2c1e9d42221e469ca5ead90afae423160601cd664dd553a51c859e04f4492f335b064aae3bf23e3bc
The `QWARN()` macro internally uses `QTest::qWarn()`, which has been
deprecated since Qt 6.3. Replacing it with `qWarning()` ensures
compatibility across all Qt versions.
6c3c619b35 test: generalize HasReason and use it in FailFmtWithError (Lőrinc)
Pull request description:
Standardized boost exception checking in recent tests introduced in https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756493521 by extending `HasReason` to accept `const char*` through `string_view` in `operator()`.
Note that `HasReason` only checks partial matches - but since we're specifying the whole error string, it doesn't affect us in this case.
ACKs for top commit:
maflcko:
review ACK 6c3c619b35
hodlinator:
ACK 6c3c619b35
Tree-SHA512: 740fb18b8fea78e4eb9740ceb0fe75d37246c28cfa2638b9d093e9514dd6d7926cc5be9ec57f8027cca3aa9d616e8c54322d2401cfa67fd25282f7816e63532d
The current order is incorrect:
```bash
./build/src/bitcoin-cli loadtxoutset -rpcclienttimeout=0 utxo-840000.dat
error code: -1
error message:
loadtxoutset "path"
```
The Mining interface uses this function in the next commit
to calculate the coinbase merkle path. Stratum v2 uses
this to send a compact work template.
This partially undoes the change in 4defdfab94,
but is not a revert, because the implementation changed in the meantime.
This commit also documents the function.
1a33281766 doc: multiprocess documentation improvements (Ryan Ofsky)
d043950ba2 multiprocess: Add serialization code for BlockValidationState (Ryan Ofsky)
33c2eee285 multiprocess: Add IPC wrapper for Mining interface (Ryan Ofsky)
06882f8401 multiprocess: Add serialization code for vector<char> (Russell Yanofsky)
095286f790 multiprocess: Add serialization code for CTransaction (Russell Yanofsky)
69dfeb1876 multiprocess: update common-types.h to use C++20 concepts (Ryan Ofsky)
206c6e78ee build: Make bitcoin_ipc_test depend on bitcoin_ipc (Ryan Ofsky)
070e6a32d5 depends: Update libmultiprocess library for cmake headers target (Ryan Ofsky)
Pull request description:
Add Cap'n Proto wrapper for the Mining interface introduced in #30200, and its associated types.
This PR combined with #30509 will allow a separate mining process, like the one being implemented in https://github.com/Sjors/bitcoin/pull/48, to connect to the node over IPC, and create, manage, and submit block templates. (#30437 shows another simpler demo of a process using the Mining interface.)
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 1a33281766
TheCharlatan:
ACK 1a33281766
itornaza:
ACK 1a33281766
Tree-SHA512: 0791078dd6885dbd81e3d14c75fffff3da8d1277873af379ea6f9633e910c11485bb324e4cde3d936d50d343b16a10b0e8fc1e0fc6d7bdca7f522211da50c01e
The for loop has been a long standing source of confusion and bugs, both
because its purpose is not clearly documented and because the body of
the for loop contains a lot of logic.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
This makes it clearer which state is being mutated by the function and
facilitates getting rid of the for loop in the following commit. Move
creation of the required options into the function too, such that the
function takes fewer arguments and is more self-contained.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
The comparison of m_best_invalid with the tip of the respective chainstate
makes no sense for the background chainstate, and can lead to incorrect
error messages.
7bd3ee62f6 test: Use shell builtins in run_command test case (Ava Chow)
Pull request description:
Uses the [suggested command](https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363906135)
Fixes#30938
ACKs for top commit:
maflcko:
review ACK 7bd3ee62f6
hebasto:
ACK 7bd3ee62f6.
Tree-SHA512: 683b15cafaf0103eeadf872ea6ce9a7d884b2605d3dcf4e66b0173cdb149c24965e7c5fa62aaddf2ac55df3f449aeb787176992c96cfee5d0b86621259e1dfe9
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
Add support for passing CTransaction and CTransactionRef types to IPC
functions.
These types can't be passed currently because IPC serialization code currently
only supports deserializing types that have an Unserialize() method, which
CTransaction does not, because it is supposed to represent immutable
transactions. Work around this by adding a CustomReadField overload that will
call CTransaction's deserialize_type constructor.
These types also can't be passed currently because serializing transactions
requires TransactionSerParams to be set. Fix this by setting TX_WITH_WITNESS as
default serialization parameters for IPC code.
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
This adds an utility header with two functions that will be needed for
PCP, `QueryDefaultGateway` and `GetLocalAddresses`.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
5e190cd11f Replace CScript _hex_v_u8 appends with _hex (Lőrinc)
cac846c2fb Allow CScript's operator<< to accept spans, not just vectors (Lőrinc)
c78d8ff4cb prevector: avoid GCC bogus warnings in insert method (Lőrinc)
Pull request description:
Split out of https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1722326803.
Replace `_hex_v_u8` for `CScript` appends to `_hex`, to skip vector conversion before serializing to the `prevector` in `CScript`.
To enable both `unsigned char` and `std::byte` values, I've extracted the existing serialization to append the size & data in separate private methods to clarify that it does more than just a simple data insertion.
There were also discussion on eliminating the operators here completely to obviate when we're serializing fixed-size collections as raw bytes, and when we're prefixing them with their size - should also be done in a separate PR.
ACKs for top commit:
achow101:
ACK 5e190cd11f
ryanofsky:
Code review ACK 5e190cd11f. Looks good!
hodlinator:
re-ACK 5e190cd11f
Tree-SHA512: 27a646629e017b2a05416d5eb964dda8b25b900d466221eff7bfa1339ded443e1c5c4cf8ff20cb3bba915a2603787a9fa6f6ec12bc0b9415d9eb07b57289192b
f482d0e366 fuzz: reduce number of iterations in `crypto_aeadchacha20poly1305` target (brunoerg)
Pull request description:
By reducing the number of iterations we improve the performance of this target and may increase coverage.
Running with `-runs=100000` from qa-assets I noticed a significant performance improvement and an increase on cov:
master:
```
#100000 DONE cov: 567 ft: 4078 corp: 124/33Kb lim: 4096 exec/s: 793 rss: 499Mb
```
PR:
```
#100000 DONE cov: 568 ft: 3833 corp: 113/15188b lim: 1746 exec/s: 1250 rss: 544Mb
```
ACKs for top commit:
achow101:
ACK f482d0e366
marcofleon:
Tested ACK f482d0e366. Saw the same slight increase in coverage. Executed 100,000 runs several times and total time went from 30-35 sec to 20-25 sec.
stratospher:
ACK f482d0e. saw similar coverage stats
Tree-SHA512: 1a96dbc22a0aed396b7f8cc9b13534b7f20a461f64f167c69c650529d535e360499f1a501abc1f957f7541ee1860b36a5580aa488a1edbfa0270c9ed83ef741d
bc52cda1f3 fix use int32_t instead of int type for risczero compile with (-march=rv32i, -mabi=ilp32) (Simon)
Pull request description:
When compile bitcoin by the toolchain(`riscv32-unknown-elf-g++`) from risc0 , the compiler argument is `-march=rv32i, -mabi=ilp32`, which will get the error which due to not serialize the value of type int .
```
blockbody-guest: cargo:warning=In file included from depend/bitcoin/src/hash.h:14,
blockbody-guest: cargo:warning= from depend/bitcoin/src/script/interpreter.h:9,
blockbody-guest: cargo:warning= from depend/bitcoin/src/script/interpreter.cpp:6:
blockbody-guest: cargo:warning=depend/bitcoin/src/serialize.h: In instantiation of 'void Serialize(Stream&, const T&) [with Stream = HashWriter; T = int]':
blockbody-guest: cargo:warning=depend/bitcoin/src/hash.h:144:20: required from 'HashWriter& HashWriter::operator<<(const T&) [with T = int]'
blockbody-guest: cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1613:12: required from 'uint256 SignatureHash(const CScript&, const T&, unsigned int, int, const CAmount&, SigVersion, const PrecomputedTransactionData*) [with T = CTransaction; CAmount = long long int]'
blockbody-guest: cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1664:36: required from 'bool GenericTransactionSignatureChecker<T>::CheckECDSASignature(const std::vector<unsigned char>&, const std::vector<unsigned char>&, const CScript&, SigVersion) const [with T = CTransaction]'
blockbody-guest: cargo:warning=depend/bitcoin/src/script/interpreter.cpp:1785:16: required from here
blockbody-guest: cargo:warning=depend/bitcoin/src/serialize.h:776:7: error: request for member 'Serialize' in 'a', which is of non-class type 'const int'
blockbody-guest: cargo:warning= 776 | a.Serialize(os);
```
--------------
### Reason
"The toolchain from RISC Zero defines int and int32_t as different types, although they have the same width. This means that `src/compat/assumptions.h` compiles fine; however, the templated serialization code cannot accept values of type int. Fix the compilation on RISC Zero by serializing int32_t instead of int values.
This patch will explicitly use the `int32_t` type instead of `int` to avoid errors when compiling with the risc0 toolchain. Additionally, this patch will not change any behavior on platforms where compilation was previously successful.
Fixes#30747
ACKs for top commit:
maflcko:
review-only ACK bc52cda1f3
achow101:
ACK bc52cda1f3
TheCharlatan:
ACK bc52cda1f3
Tree-SHA512: ef880e7dfa1335bf2704ab17c0f506f17390b8259755674dfcd57131736492b2f4cfc36babda6902202b7c55a7513991e21f6634b0cd9b2b03baf4f1c0f8d78b
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
8466329127 chain: simplify `deleteRwSettings` code and improve it's doc (ismaelsadeeq)
f8d91f49c7 chain: dont check for null settings value in `overwriteRwSetting` (ismaelsadeeq)
df601993f2 chain: ensure `updateRwSetting` doesn't update to a null settings (ismaelsadeeq)
c8e2eeeffb chain: uniformly use `SettingsAction` enum in settings methods (ismaelsadeeq)
1e9e735670 chain: move new settings safely in `overwriteRwSetting` (ismaelsadeeq)
1c409004c8 test: remove wallet context from `write_wallet_settings_concurrently` (ismaelsadeeq)
Pull request description:
This PR addresses the remaining review comments from #30697
1. Disallowed overwriting settings values with a `null` value.
2. Uniformly used the `SettingsAction` enum in all settings methods instead of a boolean parameter.
3. Updated `overwriteRwSetting` to receive the `common::SettingsValue` parameter by value, enabling it to be moved safely.
4. Removed wallet context from the `write_wallet_settings_concurrently` unit test, as it is not needed.
ACKs for top commit:
achow101:
ACK 8466329127
ryanofsky:
Code review ACK 8466329127. Looks good, thanks for taking suggestions and applying them to the right commits. Only changes since last review were documentation improvements and simplifying delete method.
furszy:
Code review ACK 8466329127
Tree-SHA512: baf2f59ed5aac4a4bda0c84fb6554a466a40d1f7b52b61dc2ff293d83ae60e82b925b7003237b633fecb65eba3a4c108e69166046895d1295809fbe0de67b052
51f7668d31 addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)
Pull request description:
With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).
Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.
I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.
ACKs for top commit:
naumenkogs:
ACK 51f7668d31
stratospher:
ACK 51f7668d31. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
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
58499b00d0 refactor: move `SignSignature` helpers to test utils (Sebastian Falbesoner)
Pull request description:
These helpers haven't been used in production code since segwit was merged more than eight years ago (see commit 605e8473, PR #8149), so it seems appropriate to move them to the test utils module. As suggested by instagibbs, see https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1697515508.
ACKs for top commit:
instagibbs:
ACK 58499b00d0
pablomartin4btc:
ACK 58499b00d0
Tree-SHA512: a52d3b92b477246f2ceb57c3690d0229a492b65a15dae331faeae9d96e5907f7fe1176edc1530243e0f088586984fd7ba435a0a2d2f2531c04d076fdf3f4095f
7025942687 build: drop superfluous `HAVE_BUILD_INFO` define (Sebastian Falbesoner)
0dd662510c build: drop obj/ subdir for generated build.h, rename to bitcoin-build-info.h (Sebastian Falbesoner)
Pull request description:
As indicated by the TODO, the obj subdirectory is not needed anymore now for the generated build.h header, since autotools are gone and we don't have in-source builds anymore (see #30454, #30664). In the second commit the superflous `HAVE_BUILD_INFO` macro is dropped, as suggested in https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2292424496.
ACKs for top commit:
theuni:
utACK 7025942687
Tree-SHA512: 0a3b2cbbcf638344ceb74e5ba5a0fe2b1718427b23a18c8890258db36ce7177006a146178ed88d9c5ae956a5426f3844e86c1f4cca7c40946359742bffda983b
bitcoin-build-info.h should always be generated before clientversion.cpp
is compiled due to the following explicit dependency in src/CMakeLists.txt:
add_dependencies(bitcoin_clientversion generate_build_info)
Hence there is no need to gate the inclusion of that header with an
extra define.
Now that this file is not in a subfolder anymore, prefix it with
"bitcoin-" to avoid potential collisions. Also add "info" for a more
descriptive name.
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
Adds invalid rpcbind port checking to
`HTTPBindAddresses()`. While movement of
`CheckHostPortOptions()` in the previous
commit handles rcpbind port errors, updating
`HTTPBindAddresses()` port checking adds
a defensive measure for potential future
changes.
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&.
The waitforblock RPC method takes a hash argument and waits for that specific block. The waitfornewblock waits for any new block. This commit fixes the documentation.
Also signal m_tip_block_cv when StopRPC is called, for
consistency with g_best_block_cv. This is handled in
StopRPC instead of OnRPCStopped() because the latter
is deleted in a later commit.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
a240e150e8 streams: remove AutoFile::Get() entirely (Pieter Wuille)
e624a9bef1 streams: cache file position within AutoFile (Pieter Wuille)
Pull request description:
Fixes#30833.
Instead of relying on frequent `ftell` calls (which appear to cause a significant slowdown on some systems) in XOR-enabled `AutoFile`s, cache the file position within `AutoFile` itself.
ACKs for top commit:
achow101:
ACK a240e150e8
davidgumberg:
untested reACK a240e150e8
theStack:
Code-review ACK a240e150e8
Tree-SHA512: fd3681edc018afaf955dc7a41a0c953ca80d46c1129e3c5b306c87c95aae93b2fe7b900794eb8b6f10491f9211645e7939918a28838295e6873eb226fca7006f
e4e3b44e9c net: call `Select` with reachable networks in `ThreadOpenConnections` (brunoerg)
829becd990 addrman: change `Select` to support multiple networks (brunoerg)
f698636ec8 net: add `All()` in `ReachableNets` (brunoerg)
Pull request description:
This PR changes addrman's `Select` to support multiple networks and change `ThreadOpenConnections` to call it with reachable networks. It can avoid unnecessary `Select` calls and avoid exceeding the max number of tries (100), especially when turning a clearnet + Tor/I2P/CJDNS node to Tor/I2P/CJDNS. Compared to #29330, this approach is "less aggresive". It does not add a new init flag and does not impact address relay.
I did an experiment of calling `Select` without passing a network until it finds an address from a network that compose 20% ~ 25% of the addrman (limited to 100 tries).
ACKs for top commit:
achow101:
ACK e4e3b44e9c
vasild:
ACK e4e3b44e9c
naumenkogs:
ACK e4e3b44e9c
Tree-SHA512: e8466b72b85bbc2ad8bfb14471eb27d2c50d4e84218f5ede2c15a6fa3653af61b488cde492dbd398f7502bd847e95bfee1abb7e01092daba2236d3ce3d6d2268
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
bb3b980dfd validation: drop maximum -dbcache (Sjors Provoost)
Pull request description:
Due to recent UTXO set growth, the current maximum value for `-dbcache` of 16GB is ~just months away from being~ insufficient (for those who wish to complete IBD with the UTXO set held in RAM).
This drops the limit. It also adds a warning that it's up to users to check that they have enough RAM.
Fixes#28249.
---
A previous version of this PR increased the maximum to 64GB. It also made startup abort if the value provided is too high, rather than quietly round it down. But this didn't get much support.
ACKs for top commit:
achow101:
ACK bb3b980dfd
tdb3:
ACK bb3b980dfd
BenWestgate:
crACK bb3b980dfd.
Tree-SHA512: 8515fff468c2387a0b04bd9523ab1df46d6325738588b7550fabddbb8624817a583d95b95ea246407f9f0ff3e43e760cf7334621bec6af79710176328528a3ef
fc7b507e9a tidy: add clang-tidy `modernize-use-starts-ends-with` check (Roman Zeyde)
Pull request description:
ACKs for top commit:
jonatack:
re-ACK fc7b507e9a only change since my previous ACK is the commit message
achow101:
ACK fc7b507e9a
stickies-v:
ACK fc7b507e9a
hebasto:
ACK fc7b507e9a, I have reviewed the code and it looks OK.
Tree-SHA512: 334e0ff91b9b108a57cdfc12ee53685b792d377e11124c7c394b8f681a8168a8d65a56c7f884555238e65e97e9ad62ede52b79219ce258979e54abdd76721df1
a93c171faa Drop unneeded nullptr check from CreateNewBlock() (Sjors Provoost)
dd87b6dff3 Have createNewBlock return BlockTemplate interface (Sjors Provoost)
Pull request description:
Suggested in https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2225337100
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a [NewTemplate](https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#72-newtemplate-server---client) message message (which doesn't include transactions) as quickly as possible. It does not include the serialized block transactions.
ACKs for top commit:
achow101:
ACK a93c171faa
ryanofsky:
Code review ACK a93c171faa. Since last review, just rebased with no changes or conflicts
itornaza:
Code review ACK a93c171faa
TheCharlatan:
Re-ACK a93c171faa
Tree-SHA512: 17cb61eb5548e9d4a69e34dd732f68a06cde2ad3d82c8339efee704c7860d5de144d93b23d6ecd6ee4ec205844e5560ad0f6d3917822fa75bb8e640c5f51af9a
a97f43d63a fuzz: Add harness for p2p headers sync (marcofleon)
a0eaa4749f Add FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION in PoW check (marcofleon)
a3f6f5acd8 build: Automatically define FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for fuzz builds (marcofleon)
0c02d4b2bd net_processing: Make MAX_HEADERS_RESULTS a PeerManager option (marcofleon)
Pull request description:
This PR reopens https://github.com/bitcoin/bitcoin/pull/28043. It's a regression fuzz test for https://github.com/bitcoin/bitcoin/pull/26355 and [a couple bugs](ed6cddd98e) that were addressed in https://github.com/bitcoin/bitcoin/pull/25717. This should help us move forward with the [removal of mainnet checkpoints](https://github.com/bitcoin/bitcoin/pull/25725).
It seems like the main concern in https://github.com/bitcoin/bitcoin/pull/28043 was the global mock function for proof of work. This PR aims to be an improvement by replacing the previous approach with a fuzz build configured using `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. This ensures that the simplified test code will never be in a release binary. If we agree this is the way to go, there are some other places (for future targets) where this method could be used.
In this target, PoW isn't being tested, so the goal is to bypass the check and let the fuzzer do its thing. In the other harnesses where PoW is actually being fuzzed, `CheckProofOfWork` is now `CheckProofOfWorkImpl`. So, the only change to that function is in the name.
More about `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` can be found at https://llvm.org/docs/LibFuzzer.html#fuzzer-friendly-build-mode and https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md#d-modifying-the-target.
ACKs for top commit:
naumenkogs:
ACK a97f43d63a
dergoegge:
reACK a97f43d63a
instagibbs:
tested ACK a97f43d63a
brunoerg:
ACK a97f43d63a
Tree-SHA512: 60b0bc6aadd8ca4c39db9cbba2da2debaaf68afcb6a8dd75c1ce48ca9e3996948fda8020930b6771a424e0f7c41b0b1068db4aa7dbe517f8fc152f1f712058ad
9ad2fe7e69 clusterlin: only start/use search when enough iterations left (Pieter Wuille)
bd044356ed clusterlin: improve heuristic to decide split transaction (optimization) (Pieter Wuille)
71f2629398 clusterlin: include topological pot subsets automatically (optimization) (Pieter Wuille)
e20fda77a2 clusterlin: reduce computation of unnecessary pot sets (optimization) (Pieter Wuille)
6060a948ca clusterlin bench: add example hard cluster benchmarks (Pieter Wuille)
2965fbf203 clusterlin: track upper bound potential set for work items (optimization) (Pieter Wuille)
9e43e4ce10 clusterlin: use feerate-sorted depgraph in SearchCandidateFinder (Pieter Wuille)
b80e6dfe78 clusterlin: add reordering support for DepGraph (Pieter Wuille)
85a285a306 clusterlin: separate initial search entries per component (optimization) (Pieter Wuille)
e4faea9ca7 clusterlin bench: have low/high iter benchmarks instead of per-iter (Pieter Wuille)
Pull request description:
Part of cluster mempool: #30289
Depends on #30126, and was split off from it.
This improves the candidate search algorithm introduced in the previous PR with a variety of optimizations.
The resulting search algorithm largely follows Section 2 of [How to linearize your cluster](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303#h-2-finding-high-feerate-subsets-5), though with a few changes:
* Connected component analysis is performed inside the search algorithm (creating initial work items per component for each candidate), rather than once at a higher level. This duplicates some work but is significantly simpler in implementation.
* No ancestor-set based presplitting inside the search is performed; instead, the `best` value is initialized with the best topologically valid set known to the LIMO algorithm before search starts: the better one out of the highest-feerate remaining ancestor set, and the highest-feerate prefix of remaining transactions in `old_linearization`.
* Work items are represented using an included set *inc* and an undefined set *und*, rather than included and excluded.
* Potential sets *pot* are not computed for work items with empty *inc*.
At a high level, the only missing optimization from that post is bottleneck analysis; my thinking is that it only really helps with clusters that are already relatively cheap to linearize (doing so would need to be done at a higher level, not inside the search algorithm).
---
Overview of the impact of each commit here on linearize performance:
* **[clusterlin bench: have low/high iter benchmarks instead of per-iter](21a184db63)**: no impact
* **[separate initial search entries per component (optimization)](c84c5c86ba)**: reduce iterations, increase start-up cost
* **[add reordering support for DepGraph](019ff29609)**: no impact
* **[use feerate-sorted depgraph in SearchCandidateFinder](8e27dd5a22)**: typically reduce iterations, increase start-up cost
* **[track upper bound potential set for work items](781e0fb3aa)**: reduce iterations, increase cost per iteration
* **[reduce computation of unnecessary pot sets](9fe834fa97)**: reduce cost per iteration
* **[include topological pot subsets automatically](30612710a4)**: reduce iterations, increase cost per iteration
* **[improve heuristic to decide split transaction](1880c00ab1)**: typically reduce iterations, increase cost per iteration
* **[only start/use search when enough iterations left](12760a57b3)**: just account for start-up cost as equivalent iterations
ACKs for top commit:
sdaftuar:
ACK 9ad2fe7e69
instagibbs:
reACK 9ad2fe7e69
glozow:
reACK 9ad2fe7e69, just have a question about the docs
Tree-SHA512: 108bcbb0676f36071eb83954059b5f3d6646c745015b644a2a5d7f5a8ac9424c2d01d339fa6318a3aff4cf313308e85bb80b0090899720a3fcba027b8025590a
bc7900f33d kernel: Move background load thread to node context (TheCharlatan)
Pull request description:
The thread handle is never used by the ChainstateManager, so move it out and into the node context. Users of the kernel library now no longer have to manually join the thread when destructing the ChainstateManager.
ACKs for top commit:
maflcko:
ACK bc7900f33d🔄
achow101:
ACK bc7900f33d
ryanofsky:
Code review ACK bc7900f33d. Nice cleanup
jonatack:
Light ACK bc7900f33d
stickies-v:
ACK bc7900f33d
Tree-SHA512: add9c4823731324e3db50f95e023e99d55db7cc75c69083ae7c9c2157e5540968caa6cf10674aa4901f91366b02ebb1ff18bb977fec0a46431e2196448958b9d
07f4cebe57 refactor: move m_is_inbound out of CNodeState (Sergi Delgado Segura)
Pull request description:
`m_is_inbound` cannot be changed throughout the life of a `Peer`. However, we are currently storing it in `CNodeState`, which requires locking `cs_main` in order to access it. This can be moved to the outside scope and only require `m_peer_mutex`.
This is a refactor in preparation for Erlay reworks.
ACKs for top commit:
maflcko:
ACK 07f4cebe57 🗞
achow101:
ACK 07f4cebe57
marcofleon:
ACK 07f4cebe57
naumenkogs:
ACK 07f4cebe57
Tree-SHA512: bcc77135646c697204a4605971774cb3ccdf11b3e363a7339bfb4d1678de1681d6ca642dc467f7d71834a94dd56e05367e7fd32a3e6dc6be30c89342f39bf695
282f0e9255 Unit test runner documentation fix and improvements (Jon Atack)
Pull request description:
Running `test_bitcoin --help` prints the list of arguments that may be passed, not the list of tests, so fix that.
Improve the content and order of the unit test documentation.
ACKs for top commit:
pablomartin4btc:
re-ACK 282f0e9255
tdb3:
re ACK 282f0e9255
Tree-SHA512: 0d25108ab641bcd9b53f99d139afeec90a34f44d5b00c3c677f7539d87782440a28fadc348663b8c28ace43552834737b9c1e8f5517c68edc8547695a9cb5063
- Running `test_bitcoin --help` prints the list of arguments that may be passed,
not the list of tests, so fix that.
- Improve the content and order of the unit test documentation.
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.
This avoids calling ReadRawBlockFromDisk() when the block is expected
not to be available because we haven't downloaded it yet and only know
the header.
The thread handle is never used by the ChainstateManager, so move it out
and into the node context. Users of the kernel library now no longer
have to manually join the thread when destructing the ChainstateManager.
An external program that uses the Mining interface may need quick access to some information in the block template, while it can wait a bit longer for the full raw transaction data.
This would be the case for a Stratum v2 Template Provider which needs to send a NewTemplate message (which doesn't include transactions) as quickly as possible.
