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
