This change is mostly a refectoring that removes some code and gets rid of an
unnecessary layer of indirection after #27861
But it is not a pure refactoring since StartShutdown, AbortShutdown, and
WaitForShutdown functions used to abort on failure, and the replacement code
logs or returns errors instead.
Having InitContext() avoids the need to add duplicate code to src/init/*.cpp
files in the next commit. It also lets these files avoid referencing global
variables like gArgs.
There is no change in behavior in this commit.
Use SignalInterrupt object instead. There is a slight change in behavior here
because the previous StartShutdown code used to abort on failure and the
new code logs errors instead.
Use SignalInterrupt object instead. There is a slight change in behavior here
because the previous StartShutdown code used to abort on failure and the
new code returns an RPC error instead.
Replace exceptions thrown by signal and wait methods with [[nodiscard]] return
values.
This is mostly a refactoring, but there is a slight change of behavior if
AbortShutdown function fails. The original behavior which was unintentionally
changed in #27861 is restored, so it now triggers an assert failure again
instead of throwing an exception. (The AbortShutdown function is only ever
called in the the GUI version of Bitcoin Core when corruption is detected on
loading and the user tries to reindex.)
Problems with using exceptions were pointed out by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707.
Pass HTTP server an interrupt object instead of having it depend on shutdown.h
and global shutdown state.
There is no change in behavior in this commit.
Add NodeContext::shutdown variable and start using it to replace the
kernel::Context::interrupt variable. The latter can't easily be removed right
away but will be removed later in this PR.
Moving the interrupt object from the kernel context to the node context
increases flexibility of the kernel API so it is possible to use multiple
interrupt objects, or avoid creating one if one is not needed. It will also
allow getting rid of the kernel::g_context global later in this PR, replacing
it with a private SignalInterrupt instance in init.cpp
There is no change in behavior in this commit outside of unit tests. In unit
tests there should be no visible change either, but internally now each test
has its own interrupt variable so the variable will be automatically reset
between tests.
Use interfaces::Node object instead.
There is a minor change in behavior in this commit, because the new code calls
InterruptRPC() and StopRPC() when previous code did not do this. But this
should be a good thing since it makes sense to interrupt RPC when the system is
shutting down, and it is better for the GUI shut down in a consistent way
regardless of how the shutdown is triggered.
8f6ab31863 init: don't delete PID file if it was not generated (willcl-ark)
Pull request description:
In a similar vein to #28784, if a second `bitcoind` is started using the same datadir it will fail to start up, but during shutdown remove the PID file from the first `bitcoind` instance.
ACKs for top commit:
achow101:
ACK 8f6ab31863
andrewtoth:
ACK 8f6ab31863
romanz:
ACK 8f6ab31863
Tree-SHA512: c9af703cbfa179d33ef9580a51e86c1b0acbd28daa18c8d2e5e5ff796ab4d3e2009a962a47e6046a0e5ece936f8a06ee8af5fdf8ff4ae1e52cbcdbec4b942271
Previously, starting a second bitcoind using the same datadir would
correctly fail to init and shutdown. However during shutdown the PID
file belonging to the first instance would be erroneously removed by
the second process shutting down.
Fix this to only delete the PID file if we created it.
05aca09381 build: Patch Qt to handle minimum macOS version properly (Hennadii Stepanov)
Pull request description:
This PR is:
- required to [switch](https://github.com/bitcoin/bitcoin/pull/28622) to macOS 14 SDK (Xcode 15).
- an alternative to https://github.com/bitcoin/bitcoin/pull/28732 and https://github.com/bitcoin/bitcoin/pull/28775.
Qt relies on the `__MAC_OS_X_VERSION_MIN_REQUIRED` macro, which is set in the `AvailabilityInternal.h` SDK header to
the value provided by the Clang driver from the `-mmacos-version-min` / `-mmacosx-version-min` option.
Xcode 12 SDK expects the OS-specific `__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#ifdef __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
/* compiler for Mac OS X sets __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ */
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#endif
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED*/
```
In the other hand, Xcode 15 SDK expects a general `__ENVIRONMENT_OS_VERSION_MIN_REQUIRED__` macro:
```c++
#ifndef __MAC_OS_X_VERSION_MIN_REQUIRED
#if defined(__has_builtin) && __has_builtin(__is_target_os)
#if __is_target_os(macos)
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_OS_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif
#elif __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MIN_REQUIRED __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__
#define __MAC_OS_X_VERSION_MAX_ALLOWED __MAC_14_0
#endif /* __has_builtin(__is_target_os) && __is_target_os(macos) */
#endif /* __MAC_OS_X_VERSION_MIN_REQUIRED */
```
The latter macro is not provided by LLVM Clang until c8e2dd8c6f, which is available in Clang 17.
The suggested patch makes Qt "borrow" the `__MAC_OS_X_VERSION_MIN_REQUIRED` value from `MAC_OS_X_VERSION_MIN_REQUIRED`, which is set in the `AvailabilityMacros.h` SDK header.
ACKs for top commit:
maflcko:
lgtm ACK 05aca09381
Tree-SHA512: 8891aefde4b8a48885abf0648f4ec71a22f7fcfca1e17ebb8c70ce1ef44751ea5db6b8b652de6ee8a716ca5f96f720fef01600bc23986162d0146c946e2e8743
97d53e90ba ci: remove libz-dev from macOS build deps (fanquake)
Pull request description:
Remove no-longer used libz-dev (from DMG compression).
Followup to #28432.
Missed this in #28932.
ACKs for top commit:
TheCharlatan:
ACK 97d53e90ba
Tree-SHA512: ea4e46d83a11cfb0d77e7c5b370359864a026ca58e02de1a3d30458e4463dabd338db9546da63b782a7fa0ce199446c418fbfdfc400f3803b0ea9dd626aed42f
91504cbe0d rpc: `SyncWithValidationInterfaceQueue` on fee estimation RPC's (ismaelsadeeq)
714523918b tx fees, policy: CBlockPolicyEstimator update from `CValidationInterface` notifications (ismaelsadeeq)
dff5ad3b99 CValidationInterface: modify the parameter of `TransactionAddedToMempool` (ismaelsadeeq)
91532bd382 tx fees, policy: update `CBlockPolicyEstimator::processBlock` parameter (ismaelsadeeq)
bfcd401368 CValidationInterface, mempool: add new callback to `CValidationInterface` (ismaelsadeeq)
0889e07987 tx fees, policy: cast with static_cast instead of C-Style cast (ismaelsadeeq)
a0e3eb7549 tx fees, policy: bugfix: move `removeTx` into reason != `BLOCK` condition (ismaelsadeeq)
Pull request description:
This is an attempt to #11775
This Pr will enable fee estimator to listen to ValidationInterface notifications to process new transactions added and removed from the mempool.
This PR includes the following changes:
- Added a new callback to the Validation Interface `MempoolTransactionsRemovedForConnectedBlock`, which notifies listeners about the transactions that have been removed due to a new block being connected, along with the height at which the transactions were removed.
- Modified the `TransactionAddedToMempool` callback parameter to include additional information about the transaction needed for fee estimation.
- Updated `CBlockPolicyEstimator` to process transactions using` CTransactionRef` instead of `CTxMempoolEntry.`
- Implemented the `CValidationInterface` interface in `CBlockPolicyEstimater` and overridden the `TransactionAddedToMempool`, `TransactionRemovedFromMempool`, and `MempoolTransactionsRemovedForConnectedBlock` methods to receive updates from their notifications.
Prior to this PR, the fee estimator updates from the mempool, i.e whenever a new block is connected all transactions in the block that are in our mempool are going to be removed using the `removeForBlock` function in `txmempool.cpp`.
This removal triggered updates to the fee estimator. As a result, the fee estimator would block mempool's `cs` until it finished updating every time a new block was connected.
Instead of being blocked only on mempool tx removal, we were blocking on both tx removal and fee estimator updating.
If we want to further improve fee estimation, or add heavy-calulation steps to it, it is currently not viable as we would be slowing down block relay in the process
This PR is smaller in terms of the changes made compared to #11775, as it focuses solely on enabling fee estimator updates from the validationInterface/cscheduler thread notifications.
I have not split the validation interface because, as I understand it, the rationale behind the split in #11775 was to have `MempoolInterface` signals come from the mempool and `CValidationInterface` events come from validation. I believe this separation can be achieved in a separate refactoring PR when the need arises.
Also left out some commits from #11775
- Some refactoring which are no longer needed.
- Handle reorgs much better in fee estimator.
- Track witness hash malleation in fee estimator
I believe they are a separate change that can come in a follow-up after this.
ACKs for top commit:
achow101:
ACK 91504cbe0d
TheCharlatan:
Re-ACK 91504cbe0d
willcl-ark:
ACK 91504cbe0d
Tree-SHA512: 846dfb9da57a8a42458827b8975722d153907fe6302ad65748d74f311e1925557ad951c3d95fe71fb90ddcc8a3710c45abb343ab86b88780871cb9c38c72c7b1
7cb9367157 rpc: keep .cookie if it was not generated (Roman Zeyde)
Pull request description:
Otherwise, starting bitcoind twice may cause the `.cookie` file generated by the first instance to be deleted by the second instance shutdown (after failing to obtain a lock).
ACKs for top commit:
willcl-ark:
re-ACK 7cb9367157
achow101:
ACK 7cb9367157
kristapsk:
re-ACK 7cb9367157
stickies-v:
ACK 7cb9367157
Tree-SHA512: 0960dbc457975b0e0535f3d814824a879d7f85c9f1191537415b3fc253429a316a8e4badde56c8bc139778f132392983cec5fbe03891fb15ff61d3bc3f6e681b
f23ba24aa0 test_submitpackage: only make a chain of 3 txns (Greg Sanders)
e67a345162 doc: submitpackage vsize results are sigops-adjusted (Greg Sanders)
b67db52c39 RPC submitpackage: change return format to allow partial errors (Greg Sanders)
Pull request description:
This was prompted by errors being returned that didn't "make any sense" to me, because it would for example return a "fee too low" error, when the "real" error was the child had something invalid, which disallowed CPFP evaluation. Rather than make judgment calls on what error is important(which is currently just return the "first"!), we simply return all errors and let the callers determine what's best.
Added a top level `package_msg` for quick eye-balling of general success of the package.
This PR also fixes a couple bugs:
1) Currently we don't actually broadcast a transaction, even if it was entered into our mempool, if a subsequent transaction causes `PKG_TX` failure.
2) "other-wtxid" is uncovered by tests, but IIUC was previously required to return "fees" and "vsize" results, but did not. I just make those results optional.
ACKs for top commit:
Sjors:
Light re-utACK f23ba24aa0
achow101:
ACK f23ba24aa0
glozow:
utACK f23ba24aa0, thanks for taking the suggestions
Tree-SHA512: ebfd716a4fed9e8c2dea3d2181ba6a6171b06718d29ac2324c67b7a30b374d199f7e1739f91ab5d036be172d0479de9bc89c32263ee62143c0338b9b622d0cca
fa98a097a3 Rename version.h to node/protocol_version.h (MarcoFalke)
fa4fbd5816 Remove unused version.h include (MarcoFalke)
fa0ae22ff2 Remove unused SER_NETWORK, SER_DISK (MarcoFalke)
fae00fe9c2 Remove unused CDataStream (MarcoFalke)
fa7eb4f5c3 fuzz: Drop unused version from fuzz input format (MarcoFalke)
Pull request description:
Seems odd to have code that is completely dead.
Fix this by removing all of it.
ACKs for top commit:
sipa:
utACK fa98a097a3
ajtowns:
ACK fa98a097a3
ryanofsky:
Seems odd to not code review ACK fa98a097a3 (looks good)
Tree-SHA512: 9f1b9d9f92bda0512610bda6653e892756f637860362a9abfa439faab62de233cbad94b7df78ebacc160d9667aadfed4d9df08c0edefa618c040a049050fb913
e67634ef19 fuzz: BIP324: damage ciphertext/aad in full byte range (Sebastian Falbesoner)
Pull request description:
This PR is a tiny improvement for the `bip324_cipher_roundtrip` fuzz target: currently the damaging of input data for decryption (either ciphertext or aad) only ever happens in the lower nibble within the byte at the damage position, as the bit position for the `damage_val` byte was calculated with `damage_bit & 3` (corresponding to `% 4`) rather than `damage_bit & 7` (corresponding to the expected `% 8`).
Noticed while reviewing #28263 which uses similar constructs.
ACKs for top commit:
stratospher:
ACK e67634ef.
dergoegge:
utACK e67634ef19
Tree-SHA512: 1bab4df28708e079874feee939beef45eff235215375c339decc696f4c9aef04e4b417322b045491c8aec6e88ec8ec2db564e27ef1b0be352b6ff4ed38bad49a
fad82fea2b ci: Reduce use of bash -c (MarcoFalke)
fafcee4874 ci: Rename test script to 03_test_script.sh (MarcoFalke)
Pull request description:
It is confusing to treat commands as a single string. This change is
also required to support paths and strings with spaces in them in the
future.
ACKs for top commit:
RandyMcMillan:
utACK fad82fea2b
vasild:
ACK fad82fea2b
Tree-SHA512: fb79469d809400739e53da203842fda838f2ec9ab8dcd5e622ccd3db651d629161323bfcc04301562f13f5c407e8865036478a4ac7f6b5265dc4dda1a320c23d
66c4b58e51 guix: switch from guix environment to guix shell (fanquake)
Pull request description:
See https://guix.gnu.org/manual/devel/en/html_node/Invoking-guix-environment.html.
> Deprecation warning: The guix environment command is deprecated
in favor of guix shell, which performs similar functions but is more convenient to use. See Invoking guix shell.
> Being deprecated, guix environment is slated for eventual removal,
but the Guix project is committed to keeping it until May 1st, 2023. Please get in touch with us at guix-devel@gnu.org if you would like to discuss it.
See also https://guix.gnu.org/blog/2021/from-guix-environment-to-guix-shell/ for a blog post and additional details.
Guix `shell` was added to Guix ~1 year ago, in this commit, https://git.savannah.gnu.org/cgit/guix.git/commit/?id=80edb7df6586464aa40e84e103f0045452de95db, which isn't part of the 1.3.0 release binaries out of the box, but invoking a `guix pull`, and updating will make it available. i.e:
```bash
bash-5.1# guix --version
guix (GNU Guix) 1.3.0
Copyright (C) 2021 the Guix authors
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
bash-5.1# guix shell
guix: shell: command not found
Try 'guix --help' for more information.
bash-5.1# guix pull
Updating channel 'guix' from Git repository at 'https://git.savannah.gnu.org/git/guix.git'...
Authenticating channel 'guix', commits 9edb3f6 to 7a980bb (6,278 new commits)...
Building from this channel:
guix https://git.savannah.gnu.org/git/guix.git7a980bb
< snip >
building /gnu/store/2wwwsczxcw61m05p4mv0kf0advx4fqsb-inferior-script.scm.drv...
building package cache...
building profile with 1 package...
New in this revision:
6,866 new packages: a2jmidid, abjad,
bash-5.1# guix help shell
Usage: guix shell [OPTION] PACKAGES... [-- COMMAND...]
Build an environment that includes PACKAGES and execute COMMAND or an
interactive shell in that environment.
```
ACKs for top commit:
TheCharlatan:
ACK 66c4b58e51
Tree-SHA512: caa3fd2ca8d0f261c50ecdda3728a75389d24d89b51293dedc704ee77ab1342b2bb08ca8c871dcb4646229f056ec86cb15500934ded1b0c501a3ffc25aaa8ae6
Behavior prior to this commit allows some transactions to
enter into the local mempool but not be reported to the user
when encountering a PackageValidationResult::PCKG_TX result.
This is further compounded with the fact that any transactions
submitted to the mempool during this call would also not be
relayed to peers, resulting in unexpected behavior.
Fix this by, if encountering a package error, reporting all
wtxids, along with a new error field, and broadcasting every
transaction that was found in the mempool after submission.
Note that this also changes fees and vsize to optional,
which should also remove an issue with other-wtxid cases.
Also, add missing includes to scriptpubkeyman.
Also, export dependecies of the BasicTestingSetup from setup_common.h,
to avoid having to include them when setup_common.h is already included.
It is confusing to treat commands as a single string. This change is
also required to support paths and strings with spaces in them in the
future.
This requires replacing TEST_RUNNER_ENV with a global export, because it
no longer works. See:
```bash
$ export ENV="A=1" && $ENV ls
bash: A=1: command not found...
```
Or in the CI task:
+ DIR_UNIT_TEST_DATA=/ci_container_base/ci/scratch/qa-assets/unit_test_data/
+ LD_LIBRARY_PATH=/ci_container_base/depends/i686-pc-linux-gnu/lib
+ BITCOIND=bitcoin-node make -j10 check VERBOSE=1
/ci_container_base/ci/test/03_test_script.sh: line 166: BITCOIND=bitcoin-node: command not found
https://github.com/bitcoin/bitcoin/pull/28954/checks?check_run_id=19096858944https://cirrus-ci.com/task/6718317604372480
fa02c08c93 refactor: Use Txid in CMerkleBlock (MarcoFalke)
Pull request description:
This should also fix a gcc-13 compiler warning, see https://github.com/bitcoin/bitcoin/pull/28922#discussion_r1407856376
```
rpc/txoutproof.cpp: In lambda function:
rpc/txoutproof.cpp:72:33: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
72 | const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
| ^~~~
rpc/txoutproof.cpp:72:52: note: the temporary was destroyed at the end of the full expression ‘AccessByTxid((*(const CCoinsViewCache*)(&(& active_chainstate)->Chainstate::CoinsTip())), transaction_identifier<false>::FromUint256((* & tx)))’
72 | const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
| ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
ACKs for top commit:
TheCharlatan:
Re-ACK fa02c08c93
dergoegge:
reACK fa02c08c93
Tree-SHA512: 2e6837b9d0c90bd6e9d766330e7086d68c6ec80bb27fe2cfc4702b251b00d91a79f8bfbc76d998cbcd90bee5317402cf617f61099eee96d94e7ac8f37ba7a642
fa1a384706 Move compat.h include from system.h to system.cpp (MarcoFalke)
88887531b7 Move compat/assumptions.h include to one place that actually needs it (MarcoFalke)
77774110f4 Remove __cplusplus from compat/assumptions.h (MarcoFalke)
faa3d4f1d8 Remove duplicate NDEBUG check from compat/assumptions.h (MarcoFalke)
Pull request description:
Generally, compile-time checks should be close to the code that use them. Especially, since `compat/assumptions.h` is only included in one place, where iwyu suggests to remove it.
Fix all issues:
* The `NDEBUG` check is used in `util/check`, so it is redundant in `compat/assumptions.h`.
* The `__cplusplus` check is redundant with `doc/dependencies.md` (see commit message).
* Add missing `// IWYU pragma: keep` to avoid removing the include by accident.
ACKs for top commit:
achow101:
ACK fa1a384706
TheCharlatan:
re-ACK fa1a384706
theuni:
ACK fa1a384706
Tree-SHA512: f8b6db84be5d8844a2267345c0b1405fcbc39b8b5eeaa24db5b8412a74145fe44cf188b6b0c39cc2b062690ed37ca5b4662473484afe28dbec6469e79961389b
9ac114e5cd Throw error if invalid parameters passed to getnetworkhashps RPC endpoint (Jameson Lopp)
Pull request description:
When writing some scripts that iterated over many blocks to generate hashrate estimates I realized that my script was going out of range of the current chain tip height but was not encountering any errors.
I believe that passing an invalid block height to this function but receiving the hashrate estimate for the chain tip instead should be considered unexpected behavior.
ACKs for top commit:
Sjors:
re-utACK 9ac114e5cd
kevkevinpal:
reACK [9ac114e](9ac114e5cd)
achow101:
ACK 9ac114e5cd
Tree-SHA512: eefb465c2dd654fc48267f444e1809597ec5363cdd131ea9ec812458fed1e4bffbbbb0617d74687c9f7bb16274b598d8292f5eeb7953421e5d2a8dc2cc081f2b
705e3f1de0 refactor: Make CTxMemPoolEntry only explicitly copyable (TheCharlatan)
Pull request description:
This has the goal of prohibiting users from accidentally creating runtime failures, e.g. by interacting with iterator_to with a copied entry. This was brought up here: https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954.
CTxMemPoolEntry is already implicitly not move-constructable. So be explicit about this and use a std::list to collect the values in the policy_estimator fuzz test instead of a std::vector.
ACKs for top commit:
maflcko:
ACK 705e3f1de0🌯
achow101:
ACK 705e3f1de0
ajtowns:
ACK 705e3f1de0
ismaelsadeeq:
ACK 705e3f1de0
Tree-SHA512: 62056905c679c919d00f9ae065ed66ac986e7e7062015aea542843d8deecda57104d7a68d002f7b20afa3164f8e9215d2d2d002c167224129540e3b1bd0712cc
35fb9930ad test: enable v2 transport for p2p_timeouts.py (Martin Zumsande)
2c1669c37a test: enable v2 transport for rpc_net.py (Sebastian Falbesoner)
cc961c2695 test: enable v2 transport for p2p_node_network_limited.py (Sebastian Falbesoner)
3598a1b5c9 test: enable --v2transport in combination with --usecli (Martin Zumsande)
68a9001751 test: persist -v2transport over restarts and respect -v2transport=0 (Martin Zumsande)
Pull request description:
This makes the functional test suite compatible with BIP324, so that
`python3 test_runner.py --v2transport`
should succeed (currently, 12 tests fail for me on master).
Includes two commits by TheStack I found in an old discussion https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1326714164
Note that even though all tests should pass, the python `p2p.py` module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2.
Some of the fixed tests were added with `--v2transport` to the test runner. Though after #24748 we might also want to consider running the entire suite with `--v2transport` in some CI.
ACKs for top commit:
sipa:
utACK 35fb9930ad. Thanks for taking care of this.
achow101:
ACK 35fb9930ad
theStack:
ACK 35fb9930ad
stratospher:
ACK 35fb993.
Tree-SHA512: 80dc0bf211fa525ff1d092043aea9f222f14c02e5832a548fb8b83b9ede1fcee03c5e8ade0d05c331bdaa492af9c1cf3d0f0b15b846673c6eacea82dd4cefbc3
fae76a1f2a scripted-diff: Use DataStream in most places (MarcoFalke)
fac39b56b7 refactor: SpanReader without nVersion (MarcoFalke)
Pull request description:
The serialize version is unused, so remove it. This also allows to remove `GCS_SER_VERSION` and allows a scripted-diff to remove most of `CDataStream`.
ACKs for top commit:
ajtowns:
ACK fae76a1f2a
ryanofsky:
Code review ACK fae76a1f2a
Tree-SHA512: 3b487dba8ea380f1eacff9fdfb9197f025dbc30906813d3f4c3e6f1e9e4d9f2a169c6f163f51d135e18af538be78e2d2b13d694073ad25c5762980ae971a4c83
