11d797e3a0 depends: Build `native_capnp` package with CMake (Hennadii Stepanov)
90389c95e9 depends: Build `capnp` package with CMake (Hennadii Stepanov)
Pull request description:
The first commit fixes two bugs when cross-compiling the `capnp` package on the master branch @ 160d23677a:
- for `x86_64-w64-mingw32` (see https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1790406668):
```
libtool: link: x86_64-w64-mingw32-g++-posix -shared -nostdlib /usr/lib/gcc/x86_64-w64-mingw32/12-posix/../../../../x86_64-w64-mingw32/lib/dllcrt2.o /usr/lib/gcc/x86_64-w64-mingw32/12-posix/crtbegin.o src/kj/.libs/cidr.o src/kj/.libs/common.o src/kj/.libs/units.o src/kj/.libs/memory.o src/kj/.libs/refcount.o src/kj/.libs/array.o src/kj/.libs/list.o src/kj/.libs/string.o src/kj/.libs/string-tree.o src/kj/.libs/source-location.o src/kj/.libs/hash.o src/kj/.libs/table.o src/kj/.libs/encoding.o src/kj/.libs/exception.o src/kj/.libs/debug.o src/kj/.libs/arena.o src/kj/.libs/io.o src/kj/.libs/mutex.o src/kj/.libs/thread.o src/kj/.libs/time.o src/kj/.libs/filesystem.o src/kj/.libs/filesystem-disk-unix.o src/kj/.libs/filesystem-disk-win32.o src/kj/.libs/test-helpers.o src/kj/.libs/main.o src/kj/parse/.libs/char.o -L/home/hebasto/git/bitcoin/depends/x86_64-w64-mingw32/lib -L/usr/lib/gcc/x86_64-w64-mingw32/12-posix -L/usr/lib/gcc/x86_64-w64-mingw32/12-posix/../../../../x86_64-w64-mingw32/lib -lstdc++ -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lkernel32 -lpthread -ladvapi32 -lshell32 -luser32 -lkernel32 -lmingw32 -lgcc_s -lgcc -lmoldname -lmingwex -lmsvcrt -lkernel32 /usr/lib/gcc/x86_64-w64-mingw32/12-posix/crtend.o -mthreads -O2 -mthreads -mthreads -o .libs/libkj-1-0-1.dll -Wl,--enable-auto-image-base -Xlinker --out-implib -Xlinker .libs/libkj.dll.a
/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x1dc): undefined reference to `__imp_inet_ntop'
/usr/bin/x86_64-w64-mingw32-ld: src/kj/.libs/cidr.o:cidr.c++:(.text+0x44b): undefined reference to `__imp_inet_pton'
collect2: error: ld returned 1 exit status
```
- for `arm64-apple-darwin`:
```
checking build system type... x86_64-pc-linux-gnu
checking host system type... Invalid configuration `arm64-apple-darwin': machine `arm64-apple' not recognized
configure: error: /bin/bash build-aux/config.sub arm64-apple-darwin failed
```
The second commit applies the same changes for the `native_capnp` package for [consistency](https://github.com/bitcoin/bitcoin/pull/28856#issuecomment-1807936546).
ACKs for top commit:
ryanofsky:
Code review ACK 11d797e3a0. Since last review arm64-apple-darwin platform is now mentioned in the commit message, and the change to `depends/packages/libmultiprocess.mk` in d1604d4b1d1ee8df279a1776303e167cc3d06193 which was unrelated (but probably still a good optimization) was reverted.
Tree-SHA512: c636e53073ce6fcda9724723bc59f3990fa9629a3b2f73d93dbc102a5a1badfbe8f4c5fef841f03588ebcad5cd4883f3ce32b128afcd75f6bc21eb801796a586
38816ff64e fuzz: txorphan check wtxids using GenTxid::Wtxid not GenTxid::Txid (Greg Sanders)
Pull request description:
Fixes the bugs in the fuzz test with no more changes as an alternative to https://github.com/bitcoin/bitcoin/pull/28658
ACKs for top commit:
naumenkogs:
ACK 38816ff64e
dergoegge:
ACK 38816ff64e
Tree-SHA512: 5e46a83f2b2a2ac0672a63eb6200b019e01089ab1aa80c4ab869b6fcf27ccf2e84a064e96397f1a1869ccfa43b0c9638cbae681a27c4ca3c96ac71f41262601e
11b7269d83 script: Enhance validations in utxo_snapshot.sh (pablomartin4btc)
Pull request description:
This PR resolves#27841 and some more:
- Ensure that the snapshot height is higher than the pruned block height when the node is pruned (Suggested by @Sjors [here](https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-1804941396)).
- Validate the correctness of the file path and check if the file already exists (@hazeycode's [#27845](https://github.com/bitcoin/bitcoin/pull/27845)).
- Make network activity disablement optional for the user (Suggested by @Sjors [here](https://github.com/bitcoin/bitcoin/pull/16899#discussion_r342735815) and [here](https://github.com/bitcoin/bitcoin/pull/16899#issuecomment-536520911)).
- Ensure the `reconsiderblock` command is triggered on exit (@hazeycode's same PR as above), even in the case of user interruption (Ctrl-C).
In order to perform some testing please follow the instructions in the description of previous @hazeycode's PR #27845.
ACKs for top commit:
Sjors:
tACK 11b7269d83
ryanofsky:
Code review ACK 11b7269d83
Tree-SHA512: 2b699894c6f732ad5104f5a2bcf5dc86ed31edcc9d664690cab55b94a8ab00e2ca5bde901ee1d63acddca7ea80ad1734d8cfe78f9c02f8470f264fe93a2af759
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
fa825975b5 fuzz: Avoid timeout in process_messages (MarcoFalke)
Pull request description:
Reduce the number of messages per fuzz input. There should be no reason to have more messages than that.
This should also avoid timeouts, such as https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=64548. CC https://github.com/bitcoin/bitcoin/issues/28812
ACKs for top commit:
dergoegge:
utACK fa825975b5
Tree-SHA512: eeff732f7b0bd9a71f23aeecbf813d31fe34d355b906fd0384a43075cbc3cebc46a26df741b0f337208d8b33b3e28210c9b9437e2eed77844f03131bb8f5f2a1
