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.
f5a2000579b140a1f51fc433706c775ca560c62c 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 f5a2000579b140a1f51fc433706c775ca560c62c
Tree-SHA512: 3da5c888db64a311276338270ba1dcad3eb2a24e205f6bb86fc92f767ecfa63682f13fafffff569fa0cfaea607ccb538f31e3934a086d482c3fe1be5d39f8791
6c3c619b35cc03e883f9d2b3326f906aedde9ba7 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 6c3c619b35cc03e883f9d2b3326f906aedde9ba7
hodlinator:
ACK 6c3c619b35cc03e883f9d2b3326f906aedde9ba7
Tree-SHA512: 740fb18b8fea78e4eb9740ceb0fe75d37246c28cfa2638b9d093e9514dd6d7926cc5be9ec57f8027cca3aa9d616e8c54322d2401cfa67fd25282f7816e63532d
fafd1a0f648fcd602166d01d47da417b0eb380df ci: Inline PACKAGE_MANAGER_INSTALL (MarcoFalke)
Pull request description:
The fallback `bash -c "$PACKAGE_MANAGER_INSTALL git"` is only needed by the `lint` task, so simplify it and inline `PACKAGE_MANAGER_INSTALL` once. Also, fixup the docs to add some other packages which are needed by podman in user-mode.
ACKs for top commit:
fanquake:
ACK fafd1a0f648fcd602166d01d47da417b0eb380df
Tree-SHA512: e1665446d0fb5a2e8f2cb912117c7a42838c840199e7504a859b9155a13a2ff2e8606ac56689446f973fce02b00240041f071ebf00520778ed81eb1a01de6663
f158993fd5575d3cbf991b6df7bcd2af7cf96c9a contrib: Update asmap link in seeds readme (Fabian Jahr)
Pull request description:
I am moving all my ASMap related repositories to an asmap org. While there is a redirect in place that works for now, GitHub doesn't guarantee that it will keep working in the long term. So we should still fix the links.
ACKs for top commit:
laanwj:
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
jarolrod:
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
brunoerg:
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
Tree-SHA512: c6f831b6ed1b520a38623a7d29c01677a45bf06fea77f4da6b421dd16e630836c9abf8191330e3887232fd688edca534067142d37d7e504a1a785ae451b11721
f951f1fab258f782a88bb006b5ae4ea486705388 ci: add timestamps to cirrus jobs (willcl-ark)
Pull request description:
Currently, debugging where time is spent in the cirrus jobs feels annoying, e.g. trying to see where time may be spent in https://github.com/bitcoin/bitcoin/issues/30969
Enable timestamps in the logs for more information.
ACKs for top commit:
maflcko:
review ACK f951f1fab258f782a88bb006b5ae4ea486705388
tdb3:
Code Review ACK f951f1fab258f782a88bb006b5ae4ea486705388
hebasto:
ACK f951f1fab258f782a88bb006b5ae4ea486705388.
Tree-SHA512: 4ca6c3722013323e794339c39af3041a519e3ecf75216093c4ca99668c0932fe41cc547819f742a3e8a47709ec6baec976c542eb3848c0248f439841110a4e85
faf801515f8fcd11a3454105cab66c38f6f240fe test: Add missing sync_mempools() to fill_mempool() (MarcoFalke)
fa48be6f0233cfbd5c1eab7b832c913912062fa5 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 faf801515f8fcd11a3454105cab66c38f6f240fe
ismaelsadeeq:
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe
marcofleon:
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe
Tree-SHA512: 2de62d168cbb6857a9fb8bc12c42a9093fedf5e9beb6f83a32b3fa72a5ba3cf03631055fd25ef553399a27a6fe0d71c44cfe60660a4d31986debd13b8ab00228
4cf84b344dea4696de540a0f053aa1ec560ac38c depends, doc: No need to specify general requirement (Hennadii Stepanov)
Pull request description:
CMake is no longer required solely for `libmultiprocess`.
ACKs for top commit:
maflcko:
review ACK 4cf84b344dea4696de540a0f053aa1ec560ac38c
Tree-SHA512: 014808ef523d6f0101aaa52a1858bebc16da9eb44664beb204e271b033c4dfa48115ce85c4ce5f151e82f90f83c64aaa25af73ef5a9b9b64953d2e07ff19633f
The current order is incorrect:
```bash
./build/src/bitcoin-cli loadtxoutset -rpcclienttimeout=0 utxo-840000.dat
error code: -1
error message:
loadtxoutset "path"
```
1a332817665f77f55090fa166930fec0e5500727 doc: multiprocess documentation improvements (Ryan Ofsky)
d043950ba245cdcd09dc389a71d43b0a58e41a8b multiprocess: Add serialization code for BlockValidationState (Ryan Ofsky)
33c2eee285e35db50b33efb6e782ff002f9d18ec multiprocess: Add IPC wrapper for Mining interface (Ryan Ofsky)
06882f84017f6b569b46a644f39b6d3c120ec6cf multiprocess: Add serialization code for vector<char> (Russell Yanofsky)
095286f790acda4a32f04c77aa86106007e2a0d8 multiprocess: Add serialization code for CTransaction (Russell Yanofsky)
69dfeb18761cfd933b8a9a6e69ce9d3c516ba949 multiprocess: update common-types.h to use C++20 concepts (Ryan Ofsky)
206c6e78eec7c71d317666a1af07acf357ba001e build: Make bitcoin_ipc_test depend on bitcoin_ipc (Ryan Ofsky)
070e6a32d5ff4be2f4f1f6a8200fba2f61df16e3 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 1a332817665f77f55090fa166930fec0e5500727
TheCharlatan:
ACK 1a332817665f77f55090fa166930fec0e5500727
itornaza:
ACK 1a332817665f77f55090fa166930fec0e5500727
Tree-SHA512: 0791078dd6885dbd81e3d14c75fffff3da8d1277873af379ea6f9633e910c11485bb324e4cde3d936d50d343b16a10b0e8fc1e0fc6d7bdca7f522211da50c01e
c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee validation: Disable CheckForkWarningConditions for background chainstate (Martin Zumsande)
Pull request description:
The comparison of `m_best_invalid` with the tip of the chainstate makes no sense for the background chainstate and can lead to incorrect log messages.
Fixes#30958
ACKs for top commit:
fjahr:
utACK c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee
achow101:
ACK c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee
TheCharlatan:
ACK c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee
tdb3:
CR ACK c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee
Tree-SHA512: 316811b1ade8521681f3a105c7db59dfdb27b1e13efeba95cd6183d750db2e07f48968e8fc54fe3834d3311519cf8c6b77d7e0fd61facbf2b07598e79110ff85
06e7e83632985bd8b648d24f9a51591d3a3bbec3 doc: correct the zmq automatic build info (tdb3)
Pull request description:
The current documentation states that the ZMQ feature is automatically included when building.
Noticed this was no longer the case when building to review PR #30942.
ACKs for top commit:
l0rinc:
ACK 06e7e83632985bd8b648d24f9a51591d3a3bbec3
hebasto:
re-ACK 06e7e83632985bd8b648d24f9a51591d3a3bbec3.
Tree-SHA512: 24269c29415e54815b105e3f830f31f055e9bdcb3bf4a7cede068651875190adfc4e72ad5448b3c30a23b0db024f743d9f18576a6fda46d87deb5ea39352e626
fa6c1946d235c6ea5b9384d8488d80aa3bcd0ad7 doc: Adjust links in OSS-Fuzz section (MarcoFalke)
Pull request description:
Adjust the links after the google issue tracker migration and replace the remaining paragraph with a link to https://bitcoincore.org/en/security-advisories/
ACKs for top commit:
dergoegge:
ACK fa6c1946d235c6ea5b9384d8488d80aa3bcd0ad7
Tree-SHA512: 4c3aba8bb5641756be5c66c2ee21b927a675622ddac021303f8196a1b2d8eb891406f6dedab0f04e12312339953c87302891df4ad5f5956c0b2ddd453452e1c9
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.
7bd3ee62f6d6f59ca599e85f81776d282dee1539 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 7bd3ee62f6d6f59ca599e85f81776d282dee1539
hebasto:
ACK 7bd3ee62f6d6f59ca599e85f81776d282dee1539.
Tree-SHA512: 683b15cafaf0103eeadf872ea6ce9a7d884b2605d3dcf4e66b0173cdb149c24965e7c5fa62aaddf2ac55df3f449aeb787176992c96cfee5d0b86621259e1dfe9
f20fe33e94c6752e5d2ed92511c0bf51a10716ee test: Add basic balance coverage to wallet_assumeutxo.py (Fabian Jahr)
037b101e808ccf9e717751619e04f6e87d614efd test: Add coverage for best block locator write in wallet_backup (Fabian Jahr)
31c0df038909e40fe9618a4595254907ed1de907 wallet: migration, write best locator before unloading wallet (furszy)
7e3dbe4180cbeb65e59b53d9fa98509e9189549d 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 f20fe33e94c6752e5d2ed92511c0bf51a10716ee
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.
7942951e3fcc58f7db0059546d03be9ea243f1db Remove unused g_best_block (Ryan Ofsky)
e3a560ca68d79b056a105a65ed0c174a9631aba9 rpc: use waitTipChanged for longpoll (Ryan Ofsky)
460687a09c2af336fce853d9ffb790d01429eec6 Remove unused CRPCSignals (Sjors Provoost)
dca923150e3ac10a57c23a7e29e76516d32ec10d Replace RPCNotifyBlockChange with waitTipChanged() (Sjors Provoost)
2a40ee1121903847cdd3f6c5b4796e4d5471b2df rpc: check for negative timeout arg in waitfor* (Sjors Provoost)
de7c855b3af99fe6b62279c5c2d08888a5437c4a rpc: recommend -rpcclienttimeout=0 for waitfor* (Sjors Provoost)
77ec072925a6d558b3c6b075becbed44727c5989 rpc: fix waitfornewblock description (Sjors Provoost)
285fe9fb51c808a9edd91b05bd3134fc18de0fb6 rpc: add test for waitforblock and waitfornewblock (Sjors Provoost)
b94b27cf05c709674117e308e441a8d1efddcd0a Add waitTipChanged to Mining interface (Sjors Provoost)
7eccdaf16081d6f624c4dc21df75b0474e049d2b node: Track last block that received a blockTip notification (Sjors Provoost)
ebb8215f23644f901c46fd4977b7d4b08fae5104 Rename getTipHash() to getTip() and return BlockRef (Sjors Provoost)
89a8f74bbbb900abfb3d8e946eea18ad7b1513ad 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 7942951e3fcc58f7db0059546d03be9ea243f1db
ryanofsky:
Code review ACK 7942951e3fcc58f7db0059546d03be9ea243f1db. Just rebased since last review
TheCharlatan:
Re-ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
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.
In the Guix environment, `${BASEPREFIX}/${HOST}/native/bin` is added to
the `PATH` environment variable, causing CMake to search for package
configurations in the `native` subdirectory first.
Explicitly specifying the top-priority search prefixes for the
`Libmultiprocess` and `LibmultiprocessNative` packages resolves the
issue.
5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7 Replace CScript _hex_v_u8 appends with _hex (Lőrinc)
cac846c2fbf6fc69bfc288fd387aa3f68d84d584 Allow CScript's operator<< to accept spans, not just vectors (Lőrinc)
c78d8ff4cb83506413bb73833fc5c04885d0ece8 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 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
ryanofsky:
Code review ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7. Looks good!
hodlinator:
re-ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
Tree-SHA512: 27a646629e017b2a05416d5eb964dda8b25b900d466221eff7bfa1339ded443e1c5c4cf8ff20cb3bba915a2603787a9fa6f6ec12bc0b9415d9eb07b57289192b
284bd17309ab3b124d9dcddfec62f5506383343b add check that chainwork doesn't exceed minimum work (marcofleon)
9aa5d1c3fcd10ecb94310ad515a8569bc2d418f8 add clarification in comment (marcofleon)
Pull request description:
A followup to https://github.com/bitcoin/bitcoin/pull/30661
The added assertion just makes sure that the fuzz test is working as intended. If we're sure that the total work of the test chain is never more than minimum chain work, then we can be sure that the later assertion failure would actually mean that a bug in the headers presync logic was found.
This PR also addresses:
https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1746614616https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764943665https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764961991
ACKs for top commit:
instagibbs:
reACK 284bd17309ab3b124d9dcddfec62f5506383343b
maflcko:
review ACK 284bd17309ab3b124d9dcddfec62f5506383343b
achow101:
ACK 284bd17309ab3b124d9dcddfec62f5506383343b
Tree-SHA512: 76a9dffea4b6e13499c636d6ad26af06135319d25117c0eb40cf8dfcfdca6a4549c9b4d2ba835192ca355e0f8d476227aeabf8bdb68770def72a9fb521533fe5
f482d0e366a84008129913b442f0c955de79ac93 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 f482d0e366a84008129913b442f0c955de79ac93
marcofleon:
Tested ACK f482d0e366a84008129913b442f0c955de79ac93. 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
bc52cda1f3c007bdf1ed00aa3011e207c7531017 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 bc52cda1f3c007bdf1ed00aa3011e207c7531017
achow101:
ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017
TheCharlatan:
ACK bc52cda1f3c007bdf1ed00aa3011e207c7531017
Tree-SHA512: ef880e7dfa1335bf2704ab17c0f506f17390b8259755674dfcd57131736492b2f4cfc36babda6902202b7c55a7513991e21f6634b0cd9b2b03baf4f1c0f8d78b
e6994efe08b282dd9e46602bcbad69567fe91dcd fix: increase rpcbind check robustness (tdb3)
d38e3aed89eea665399eef07f5c3b64b14d4f586 fix: handle invalid rpcbind port earlier (tdb3)
83b67f2e6d59ea5de6573314ea4fe54ae52b7c12 refactor: move host/port checking (tdb3)
73c243965ab256ece089d14173c2d285955e83d5 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 e6994efe08b282dd9e46602bcbad69567fe91dcd
ryanofsky:
Code review ACK e6994efe08b282dd9e46602bcbad69567fe91dcd
zaidmstrr:
Code review ACK [e6994ef](e6994efe08)
Tree-SHA512: bcc3e5ceef21963821cd16ce6ecb83d5c5657755882c05872a7cfe661a1492b1d631f54de22f41fdd173512d62dd15dc37e394fe1a7abe4de484b82cd2438b92
84663291275248fd52da644b0c2566bbf9cc780b chain: simplify `deleteRwSettings` code and improve it's doc (ismaelsadeeq)
f8d91f49c7091102138fcb123850399e8fadfbc7 chain: dont check for null settings value in `overwriteRwSetting` (ismaelsadeeq)
df601993f2d7454f081316d6a8ddefbcefa49b3d chain: ensure `updateRwSetting` doesn't update to a null settings (ismaelsadeeq)
c8e2eeeffb494d99d95c1c45efeda5a98dce94cd chain: uniformly use `SettingsAction` enum in settings methods (ismaelsadeeq)
1e9e735670f029c975d3b7486a54e5bb67135df7 chain: move new settings safely in `overwriteRwSetting` (ismaelsadeeq)
1c409004c80bc5f2314e20cc922076e22931cf73 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 84663291275248fd52da644b0c2566bbf9cc780b
ryanofsky:
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b. 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 84663291275248fd52da644b0c2566bbf9cc780b
Tree-SHA512: baf2f59ed5aac4a4bda0c84fb6554a466a40d1f7b52b61dc2ff293d83ae60e82b925b7003237b633fecb65eba3a4c108e69166046895d1295809fbe0de67b052
51f7668d31e2624e41c7ce77fe33162802808f3f addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e30e210bfc50dea974063053313ad3e 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 51f7668d31e2624e41c7ce77fe33162802808f3f
stratospher:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31e2624e41c7ce77fe33162802808f3f
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
54227e681a4efa8961f1ad05d43366d88a9b686a 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 54227e681a4efa8961f1ad05d43366d88a9b686a
achow101:
ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
furszy:
utACK 54227e681a4
mzumsande:
Code Review ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
jonatack:
ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
Tree-SHA512: 51ff24f64858aa6be6adf6f20105c9f076ebea743780bf2a4399f7fe8b5239cbb1ea06d32b2ef5e850da2369abb0ef7a52c50c2b8f31f4ca90d3a486abc9b77e
